diff --git a/lib/resourceapply/admissionregistration.go b/lib/resourceapply/admissionregistration.go index faceeb35b9..e99589f39a 100644 --- a/lib/resourceapply/admissionregistration.go +++ b/lib/resourceapply/admissionregistration.go @@ -3,8 +3,6 @@ package resourceapply import ( "context" - "github.com/google/go-cmp/cmp" - admissionregv1 "k8s.io/api/admissionregistration/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -38,7 +36,7 @@ func ApplyValidatingWebhookConfigurationv1(ctx context.Context, client admission return existing, false, nil } if reconciling { - if diff := cmp.Diff(&original, existing); diff != "" { + if diff := ManifestDiff(&original, existing); diff != "" { klog.V(2).Infof("Updating ValidatingWebhookConfiguration %s/%s due to diff: %v", required.Namespace, required.Name, diff) } else { klog.V(2).Infof("Updating ValidatingWebhookConfiguration %s/%s with empty diff: possible hotloop after wrong comparison", required.Namespace, required.Name) diff --git a/lib/resourceapply/apiext.go b/lib/resourceapply/apiext.go index bb51453a54..e8e1ed0108 100644 --- a/lib/resourceapply/apiext.go +++ b/lib/resourceapply/apiext.go @@ -3,8 +3,6 @@ package resourceapply import ( "context" - "github.com/google/go-cmp/cmp" - apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apiextclientv1 "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -39,7 +37,7 @@ func ApplyCustomResourceDefinitionv1(ctx context.Context, client apiextclientv1. return existing, false, nil } if reconciling { - if diff := cmp.Diff(&original, existing); diff != "" { + if diff := ManifestDiff(&original, existing); diff != "" { klog.V(2).Infof("Updating CRD %s due to diff: %v", required.Name, diff) } else { klog.V(2).Infof("Updating CRD %s with empty diff: possible hotloop after wrong comparison", required.Name) diff --git a/lib/resourceapply/apps.go b/lib/resourceapply/apps.go index 6c8d42dca1..04f41c10c7 100644 --- a/lib/resourceapply/apps.go +++ b/lib/resourceapply/apps.go @@ -3,8 +3,6 @@ package resourceapply import ( "context" - "github.com/google/go-cmp/cmp" - appsv1 "k8s.io/api/apps/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -39,7 +37,7 @@ func ApplyDeploymentv1(ctx context.Context, client appsclientv1.DeploymentsGette return existing, false, nil } if reconciling { - if diff := cmp.Diff(&original, existing); diff != "" { + if diff := ManifestDiff(&original, existing); diff != "" { klog.V(2).Infof("Updating Deployment %s/%s due to diff: %v", required.Namespace, required.Name, diff) } else { klog.V(2).Infof("Updating Deployment %s/%s with empty diff: possible hotloop after wrong comparison", required.Namespace, required.Name) @@ -66,6 +64,8 @@ func ApplyDaemonSetv1(ctx context.Context, client appsclientv1.DaemonSetsGetter, return nil, false, nil } + var original appsv1.DaemonSet + existing.DeepCopyInto(&original) modified := ptr.To(false) resourcemerge.EnsureDaemonSet(modified, existing, *required) if !*modified { @@ -73,7 +73,7 @@ func ApplyDaemonSetv1(ctx context.Context, client appsclientv1.DaemonSetsGetter, } if reconciling { - klog.V(2).Infof("Updating DaemonSet %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required)) + klog.V(2).Infof("Updating DaemonSet %s/%s due to diff: %v", required.Namespace, required.Name, ManifestDiff(&original, existing)) } actual, err := client.DaemonSets(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) diff --git a/lib/resourceapply/batch.go b/lib/resourceapply/batch.go index f56fffe1c4..69f6427cd9 100644 --- a/lib/resourceapply/batch.go +++ b/lib/resourceapply/batch.go @@ -3,8 +3,6 @@ package resourceapply import ( "context" - "github.com/google/go-cmp/cmp" - batchv1 "k8s.io/api/batch/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -39,7 +37,7 @@ func ApplyJobv1(ctx context.Context, client batchclientv1.JobsGetter, required * return existing, false, nil } if reconciling { - if diff := cmp.Diff(&original, existing); diff != "" { + if diff := ManifestDiff(&original, existing); diff != "" { klog.V(2).Infof("Updating Job %s/%s due to diff: %v", required.Namespace, required.Name, diff) } else { klog.V(2).Infof("Updating Job %s/%s with empty diff: possible hotloop after wrong comparison", required.Namespace, required.Name) @@ -74,7 +72,7 @@ func ApplyCronJobv1(ctx context.Context, client batchclientv1.CronJobsGetter, re return existing, false, nil } if reconciling { - if diff := cmp.Diff(&original, existing); diff != "" { + if diff := ManifestDiff(&original, existing); diff != "" { klog.V(2).Infof("Updating CronJob %s/%s due to diff: %v", required.Namespace, required.Name, diff) } else { klog.V(2).Infof("Updating CronJob %s/%s with empty diff: possible hotloop after wrong comparison", required.Namespace, required.Name) diff --git a/lib/resourceapply/core.go b/lib/resourceapply/core.go index aa4a9dc69c..26d90fa858 100644 --- a/lib/resourceapply/core.go +++ b/lib/resourceapply/core.go @@ -3,8 +3,6 @@ package resourceapply import ( "context" - "github.com/google/go-cmp/cmp" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -32,13 +30,15 @@ func ApplyNamespacev1(ctx context.Context, client coreclientv1.NamespacesGetter, return nil, false, nil } + var original corev1.Namespace + existing.DeepCopyInto(&original) modified := ptr.To(false) resourcemerge.EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta) if !*modified { return existing, false, nil } if reconciling { - klog.V(2).Infof("Updating Namespace %s due to diff: %v", required.Name, cmp.Diff(existing, required)) + klog.V(2).Infof("Updating Namespace %s due to diff: %v", required.Name, ManifestDiff(&original, existing)) } actual, err := client.Namespaces().Update(ctx, existing, metav1.UpdateOptions{}) @@ -63,6 +63,8 @@ func ApplyServicev1(ctx context.Context, client coreclientv1.ServicesGetter, req return nil, false, nil } + var original corev1.Service + existing.DeepCopyInto(&original) modified := ptr.To(false) resourcemerge.EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta) resourcemerge.EnsureServicePorts(modified, &existing.Spec.Ports, required.Spec.Ports) @@ -75,7 +77,7 @@ func ApplyServicev1(ctx context.Context, client coreclientv1.ServicesGetter, req existing.Spec.Selector = required.Spec.Selector if reconciling { - klog.V(2).Infof("Updating Service %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required)) + klog.V(2).Infof("Updating Service %s/%s due to diff: %v", required.Namespace, required.Name, ManifestDiff(&original, existing)) } actual, err := client.Services(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) @@ -98,6 +100,8 @@ func ApplyServiceAccountv1(ctx context.Context, client coreclientv1.ServiceAccou return nil, false, nil } + var original corev1.ServiceAccount + existing.DeepCopyInto(&original) modified := ptr.To(false) resourcemerge.EnsureServiceAccount(modified, existing, *required) if !*modified { @@ -105,7 +109,7 @@ func ApplyServiceAccountv1(ctx context.Context, client coreclientv1.ServiceAccou } if reconciling { - klog.V(2).Infof("Updating ServiceAccount %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required)) + klog.V(2).Infof("Updating ServiceAccount %s/%s due to diff: %v", required.Namespace, required.Name, ManifestDiff(&original, existing)) } actual, err := client.ServiceAccounts(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) @@ -128,6 +132,8 @@ func ApplyConfigMapv1(ctx context.Context, client coreclientv1.ConfigMapsGetter, return nil, false, nil } + var original corev1.ConfigMap + existing.DeepCopyInto(&original) modified := ptr.To(false) resourcemerge.EnsureConfigMap(modified, existing, *required) if !*modified { @@ -135,7 +141,7 @@ func ApplyConfigMapv1(ctx context.Context, client coreclientv1.ConfigMapsGetter, } if reconciling { - klog.V(2).Infof("Updating ConfigMap %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required)) + klog.V(2).Infof("Updating ConfigMap %s/%s due to diff: %v", required.Namespace, required.Name, ManifestDiff(&original, existing)) } actual, err := client.ConfigMaps(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) diff --git a/lib/resourceapply/cv.go b/lib/resourceapply/cv.go index e45ef191c0..2ccece9bc5 100644 --- a/lib/resourceapply/cv.go +++ b/lib/resourceapply/cv.go @@ -3,8 +3,6 @@ package resourceapply import ( "context" - "github.com/google/go-cmp/cmp" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/klog/v2" @@ -39,7 +37,7 @@ func ApplyClusterVersionFromCache(ctx context.Context, lister configlistersv1.Cl return existing, false, nil } - klog.V(2).Infof("Updating ClusterVersion %s due to diff: %v", required.Name, cmp.Diff(existing, required)) + klog.V(2).Infof("Updating ClusterVersion %s due to diff: %v", required.Name, ManifestDiff(obj, existing)) actual, err := client.ClusterVersions().Update(ctx, existing, metav1.UpdateOptions{}) return actual, true, err diff --git a/lib/resourceapply/imagestream.go b/lib/resourceapply/imagestream.go index 8255041071..241df9d61b 100644 --- a/lib/resourceapply/imagestream.go +++ b/lib/resourceapply/imagestream.go @@ -3,8 +3,6 @@ package resourceapply import ( "context" - "github.com/google/go-cmp/cmp" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/klog/v2" @@ -38,7 +36,7 @@ func ApplyImageStreamv1(ctx context.Context, client imageclientv1.ImageStreamsGe return existing, false, nil } if reconciling { - if diff := cmp.Diff(&original, existing); diff != "" { + if diff := ManifestDiff(&original, existing); diff != "" { klog.V(2).Infof("Updating ImageStream %s/%s due to diff: %v", required.Namespace, required.Name, diff) } else { klog.V(2).Infof("Updating ImageStream %s/%s with empty diff: possible hotloop after wrong comparison", required.Namespace, required.Name) diff --git a/lib/resourceapply/interface.go b/lib/resourceapply/interface.go index 2f648602f2..622c550894 100644 --- a/lib/resourceapply/interface.go +++ b/lib/resourceapply/interface.go @@ -3,6 +3,8 @@ package resourceapply import ( "strings" + "github.com/google/go-cmp/cmp" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -16,3 +18,30 @@ const CreateOnlyAnnotation = "release.openshift.io/create-only" func IsCreateOnly(metadata metav1.Object) bool { return strings.EqualFold(metadata.GetAnnotations()[CreateOnlyAnnotation], "true") } + +// ManifestDiff is a specialized case of cmp.Diff that ignores fields unlikely to match when one side is read +// from the APIServer (`fromKAS`) and the other from a manifest file (`fromManifest`). It is specifically +// aligned with EnsureObjectMeta when it comes to ObjectMeta. +func ManifestDiff(fromKAS, fromManifest any) string { + return cmp.Diff(fromKAS, fromManifest, cmp.FilterPath( + func(p cmp.Path) bool { + path := p.String() + if path == "TypeMeta" || path == "Status" { + // Ignore + return true + } + if strings.HasPrefix(path, "ObjectMeta.") { + if parts := strings.SplitN(path, ".", 3); len(parts) >= 2 { + switch parts[1] { + case "GenerateName", "SelfLink", "UID", "ResourceVersion", "Generation", "CreationTimestamp", "DeletionTimestamp", "DeletionGracePeriodSeconds", "Finalizers", "ManagedFields": + // Ignore + return true + } + } + // The remaining ObjectMeta fields are those curated by EnsureObjectMeta + } + return false + }, + cmp.Ignore(), + )) +} diff --git a/lib/resourceapply/operators.go b/lib/resourceapply/operators.go index 3ba05161cc..7a588c9140 100644 --- a/lib/resourceapply/operators.go +++ b/lib/resourceapply/operators.go @@ -3,7 +3,6 @@ package resourceapply import ( "context" - "github.com/google/go-cmp/cmp" operatorsv1 "github.com/operator-framework/api/pkg/operators/v1" operatorsclientv1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/typed/operators/v1" @@ -39,7 +38,7 @@ func ApplyOperatorGroupv1(ctx context.Context, client operatorsclientv1.Operator return existing, false, nil } if reconciling { - if diff := cmp.Diff(&original, existing); diff != "" { + if diff := ManifestDiff(&original, existing); diff != "" { klog.V(2).Infof("Updating OperatorGroup %s/%s due to diff: %v", required.Namespace, required.Name, diff) } else { klog.V(2).Infof("Updating OperatorGroup %s/%s with empty diff: possible hotloop after wrong comparison", required.Namespace, required.Name) diff --git a/lib/resourceapply/rbac.go b/lib/resourceapply/rbac.go index 7e463c0944..5da781a3ab 100644 --- a/lib/resourceapply/rbac.go +++ b/lib/resourceapply/rbac.go @@ -3,8 +3,6 @@ package resourceapply import ( "context" - "github.com/google/go-cmp/cmp" - rbacv1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -31,13 +29,15 @@ func ApplyClusterRoleBindingv1(ctx context.Context, client rbacclientv1.ClusterR return nil, false, nil } + var original rbacv1.ClusterRoleBinding + existing.DeepCopyInto(&original) modified := ptr.To(false) resourcemerge.EnsureClusterRoleBinding(modified, 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)) + klog.V(2).Infof("Updating ClusterRoleBinding %s due to diff: %v", required.Name, ManifestDiff(&original, existing)) } actual, err := client.ClusterRoleBindings().Update(ctx, existing, metav1.UpdateOptions{}) @@ -60,13 +60,15 @@ func ApplyClusterRolev1(ctx context.Context, client rbacclientv1.ClusterRolesGet return nil, false, nil } + var original rbacv1.ClusterRole + existing.DeepCopyInto(&original) modified := ptr.To(false) resourcemerge.EnsureClusterRole(modified, 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)) + klog.V(2).Infof("Updating ClusterRole %s due to diff: %v", required.Name, ManifestDiff(&original, existing)) } actual, err := client.ClusterRoles().Update(ctx, existing, metav1.UpdateOptions{}) @@ -89,13 +91,15 @@ func ApplyRoleBindingv1(ctx context.Context, client rbacclientv1.RoleBindingsGet return nil, false, nil } + var original rbacv1.RoleBinding + existing.DeepCopyInto(&original) modified := ptr.To(false) resourcemerge.EnsureRoleBinding(modified, 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)) + klog.V(2).Infof("Updating RoleBinding %s/%s due to diff: %v", required.Namespace, required.Name, ManifestDiff(&original, existing)) } actual, err := client.RoleBindings(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) @@ -118,13 +122,15 @@ func ApplyRolev1(ctx context.Context, client rbacclientv1.RolesGetter, required return nil, false, nil } + var original rbacv1.Role + existing.DeepCopyInto(&original) modified := ptr.To(false) resourcemerge.EnsureRole(modified, 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)) + klog.V(2).Infof("Updating Role %s/%s due to diff: %v", required.Namespace, required.Name, ManifestDiff(&original, existing)) } actual, err := client.Roles(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) diff --git a/lib/resourceapply/security.go b/lib/resourceapply/security.go index 0538c0e879..9ce11b0472 100644 --- a/lib/resourceapply/security.go +++ b/lib/resourceapply/security.go @@ -3,8 +3,6 @@ package resourceapply import ( "context" - "github.com/google/go-cmp/cmp" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/klog/v2" @@ -31,13 +29,15 @@ func ApplySecurityContextConstraintsv1(ctx context.Context, client securityclien return nil, false, nil } + var original securityv1.SecurityContextConstraints + existing.DeepCopyInto(&original) reconcile := resourcemerge.EnsureSecurityContextConstraints(*existing, *required) if reconcile == nil { return existing, false, nil } if reconciling { - if diff := cmp.Diff(existing, reconcile); diff != "" { + if diff := ManifestDiff(&original, reconcile); diff != "" { klog.V(2).Infof("Updating SCC %s/%s due to diff: %v", required.Namespace, required.Name, diff) } else { klog.V(2).Infof("Updating SCC %s/%s with empty diff: possible hotloop after wrong comparison", required.Namespace, required.Name) diff --git a/pkg/cvo/internal/generic.go b/pkg/cvo/internal/generic.go index f9596fe79b..f4467caa90 100644 --- a/pkg/cvo/internal/generic.go +++ b/pkg/cvo/internal/generic.go @@ -77,10 +77,10 @@ func applyUnstructured(ctx context.Context, client dynamic.ResourceInterface, re return nil, false, nil } - skipKeys := sets.New[string]("apiVersion", "kind", "metadata", "status") + skipKeys := sets.New("apiVersion", "kind", "metadata", "status") - // create a copy of required, but copy skipKeys from existing - // this would copy skipKeys data into expected from existing + // create a copy of required, but copy skipKeys -- the parts that don't matter if they differ -- from existing + // so that our comparison only triggers on differences in the remaining fields expected := required.DeepCopy() for k, v := range existing.Object { if skipKeys.Has(k) { @@ -90,7 +90,7 @@ func applyUnstructured(ctx context.Context, client dynamic.ResourceInterface, re objDiff := cmp.Diff(expected, existing) if objDiff == "" { - // Skip update, as no changes found + // Skip update, as no relevant changes found return existing, false, nil } diff --git a/pkg/cvo/internal/operatorstatus.go b/pkg/cvo/internal/operatorstatus.go index d33b8dd914..b2342825a7 100644 --- a/pkg/cvo/internal/operatorstatus.go +++ b/pkg/cvo/internal/operatorstatus.go @@ -6,8 +6,6 @@ import ( "sort" "strings" - "github.com/google/go-cmp/cmp" - kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -19,6 +17,7 @@ import ( configclientv1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" "github.com/openshift/library-go/pkg/manifest" + "github.com/openshift/cluster-version-operator/lib/resourceapply" "github.com/openshift/cluster-version-operator/lib/resourcebuilder" "github.com/openshift/cluster-version-operator/lib/resourcemerge" "github.com/openshift/cluster-version-operator/pkg/payload" @@ -134,7 +133,7 @@ func (b *clusterOperatorBuilder) Do(ctx context.Context) error { var modified bool resourcemerge.EnsureObjectMeta(&modified, &existing.ObjectMeta, co.ObjectMeta) if modified { - if diff := cmp.Diff(&original, existing); diff != "" { + if diff := resourceapply.ManifestDiff(&original, existing); diff != "" { klog.V(2).Infof("Updating ClusterOperator metadata %s due to diff: %v", co.Name, diff) } else { klog.V(2).Infof("Updating ClusterOperator metadata %s with empty diff: possible hotloop after wrong comparison", co.Name)