Skip to content

Commit ba792d2

Browse files
author
OpenShift Bot
authored
Merge pull request #11328 from enj/enj/f/role_annotations/11268
Merged by openshift-bot
2 parents e4b43ee + d89c5cd commit ba792d2

File tree

23 files changed

+626
-95
lines changed

23 files changed

+626
-95
lines changed

‎pkg/api/constants.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
package api
2+
3+
// annotation keys
4+
const (
5+
// OpenShiftDisplayName is a common, optional annotation that stores the name displayed by a UI when referencing a resource.
6+
OpenShiftDisplayName = "openshift.io/display-name"
7+
8+
// OpenShiftDescription is a common, optional annotation that stores the description for a resource.
9+
OpenShiftDescription = "openshift.io/description"
10+
)

‎pkg/cmd/admin/policy/reconcile_clusterroles.go

Lines changed: 47 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -206,27 +206,11 @@ func (o *ReconcileClusterRolesOptions) ChangedClusterRoles() ([]*authorizationap
206206
return nil, nil, err
207207
}
208208

209-
// Copy any existing labels/annotations, so the displayed update is correct
210-
// This assumes bootstrap roles will not set any labels/annotations
211-
// These aren't actually used during update; the latest labels/annotations are pulled from the existing object again
212-
expectedClusterRole.Labels = actualClusterRole.Labels
213-
expectedClusterRole.Annotations = actualClusterRole.Annotations
214-
215-
_, extraRules := rulevalidation.Covers(expectedClusterRole.Rules, actualClusterRole.Rules)
216-
_, missingRules := rulevalidation.Covers(actualClusterRole.Rules, expectedClusterRole.Rules)
217-
218-
// We need to reconcile:
219-
// 1. if we're missing rules
220-
// 2. if there are extra rules we need to remove
221-
if (len(missingRules) > 0) || (!o.Union && len(extraRules) > 0) {
222-
if o.Union {
223-
expectedClusterRole.Rules = append(expectedClusterRole.Rules, extraRules...)
224-
}
225-
209+
if reconciledClusterRole, needsReconciliation := computeReconciledRole(*expectedClusterRole, *actualClusterRole, o.Union); needsReconciliation {
226210
if actualClusterRole.Annotations[ReconcileProtectAnnotation] == "true" {
227-
skippedRoles = append(skippedRoles, expectedClusterRole)
211+
skippedRoles = append(skippedRoles, reconciledClusterRole)
228212
} else {
229-
changedRoles = append(changedRoles, expectedClusterRole)
213+
changedRoles = append(changedRoles, reconciledClusterRole)
230214
}
231215
}
232216
}
@@ -239,6 +223,38 @@ func (o *ReconcileClusterRolesOptions) ChangedClusterRoles() ([]*authorizationap
239223
return changedRoles, skippedRoles, nil
240224
}
241225

226+
func computeReconciledRole(expected authorizationapi.ClusterRole, actual authorizationapi.ClusterRole, union bool) (*authorizationapi.ClusterRole, bool) {
227+
existingAnnotationKeys := sets.StringKeySet(actual.Annotations)
228+
expectedAnnotationKeys := sets.StringKeySet(expected.Annotations)
229+
missingAnnotationKeys := !existingAnnotationKeys.HasAll(expectedAnnotationKeys.List()...)
230+
231+
// Copy any existing labels, so the displayed update is correct
232+
// This assumes bootstrap roles will not set any labels
233+
// These labels aren't actually used during update; the latest labels are pulled from the existing object again
234+
// Annotations are merged in a way that guarantees that user made changes have precedence over the defaults
235+
// The latest annotations are pulled from the existing object again during update before doing the actual merge
236+
expected.Labels = actual.Labels
237+
expected.Annotations = mergeAnnotations(expected.Annotations, actual.Annotations)
238+
239+
_, extraRules := rulevalidation.Covers(expected.Rules, actual.Rules)
240+
_, missingRules := rulevalidation.Covers(actual.Rules, expected.Rules)
241+
242+
// We need to reconcile:
243+
// 1. if we're missing rules
244+
// 2. if there are extra rules we need to remove
245+
// 3. if we are missing annotations
246+
needsReconciliation := (len(missingRules) > 0) || (!union && len(extraRules) > 0) || missingAnnotationKeys
247+
248+
if !needsReconciliation {
249+
return nil, false
250+
}
251+
252+
if union {
253+
expected.Rules = append(expected.Rules, extraRules...)
254+
}
255+
return &expected, true
256+
}
257+
242258
// ReplaceChangedRoles will reconcile all the changed roles back to the recommended bootstrap policy
243259
func (o *ReconcileClusterRolesOptions) ReplaceChangedRoles(changedRoles []*authorizationapi.ClusterRole) error {
244260
errs := []error{}
@@ -261,6 +277,7 @@ func (o *ReconcileClusterRolesOptions) ReplaceChangedRoles(changedRoles []*autho
261277
}
262278

263279
role.Rules = changedRoles[i].Rules
280+
role.Annotations = mergeAnnotations(changedRoles[i].Annotations, role.Annotations)
264281
updatedRole, err := o.RoleClient.Update(role)
265282
if err != nil {
266283
errs = append(errs, err)
@@ -272,3 +289,14 @@ func (o *ReconcileClusterRolesOptions) ReplaceChangedRoles(changedRoles []*autho
272289

273290
return kerrors.NewAggregate(errs)
274291
}
292+
293+
// mergeAnnotations combines the given annotation maps with the later annotations having higher precedence
294+
func mergeAnnotations(maps ...map[string]string) map[string]string {
295+
output := map[string]string{}
296+
for _, m := range maps {
297+
for k, v := range m {
298+
output[k] = v
299+
}
300+
}
301+
return output
302+
}

‎pkg/cmd/admin/policy/reconcile_clusterroles_test.go

Lines changed: 246 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,246 @@
1+
package policy
2+
3+
import (
4+
"testing"
5+
6+
kapi "k8s.io/kubernetes/pkg/api"
7+
"k8s.io/kubernetes/pkg/util/sets"
8+
9+
authorizationapi "github.com/openshift/origin/pkg/authorization/api"
10+
)
11+
12+
func role(rules []authorizationapi.PolicyRule, labels map[string]string, annotations map[string]string) *authorizationapi.ClusterRole {
13+
return &authorizationapi.ClusterRole{Rules: rules, ObjectMeta: kapi.ObjectMeta{Labels: labels, Annotations: annotations}}
14+
}
15+
16+
func rules(resources ...string) []authorizationapi.PolicyRule {
17+
r := []authorizationapi.PolicyRule{}
18+
for _, resource := range resources {
19+
r = append(r, authorizationapi.PolicyRule{Verbs: sets.NewString("get"), Resources: sets.NewString(resource)})
20+
}
21+
return r
22+
}
23+
24+
type ss map[string]string
25+
26+
func TestComputeReconciledRole(t *testing.T) {
27+
tests := map[string]struct {
28+
expectedRole *authorizationapi.ClusterRole
29+
actualRole *authorizationapi.ClusterRole
30+
union bool
31+
32+
expectedReconciledRole *authorizationapi.ClusterRole
33+
expectedReconciliationNeeded bool
34+
}{
35+
"empty": {
36+
expectedRole: role(rules(), nil, nil),
37+
actualRole: role(rules(), nil, nil),
38+
union: false,
39+
40+
expectedReconciledRole: nil,
41+
expectedReconciliationNeeded: false,
42+
},
43+
"match without union": {
44+
expectedRole: role(rules("a"), nil, nil),
45+
actualRole: role(rules("a"), nil, nil),
46+
union: false,
47+
48+
expectedReconciledRole: nil,
49+
expectedReconciliationNeeded: false,
50+
},
51+
"match with union": {
52+
expectedRole: role(rules("a"), nil, nil),
53+
actualRole: role(rules("a"), nil, nil),
54+
union: true,
55+
56+
expectedReconciledRole: nil,
57+
expectedReconciliationNeeded: false,
58+
},
59+
"different rules without union": {
60+
expectedRole: role(rules("a"), nil, nil),
61+
actualRole: role(rules("b"), nil, nil),
62+
union: false,
63+
64+
expectedReconciledRole: role(rules("a"), nil, nil),
65+
expectedReconciliationNeeded: true,
66+
},
67+
"different rules with union": {
68+
expectedRole: role(rules("a"), nil, nil),
69+
actualRole: role(rules("b"), nil, nil),
70+
union: true,
71+
72+
expectedReconciledRole: role(rules("a", "b"), nil, nil),
73+
expectedReconciliationNeeded: true,
74+
},
75+
"match labels without union": {
76+
expectedRole: role(rules("a"), ss{"1": "a"}, nil),
77+
actualRole: role(rules("a"), ss{"1": "a"}, nil),
78+
union: false,
79+
80+
expectedReconciledRole: nil,
81+
expectedReconciliationNeeded: false,
82+
},
83+
"match labels with union": {
84+
expectedRole: role(rules("a"), ss{"1": "a"}, nil),
85+
actualRole: role(rules("a"), ss{"1": "a"}, nil),
86+
union: true,
87+
88+
expectedReconciledRole: nil,
89+
expectedReconciliationNeeded: false,
90+
},
91+
"different labels without union": {
92+
expectedRole: role(rules("a"), ss{"1": "a"}, nil),
93+
actualRole: role(rules("a"), ss{"2": "b"}, nil),
94+
union: false,
95+
96+
expectedReconciledRole: nil,
97+
expectedReconciliationNeeded: false,
98+
},
99+
"different labels with union": {
100+
expectedRole: role(rules("a"), ss{"1": "a"}, nil),
101+
actualRole: role(rules("a"), ss{"2": "b"}, nil),
102+
union: true,
103+
104+
expectedReconciledRole: nil,
105+
expectedReconciliationNeeded: false,
106+
},
107+
"different labels and rules without union": {
108+
expectedRole: role(rules("a"), ss{"1": "a"}, nil),
109+
actualRole: role(rules("b"), ss{"2": "b"}, nil),
110+
union: false,
111+
112+
expectedReconciledRole: role(rules("a"), ss{"2": "b"}, nil),
113+
expectedReconciliationNeeded: true,
114+
},
115+
"different labels and rules with union": {
116+
expectedRole: role(rules("a"), ss{"1": "a"}, nil),
117+
actualRole: role(rules("b"), ss{"2": "b"}, nil),
118+
union: true,
119+
120+
expectedReconciledRole: role(rules("a", "b"), ss{"2": "b"}, nil),
121+
expectedReconciliationNeeded: true,
122+
},
123+
"conflicting labels and rules without union": {
124+
expectedRole: role(rules("a"), ss{"1": "a"}, nil),
125+
actualRole: role(rules("b"), ss{"1": "b"}, nil),
126+
union: false,
127+
128+
expectedReconciledRole: role(rules("a"), ss{"1": "b"}, nil),
129+
expectedReconciliationNeeded: true,
130+
},
131+
"conflicting labels and rules with union": {
132+
expectedRole: role(rules("a"), ss{"1": "a"}, nil),
133+
actualRole: role(rules("b"), ss{"1": "b"}, nil),
134+
union: true,
135+
136+
expectedReconciledRole: role(rules("a", "b"), ss{"1": "b"}, nil),
137+
expectedReconciliationNeeded: true,
138+
},
139+
"match annotations without union": {
140+
expectedRole: role(rules("a"), nil, ss{"1": "a"}),
141+
actualRole: role(rules("a"), nil, ss{"1": "a"}),
142+
union: false,
143+
144+
expectedReconciledRole: nil,
145+
expectedReconciliationNeeded: false,
146+
},
147+
"match annotations with union": {
148+
expectedRole: role(rules("a"), nil, ss{"1": "a"}),
149+
actualRole: role(rules("a"), nil, ss{"1": "a"}),
150+
union: true,
151+
152+
expectedReconciledRole: nil,
153+
expectedReconciliationNeeded: false,
154+
},
155+
"different annotations without union": {
156+
expectedRole: role(rules("a"), nil, ss{"1": "a"}),
157+
actualRole: role(rules("a"), nil, ss{"2": "b"}),
158+
union: false,
159+
160+
expectedReconciledRole: role(rules("a"), nil, ss{"1": "a", "2": "b"}),
161+
expectedReconciliationNeeded: true,
162+
},
163+
"different annotations with union": {
164+
expectedRole: role(rules("a"), nil, ss{"1": "a"}),
165+
actualRole: role(rules("a"), nil, ss{"2": "b"}),
166+
union: true,
167+
168+
expectedReconciledRole: role(rules("a"), nil, ss{"1": "a", "2": "b"}),
169+
expectedReconciliationNeeded: true,
170+
},
171+
"different annotations and rules without union": {
172+
expectedRole: role(rules("a"), nil, ss{"1": "a"}),
173+
actualRole: role(rules("b"), nil, ss{"2": "b"}),
174+
union: false,
175+
176+
expectedReconciledRole: role(rules("a"), nil, ss{"1": "a", "2": "b"}),
177+
expectedReconciliationNeeded: true,
178+
},
179+
"different annotations and rules with union": {
180+
expectedRole: role(rules("a"), nil, ss{"1": "a"}),
181+
actualRole: role(rules("b"), nil, ss{"2": "b"}),
182+
union: true,
183+
184+
expectedReconciledRole: role(rules("a", "b"), nil, ss{"1": "a", "2": "b"}),
185+
expectedReconciliationNeeded: true,
186+
},
187+
"conflicting annotations and rules without union": {
188+
expectedRole: role(rules("a"), nil, ss{"1": "a"}),
189+
actualRole: role(rules("b"), nil, ss{"1": "b"}),
190+
union: false,
191+
192+
expectedReconciledRole: role(rules("a"), nil, ss{"1": "b"}),
193+
expectedReconciliationNeeded: true,
194+
},
195+
"conflicting annotations and rules with union": {
196+
expectedRole: role(rules("a"), nil, ss{"1": "a"}),
197+
actualRole: role(rules("b"), nil, ss{"1": "b"}),
198+
union: true,
199+
200+
expectedReconciledRole: role(rules("a", "b"), nil, ss{"1": "b"}),
201+
expectedReconciliationNeeded: true,
202+
},
203+
"conflicting labels/annotations and rules without union": {
204+
expectedRole: role(rules("a"), ss{"3": "d"}, ss{"1": "a"}),
205+
actualRole: role(rules("b"), ss{"4": "e"}, ss{"1": "b"}),
206+
union: false,
207+
208+
expectedReconciledRole: role(rules("a"), ss{"4": "e"}, ss{"1": "b"}),
209+
expectedReconciliationNeeded: true,
210+
},
211+
"conflicting labels/annotations and rules with union": {
212+
expectedRole: role(rules("a"), ss{"3": "d"}, ss{"1": "a"}),
213+
actualRole: role(rules("b"), ss{"4": "e"}, ss{"1": "b"}),
214+
union: true,
215+
216+
expectedReconciledRole: role(rules("a", "b"), ss{"4": "e"}, ss{"1": "b"}),
217+
expectedReconciliationNeeded: true,
218+
},
219+
"complex labels/annotations and rules without union": {
220+
expectedRole: role(rules("pods", "nodes", "secrets"), ss{"env": "prod", "color": "blue"}, ss{"description": "fancy", "system": "true"}),
221+
actualRole: role(rules("nodes", "images", "projects"), ss{"color": "red", "team": "pm"}, ss{"system": "false", "owner": "admin", "vip": "yes"}),
222+
union: false,
223+
224+
expectedReconciledRole: role(rules("pods", "nodes", "secrets"), ss{"color": "red", "team": "pm"}, ss{"description": "fancy", "system": "false", "owner": "admin", "vip": "yes"}),
225+
expectedReconciliationNeeded: true,
226+
},
227+
"complex labels/annotations and rules with union": {
228+
expectedRole: role(rules("pods", "nodes", "secrets"), ss{"env": "prod", "color": "blue", "manager": "randy"}, ss{"description": "fancy", "system": "true", "up": "true"}),
229+
actualRole: role(rules("nodes", "images", "projects"), ss{"color": "red", "team": "pm"}, ss{"system": "false", "owner": "admin", "vip": "yes", "rate": "down"}),
230+
union: true,
231+
232+
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"}),
233+
expectedReconciliationNeeded: true,
234+
},
235+
}
236+
237+
for k, tc := range tests {
238+
reconciledRole, reconciliationNeeded := computeReconciledRole(*tc.expectedRole, *tc.actualRole, tc.union)
239+
if reconciliationNeeded != tc.expectedReconciliationNeeded {
240+
t.Errorf("%s: Expected\n\t%v\ngot\n\t%v", k, tc.expectedReconciliationNeeded, reconciliationNeeded)
241+
}
242+
if !kapi.Semantic.DeepEqual(reconciledRole, tc.expectedReconciledRole) {
243+
t.Errorf("%s: Expected\n\t%#v\ngot\n\t%#v", k, tc.expectedReconciledRole, reconciledRole)
244+
}
245+
}
246+
}

‎pkg/cmd/admin/project/new_project.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
1212
errorsutil "k8s.io/kubernetes/pkg/util/errors"
1313

14+
oapi "github.com/openshift/origin/pkg/api"
1415
"github.com/openshift/origin/pkg/client"
1516
"github.com/openshift/origin/pkg/cmd/admin/policy"
1617
"github.com/openshift/origin/pkg/cmd/server/bootstrappolicy"
@@ -99,8 +100,8 @@ func (o *NewProjectOptions) Run(useNodeSelector bool) error {
99100
project := &projectapi.Project{}
100101
project.Name = o.ProjectName
101102
project.Annotations = make(map[string]string)
102-
project.Annotations[projectapi.ProjectDescription] = o.Description
103-
project.Annotations[projectapi.ProjectDisplayName] = o.DisplayName
103+
project.Annotations[oapi.OpenShiftDescription] = o.Description
104+
project.Annotations[oapi.OpenShiftDisplayName] = o.DisplayName
104105
if useNodeSelector {
105106
project.Annotations[projectapi.ProjectNodeSelector] = o.NodeSelector
106107
}

‎pkg/cmd/cli/cmd/project.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"github.com/openshift/origin/pkg/cmd/templates"
2020
"github.com/openshift/origin/pkg/cmd/util/clientcmd"
2121
"github.com/openshift/origin/pkg/project/api"
22+
projectapihelpers "github.com/openshift/origin/pkg/project/api/helpers"
2223
projectutil "github.com/openshift/origin/pkg/project/util"
2324

2425
"github.com/spf13/cobra"
@@ -210,11 +211,11 @@ func (o ProjectOptions) RunProject() error {
210211
case 0:
211212
msg += "\nYou are not a member of any projects. You can request a project to be created with the 'new-project' command."
212213
case 1:
213-
msg += fmt.Sprintf("\nYou have one project on this server: %s", api.DisplayNameAndNameForProject(&projects[0]))
214+
msg += fmt.Sprintf("\nYou have one project on this server: %s", projectapihelpers.DisplayNameAndNameForProject(&projects[0]))
214215
default:
215216
msg += "\nYour projects are:"
216217
for _, project := range projects {
217-
msg += fmt.Sprintf("\n* %s", api.DisplayNameAndNameForProject(&project))
218+
msg += fmt.Sprintf("\n* %s", projectapihelpers.DisplayNameAndNameForProject(&project))
218219
}
219220
}
220221
}

0 commit comments

Comments
 (0)