From 78ce949c270dd0a839097d7c97f36dd877ebd721 Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Fri, 20 Jun 2025 15:06:19 +0200 Subject: [PATCH] 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. --- lib/resourceapply/rbac.go | 61 ++++++++++++++++++++++++---------- lib/resourcemerge/rbac.go | 51 +++++++++++++++++----------- lib/resourcemerge/rbac_test.go | 8 ++--- 3 files changed, 78 insertions(+), 42 deletions(-) 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) {