diff --git a/lib/resourceapply/rbac.go b/lib/resourceapply/rbac.go index 697fac52c..bef941376 100644 --- a/lib/resourceapply/rbac.go +++ b/lib/resourceapply/rbac.go @@ -10,7 +10,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" rbacclientv1 "k8s.io/client-go/kubernetes/typed/rbac/v1" "k8s.io/klog/v2" - "k8s.io/utils/ptr" ) // ApplyClusterRoleBindingv1 applies the required clusterrolebinding to the cluster. @@ -29,13 +28,20 @@ func ApplyClusterRoleBindingv1(ctx context.Context, client rbacclientv1.ClusterR return nil, false, nil } - modified := ptr.To(false) - resourcemerge.EnsureClusterRoleBinding(modified, existing, *required) - if !*modified { + var original rbacv1.ClusterRoleBinding + existing.DeepCopyInto(&original) + + modified := resourcemerge.EnsureClusterRoleBinding(existing, *required) + if !modified { return existing, false, nil } + if reconciling { - klog.V(2).Infof("Updating ClusterRoleBinding %s due to diff: %v", required.Name, cmp.Diff(existing, required)) + if diff := cmp.Diff(&original, existing); diff != "" { + klog.V(2).Infof("Updating ClusterRoleBinding %s due to diff: %v", required.Name, diff) + } else { + klog.V(2).Infof("Updating ClusterRoleBinding %s with empty diff: possible hotloop after wrong comparison", required.Name) + } } actual, err := client.ClusterRoleBindings().Update(ctx, existing, metav1.UpdateOptions{}) @@ -58,13 +64,20 @@ func ApplyClusterRolev1(ctx context.Context, client rbacclientv1.ClusterRolesGet return nil, false, nil } - modified := ptr.To(false) - resourcemerge.EnsureClusterRole(modified, existing, *required) - if !*modified { + var original rbacv1.ClusterRole + existing.DeepCopyInto(&original) + + modified := resourcemerge.EnsureClusterRole(existing, *required) + if !modified { return existing, false, nil } + if reconciling { - klog.V(2).Infof("Updating ClusterRole %s due to diff: %v", required.Name, cmp.Diff(existing, required)) + if diff := cmp.Diff(&original, existing); diff != "" { + klog.V(2).Infof("Updating ClusterRole %s due to diff: %v", required.Name, diff) + } else { + klog.V(2).Infof("Updating ClusterRole %s with empty diff: possible hotloop after wrong comparison", required.Name) + } } actual, err := client.ClusterRoles().Update(ctx, existing, metav1.UpdateOptions{}) @@ -87,13 +100,20 @@ func ApplyRoleBindingv1(ctx context.Context, client rbacclientv1.RoleBindingsGet return nil, false, nil } - modified := ptr.To(false) - resourcemerge.EnsureRoleBinding(modified, existing, *required) - if !*modified { + var original rbacv1.RoleBinding + existing.DeepCopyInto(&original) + + modified := resourcemerge.EnsureRoleBinding(existing, *required) + if !modified { return existing, false, nil } + if reconciling { - klog.V(2).Infof("Updating RoleBinding %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required)) + if diff := cmp.Diff(&original, existing); diff != "" { + klog.V(2).Infof("Updating RoleBinding %s due to diff: %v", required.Name, diff) + } else { + klog.V(2).Infof("Updating RoleBinding %s with empty diff: possible hotloop after wrong comparison", required.Name) + } } actual, err := client.RoleBindings(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) @@ -116,13 +136,20 @@ func ApplyRolev1(ctx context.Context, client rbacclientv1.RolesGetter, required return nil, false, nil } - modified := ptr.To(false) - resourcemerge.EnsureRole(modified, existing, *required) - if !*modified { + var original rbacv1.Role + original.DeepCopyInto(&original) + + modified := resourcemerge.EnsureRole(existing, *required) + if !modified { return existing, false, nil } + if reconciling { - klog.V(2).Infof("Updating Role %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required)) + if diff := cmp.Diff(&original, existing); diff != "" { + klog.V(2).Infof("Updating Role %s due to diff: %v", required.Name, diff) + } else { + klog.V(2).Infof("Updating Role %s with empty diff: possible hotloop after wrong comparison", required.Name) + } } actual, err := client.Roles(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) diff --git a/lib/resourcemerge/rbac.go b/lib/resourcemerge/rbac.go index 670f766f2..fbeac0f8e 100644 --- a/lib/resourcemerge/rbac.go +++ b/lib/resourcemerge/rbac.go @@ -6,52 +6,60 @@ import ( ) // EnsureClusterRoleBinding ensures that the existing matches the required. -// modified is set to true when existing had to be updated with required. -func EnsureClusterRoleBinding(modified *bool, existing *rbacv1.ClusterRoleBinding, required rbacv1.ClusterRoleBinding) { - EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta) +// Returns true when existing had to be updated with required. +func EnsureClusterRoleBinding(existing *rbacv1.ClusterRoleBinding, required rbacv1.ClusterRoleBinding) bool { + var modified bool + EnsureObjectMeta(&modified, &existing.ObjectMeta, required.ObjectMeta) ensureRoleRefDefaultsv1(&required.RoleRef) if !equality.Semantic.DeepEqual(existing.Subjects, required.Subjects) { - *modified = true + modified = true existing.Subjects = required.Subjects } if !equality.Semantic.DeepEqual(existing.RoleRef, required.RoleRef) { - *modified = true + modified = true existing.RoleRef = required.RoleRef } + + return modified } // EnsureClusterRole ensures that the existing matches the required. -// modified is set to true when existing had to be updated with required. -func EnsureClusterRole(modified *bool, existing *rbacv1.ClusterRole, required rbacv1.ClusterRole) { - EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta) +// Returns true when existing had to be updated with required. +func EnsureClusterRole(existing *rbacv1.ClusterRole, required rbacv1.ClusterRole) bool { + var modified bool + EnsureObjectMeta(&modified, &existing.ObjectMeta, required.ObjectMeta) if !equality.Semantic.DeepEqual(existing.AggregationRule, required.AggregationRule) { - *modified = true + modified = true existing.AggregationRule = required.AggregationRule } if required.AggregationRule != nil { // The control plane overwrites any values that are manually specified in the rules field of an aggregate ClusterRole. // Skip reconciling on Rules field - return + return modified } if !equality.Semantic.DeepEqual(existing.Rules, required.Rules) { - *modified = true + modified = true existing.Rules = required.Rules } + return modified } // EnsureRoleBinding ensures that the existing matches the required. -// modified is set to true when existing had to be updated with required. -func EnsureRoleBinding(modified *bool, existing *rbacv1.RoleBinding, required rbacv1.RoleBinding) { - EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta) +// Returns true when existing had to be updated with required. +func EnsureRoleBinding(existing *rbacv1.RoleBinding, required rbacv1.RoleBinding) bool { + var modified bool + EnsureObjectMeta(&modified, &existing.ObjectMeta, required.ObjectMeta) ensureRoleRefDefaultsv1(&required.RoleRef) if !equality.Semantic.DeepEqual(existing.Subjects, required.Subjects) { - *modified = true + modified = true existing.Subjects = required.Subjects } if !equality.Semantic.DeepEqual(existing.RoleRef, required.RoleRef) { - *modified = true + modified = true existing.RoleRef = required.RoleRef } + + return modified } func ensureRoleRefDefaultsv1(roleRef *rbacv1.RoleRef) { @@ -61,11 +69,14 @@ func ensureRoleRefDefaultsv1(roleRef *rbacv1.RoleRef) { } // EnsureRole ensures that the existing matches the required. -// modified is set to true when existing had to be updated with required. -func EnsureRole(modified *bool, existing *rbacv1.Role, required rbacv1.Role) { - EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta) +// Returns true when existing had to be updated with required. +func EnsureRole(existing *rbacv1.Role, required rbacv1.Role) bool { + var modified bool + EnsureObjectMeta(&modified, &existing.ObjectMeta, required.ObjectMeta) if !equality.Semantic.DeepEqual(existing.Rules, required.Rules) { - *modified = true + modified = true existing.Rules = required.Rules } + + return modified } diff --git a/lib/resourcemerge/rbac_test.go b/lib/resourcemerge/rbac_test.go index 82f78f559..f5c2f0bee 100644 --- a/lib/resourcemerge/rbac_test.go +++ b/lib/resourcemerge/rbac_test.go @@ -6,7 +6,6 @@ import ( "github.com/google/go-cmp/cmp" rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/equality" - "k8s.io/utils/ptr" ) func TestEnsureClusterRole2Bindingsv1(t *testing.T) { @@ -264,10 +263,9 @@ func TestEnsureClusterRole2Bindingsv1(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - modified := ptr.To(false) - EnsureClusterRoleBinding(modified, &test.existing, test.input) - if *modified != test.expectedModified { - t.Errorf("mismatch modified got: %v want: %v", *modified, test.expectedModified) + modified := EnsureClusterRoleBinding(&test.existing, test.input) + if modified != test.expectedModified { + t.Errorf("mismatch modified got: %v want: %v", modified, test.expectedModified) } if !equality.Semantic.DeepEqual(test.existing, test.expected) {