Skip to content

OCPBUGS-22456: resourceapply/rbac: fix resource diff logs #1205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

OCPBUGS-22456: resourceapply/rbac: fix resource diff logs #1205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 44 additions & 17 deletions lib/resourceapply/rbac.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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{})
Expand All @@ -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{})
Expand All @@ -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{})
Expand All @@ -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{})
Expand Down
51 changes: 31 additions & 20 deletions lib/resourcemerge/rbac.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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
}
8 changes: 3 additions & 5 deletions lib/resourcemerge/rbac_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down