Add annotations to roles.

Signed-off-by: Monis Khan <[email protected]>

pkg/api/constants.go

@@ -0,0 +1,10 @@
+package api
+
+// annotation keys
+const (
+	// OpenShiftDisplayName is a common, optional annotation that stores the name displayed by a UI when referencing a resource.
+	OpenShiftDisplayName = "openshift.io/display-name"
+
+	// OpenShiftDescription is a common, optional annotation that stores the description for a resource.
+	OpenShiftDescription = "openshift.io/description"
+)

pkg/cmd/admin/policy/reconcile_clusterroles.go

@@ -206,27 +206,11 @@ func (o *ReconcileClusterRolesOptions) ChangedClusterRoles() ([]*authorizationap
 			return nil, nil, err
 		}
 
-		// Copy any existing labels/annotations, so the displayed update is correct
-		// This assumes bootstrap roles will not set any labels/annotations
-		// These aren't actually used during update; the latest labels/annotations are pulled from the existing object again
-		expectedClusterRole.Labels = actualClusterRole.Labels
-		expectedClusterRole.Annotations = actualClusterRole.Annotations
-
-		_, extraRules := rulevalidation.Covers(expectedClusterRole.Rules, actualClusterRole.Rules)
-		_, missingRules := rulevalidation.Covers(actualClusterRole.Rules, expectedClusterRole.Rules)
-
-		// We need to reconcile:
-		// 1. if we're missing rules
-		// 2. if there are extra rules we need to remove
-		if (len(missingRules) > 0) || (!o.Union && len(extraRules) > 0) {
-			if o.Union {
-				expectedClusterRole.Rules = append(expectedClusterRole.Rules, extraRules...)
-			}
-
+		if reconciledClusterRole, needsReconciliation := computeReconciledRole(*expectedClusterRole, *actualClusterRole, o.Union); needsReconciliation {
 			if actualClusterRole.Annotations[ReconcileProtectAnnotation] == "true" {
-				skippedRoles = append(skippedRoles, expectedClusterRole)
+				skippedRoles = append(skippedRoles, reconciledClusterRole)
 			} else {
-				changedRoles = append(changedRoles, expectedClusterRole)
+				changedRoles = append(changedRoles, reconciledClusterRole)
 			}
 		}
 	}
@@ -239,6 +223,38 @@ func (o *ReconcileClusterRolesOptions) ChangedClusterRoles() ([]*authorizationap
 	return changedRoles, skippedRoles, nil
 }
 
+func computeReconciledRole(expected authorizationapi.ClusterRole, actual authorizationapi.ClusterRole, union bool) (*authorizationapi.ClusterRole, bool) {
+	existingAnnotationKeys := sets.StringKeySet(actual.Annotations)
+	expectedAnnotationKeys := sets.StringKeySet(expected.Annotations)
+	missingAnnotationKeys := !existingAnnotationKeys.HasAll(expectedAnnotationKeys.List()...)
+
+	// Copy any existing labels, so the displayed update is correct
+	// This assumes bootstrap roles will not set any labels
+	// These labels aren't actually used during update; the latest labels are pulled from the existing object again
+	// Annotations are merged in a way that guarantees that user made changes have precedence over the defaults
+	// The latest annotations are pulled from the existing object again during update before doing the actual merge
+	expected.Labels = actual.Labels
+	expected.Annotations = mergeAnnotations(expected.Annotations, actual.Annotations)
+
+	_, extraRules := rulevalidation.Covers(expected.Rules, actual.Rules)
+	_, missingRules := rulevalidation.Covers(actual.Rules, expected.Rules)
+
+	// We need to reconcile:
+	// 1. if we're missing rules
+	// 2. if there are extra rules we need to remove
+	// 3. if we are missing annotations
+	needsReconciliation := (len(missingRules) > 0) || (!union && len(extraRules) > 0) || missingAnnotationKeys
+
+	if !needsReconciliation {
+		return nil, false
+	}
+
+	if union {
+		expected.Rules = append(expected.Rules, extraRules...)
+	}
+	return &expected, true
+}
+
 // ReplaceChangedRoles will reconcile all the changed roles back to the recommended bootstrap policy
 func (o *ReconcileClusterRolesOptions) ReplaceChangedRoles(changedRoles []*authorizationapi.ClusterRole) error {
 	errs := []error{}
@@ -261,6 +277,7 @@ func (o *ReconcileClusterRolesOptions) ReplaceChangedRoles(changedRoles []*autho
 		}
 
 		role.Rules = changedRoles[i].Rules
+		role.Annotations = mergeAnnotations(changedRoles[i].Annotations, role.Annotations)
 		updatedRole, err := o.RoleClient.Update(role)
 		if err != nil {
 			errs = append(errs, err)
@@ -272,3 +289,14 @@ func (o *ReconcileClusterRolesOptions) ReplaceChangedRoles(changedRoles []*autho
 
 	return kerrors.NewAggregate(errs)
 }
+
+// mergeAnnotations combines the given annotation maps with the later annotations having higher precedence
+func mergeAnnotations(maps ...map[string]string) map[string]string {
+	output := map[string]string{}
+	for _, m := range maps {
+		for k, v := range m {
+			output[k] = v
+		}
+	}
+	return output
+}

pkg/cmd/admin/policy/reconcile_clusterroles_test.go

@@ -0,0 +1,246 @@
+package policy
+
+import (
+	"testing"
+
+	kapi "k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/util/sets"
+
+	authorizationapi "github.com/openshift/origin/pkg/authorization/api"
+)
+
+func role(rules []authorizationapi.PolicyRule, labels map[string]string, annotations map[string]string) *authorizationapi.ClusterRole {
+	return &authorizationapi.ClusterRole{Rules: rules, ObjectMeta: kapi.ObjectMeta{Labels: labels, Annotations: annotations}}
+}
+
+func rules(resources ...string) []authorizationapi.PolicyRule {
+	r := []authorizationapi.PolicyRule{}
+	for _, resource := range resources {
+		r = append(r, authorizationapi.PolicyRule{Verbs: sets.NewString("get"), Resources: sets.NewString(resource)})
+	}
+	return r
+}
+
+type ss map[string]string
+
+func TestComputeReconciledRole(t *testing.T) {
+	tests := map[string]struct {
+		expectedRole *authorizationapi.ClusterRole
+		actualRole   *authorizationapi.ClusterRole
+		union        bool
+
+		expectedReconciledRole       *authorizationapi.ClusterRole
+		expectedReconciliationNeeded bool
+	}{
+		"empty": {
+			expectedRole: role(rules(), nil, nil),
+			actualRole:   role(rules(), nil, nil),
+			union:        false,
+
+			expectedReconciledRole:       nil,
+			expectedReconciliationNeeded: false,
+		},
+		"match without union": {
+			expectedRole: role(rules("a"), nil, nil),
+			actualRole:   role(rules("a"), nil, nil),
+			union:        false,
+
+			expectedReconciledRole:       nil,
+			expectedReconciliationNeeded: false,
+		},
+		"match with union": {
+			expectedRole: role(rules("a"), nil, nil),
+			actualRole:   role(rules("a"), nil, nil),
+			union:        true,
+
+			expectedReconciledRole:       nil,
+			expectedReconciliationNeeded: false,
+		},
+		"different rules without union": {
+			expectedRole: role(rules("a"), nil, nil),
+			actualRole:   role(rules("b"), nil, nil),
+			union:        false,
+
+			expectedReconciledRole:       role(rules("a"), nil, nil),
+			expectedReconciliationNeeded: true,
+		},
+		"different rules with union": {
+			expectedRole: role(rules("a"), nil, nil),
+			actualRole:   role(rules("b"), nil, nil),
+			union:        true,
+
+			expectedReconciledRole:       role(rules("a", "b"), nil, nil),
+			expectedReconciliationNeeded: true,
+		},
+		"match labels without union": {
+			expectedRole: role(rules("a"), ss{"1": "a"}, nil),
+			actualRole:   role(rules("a"), ss{"1": "a"}, nil),
+			union:        false,
+
+			expectedReconciledRole:       nil,
+			expectedReconciliationNeeded: false,
+		},
+		"match labels with union": {
+			expectedRole: role(rules("a"), ss{"1": "a"}, nil),
+			actualRole:   role(rules("a"), ss{"1": "a"}, nil),
+			union:        true,
+
+			expectedReconciledRole:       nil,
+			expectedReconciliationNeeded: false,
+		},
+		"different labels without union": {
+			expectedRole: role(rules("a"), ss{"1": "a"}, nil),
+			actualRole:   role(rules("a"), ss{"2": "b"}, nil),
+			union:        false,
+
+			expectedReconciledRole:       nil,
+			expectedReconciliationNeeded: false,
+		},
+		"different labels with union": {
+			expectedRole: role(rules("a"), ss{"1": "a"}, nil),
+			actualRole:   role(rules("a"), ss{"2": "b"}, nil),
+			union:        true,
+
+			expectedReconciledRole:       nil,
+			expectedReconciliationNeeded: false,
+		},
+		"different labels and rules without union": {
+			expectedRole: role(rules("a"), ss{"1": "a"}, nil),
+			actualRole:   role(rules("b"), ss{"2": "b"}, nil),
+			union:        false,
+
+			expectedReconciledRole:       role(rules("a"), ss{"2": "b"}, nil),
+			expectedReconciliationNeeded: true,
+		},
+		"different labels and rules with union": {
+			expectedRole: role(rules("a"), ss{"1": "a"}, nil),
+			actualRole:   role(rules("b"), ss{"2": "b"}, nil),
+			union:        true,
+
+			expectedReconciledRole:       role(rules("a", "b"), ss{"2": "b"}, nil),
+			expectedReconciliationNeeded: true,
+		},
+		"conflicting labels and rules without union": {
+			expectedRole: role(rules("a"), ss{"1": "a"}, nil),
+			actualRole:   role(rules("b"), ss{"1": "b"}, nil),
+			union:        false,
+
+			expectedReconciledRole:       role(rules("a"), ss{"1": "b"}, nil),
+			expectedReconciliationNeeded: true,
+		},
+		"conflicting labels and rules with union": {
+			expectedRole: role(rules("a"), ss{"1": "a"}, nil),
+			actualRole:   role(rules("b"), ss{"1": "b"}, nil),
+			union:        true,
+
+			expectedReconciledRole:       role(rules("a", "b"), ss{"1": "b"}, nil),
+			expectedReconciliationNeeded: true,
+		},
+		"match annotations without union": {
+			expectedRole: role(rules("a"), nil, ss{"1": "a"}),
+			actualRole:   role(rules("a"), nil, ss{"1": "a"}),
+			union:        false,
+
+			expectedReconciledRole:       nil,
+			expectedReconciliationNeeded: false,
+		},
+		"match annotations with union": {
+			expectedRole: role(rules("a"), nil, ss{"1": "a"}),
+			actualRole:   role(rules("a"), nil, ss{"1": "a"}),
+			union:        true,
+
+			expectedReconciledRole:       nil,
+			expectedReconciliationNeeded: false,
+		},
+		"different annotations without union": {
+			expectedRole: role(rules("a"), nil, ss{"1": "a"}),
+			actualRole:   role(rules("a"), nil, ss{"2": "b"}),
+			union:        false,
+
+			expectedReconciledRole:       role(rules("a"), nil, ss{"1": "a", "2": "b"}),
+			expectedReconciliationNeeded: true,
+		},
+		"different annotations with union": {
+			expectedRole: role(rules("a"), nil, ss{"1": "a"}),
+			actualRole:   role(rules("a"), nil, ss{"2": "b"}),
+			union:        true,
+
+			expectedReconciledRole:       role(rules("a"), nil, ss{"1": "a", "2": "b"}),
+			expectedReconciliationNeeded: true,
+		},
+		"different annotations and rules without union": {
+			expectedRole: role(rules("a"), nil, ss{"1": "a"}),
+			actualRole:   role(rules("b"), nil, ss{"2": "b"}),
+			union:        false,
+
+			expectedReconciledRole:       role(rules("a"), nil, ss{"1": "a", "2": "b"}),
+			expectedReconciliationNeeded: true,
+		},
+		"different annotations and rules with union": {
+			expectedRole: role(rules("a"), nil, ss{"1": "a"}),
+			actualRole:   role(rules("b"), nil, ss{"2": "b"}),
+			union:        true,
+
+			expectedReconciledRole:       role(rules("a", "b"), nil, ss{"1": "a", "2": "b"}),
+			expectedReconciliationNeeded: true,
+		},
+		"conflicting annotations and rules without union": {
+			expectedRole: role(rules("a"), nil, ss{"1": "a"}),
+			actualRole:   role(rules("b"), nil, ss{"1": "b"}),
+			union:        false,
+
+			expectedReconciledRole:       role(rules("a"), nil, ss{"1": "b"}),
+			expectedReconciliationNeeded: true,
+		},
+		"conflicting annotations and rules with union": {
+			expectedRole: role(rules("a"), nil, ss{"1": "a"}),
+			actualRole:   role(rules("b"), nil, ss{"1": "b"}),
+			union:        true,
+
+			expectedReconciledRole:       role(rules("a", "b"), nil, ss{"1": "b"}),
+			expectedReconciliationNeeded: true,
+		},
+		"conflicting labels/annotations and rules without union": {
+			expectedRole: role(rules("a"), ss{"3": "d"}, ss{"1": "a"}),
+			actualRole:   role(rules("b"), ss{"4": "e"}, ss{"1": "b"}),
+			union:        false,
+
+			expectedReconciledRole:       role(rules("a"), ss{"4": "e"}, ss{"1": "b"}),
+			expectedReconciliationNeeded: true,
+		},
+		"conflicting labels/annotations and rules with union": {
+			expectedRole: role(rules("a"), ss{"3": "d"}, ss{"1": "a"}),
+			actualRole:   role(rules("b"), ss{"4": "e"}, ss{"1": "b"}),
+			union:        true,
+
+			expectedReconciledRole:       role(rules("a", "b"), ss{"4": "e"}, ss{"1": "b"}),
+			expectedReconciliationNeeded: true,
+		},
+		"complex labels/annotations and rules without union": {
+			expectedRole: role(rules("pods", "nodes", "secrets"), ss{"env": "prod", "color": "blue"}, ss{"description": "fancy", "system": "true"}),
+			actualRole:   role(rules("nodes", "images", "projects"), ss{"color": "red", "team": "pm"}, ss{"system": "false", "owner": "admin", "vip": "yes"}),
+			union:        false,
+
+			expectedReconciledRole:       role(rules("pods", "nodes", "secrets"), ss{"color": "red", "team": "pm"}, ss{"description": "fancy", "system": "false", "owner": "admin", "vip": "yes"}),
+			expectedReconciliationNeeded: true,
+		},
+		"complex labels/annotations and rules with union": {
+			expectedRole: role(rules("pods", "nodes", "secrets"), ss{"env": "prod", "color": "blue", "manager": "randy"}, ss{"description": "fancy", "system": "true", "up": "true"}),
+			actualRole:   role(rules("nodes", "images", "projects"), ss{"color": "red", "team": "pm"}, ss{"system": "false", "owner": "admin", "vip": "yes", "rate": "down"}),
+			union:        true,
+
+			expectedReconciledRole:       role(rules("pods", "nodes", "secrets", "images", "projects"), ss{"color": "red", "team": "pm"}, ss{"description": "fancy", "system": "false", "owner": "admin", "vip": "yes", "rate": "down", "up": "true"}),
+			expectedReconciliationNeeded: true,
+		},
+	}
+
+	for k, tc := range tests {
+		reconciledRole, reconciliationNeeded := computeReconciledRole(*tc.expectedRole, *tc.actualRole, tc.union)
+		if reconciliationNeeded != tc.expectedReconciliationNeeded {
+			t.Errorf("%s: Expected\n\t%v\ngot\n\t%v", k, tc.expectedReconciliationNeeded, reconciliationNeeded)
+		}
+		if !kapi.Semantic.DeepEqual(reconciledRole, tc.expectedReconciledRole) {
+			t.Errorf("%s: Expected\n\t%#v\ngot\n\t%#v", k, tc.expectedReconciledRole, reconciledRole)
+		}
+	}
+}

pkg/cmd/admin/project/new_project.go

@@ -11,6 +11,7 @@ import (
 	kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
 	errorsutil "k8s.io/kubernetes/pkg/util/errors"
 
+	oapi "github.com/openshift/origin/pkg/api"
 	"github.com/openshift/origin/pkg/client"
 	"github.com/openshift/origin/pkg/cmd/admin/policy"
 	"github.com/openshift/origin/pkg/cmd/server/bootstrappolicy"
@@ -99,8 +100,8 @@ func (o *NewProjectOptions) Run(useNodeSelector bool) error {
 	project := &projectapi.Project{}
 	project.Name = o.ProjectName
 	project.Annotations = make(map[string]string)
-	project.Annotations[projectapi.ProjectDescription] = o.Description
-	project.Annotations[projectapi.ProjectDisplayName] = o.DisplayName
+	project.Annotations[oapi.OpenShiftDescription] = o.Description
+	project.Annotations[oapi.OpenShiftDisplayName] = o.DisplayName
 	if useNodeSelector {
 		project.Annotations[projectapi.ProjectNodeSelector] = o.NodeSelector
 	}

pkg/cmd/cli/cmd/project.go

@@ -19,6 +19,7 @@ import (
 	"github.com/openshift/origin/pkg/cmd/templates"
 	"github.com/openshift/origin/pkg/cmd/util/clientcmd"
 	"github.com/openshift/origin/pkg/project/api"
+	projectapihelpers "github.com/openshift/origin/pkg/project/api/helpers"
 	projectutil "github.com/openshift/origin/pkg/project/util"
 
 	"github.com/spf13/cobra"
@@ -210,11 +211,11 @@ func (o ProjectOptions) RunProject() error {
 						case 0:
 							msg += "\nYou are not a member of any projects. You can request a project to be created with the 'new-project' command."
 						case 1:
-							msg += fmt.Sprintf("\nYou have one project on this server: %s", api.DisplayNameAndNameForProject(&projects[0]))
+							msg += fmt.Sprintf("\nYou have one project on this server: %s", projectapihelpers.DisplayNameAndNameForProject(&projects[0]))
 						default:
 							msg += "\nYour projects are:"
 							for _, project := range projects {
-								msg += fmt.Sprintf("\n* %s", api.DisplayNameAndNameForProject(&project))
+								msg += fmt.Sprintf("\n* %s", projectapihelpers.DisplayNameAndNameForProject(&project))
 							}
 						}
 					}