Skip to content

Commit 743922f

Browse files
sbueringersbueringer
committed
Drop DefaultUnstructuredConverter workaround
1 parent 865f730 commit 743922f

File tree

5 files changed

+37
-245
lines changed

5 files changed

+37
-245
lines changed

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,9 +68,8 @@ func dryRunSSAPatch(ctx context.Context, dryRunCtx *dryRunSSAPatchInput) (bool,
6868
// For dry run we use the same options as for the intent but with adding metadata.managedFields
6969
// to ensure that changes to ownership are detected.
7070
filterObjectInput := &ssa.FilterObjectInput{
71-
AllowedPaths: append(dryRunCtx.helperOptions.AllowedPaths, []string{"metadata", "managedFields"}),
72-
IgnorePaths: dryRunCtx.helperOptions.IgnorePaths,
73-
DropEmptyStructAndNil: dryRunCtx.helperOptions.DropEmptyStructAndNil,
71+
AllowedPaths: append(dryRunCtx.helperOptions.AllowedPaths, []string{"metadata", "managedFields"}),
72+
IgnorePaths: dryRunCtx.helperOptions.IgnorePaths,
7473
}
7574

7675
// Add TopologyDryRunAnnotation to notify validation webhooks to skip immutability checks.

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

Lines changed: 3 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ limitations under the License.
1717
package structuredmerge
1818

1919
import (
20-
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
2120
"sigs.k8s.io/controller-runtime/pkg/client"
2221

2322
clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
@@ -79,27 +78,15 @@ type HelperOptions struct {
7978
func newHelperOptions(target client.Object, opts ...HelperOption) *HelperOptions {
8079
helperOptions := &HelperOptions{
8180
FilterObjectInput: ssa.FilterObjectInput{
82-
AllowedPaths: defaultAllowedPaths,
83-
IgnorePaths: []contract.Path{},
84-
DropEmptyStructAndNil: false,
81+
AllowedPaths: defaultAllowedPaths,
82+
IgnorePaths: []contract.Path{},
8583
},
8684
}
8785
// Overwrite the allowedPaths for Cluster objects to prevent the topology controller
8886
// to take ownership of fields it is not supposed to.
89-
switch target.(type) {
90-
case *clusterv1.Cluster:
87+
if _, ok := target.(*clusterv1.Cluster); ok {
9188
helperOptions.AllowedPaths = allowedPathsCluster
92-
helperOptions.DropEmptyStructAndNil = true
93-
// NOTE: DropEmptyStructAndNil is required for Cluster, because it is converted to unstructured using the DefaultUnstructuredConverter,
94-
// and it does not handle omitzero (yet).
95-
case *unstructured.Unstructured:
96-
// NOTE: DropEmptyStructAndNil is not required for unstructured objects, because DefaultUnstructuredConverter is not called.
97-
default:
98-
helperOptions.DropEmptyStructAndNil = true
99-
// NOTE: DropEmptyStructAndNil is required for typed objects, because they are converted to unstructured using the DefaultUnstructuredConverter,
100-
// and it does not handle omitzero (yet).
10189
}
102-
10390
helperOptions = helperOptions.ApplyOptions(opts)
10491
return helperOptions
10592
}
@@ -122,13 +109,3 @@ type IgnorePaths []contract.Path
122109
func (i IgnorePaths) ApplyToHelper(opts *HelperOptions) {
123110
opts.IgnorePaths = i
124111
}
125-
126-
// DropEmptyStructAndNil instructs the Helper to drop all fields with values equal to empty struct or nil.
127-
// NOTE: This is required when using typed objects, because the DefaultUnstructuredConverter does
128-
// not handle omitzero (yet).
129-
type DropEmptyStructAndNil bool
130-
131-
// ApplyToHelper applies this configuration to the given helper options.
132-
func (i DropEmptyStructAndNil) ApplyToHelper(opts *HelperOptions) {
133-
opts.DropEmptyStructAndNil = bool(i)
134-
}

‎internal/util/ssa/filterintent.go‎

Lines changed: 32 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,6 @@ limitations under the License.
1717
package ssa
1818

1919
import (
20-
"fmt"
21-
"reflect"
22-
"strings"
23-
2420
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
2521

2622
"sigs.k8s.io/cluster-api/internal/contract"
@@ -36,43 +32,26 @@ type FilterObjectInput struct {
3632
// spec.ControlPlaneEndpoint.
3733
// NOTE: ignore paths which point to an array are not supported by the current implementation.
3834
IgnorePaths []contract.Path
39-
40-
// DropEmptyStructAndNil instructs the Helper to drop all fields with values equal to empty struct or nil.
41-
// NOTE: This is required when using typed objects, because the DefaultUnstructuredConverter does
42-
// not handle omitzero (yet).
43-
DropEmptyStructAndNil bool
4435
}
4536

4637
// FilterObject filter out changes not relevant for the controller.
4738
func FilterObject(obj *unstructured.Unstructured, input *FilterObjectInput) {
4839
// filter out changes not in the allowed paths (fields to not consider, e.g. status);
49-
// also drop empty struct if required.
5040
if len(input.AllowedPaths) > 0 {
5141
FilterIntent(&FilterIntentInput{
52-
Path: contract.Path{},
53-
Value: obj.Object,
54-
ShouldFilter: IsPathNotAllowed(input.AllowedPaths),
55-
DropEmptyStructAndNil: input.DropEmptyStructAndNil,
42+
Path: contract.Path{},
43+
Value: obj.Object,
44+
ShouldFilter: IsPathNotAllowed(input.AllowedPaths),
5645
})
5746
}
5847

5948
// filter out changes for ignore paths (well known fields owned by other controllers, e.g.
60-
// spec.controlPlaneEndpoint in the InfrastructureCluster object); also drop empty struct if required.
49+
// spec.controlPlaneEndpoint in the InfrastructureCluster object);
6150
if len(input.IgnorePaths) > 0 {
6251
FilterIntent(&FilterIntentInput{
63-
Path: contract.Path{},
64-
Value: obj.Object,
65-
ShouldFilter: IsPathIgnored(input.IgnorePaths),
66-
DropEmptyStructAndNil: input.DropEmptyStructAndNil,
67-
})
68-
}
69-
70-
// DropEmptyStructAndNil if not already done above.
71-
if input.DropEmptyStructAndNil && len(input.AllowedPaths) == 0 && len(input.IgnorePaths) == 0 {
72-
FilterIntent(&FilterIntentInput{
73-
Path: contract.Path{},
74-
Value: obj.Object,
75-
DropEmptyStructAndNil: input.DropEmptyStructAndNil,
52+
Path: contract.Path{},
53+
Value: obj.Object,
54+
ShouldFilter: IsPathIgnored(input.IgnorePaths),
7655
})
7756
}
7857
}
@@ -84,71 +63,38 @@ func FilterObject(obj *unstructured.Unstructured, input *FilterObjectInput) {
8463
// all of them are defined in reconcile_state.go and are targeting well-known fields inside nested maps.
8564
// Allowed paths / ignore paths which point to an array are not supported by the current implementation.
8665
func FilterIntent(ctx *FilterIntentInput) bool {
66+
value, ok := ctx.Value.(map[string]interface{})
67+
if !ok {
68+
return false
69+
}
70+
8771
gotDeletions := false
72+
for field := range value {
73+
fieldCtx := &FilterIntentInput{
74+
// Compose the Path for the nested field.
75+
Path: ctx.Path.Append(field),
76+
// Gets the original and the modified Value for the field.
77+
Value: value[field],
78+
// Carry over global values from the context.
79+
ShouldFilter: ctx.ShouldFilter,
80+
}
8881

89-
switch value := ctx.Value.(type) {
90-
case map[string]interface{}:
91-
for field := range value {
92-
fieldCtx := &FilterIntentInput{
93-
// Compose the Path for the nested field.
94-
Path: ctx.Path.Append(field),
95-
// Gets the original and the modified Value for the field.
96-
Value: value[field],
97-
// Carry over global values from the context.
98-
ShouldFilter: ctx.ShouldFilter,
99-
DropEmptyStructAndNil: ctx.DropEmptyStructAndNil,
100-
}
82+
// If the field should be filtered out, delete it from the modified object.
83+
if fieldCtx.ShouldFilter != nil && fieldCtx.ShouldFilter(fieldCtx.Path) {
84+
delete(value, field)
85+
gotDeletions = true
86+
continue
87+
}
10188

102-
// If the field should be filtered out, delete it from the modified object.
103-
if fieldCtx.ShouldFilter != nil && fieldCtx.ShouldFilter(fieldCtx.Path) {
89+
// Process nested fields and get in return if FilterIntent removed fields.
90+
if FilterIntent(fieldCtx) {
91+
gotDeletions = true
92+
// Ensure we are not leaving empty maps around.
93+
if v, ok := fieldCtx.Value.(map[string]interface{}); ok && len(v) == 0 {
10494
delete(value, field)
105-
gotDeletions = true
106-
continue
107-
}
108-
109-
// TODO: Can be removed once we bumped to k8s.io v0.34 because the DefaultUnstructuredConverter will then handle omitzero
110-
if strings.HasPrefix(fieldCtx.Path.String(), "spec") && fieldCtx.DropEmptyStructAndNil {
111-
// If empty struct should be dropped and the value is a empty struct, delete it from the modified object.
112-
if reflect.DeepEqual(fieldCtx.Value, map[string]interface{}{}) {
113-
delete(value, field)
114-
gotDeletions = true
115-
continue
116-
}
117-
// If nil should be dropped and the value is nil, delete it from the modified object.
118-
if reflect.DeepEqual(fieldCtx.Value, nil) {
119-
delete(value, field)
120-
gotDeletions = true
121-
continue
122-
}
123-
}
124-
125-
// Process nested fields and get in return if FilterIntent removed fields.
126-
if FilterIntent(fieldCtx) {
127-
gotDeletions = true
128-
// Ensure we are not leaving empty maps around.
129-
if v, ok := fieldCtx.Value.(map[string]interface{}); ok && len(v) == 0 {
130-
delete(value, field)
131-
}
132-
}
133-
}
134-
case []interface{}:
135-
// TODO: Can be removed once we bumped to k8s.io v0.34 because the DefaultUnstructuredConverter will then handle omitzero
136-
if strings.HasPrefix(ctx.Path.String(), "spec") && ctx.DropEmptyStructAndNil {
137-
for i, v := range value {
138-
fieldCtx := &FilterIntentInput{
139-
// Compose the Path for the nested field.
140-
Path: ctx.Path.Append(fmt.Sprintf("[%d]", i)),
141-
// Not supporting ShouldFilter within arrays, so not setting it.
142-
Value: v,
143-
DropEmptyStructAndNil: ctx.DropEmptyStructAndNil,
144-
}
145-
if FilterIntent(fieldCtx) {
146-
gotDeletions = true
147-
}
14895
}
14996
}
15097
}
151-
15298
return gotDeletions
15399
}
154100

@@ -163,8 +109,6 @@ type FilterIntentInput struct {
163109

164110
// ShouldFilter handle the func that determine if the current Path should be dropped or not.
165111
ShouldFilter func(path contract.Path) bool
166-
167-
DropEmptyStructAndNil bool
168112
}
169113

170114
// IsPathAllowed returns true when the Path is one of the AllowedPaths.

‎internal/util/ssa/filterintent_test.go‎

Lines changed: 0 additions & 123 deletions
Original file line numberDiff line numberDiff line change
@@ -199,126 +199,3 @@ func Test_filterIgnoredPaths(t *testing.T) {
199199
})
200200
}
201201
}
202-
203-
func Test_filterDropEmptyStructAndNil(t *testing.T) {
204-
tests := []struct {
205-
name string
206-
ctx *FilterIntentInput
207-
wantValue map[string]interface{}
208-
}{
209-
{
210-
name: "Cleanup empty maps",
211-
ctx: &FilterIntentInput{
212-
Path: contract.Path{},
213-
Value: map[string]interface{}{
214-
"spec": map[string]interface{}{},
215-
},
216-
DropEmptyStructAndNil: true,
217-
},
218-
wantValue: map[string]interface{}{
219-
// we are filtering out spec given that it is an empty map
220-
},
221-
},
222-
{
223-
name: "Cleanup empty nested maps",
224-
ctx: &FilterIntentInput{
225-
Path: contract.Path{},
226-
Value: map[string]interface{}{
227-
"spec": map[string]interface{}{
228-
"bar": map[string]interface{}{},
229-
},
230-
},
231-
DropEmptyStructAndNil: true,
232-
},
233-
wantValue: map[string]interface{}{
234-
// we are filtering out spec.bar and then spec given that it is an empty map
235-
},
236-
},
237-
{
238-
name: "Cleanup nil and parent empty nested maps",
239-
ctx: &FilterIntentInput{
240-
Path: contract.Path{},
241-
Value: map[string]interface{}{
242-
"spec": map[string]interface{}{
243-
"bar": map[string]interface{}{
244-
"field": nil,
245-
},
246-
},
247-
},
248-
DropEmptyStructAndNil: true,
249-
},
250-
wantValue: map[string]interface{}{
251-
// we are filtering out spec.bar.field and then spec.bar and spec given that they are empty maps
252-
},
253-
},
254-
{
255-
name: "Cleanup fields in arrays and don't cleanup empty leaf arrays",
256-
ctx: &FilterIntentInput{
257-
Path: contract.Path{},
258-
Value: map[string]interface{}{
259-
"spec": map[string]interface{}{
260-
"template": map[string]interface{}{
261-
"spec": map[string]interface{}{
262-
"joinConfiguration": map[string]interface{}{
263-
"nodeRegistration": map[string]interface{}{
264-
"taints": []interface{}{}, // should be preserved
265-
"kubeletExtraArgs": []interface{}{
266-
map[string]interface{}{
267-
"name": "eviction-hard",
268-
"value": "nodefs.available<0%,nodefs.inodesFree<0%,imagefs.available<0%",
269-
},
270-
},
271-
},
272-
"discovery": map[string]interface{}{}, // should be cleaned up
273-
},
274-
"users": []interface{}{
275-
map[string]interface{}{
276-
"name": "default-user",
277-
"passwdFrom": map[string]interface{}{ // should be cleaned up
278-
"secret": map[string]interface{}{}},
279-
},
280-
},
281-
},
282-
},
283-
},
284-
},
285-
DropEmptyStructAndNil: true,
286-
},
287-
wantValue: map[string]interface{}{
288-
"spec": map[string]interface{}{
289-
"template": map[string]interface{}{
290-
"spec": map[string]interface{}{
291-
"joinConfiguration": map[string]interface{}{
292-
"nodeRegistration": map[string]interface{}{
293-
"taints": []interface{}{}, // taints was preserved
294-
"kubeletExtraArgs": []interface{}{
295-
map[string]interface{}{
296-
"name": "eviction-hard",
297-
"value": "nodefs.available<0%,nodefs.inodesFree<0%,imagefs.available<0%",
298-
},
299-
},
300-
},
301-
// discovery was cleaned up
302-
},
303-
"users": []interface{}{
304-
map[string]interface{}{
305-
"name": "default-user",
306-
// passwdFrom was cleaned up
307-
},
308-
},
309-
},
310-
},
311-
},
312-
},
313-
},
314-
}
315-
for _, tt := range tests {
316-
t.Run(tt.name, func(t *testing.T) {
317-
g := NewWithT(t)
318-
319-
FilterIntent(tt.ctx)
320-
321-
g.Expect(tt.ctx.Value).To(BeComparableTo(tt.wantValue))
322-
})
323-
}
324-
}

‎internal/util/ssa/patch.go‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -128,17 +128,13 @@ func Patch(ctx context.Context, c client.Client, fieldManager string, modified c
128128
// prepareModified converts obj into an Unstructured and filters out undesired fields.
129129
func prepareModified(scheme *runtime.Scheme, obj client.Object) (*unstructured.Unstructured, error) {
130130
u := &unstructured.Unstructured{}
131-
dropEmptyStructAndNil := false
132131
switch obj.(type) {
133132
case *unstructured.Unstructured:
134133
u = obj.DeepCopyObject().(*unstructured.Unstructured)
135134
default:
136135
if err := scheme.Convert(obj, u, nil); err != nil {
137136
return nil, errors.Wrap(err, "failed to convert object to Unstructured")
138137
}
139-
// NOTE: DropEmptyStructAndNil is required for typed objects, because they are converted to unstructured using the DefaultUnstructuredConverter,
140-
// and it does not handle omitzero (yet).
141-
dropEmptyStructAndNil = true
142138
}
143139

144140
// Only keep the paths that we have opinions on.
@@ -158,7 +154,6 @@ func prepareModified(scheme *runtime.Scheme, obj client.Object) (*unstructured.U
158154
{"metadata", "ownerReferences"},
159155
{"spec"},
160156
},
161-
DropEmptyStructAndNil: dropEmptyStructAndNil,
162157
})
163158
return u, nil
164159
}

0 commit comments

Comments
 (0)