Merge pull request #12442 from fabriziopandini/add-dropEmptyStruct-to-ssa-helper

🌱 Add DropEmptyStruct to ssa patch helper

Author: k8s-ci-robot

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

@@ -68,8 +68,9 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool,
 	// For dry run we use the same options as for the intent but with adding metadata.managedFields
 	// to ensure that changes to ownership are detected.
 	filterObjectInput := &ssa.FilterObjectInput{
-		AllowedPaths: append(dryRunCtx.helperOptions.AllowedPaths, []string{"metadata", "managedFields"}),
-		IgnorePaths:  dryRunCtx.helperOptions.IgnorePaths,
+		AllowedPaths:    append(dryRunCtx.helperOptions.AllowedPaths, []string{"metadata", "managedFields"}),
+		IgnorePaths:     dryRunCtx.helperOptions.IgnorePaths,
+		DropEmptyStruct: dryRunCtx.helperOptions.DropEmptyStruct,
 	}
 
 	// Add TopologyDryRunAnnotation to notify validation webhooks to skip immutability checks.

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

@@ -17,6 +17,7 @@ limitations under the License.
 package structuredmerge
 
 import (
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
@@ -78,15 +79,26 @@ type HelperOptions struct {
 func newHelperOptions(target client.Object, opts ...HelperOption) *HelperOptions {
 	helperOptions := &HelperOptions{
 		FilterObjectInput: ssa.FilterObjectInput{
-			AllowedPaths: defaultAllowedPaths,
-			IgnorePaths:  []contract.Path{},
+			AllowedPaths:    defaultAllowedPaths,
+			IgnorePaths:     []contract.Path{},
+			DropEmptyStruct: false,
 		},
 	}
 	// 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 {
+	switch target.(type) {
+	case *clusterv1.Cluster:
 		helperOptions.AllowedPaths = allowedPathsCluster
+		// NOTE: DropEmptyStruct is not required for the cluster, because even if it is converted to unstructured using the DefaultUnstructuredConverter,
+		// and it does not handle omitzero (yet), none of the allowedPaths is using omitzero.
+	case *unstructured.Unstructured:
+		// NOTE: DropEmptyStruct is not required for unstructured objects, because DefaultUnstructuredConverter is not called.
+	default:
+		helperOptions.DropEmptyStruct = true
+		// NOTE: DropEmptyStruct is required for typed objects, because they are converted to unstructured using the DefaultUnstructuredConverter,
+		// and it does not handle omitzero (yet).
 	}
+
 	helperOptions = helperOptions.ApplyOptions(opts)
 	return helperOptions
 }
@@ -109,3 +121,13 @@ type IgnorePaths []contract.Path
 func (i IgnorePaths) ApplyToHelper(opts *HelperOptions) {
 	opts.IgnorePaths = i
 }
+
+// DropEmptyStruct instruct the Helper to drop all fields with values equals to empty struct.
+// NOTE: This is required when using typed objects, because the DefaultUnstructuredConverter does
+// not handle omitzero (yet).
+type DropEmptyStruct bool
+
+// ApplyToHelper applies this configuration to the given helper options.
+func (i DropEmptyStruct) ApplyToHelper(opts *HelperOptions) {
+	opts.DropEmptyStruct = bool(i)
+}

internal/util/ssa/filterintent.go

@@ -17,6 +17,8 @@ limitations under the License.
 package ssa
 
 import (
+	"reflect"
+
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
 	"sigs.k8s.io/cluster-api/internal/contract"
@@ -32,26 +34,43 @@ type FilterObjectInput struct {
 	// spec.ControlPlaneEndpoint.
 	// NOTE: ignore paths which point to an array are not supported by the current implementation.
 	IgnorePaths []contract.Path
+
+	// DropEmptyStruct instruct the Helper to drop all fields with values equals to empty struct.
+	// NOTE: This is required when using typed objects, because the DefaultUnstructuredConverter does
+	// not handle omitzero (yet).
+	DropEmptyStruct bool
 }
 
 // FilterObject filter out changes not relevant for the controller.
 func FilterObject(obj *unstructured.Unstructured, input *FilterObjectInput) {
 	// filter out changes not in the allowed paths (fields to not consider, e.g. status);
+	// also drop empty struct if required.
 	if len(input.AllowedPaths) > 0 {
 		FilterIntent(&FilterIntentInput{
-			Path:         contract.Path{},
-			Value:        obj.Object,
-			ShouldFilter: IsPathNotAllowed(input.AllowedPaths),
+			Path:            contract.Path{},
+			Value:           obj.Object,
+			ShouldFilter:    IsPathNotAllowed(input.AllowedPaths),
+			DropEmptyStruct: input.DropEmptyStruct,
 		})
 	}
 
 	// filter out changes for ignore paths (well known fields owned by other controllers, e.g.
-	//   spec.controlPlaneEndpoint in the InfrastructureCluster object);
+	//   spec.controlPlaneEndpoint in the InfrastructureCluster object); also drop empty struct if required.
 	if len(input.IgnorePaths) > 0 {
 		FilterIntent(&FilterIntentInput{
-			Path:         contract.Path{},
-			Value:        obj.Object,
-			ShouldFilter: IsPathIgnored(input.IgnorePaths),
+			Path:            contract.Path{},
+			Value:           obj.Object,
+			ShouldFilter:    IsPathIgnored(input.IgnorePaths),
+			DropEmptyStruct: input.DropEmptyStruct,
+		})
+	}
+
+	// DropEmptyStruct if not already done above.
+	if input.DropEmptyStruct && len(input.AllowedPaths) == 0 && len(input.IgnorePaths) == 0 {
+		FilterIntent(&FilterIntentInput{
+			Path:            contract.Path{},
+			Value:           obj.Object,
+			DropEmptyStruct: input.DropEmptyStruct,
 		})
 	}
 }
@@ -76,11 +95,19 @@ func FilterIntent(ctx *FilterIntentInput) bool {
 			// Gets the original and the modified Value for the field.
 			Value: value[field],
 			// Carry over global values from the context.
-			ShouldFilter: ctx.ShouldFilter,
+			ShouldFilter:    ctx.ShouldFilter,
+			DropEmptyStruct: ctx.DropEmptyStruct,
 		}
 
 		// If the field should be filtered out, delete it from the modified object.
-		if fieldCtx.ShouldFilter(fieldCtx.Path) {
+		if fieldCtx.ShouldFilter != nil && fieldCtx.ShouldFilter(fieldCtx.Path) {
+			delete(value, field)
+			gotDeletions = true
+			continue
+		}
+
+		// If empty struct should be dropped and the value is a empty struct, delete it from the modified object.
+		if fieldCtx.DropEmptyStruct && reflect.DeepEqual(fieldCtx.Value, map[string]interface{}{}) {
 			delete(value, field)
 			gotDeletions = true
 			continue
@@ -109,6 +136,8 @@ type FilterIntentInput struct {
 
 	// ShouldFilter handle the func that determine if the current Path should be dropped or not.
 	ShouldFilter func(path contract.Path) bool
+
+	DropEmptyStruct bool
 }
 
 // IsPathAllowed returns true when the Path is one of the AllowedPaths.

internal/util/ssa/filterintent_test.go

@@ -199,3 +199,49 @@ func Test_filterIgnoredPaths(t *testing.T) {
 		})
 	}
 }
+
+func Test_filterDropEmptyStruct(t *testing.T) {
+	tests := []struct {
+		name      string
+		ctx       *FilterIntentInput
+		wantValue map[string]interface{}
+	}{
+		{
+			name: "Cleanup empty maps",
+			ctx: &FilterIntentInput{
+				Path: contract.Path{},
+				Value: map[string]interface{}{
+					"spec": map[string]interface{}{},
+				},
+				DropEmptyStruct: true,
+			},
+			wantValue: map[string]interface{}{
+				// we are filtering out spec.foo and then spec given that it is an empty map
+			},
+		},
+		{
+			name: "Cleanup empty nested maps",
+			ctx: &FilterIntentInput{
+				Path: contract.Path{},
+				Value: map[string]interface{}{
+					"spec": map[string]interface{}{
+						"bar": map[string]interface{}{},
+					},
+				},
+				DropEmptyStruct: true,
+			},
+			wantValue: map[string]interface{}{
+				// we are filtering out spec.bar.foo and then spec given that it is an empty map
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			FilterIntent(tt.ctx)
+
+			g.Expect(tt.ctx.Value).To(BeComparableTo(tt.wantValue))
+		})
+	}
+}