From 36a0662531e9b0ebb5860d0b9c0c14474118a4f2 Mon Sep 17 00:00:00 2001 From: Daniel Lipovetsky Date: Thu, 25 Apr 2024 09:50:12 -0700 Subject: [PATCH] feat: Preserve user-managed fields when applying resources (#556) **What problem does this PR solve?**: We want to preserve user-managed fields when we create/update specific resources. If a value in a user-managed field conflicts with our value, we want to alert the user by returning an error, rather than overwriting the value. This PR exposes the `ForceOwnership` option to callers, but does not change the behavior of existing callers. **Which issue(s) this PR fixes**: Fixes # **How Has This Been Tested?**: **Special notes for your reviewer**: --- common/pkg/k8s/client/apply.go | 34 +++++++++++-------- .../generic/lifecycle/ccm/aws/handler.go | 2 +- .../generic/lifecycle/ccm/nutanix/handler.go | 2 +- .../clusterautoscaler/strategy_crs.go | 2 +- .../clusterautoscaler/strategy_helmaddon.go | 2 +- .../lifecycle/cni/calico/strategy_crs.go | 4 +-- .../cni/calico/strategy_helmaddon.go | 2 +- .../lifecycle/cni/cilium/strategy_crs.go | 2 +- .../cni/cilium/strategy_helmaddon.go | 2 +- .../generic/lifecycle/csi/aws-ebs/handler.go | 2 +- .../lifecycle/csi/nutanix-csi/handler.go | 4 +-- .../generic/lifecycle/nfd/strategy_crs.go | 2 +- .../lifecycle/nfd/strategy_helmaddon.go | 2 +- pkg/handlers/generic/lifecycle/utils/scs.go | 2 +- .../generic/lifecycle/utils/secrets.go | 2 +- pkg/handlers/generic/lifecycle/utils/utils.go | 4 +-- .../imageregistries/credentials/inject.go | 2 +- 17 files changed, 38 insertions(+), 34 deletions(-) diff --git a/common/pkg/k8s/client/apply.go b/common/pkg/k8s/client/apply.go index a89982d95..bbfb775e5 100644 --- a/common/pkg/k8s/client/apply.go +++ b/common/pkg/k8s/client/apply.go @@ -10,25 +10,29 @@ import ( ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" ) -// ServerSideApply will apply (i.e. create or update) objects via server-side apply. This will overwrite any changes -// that have been manually applied. +const ( + // FieldOwner is the field manager name used for server-side apply. + FieldOwner = "d2iq-cluster-api-runtime-extensions-nutanix" +) + +// ForceOwnership is an convenience alias of the same option in the controller-runtime client. +var ForceOwnership = ctrlclient.ForceOwnership + +// ServerSideApply will apply (i.e. create or update) objects via server-side apply. func ServerSideApply( ctx context.Context, c ctrlclient.Client, - objs ...ctrlclient.Object, + obj ctrlclient.Object, + opts ...ctrlclient.PatchOption, ) error { - for i := range objs { - err := c.Patch( - ctx, - objs[i], - ctrlclient.Apply, - ctrlclient.ForceOwnership, - ctrlclient.FieldOwner("d2iq-cluster-api-runtime-extensions-nutanix"), - ) - if err != nil { - return fmt.Errorf("server-side apply failed: %w", err) - } + err := c.Patch( + ctx, + obj, + ctrlclient.Apply, + ctrlclient.FieldOwner(FieldOwner), + ) + if err != nil { + return fmt.Errorf("server-side apply failed: %w", err) } - return nil } diff --git a/pkg/handlers/generic/lifecycle/ccm/aws/handler.go b/pkg/handlers/generic/lifecycle/ccm/aws/handler.go index 45543edce..fcc164fca 100644 --- a/pkg/handlers/generic/lifecycle/ccm/aws/handler.go +++ b/pkg/handlers/generic/lifecycle/ccm/aws/handler.go @@ -87,7 +87,7 @@ func (a *AWSCCM) Apply( } ccmConfigMap := generateCCMConfigMapForCluster(ccmConfigMapForMinorVersion, cluster) - if err = client.ServerSideApply(ctx, a.client, ccmConfigMap); err != nil { + if err = client.ServerSideApply(ctx, a.client, ccmConfigMap, client.ForceOwnership); err != nil { log.Error(err, "failed to apply CCM configmap for cluster") return fmt.Errorf( "failed to apply AWS CCM manifests ConfigMap: %w", diff --git a/pkg/handlers/generic/lifecycle/ccm/nutanix/handler.go b/pkg/handlers/generic/lifecycle/ccm/nutanix/handler.go index 9e0d90345..25a86cc36 100644 --- a/pkg/handlers/generic/lifecycle/ccm/nutanix/handler.go +++ b/pkg/handlers/generic/lifecycle/ccm/nutanix/handler.go @@ -157,7 +157,7 @@ func (p *provider) Apply( ) } - if err = client.ServerSideApply(ctx, p.client, hcp); err != nil { + if err = client.ServerSideApply(ctx, p.client, hcp, client.ForceOwnership); err != nil { return fmt.Errorf("failed to apply nutanix-ccm installation HelmChartProxy: %w", err) } diff --git a/pkg/handlers/generic/lifecycle/clusterautoscaler/strategy_crs.go b/pkg/handlers/generic/lifecycle/clusterautoscaler/strategy_crs.go index 52019216d..27ed3d7c2 100644 --- a/pkg/handlers/generic/lifecycle/clusterautoscaler/strategy_crs.go +++ b/pkg/handlers/generic/lifecycle/clusterautoscaler/strategy_crs.go @@ -81,7 +81,7 @@ func (s crsStrategy) apply( Data: data, } - if err := client.ServerSideApply(ctx, s.client, cm); err != nil { + if err := client.ServerSideApply(ctx, s.client, cm, client.ForceOwnership); err != nil { return fmt.Errorf( "failed to apply cluster-autoscaler installation ConfigMap: %w", err, diff --git a/pkg/handlers/generic/lifecycle/clusterautoscaler/strategy_helmaddon.go b/pkg/handlers/generic/lifecycle/clusterautoscaler/strategy_helmaddon.go index 691eaf681..a1d578fee 100644 --- a/pkg/handlers/generic/lifecycle/clusterautoscaler/strategy_helmaddon.go +++ b/pkg/handlers/generic/lifecycle/clusterautoscaler/strategy_helmaddon.go @@ -102,7 +102,7 @@ func (s helmAddonStrategy) apply( ) } - if err = client.ServerSideApply(ctx, s.client, hcp); err != nil { + if err = client.ServerSideApply(ctx, s.client, hcp, client.ForceOwnership); err != nil { return fmt.Errorf("failed to apply cluster-autoscaler installation HelmChartProxy: %w", err) } diff --git a/pkg/handlers/generic/lifecycle/cni/calico/strategy_crs.go b/pkg/handlers/generic/lifecycle/cni/calico/strategy_crs.go index 379b791dd..d61413a76 100644 --- a/pkg/handlers/generic/lifecycle/cni/calico/strategy_crs.go +++ b/pkg/handlers/generic/lifecycle/cni/calico/strategy_crs.go @@ -144,7 +144,7 @@ func (s crsStrategy) ensureCNICRSForCluster( ) } - if err := client.ServerSideApply(ctx, s.client, cm); err != nil { + if err := client.ServerSideApply(ctx, s.client, cm, client.ForceOwnership); err != nil { return fmt.Errorf( "failed to apply Calico CNI installation manifests ConfigMap: %w", err, @@ -185,7 +185,7 @@ func (s crsStrategy) ensureTigeraOperatorConfigMap( } tigeraConfigMap := generateTigeraOperatorConfigMap(defaultTigeraOperatorConfigMap, cluster) - if err := client.ServerSideApply(ctx, s.client, tigeraConfigMap); err != nil { + if err := client.ServerSideApply(ctx, s.client, tigeraConfigMap, client.ForceOwnership); err != nil { return nil, fmt.Errorf( "failed to apply Tigera Operator manifests ConfigMap: %w", err, diff --git a/pkg/handlers/generic/lifecycle/cni/calico/strategy_helmaddon.go b/pkg/handlers/generic/lifecycle/cni/calico/strategy_helmaddon.go index 82be205e9..c2fe5667d 100644 --- a/pkg/handlers/generic/lifecycle/cni/calico/strategy_helmaddon.go +++ b/pkg/handlers/generic/lifecycle/cni/calico/strategy_helmaddon.go @@ -110,7 +110,7 @@ func (s helmAddonStrategy) apply( ) } - if err := client.ServerSideApply(ctx, s.client, hcp); err != nil { + if err := client.ServerSideApply(ctx, s.client, hcp, client.ForceOwnership); err != nil { return fmt.Errorf("failed to apply Calico CNI installation HelmChartProxy: %w", err) } diff --git a/pkg/handlers/generic/lifecycle/cni/cilium/strategy_crs.go b/pkg/handlers/generic/lifecycle/cni/cilium/strategy_crs.go index c494a0f2f..a17b2a5b0 100644 --- a/pkg/handlers/generic/lifecycle/cni/cilium/strategy_crs.go +++ b/pkg/handlers/generic/lifecycle/cni/cilium/strategy_crs.go @@ -80,7 +80,7 @@ func (s crsStrategy) apply( BinaryData: defaultCiliumConfigMap.BinaryData, } - if err := client.ServerSideApply(ctx, s.client, cm); err != nil { + if err := client.ServerSideApply(ctx, s.client, cm, client.ForceOwnership); err != nil { return fmt.Errorf( "failed to apply Cilium CNI installation ConfigMap: %w", err, diff --git a/pkg/handlers/generic/lifecycle/cni/cilium/strategy_helmaddon.go b/pkg/handlers/generic/lifecycle/cni/cilium/strategy_helmaddon.go index f481c5450..8cb083ca8 100644 --- a/pkg/handlers/generic/lifecycle/cni/cilium/strategy_helmaddon.go +++ b/pkg/handlers/generic/lifecycle/cni/cilium/strategy_helmaddon.go @@ -94,7 +94,7 @@ func (s helmAddonStrategy) apply( ) } - if err := client.ServerSideApply(ctx, s.client, hcp); err != nil { + if err := client.ServerSideApply(ctx, s.client, hcp, client.ForceOwnership); err != nil { return fmt.Errorf("failed to apply Cilium CNI installation HelmChartProxy: %w", err) } diff --git a/pkg/handlers/generic/lifecycle/csi/aws-ebs/handler.go b/pkg/handlers/generic/lifecycle/csi/aws-ebs/handler.go index 7f55909aa..79fcd1def 100644 --- a/pkg/handlers/generic/lifecycle/csi/aws-ebs/handler.go +++ b/pkg/handlers/generic/lifecycle/csi/aws-ebs/handler.go @@ -111,7 +111,7 @@ func (a *AWSEBS) handleCRSApply(ctx context.Context, } cluster := req.Cluster cm := generateAWSEBSCSIConfigMap(awsEBSCSIConfigMap, &cluster) - if err := client.ServerSideApply(ctx, a.client, cm); err != nil { + if err := client.ServerSideApply(ctx, a.client, cm, client.ForceOwnership); err != nil { return fmt.Errorf( "failed to apply AWS EBS CSI manifests ConfigMap: %w", err, diff --git a/pkg/handlers/generic/lifecycle/csi/nutanix-csi/handler.go b/pkg/handlers/generic/lifecycle/csi/nutanix-csi/handler.go index c51b93af9..5c2c280b0 100644 --- a/pkg/handlers/generic/lifecycle/csi/nutanix-csi/handler.go +++ b/pkg/handlers/generic/lifecycle/csi/nutanix-csi/handler.go @@ -185,7 +185,7 @@ func (n *NutanixCSI) handleHelmAddonApply( ) } - if err = client.ServerSideApply(ctx, n.client, hcp); err != nil { + if err = client.ServerSideApply(ctx, n.client, hcp, client.ForceOwnership); err != nil { return fmt.Errorf("failed to apply nutanix-csi installation HelmChartProxy: %w", err) } @@ -215,7 +215,7 @@ func (n *NutanixCSI) handleHelmAddonApply( }, } - if err = client.ServerSideApply(ctx, n.client, snapshotChart); err != nil { + if err = client.ServerSideApply(ctx, n.client, snapshotChart, client.ForceOwnership); err != nil { return fmt.Errorf( "failed to apply nutanix-csi-snapshot installation HelmChartProxy: %w", err, diff --git a/pkg/handlers/generic/lifecycle/nfd/strategy_crs.go b/pkg/handlers/generic/lifecycle/nfd/strategy_crs.go index 9297e5593..a6708965b 100644 --- a/pkg/handlers/generic/lifecycle/nfd/strategy_crs.go +++ b/pkg/handlers/generic/lifecycle/nfd/strategy_crs.go @@ -80,7 +80,7 @@ func (s crsStrategy) apply( BinaryData: defaultCM.BinaryData, } - if err := client.ServerSideApply(ctx, s.client, cm); err != nil { + if err := client.ServerSideApply(ctx, s.client, cm, client.ForceOwnership); err != nil { return fmt.Errorf( "failed to apply NFD installation ConfigMap: %w", err, diff --git a/pkg/handlers/generic/lifecycle/nfd/strategy_helmaddon.go b/pkg/handlers/generic/lifecycle/nfd/strategy_helmaddon.go index 54caeeee9..39b9a4151 100644 --- a/pkg/handlers/generic/lifecycle/nfd/strategy_helmaddon.go +++ b/pkg/handlers/generic/lifecycle/nfd/strategy_helmaddon.go @@ -100,7 +100,7 @@ image: ) } - if err := client.ServerSideApply(ctx, s.client, hcp); err != nil { + if err := client.ServerSideApply(ctx, s.client, hcp, client.ForceOwnership); err != nil { return fmt.Errorf("failed to apply NFD installation HelmChartProxy: %w", err) } diff --git a/pkg/handlers/generic/lifecycle/utils/scs.go b/pkg/handlers/generic/lifecycle/utils/scs.go index 95eb679e3..c07f26831 100644 --- a/pkg/handlers/generic/lifecycle/utils/scs.go +++ b/pkg/handlers/generic/lifecycle/utils/scs.go @@ -85,7 +85,7 @@ func CreateStorageClassOnRemote( return fmt.Errorf("error creating client for remote cluster: %w", err) } for _, sc := range allStorageClasses { - err = client.ServerSideApply(ctx, remoteClient, sc) + err = client.ServerSideApply(ctx, remoteClient, sc, client.ForceOwnership) if err != nil { return fmt.Errorf("error creating storage class %v on remote cluster %w", sc, err) } diff --git a/pkg/handlers/generic/lifecycle/utils/secrets.go b/pkg/handlers/generic/lifecycle/utils/secrets.go index 80d1062d7..bd3c5a9ac 100644 --- a/pkg/handlers/generic/lifecycle/utils/secrets.go +++ b/pkg/handlers/generic/lifecycle/utils/secrets.go @@ -64,7 +64,7 @@ func CopySecretToRemoteCluster( return fmt.Errorf("error creating namespace on the remote cluster: %w", err) } - err = client.ServerSideApply(ctx, remoteClient, credentialsOnRemote) + err = client.ServerSideApply(ctx, remoteClient, credentialsOnRemote, client.ForceOwnership) if err != nil { return fmt.Errorf("error creating Secret on the remote cluster: %w", err) } diff --git a/pkg/handlers/generic/lifecycle/utils/utils.go b/pkg/handlers/generic/lifecycle/utils/utils.go index a58a078bd..2daaff4bc 100644 --- a/pkg/handlers/generic/lifecycle/utils/utils.go +++ b/pkg/handlers/generic/lifecycle/utils/utils.go @@ -85,7 +85,7 @@ func EnsureCRSForClusterFromObjects( return fmt.Errorf("failed to set owner reference: %w", err) } - err := client.ServerSideApply(ctx, c, crs) + err := client.ServerSideApply(ctx, c, crs, client.ForceOwnership) if err != nil { return fmt.Errorf("failed to server side apply %w", err) } @@ -110,7 +110,7 @@ func EnsureNamespace(ctx context.Context, c ctrlclient.Client, name string) erro return nil } - err := client.ServerSideApply(ctx, c, ns) + err := client.ServerSideApply(ctx, c, ns, client.ForceOwnership) if err != nil { return fmt.Errorf("failed to server side apply %w", err) } diff --git a/pkg/handlers/generic/mutation/imageregistries/credentials/inject.go b/pkg/handlers/generic/mutation/imageregistries/credentials/inject.go index 5ef9832a7..fc308a9a8 100644 --- a/pkg/handlers/generic/mutation/imageregistries/credentials/inject.go +++ b/pkg/handlers/generic/mutation/imageregistries/credentials/inject.go @@ -349,7 +349,7 @@ func createSecretIfNeeded( ) } if credentialsSecret != nil { - if err := client.ServerSideApply(ctx, c, credentialsSecret); err != nil { + if err := client.ServerSideApply(ctx, c, credentialsSecret, client.ForceOwnership); err != nil { return fmt.Errorf("failed to apply Image Registry Credentials Secret: %w", err) } }