Skip to content

Commit 78ce949

Browse files
petr-mullerpetr-muller
committed
NO-JIRA: resourceapply/rbac: fix resource diff logs
Previously the logs were wrong and misleading because they were created by diffing a resource _after modification_ which means the actual change is never shown in the diff. These diffs should be created by comparing the resource before and after it is modified. Partially related to OCPBUGS-36246 but is just a prep for an eventual fix, so marking as NO-JIRA.
1 parent cada4f4 commit 78ce949

File tree

3 files changed

+78
-42
lines changed

3 files changed

+78
-42
lines changed

‎lib/resourceapply/rbac.go

Lines changed: 44 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1111
rbacclientv1 "k8s.io/client-go/kubernetes/typed/rbac/v1"
1212
"k8s.io/klog/v2"
13-
"k8s.io/utils/ptr"
1413
)
1514

1615
// ApplyClusterRoleBindingv1 applies the required clusterrolebinding to the cluster.
@@ -29,13 +28,20 @@ func ApplyClusterRoleBindingv1(ctx context.Context, client rbacclientv1.ClusterR
2928
return nil, false, nil
3029
}
3130

32-
modified := ptr.To(false)
33-
resourcemerge.EnsureClusterRoleBinding(modified, existing, *required)
34-
if !*modified {
31+
var original rbacv1.ClusterRoleBinding
32+
existing.DeepCopyInto(&original)
33+
34+
modified := resourcemerge.EnsureClusterRoleBinding(existing, *required)
35+
if !modified {
3536
return existing, false, nil
3637
}
38+
3739
if reconciling {
38-
klog.V(2).Infof("Updating ClusterRoleBinding %s due to diff: %v", required.Name, cmp.Diff(existing, required))
40+
if diff := cmp.Diff(&original, existing); diff != "" {
41+
klog.V(2).Infof("Updating ClusterRoleBinding %s due to diff: %v", required.Name, diff)
42+
} else {
43+
klog.V(2).Infof("Updating ClusterRoleBinding %s with empty diff: possible hotloop after wrong comparison", required.Name)
44+
}
3945
}
4046

4147
actual, err := client.ClusterRoleBindings().Update(ctx, existing, metav1.UpdateOptions{})
@@ -58,13 +64,20 @@ func ApplyClusterRolev1(ctx context.Context, client rbacclientv1.ClusterRolesGet
5864
return nil, false, nil
5965
}
6066

61-
modified := ptr.To(false)
62-
resourcemerge.EnsureClusterRole(modified, existing, *required)
63-
if !*modified {
67+
var original rbacv1.ClusterRole
68+
existing.DeepCopyInto(&original)
69+
70+
modified := resourcemerge.EnsureClusterRole(existing, *required)
71+
if !modified {
6472
return existing, false, nil
6573
}
74+
6675
if reconciling {
67-
klog.V(2).Infof("Updating ClusterRole %s due to diff: %v", required.Name, cmp.Diff(existing, required))
76+
if diff := cmp.Diff(&original, existing); diff != "" {
77+
klog.V(2).Infof("Updating ClusterRole %s due to diff: %v", required.Name, diff)
78+
} else {
79+
klog.V(2).Infof("Updating ClusterRole %s with empty diff: possible hotloop after wrong comparison", required.Name)
80+
}
6881
}
6982

7083
actual, err := client.ClusterRoles().Update(ctx, existing, metav1.UpdateOptions{})
@@ -87,13 +100,20 @@ func ApplyRoleBindingv1(ctx context.Context, client rbacclientv1.RoleBindingsGet
87100
return nil, false, nil
88101
}
89102

90-
modified := ptr.To(false)
91-
resourcemerge.EnsureRoleBinding(modified, existing, *required)
92-
if !*modified {
103+
var original rbacv1.RoleBinding
104+
existing.DeepCopyInto(&original)
105+
106+
modified := resourcemerge.EnsureRoleBinding(existing, *required)
107+
if !modified {
93108
return existing, false, nil
94109
}
110+
95111
if reconciling {
96-
klog.V(2).Infof("Updating RoleBinding %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required))
112+
if diff := cmp.Diff(&original, existing); diff != "" {
113+
klog.V(2).Infof("Updating RoleBinding %s due to diff: %v", required.Name, diff)
114+
} else {
115+
klog.V(2).Infof("Updating RoleBinding %s with empty diff: possible hotloop after wrong comparison", required.Name)
116+
}
97117
}
98118

99119
actual, err := client.RoleBindings(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{})
@@ -116,13 +136,20 @@ func ApplyRolev1(ctx context.Context, client rbacclientv1.RolesGetter, required
116136
return nil, false, nil
117137
}
118138

119-
modified := ptr.To(false)
120-
resourcemerge.EnsureRole(modified, existing, *required)
121-
if !*modified {
139+
var original rbacv1.Role
140+
original.DeepCopyInto(&original)
141+
142+
modified := resourcemerge.EnsureRole(existing, *required)
143+
if !modified {
122144
return existing, false, nil
123145
}
146+
124147
if reconciling {
125-
klog.V(2).Infof("Updating Role %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required))
148+
if diff := cmp.Diff(&original, existing); diff != "" {
149+
klog.V(2).Infof("Updating Role %s due to diff: %v", required.Name, diff)
150+
} else {
151+
klog.V(2).Infof("Updating Role %s with empty diff: possible hotloop after wrong comparison", required.Name)
152+
}
126153
}
127154

128155
actual, err := client.Roles(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{})

‎lib/resourcemerge/rbac.go

Lines changed: 31 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -6,52 +6,60 @@ import (
66
)
77

88
// EnsureClusterRoleBinding ensures that the existing matches the required.
9-
// modified is set to true when existing had to be updated with required.
10-
func EnsureClusterRoleBinding(modified *bool, existing *rbacv1.ClusterRoleBinding, required rbacv1.ClusterRoleBinding) {
11-
EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta)
9+
// Returns true when existing had to be updated with required.
10+
func EnsureClusterRoleBinding(existing *rbacv1.ClusterRoleBinding, required rbacv1.ClusterRoleBinding) bool {
11+
var modified bool
12+
EnsureObjectMeta(&modified, &existing.ObjectMeta, required.ObjectMeta)
1213
ensureRoleRefDefaultsv1(&required.RoleRef)
1314
if !equality.Semantic.DeepEqual(existing.Subjects, required.Subjects) {
14-
*modified = true
15+
modified = true
1516
existing.Subjects = required.Subjects
1617
}
1718
if !equality.Semantic.DeepEqual(existing.RoleRef, required.RoleRef) {
18-
*modified = true
19+
modified = true
1920
existing.RoleRef = required.RoleRef
2021
}
22+
23+
return modified
2124
}
2225

2326
// EnsureClusterRole ensures that the existing matches the required.
24-
// modified is set to true when existing had to be updated with required.
25-
func EnsureClusterRole(modified *bool, existing *rbacv1.ClusterRole, required rbacv1.ClusterRole) {
26-
EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta)
27+
// Returns true when existing had to be updated with required.
28+
func EnsureClusterRole(existing *rbacv1.ClusterRole, required rbacv1.ClusterRole) bool {
29+
var modified bool
30+
EnsureObjectMeta(&modified, &existing.ObjectMeta, required.ObjectMeta)
2731
if !equality.Semantic.DeepEqual(existing.AggregationRule, required.AggregationRule) {
28-
*modified = true
32+
modified = true
2933
existing.AggregationRule = required.AggregationRule
3034
}
3135
if required.AggregationRule != nil {
3236
// The control plane overwrites any values that are manually specified in the rules field of an aggregate ClusterRole.
3337
// Skip reconciling on Rules field
34-
return
38+
return modified
3539
}
3640
if !equality.Semantic.DeepEqual(existing.Rules, required.Rules) {
37-
*modified = true
41+
modified = true
3842
existing.Rules = required.Rules
3943
}
44+
return modified
4045
}
4146

4247
// EnsureRoleBinding ensures that the existing matches the required.
43-
// modified is set to true when existing had to be updated with required.
44-
func EnsureRoleBinding(modified *bool, existing *rbacv1.RoleBinding, required rbacv1.RoleBinding) {
45-
EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta)
48+
// Returns true when existing had to be updated with required.
49+
func EnsureRoleBinding(existing *rbacv1.RoleBinding, required rbacv1.RoleBinding) bool {
50+
var modified bool
51+
EnsureObjectMeta(&modified, &existing.ObjectMeta, required.ObjectMeta)
4652
ensureRoleRefDefaultsv1(&required.RoleRef)
4753
if !equality.Semantic.DeepEqual(existing.Subjects, required.Subjects) {
48-
*modified = true
54+
modified = true
4955
existing.Subjects = required.Subjects
5056
}
5157
if !equality.Semantic.DeepEqual(existing.RoleRef, required.RoleRef) {
52-
*modified = true
58+
modified = true
5359
existing.RoleRef = required.RoleRef
5460
}
61+
62+
return modified
5563
}
5664

5765
func ensureRoleRefDefaultsv1(roleRef *rbacv1.RoleRef) {
@@ -61,11 +69,14 @@ func ensureRoleRefDefaultsv1(roleRef *rbacv1.RoleRef) {
6169
}
6270

6371
// EnsureRole ensures that the existing matches the required.
64-
// modified is set to true when existing had to be updated with required.
65-
func EnsureRole(modified *bool, existing *rbacv1.Role, required rbacv1.Role) {
66-
EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta)
72+
// Returns true when existing had to be updated with required.
73+
func EnsureRole(existing *rbacv1.Role, required rbacv1.Role) bool {
74+
var modified bool
75+
EnsureObjectMeta(&modified, &existing.ObjectMeta, required.ObjectMeta)
6776
if !equality.Semantic.DeepEqual(existing.Rules, required.Rules) {
68-
*modified = true
77+
modified = true
6978
existing.Rules = required.Rules
7079
}
80+
81+
return modified
7182
}

‎lib/resourcemerge/rbac_test.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import (
66
"github.com/google/go-cmp/cmp"
77
rbacv1 "k8s.io/api/rbac/v1"
88
"k8s.io/apimachinery/pkg/api/equality"
9-
"k8s.io/utils/ptr"
109
)
1110

1211
func TestEnsureClusterRole2Bindingsv1(t *testing.T) {
@@ -264,10 +263,9 @@ func TestEnsureClusterRole2Bindingsv1(t *testing.T) {
264263

265264
for _, test := range tests {
266265
t.Run(test.name, func(t *testing.T) {
267-
modified := ptr.To(false)
268-
EnsureClusterRoleBinding(modified, &test.existing, test.input)
269-
if *modified != test.expectedModified {
270-
t.Errorf("mismatch modified got: %v want: %v", *modified, test.expectedModified)
266+
modified := EnsureClusterRoleBinding(&test.existing, test.input)
267+
if modified != test.expectedModified {
268+
t.Errorf("mismatch modified got: %v want: %v", modified, test.expectedModified)
271269
}
272270

273271
if !equality.Semantic.DeepEqual(test.existing, test.expected) {

0 commit comments

Comments
 (0)