diff --git a/controllers/topology/cluster_controller_test.go b/controllers/topology/cluster_controller_test.go index 09d2be3a9e7c..8d1e046e91b7 100644 --- a/controllers/topology/cluster_controller_test.go +++ b/controllers/topology/cluster_controller_test.go @@ -52,8 +52,6 @@ func TestClusterReconciler_reconcile(t *testing.T) { // 1) Templates for Machine, Cluster, ControlPlane and Bootstrap. infrastructureMachineTemplate := builder.InfrastructureMachineTemplate(ns.Name, "inframachinetemplate").Build() infrastructureClusterTemplate := builder.InfrastructureClusterTemplate(ns.Name, "infraclustertemplate"). - // Create spec fake setting to assert that template spec is non-empty for tests. - WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}). Build() controlPlaneTemplate := builder.ControlPlaneTemplate(ns.Name, "cp1"). WithInfrastructureMachineTemplate(infrastructureMachineTemplate). diff --git a/controllers/topology/current_state_test.go b/controllers/topology/current_state_test.go index c28e65fe15d8..be89b293c45e 100644 --- a/controllers/topology/current_state_test.go +++ b/controllers/topology/current_state_test.go @@ -44,12 +44,10 @@ func TestGetCurrentState(t *testing.T) { // InfrastructureCluster objects. infraCluster := builder.InfrastructureCluster(metav1.NamespaceDefault, "infraOne"). - WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}). Build() // ControlPlane and ControlPlaneInfrastructureMachineTemplate objects. controlPlaneInfrastructureMachineTemplate := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "cpInfraTemplate"). - WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}). Build() controlPlaneTemplateWithInfrastructureMachine := builder.ControlPlaneTemplate(metav1.NamespaceDefault, "cpTemplateWithInfra1"). WithInfrastructureMachineTemplate(controlPlaneInfrastructureMachineTemplate). diff --git a/controllers/topology/desired_state_test.go b/controllers/topology/desired_state_test.go index fcb60d0ea090..af32ae6a8d40 100644 --- a/controllers/topology/desired_state_test.go +++ b/controllers/topology/desired_state_test.go @@ -51,7 +51,6 @@ var ( func TestComputeInfrastructureCluster(t *testing.T) { // templates and ClusterClass infrastructureClusterTemplate := builder.InfrastructureClusterTemplate(metav1.NamespaceDefault, "template1"). - WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}). Build() clusterClass := builder.ClusterClass(metav1.NamespaceDefault, "class1"). WithInfrastructureClusterTemplate(infrastructureClusterTemplate). @@ -143,7 +142,6 @@ func TestComputeControlPlaneInfrastructureMachineTemplate(t *testing.T) { } infrastructureMachineTemplate := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "template1"). - WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}). Build() clusterClass := builder.ClusterClass(metav1.NamespaceDefault, "class1"). WithControlPlaneMetadata(labels, annotations). @@ -215,7 +213,6 @@ func TestComputeControlPlane(t *testing.T) { annotations := map[string]string{"a1": ""} controlPlaneTemplate := builder.ControlPlaneTemplate(metav1.NamespaceDefault, "template1"). - WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}). Build() clusterClass := builder.ClusterClass(metav1.NamespaceDefault, "class1"). WithControlPlaneMetadata(labels, annotations). @@ -666,7 +663,6 @@ func TestComputeCluster(t *testing.T) { func TestComputeMachineDeployment(t *testing.T) { workerInfrastructureMachineTemplate := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "linux-worker-inframachinetemplate"). - WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}). Build() workerBootstrapTemplate := builder.BootstrapTemplate(metav1.NamespaceDefault, "linux-worker-bootstraptemplate"). Build() diff --git a/controllers/topology/reconcile_state_test.go b/controllers/topology/reconcile_state_test.go index ad3f7c6fb2e3..049f0c805bfd 100644 --- a/controllers/topology/reconcile_state_test.go +++ b/controllers/topology/reconcile_state_test.go @@ -82,8 +82,6 @@ func TestReconcileCluster(t *testing.T) { s := scope.New(tt.current) - // TODO: stop setting ResourceVersion when building objects - tt.desired.SetResourceVersion("") s.Desired = &scope.ClusterState{Cluster: tt.desired} r := ClusterReconciler{ @@ -188,8 +186,6 @@ func TestReconcileInfrastructureCluster(t *testing.T) { s := scope.New(nil) s.Current.InfrastructureCluster = tt.current - // TODO: stop setting ResourceVersion when building objects - tt.desired.SetResourceVersion("") s.Desired = &scope.ClusterState{InfrastructureCluster: tt.desired} r := ClusterReconciler{ @@ -266,47 +262,47 @@ func TestReconcileControlPlaneObject(t *testing.T) { name: "Should create desired ControlPlane if the current does not exist", class: ccWithoutControlPlaneInfrastructure, current: nil, - desired: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate}, - want: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate}, + desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + want: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, wantErr: false, }, { name: "Fail on updating ControlPlaneObject with incompatible changes, here a different Kind for the infrastructureMachineTemplate", class: ccWithoutControlPlaneInfrastructure, - current: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate}, - desired: &scope.ControlPlaneState{Object: controlPlane2, InfrastructureMachineTemplate: infrastructureMachineTemplate}, + current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + desired: &scope.ControlPlaneState{Object: controlPlane2.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, wantErr: true, }, { name: "Update to ControlPlaneObject with no update to the underlying infrastructure", class: ccWithoutControlPlaneInfrastructure, - current: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate}, - desired: &scope.ControlPlaneState{Object: controlPlane3, InfrastructureMachineTemplate: infrastructureMachineTemplate}, - want: &scope.ControlPlaneState{Object: controlPlane3, InfrastructureMachineTemplate: infrastructureMachineTemplate}, + current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + desired: &scope.ControlPlaneState{Object: controlPlane3.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + want: &scope.ControlPlaneState{Object: controlPlane3.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, wantErr: false, }, { name: "Update to ControlPlaneObject with underlying infrastructure.", class: ccWithControlPlaneInfrastructure, current: &scope.ControlPlaneState{InfrastructureMachineTemplate: nil}, - desired: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, - want: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + want: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, wantErr: false, }, { name: "Update to ControlPlaneObject with no underlying infrastructure", class: ccWithoutControlPlaneInfrastructure, - current: &scope.ControlPlaneState{Object: controlPlane1}, - desired: &scope.ControlPlaneState{Object: controlPlane3}, - want: &scope.ControlPlaneState{Object: controlPlane3}, + current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy()}, + desired: &scope.ControlPlaneState{Object: controlPlane3.DeepCopy()}, + want: &scope.ControlPlaneState{Object: controlPlane3.DeepCopy()}, wantErr: false, }, { name: "Preserve specific changes to the ControlPlaneObject", class: ccWithoutControlPlaneInfrastructure, - current: &scope.ControlPlaneState{Object: controlPlaneWithInstanceSpecificChanges, InfrastructureMachineTemplate: infrastructureMachineTemplate}, - desired: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate}, - want: &scope.ControlPlaneState{Object: controlPlaneWithInstanceSpecificChanges, InfrastructureMachineTemplate: infrastructureMachineTemplate}, + current: &scope.ControlPlaneState{Object: controlPlaneWithInstanceSpecificChanges.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + want: &scope.ControlPlaneState{Object: controlPlaneWithInstanceSpecificChanges.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, wantErr: false, }, } @@ -338,13 +334,6 @@ func TestReconcileControlPlaneObject(t *testing.T) { WithObjects(fakeObjs...). Build() - // TODO: stop setting ResourceVersion when building objects - if tt.desired.InfrastructureMachineTemplate != nil { - tt.desired.InfrastructureMachineTemplate.SetResourceVersion("") - } - if tt.desired.Object != nil { - tt.desired.Object.SetResourceVersion("") - } r := ClusterReconciler{ Client: fakeClient, } @@ -393,10 +382,8 @@ func TestReconcileControlPlaneInfrastructureMachineTemplate(t *testing.T) { // Create InfrastructureMachineTemplates for test cases infrastructureMachineTemplate := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra1"). - WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}). Build() infrastructureMachineTemplate2 := builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "infra2"). - WithSpecFields(map[string]interface{}{"spec.template.spec.fakeSetting": true}). Build() // Create the blueprint mandating controlPlaneInfrastructure. @@ -443,22 +430,22 @@ func TestReconcileControlPlaneInfrastructureMachineTemplate(t *testing.T) { }{ { name: "Create desired InfrastructureMachineTemplate where it doesn't exist", - current: &scope.ControlPlaneState{Object: controlPlane1}, - desired: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate}, - want: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate}, + current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy()}, + desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + want: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, wantErr: false, }, { name: "Update desired InfrastructureMachineTemplate connected to controlPlane", - current: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate}, + current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, desired: &scope.ControlPlaneState{Object: controlPlane3, InfrastructureMachineTemplate: updatedInfrastructureMachineTemplate}, want: &scope.ControlPlaneState{Object: controlPlane3, InfrastructureMachineTemplate: updatedInfrastructureMachineTemplate}, wantErr: false, }, { name: "Fail on updating infrastructure with incompatible changes", - current: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: infrastructureMachineTemplate}, - desired: &scope.ControlPlaneState{Object: controlPlane1, InfrastructureMachineTemplate: incompatibleInfrastructureMachineTemplate}, + current: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: infrastructureMachineTemplate.DeepCopy()}, + desired: &scope.ControlPlaneState{Object: controlPlane1.DeepCopy(), InfrastructureMachineTemplate: incompatibleInfrastructureMachineTemplate}, wantErr: true, }, } @@ -481,13 +468,6 @@ func TestReconcileControlPlaneInfrastructureMachineTemplate(t *testing.T) { WithObjects(fakeObjs...). Build() - // TODO: stop setting ResourceVersion when building objects - if tt.desired.InfrastructureMachineTemplate != nil { - tt.desired.InfrastructureMachineTemplate.SetResourceVersion("") - } - if tt.desired.Object != nil { - tt.desired.Object.SetResourceVersion("") - } r := ClusterReconciler{ Client: fakeClient, } @@ -732,12 +712,6 @@ func TestReconcileMachineDeployments(t *testing.T) { s := scope.New(builder.Cluster(metav1.NamespaceDefault, "cluster-1").Build()) s.Current.MachineDeployments = currentMachineDeploymentStates - // TODO: stop setting ResourceVersion when building objects - for _, md := range tt.desired { - md.Object.SetResourceVersion("") - md.BootstrapTemplate.SetResourceVersion("") - md.InfrastructureMachineTemplate.SetResourceVersion("") - } s.Desired = &scope.ClusterState{MachineDeployments: toMachineDeploymentTopologyStateMap(tt.desired)} r := ClusterReconciler{ @@ -779,11 +753,8 @@ func TestReconcileMachineDeployments(t *testing.T) { }, &gotBootstrapTemplate) g.Expect(err).ToNot(HaveOccurred()) - // We don't want to compare resourceVersions as they are slightly different between the test cases - // and it's not worth the effort. - gotBootstrapTemplate.SetResourceVersion("") - wantMachineDeploymentState.BootstrapTemplate.SetResourceVersion("") - g.Expect(gotBootstrapTemplate).To(Equal(*wantMachineDeploymentState.BootstrapTemplate)) + + g.Expect(&gotBootstrapTemplate).To(EqualObject(wantMachineDeploymentState.BootstrapTemplate, IgnoreAutogeneratedMetadata)) // Check BootstrapTemplate rotation if there was a previous MachineDeployment/Template. if currentMachineDeploymentState != nil && currentMachineDeploymentState.BootstrapTemplate != nil { @@ -806,11 +777,8 @@ func TestReconcileMachineDeployments(t *testing.T) { }, &gotInfrastructureMachineTemplate) g.Expect(err).ToNot(HaveOccurred()) - // We don't want to compare resourceVersions as they are slightly different between the test cases - // and it's not worth the effort. - gotInfrastructureMachineTemplate.SetResourceVersion("") - wantMachineDeploymentState.InfrastructureMachineTemplate.SetResourceVersion("") - g.Expect(gotInfrastructureMachineTemplate).To(Equal(*wantMachineDeploymentState.InfrastructureMachineTemplate)) + + g.Expect(&gotInfrastructureMachineTemplate).To(EqualObject(wantMachineDeploymentState.InfrastructureMachineTemplate, IgnoreAutogeneratedMetadata)) // Check InfrastructureMachineTemplate rotation if there was a previous MachineDeployment/Template. if currentMachineDeploymentState != nil && currentMachineDeploymentState.InfrastructureMachineTemplate != nil { diff --git a/internal/builder/builders.go b/internal/builder/builders.go index 6fe945a56b77..1745ce521d08 100644 --- a/internal/builder/builders.go +++ b/internal/builder/builders.go @@ -345,7 +345,14 @@ func InfrastructureMachineTemplate(namespace, name string) *InfrastructureMachin } } -// WithSpecFields will add fields of any type to the object spec. It takes an argument, fields, which is of the form path: object. +// WithSpecFields sets a map of spec fields on the unstructured object. The keys in the map represent the path and the value corresponds +// to the value of the spec field. +// +// Note: all the paths should start with "spec." +// +// Example map: map[string]interface{}{ +// "spec.version": "v1.2.3", +// }. func (i *InfrastructureMachineTemplateBuilder) WithSpecFields(fields map[string]interface{}) *InfrastructureMachineTemplateBuilder { i.specFields = fields return i @@ -359,6 +366,9 @@ func (i *InfrastructureMachineTemplateBuilder) Build() *unstructured.Unstructure obj.SetNamespace(i.namespace) obj.SetName(i.name) + // Initialize the spec.template.spec to make the object valid in reconciliation. + setSpecFields(obj, map[string]interface{}{"spec.template.spec": map[string]interface{}{}}) + setSpecFields(obj, i.specFields) return obj } @@ -385,6 +395,9 @@ func (b *BootstrapTemplateBuilder) Build() *unstructured.Unstructured { obj.SetNamespace(b.namespace) obj.SetName(b.name) + // Initialize the spec.template.spec to make the object valid in reconciliation. + setSpecFields(obj, map[string]interface{}{"spec.template.spec": map[string]interface{}{}}) + return obj } @@ -403,7 +416,14 @@ func InfrastructureClusterTemplate(namespace, name string) *InfrastructureCluste } } -// WithSpecFields will add fields of any type to the object spec. It takes an argument, fields, which is of the form path: object. +// WithSpecFields sets a map of spec fields on the unstructured object. The keys in the map represent the path and the value corresponds +// to the value of the spec field. +// +// Note: all the paths should start with "spec." +// +// Example map: map[string]interface{}{ +// "spec.version": "v1.2.3", +// }. func (i *InfrastructureClusterTemplateBuilder) WithSpecFields(fields map[string]interface{}) *InfrastructureClusterTemplateBuilder { i.specFields = fields return i @@ -417,6 +437,9 @@ func (i *InfrastructureClusterTemplateBuilder) Build() *unstructured.Unstructure obj.SetNamespace(i.namespace) obj.SetName(i.name) + // Initialize the spec.template.spec to make the object valid in reconciliation. + setSpecFields(obj, map[string]interface{}{"spec.template.spec": map[string]interface{}{}}) + setSpecFields(obj, i.specFields) return obj @@ -438,7 +461,14 @@ func ControlPlaneTemplate(namespace, name string) *ControlPlaneTemplateBuilder { } } -// WithSpecFields will add fields of any type to the object spec. It takes an argument, fields, which is of the form path: object. +// WithSpecFields sets a map of spec fields on the unstructured object. The keys in the map represent the path and the value corresponds +// to the value of the spec field. +// +// Note: all the paths should start with "spec." +// +// Example map: map[string]interface{}{ +// "spec.version": "v1.2.3", +// }. func (c *ControlPlaneTemplateBuilder) WithSpecFields(fields map[string]interface{}) *ControlPlaneTemplateBuilder { c.specFields = fields return c @@ -458,6 +488,9 @@ func (c *ControlPlaneTemplateBuilder) Build() *unstructured.Unstructured { obj.SetNamespace(c.namespace) obj.SetName(c.name) + // Initialize the spec.template.spec to make the object valid in reconciliation. + setSpecFields(obj, map[string]interface{}{"spec.template.spec": map[string]interface{}{}}) + setSpecFields(obj, c.specFields) if c.infrastructureMachineTemplate != nil { @@ -475,7 +508,14 @@ type InfrastructureClusterBuilder struct { specFields map[string]interface{} } -// WithSpecFields will add fields of any type to the object spec. It takes an argument, fields, which is of the form path: object. +// WithSpecFields sets a map of spec fields on the unstructured object. The keys in the map represent the path and the value corresponds +// to the value of the spec field. +// +// Note: all the paths should start with "spec." +// +// Example map: map[string]interface{}{ +// "spec.version": "v1.2.3", +// }. func (i *InfrastructureClusterBuilder) WithSpecFields(fields map[string]interface{}) *InfrastructureClusterBuilder { i.specFields = fields return i @@ -628,7 +668,6 @@ func (m *MachineDeploymentBuilder) WithGeneration(generation int64) *MachineDepl } // WithStatus sets the passed status object as the status of the machine deployment object. -// TODO (killianmuldoon): Revise making this method consistent with WithSpec fields in objectbuilders. func (m *MachineDeploymentBuilder) WithStatus(status clusterv1.MachineDeploymentStatus) *MachineDeploymentBuilder { m.status = &status return m