Merge pull request #6773 from chrischdi/pr-topology-cluster-prevent-coownership

🐛 Adjust structuredmerge patch helper options to set correct allow list for Cluster objects to prevent co-ownership

Author: k8s-ci-robot

internal/controllers/topology/cluster/reconcile_state.go

@@ -439,9 +439,7 @@ func (r *Reconciler) reconcileCluster(ctx context.Context, s *scope.Scope) error
 	ctx, log := tlog.LoggerFrom(ctx).WithObject(s.Desired.Cluster).Into(ctx)
 
 	// Check differences between current and desired state, and eventually patch the current object.
-	patchHelper, err := r.patchHelperFactory(s.Current.Cluster, s.Desired.Cluster, structuredmerge.IgnorePaths{
-		{"spec", "controlPlaneEndpoint"}, // this is a well known field that is managed by the Cluster controller, topology should not express opinions on it.
-	})
+	patchHelper, err := r.patchHelperFactory(s.Current.Cluster, s.Desired.Cluster)
 	if err != nil {
 		return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: s.Current.Cluster})
 	}

internal/controllers/topology/cluster/structuredmerge/options.go

@@ -17,9 +17,47 @@ limitations under the License.
 package structuredmerge
 
 import (
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/internal/contract"
 )
 
+var (
+	// defaultAllowedPaths are the allowed paths for all objects except Clusters.
+	defaultAllowedPaths = []contract.Path{
+		// apiVersion, kind, name and namespace are required field for a server side apply intent.
+		{"apiVersion"},
+		{"kind"},
+		{"metadata", "name"},
+		{"metadata", "namespace"},
+		// the topology controller controls/has an opinion for labels, annotation, ownerReferences and spec only.
+		{"metadata", "labels"},
+		{"metadata", "annotations"},
+		{"metadata", "ownerReferences"},
+		{"spec"},
+	}
+
+	// allowedPathsCluster are the allowed paths specific for Clusters.
+	// The cluster object is not created by the topology controller and already contains fields which are
+	// not supposed for the topology controller to have an opinion on / take (co-)ownership of.
+	// Because of that the allowedPaths are different to other objects.
+	// NOTE: This is mostly the same as defaultAllowedPaths but having more restrictions.
+	allowedPathsCluster = []contract.Path{
+		// apiVersion, kind, name and namespace are required field for a server side apply intent.
+		{"apiVersion"},
+		{"kind"},
+		{"metadata", "name"},
+		{"metadata", "namespace"},
+		// the topology controller controls/has an opinion for the labels ClusterLabelName
+		// and ClusterTopologyOwnedLabel as well as infrastructureRef and controlPlaneRef in spec.
+		{"metadata", "labels", clusterv1.ClusterLabelName},
+		{"metadata", "labels", clusterv1.ClusterTopologyOwnedLabel},
+		{"spec", "infrastructureRef"},
+		{"spec", "controlPlaneRef"},
+	}
+)
+
 // HelperOption is some configuration that modifies options for Helper.
 type HelperOption interface {
 	// ApplyToHelper applies this configuration to the given helper options.
@@ -42,6 +80,20 @@ type HelperOptions struct {
 	ignorePaths []contract.Path
 }
 
+// newHelperOptions returns initialized HelperOptions.
+func newHelperOptions(target client.Object, opts ...HelperOption) *HelperOptions {
+	helperOptions := &HelperOptions{
+		allowedPaths: defaultAllowedPaths,
+	}
+	// Overwrite the allowedPaths for Cluster objects to prevent the topology controller
+	// to take ownership of fields it is not supposed to.
+	if _, ok := target.(*clusterv1.Cluster); ok {
+		helperOptions.allowedPaths = allowedPathsCluster
+	}
+	helperOptions = helperOptions.ApplyOptions(opts)
+	return helperOptions
+}
+
 // ApplyOptions applies the given patch options on these options,
 // and then returns itself (for convenient chaining).
 func (o *HelperOptions) ApplyOptions(opts []HelperOption) *HelperOptions {

internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go

@@ -42,20 +42,8 @@ type serverSidePatchHelper struct {
 
 // NewServerSidePatchHelper returns a new PatchHelper using server side apply.
 func NewServerSidePatchHelper(original, modified client.Object, c client.Client, opts ...HelperOption) (PatchHelper, error) {
-	helperOptions := &HelperOptions{}
-	helperOptions = helperOptions.ApplyOptions(opts)
-	helperOptions.allowedPaths = []contract.Path{
-		// apiVersion, kind, name and namespace are required field for a server side apply intent.
-		{"apiVersion"},
-		{"kind"},
-		{"metadata", "name"},
-		{"metadata", "namespace"},
-		// the topology controller controls/has an opinion for labels, annotation, ownerReferences and spec only.
-		{"metadata", "labels"},
-		{"metadata", "annotations"},
-		{"metadata", "ownerReferences"},
-		{"spec"},
-	}
+	// Create helperOptions for filtering the original and modified objects to the desired intent.
+	helperOptions := newHelperOptions(modified, opts...)
 
 	// If required, convert the original and modified objects to unstructured and filter out all the information
 	// not relevant for the topology controller.