From c7c2ef6caae92f5531826be5755f6e34d1727bd6 Mon Sep 17 00:00:00 2001 From: Simon Kollberg Date: Mon, 13 May 2024 16:00:17 +0200 Subject: [PATCH 1/6] Fix loadbalancer timeout panic --- .../services/loadbalancer/loadbalancer.go | 5 ++-- .../loadbalancer/loadbalancer_test.go | 28 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/pkg/cloud/services/loadbalancer/loadbalancer.go b/pkg/cloud/services/loadbalancer/loadbalancer.go index 11fcf3c286..ccdca0c0d9 100644 --- a/pkg/cloud/services/loadbalancer/loadbalancer.go +++ b/pkg/cloud/services/loadbalancer/loadbalancer.go @@ -92,9 +92,10 @@ func (s *Service) ReconcileLoadBalancer(openStackCluster *infrav1.OpenStackClust if lb.ProvisioningStatus != loadBalancerProvisioningStatusActive { var err error - lb, err = s.waitForLoadBalancerActive(lb.ID) + lbID := lb.ID + lb, err = s.waitForLoadBalancerActive(lbID) if err != nil { - return false, fmt.Errorf("load balancer %q with id %s is not active after timeout: %v", loadBalancerName, lb.ID, err) + return false, fmt.Errorf("load balancer %q with id %s is not active after timeout: %v", loadBalancerName, lbID, err) } } diff --git a/pkg/cloud/services/loadbalancer/loadbalancer_test.go b/pkg/cloud/services/loadbalancer/loadbalancer_test.go index bc8317e61e..4b43a6d488 100644 --- a/pkg/cloud/services/loadbalancer/loadbalancer_test.go +++ b/pkg/cloud/services/loadbalancer/loadbalancer_test.go @@ -45,6 +45,13 @@ func Test_ReconcileLoadBalancer(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() + // Shortcut wait timeout + backoffDurationPrev := backoff.Duration + backoff.Duration = 0 + defer func() { + backoff.Duration = backoffDurationPrev + }() + // Stub the call to net.LookupHost lookupHost = func(host string) (addrs *string, err error) { if net.ParseIP(host) != nil { @@ -139,6 +146,27 @@ func Test_ReconcileLoadBalancer(t *testing.T) { }, wantError: nil, }, + { + name: "reconcile loadbalancer in non active state should timeout", + expectNetwork: func(*mock.MockNetworkClientMockRecorder) { + // add network api call results here + }, + expectLoadBalancer: func(m *mock.MockLbClientMockRecorder) { + pendingLB := loadbalancers.LoadBalancer{ + ID: "aaaaaaaa-bbbb-cccc-dddd-333333333333", + Name: "k8s-clusterapi-cluster-AAAAA-kubeapi", + ProvisioningStatus: "PENDING_CREATE", + } + + // return existing loadbalancer in non-active state + lbList := []loadbalancers.LoadBalancer{pendingLB} + m.ListLoadBalancers(loadbalancers.ListOpts{Name: pendingLB.Name}).Return(lbList, nil) + + // wait for loadbalancer until it times out + m.GetLoadBalancer("aaaaaaaa-bbbb-cccc-dddd-333333333333").Return(&pendingLB, nil).Return(&pendingLB, nil).AnyTimes() + }, + wantError: fmt.Errorf("load balancer \"k8s-clusterapi-cluster-AAAAA-kubeapi\" with id aaaaaaaa-bbbb-cccc-dddd-333333333333 is not active after timeout: timed out waiting for the condition"), + }, } for _, tt := range lbtests { t.Run(tt.name, func(t *testing.T) { From dce0af359929da2bac1d2b7d30d9df4884ea391a Mon Sep 17 00:00:00 2001 From: Emilien Macchi Date: Tue, 14 May 2024 11:40:42 -0400 Subject: [PATCH 2/6] allNodesSecurityGroupRules: relax remote fields Don't make `remoteManagedGroups` required, since a user can use `remoteIPPrefix` instead or even no remote parameter at all. It's fine because Neutron will set it to an fully-open CIDR if no remote field is provided. --- .../src/clusteropenstack/configuration.md | 1 + .../topics/crd-changes/v1alpha7-to-v1beta1.md | 3 + .../services/networking/securitygroups.go | 4 - .../networking/securitygroups_test.go | 81 ++++++++++++++++--- 4 files changed, 74 insertions(+), 15 deletions(-) diff --git a/docs/book/src/clusteropenstack/configuration.md b/docs/book/src/clusteropenstack/configuration.md index cb9e20360a..193720927e 100644 --- a/docs/book/src/clusteropenstack/configuration.md +++ b/docs/book/src/clusteropenstack/configuration.md @@ -565,6 +565,7 @@ permitted from anywhere, as with the default rules). We can add security group rules that authorize traffic from all nodes via `allNodesSecurityGroupRules`. It takes a list of security groups rules that should be applied to selected nodes. The following rule fields are mutually exclusive: `remoteManagedGroups`, `remoteGroupID` and `remoteIPPrefix`. +If none of these fields are set, the rule will have a remote IP prefix of `0.0.0.0/0` per Neutron default. Valid values for `remoteManagedGroups` are `controlplane`, `worker` and `bastion`. diff --git a/docs/book/src/topics/crd-changes/v1alpha7-to-v1beta1.md b/docs/book/src/topics/crd-changes/v1alpha7-to-v1beta1.md index 975d226275..9d1aaaad02 100644 --- a/docs/book/src/topics/crd-changes/v1alpha7-to-v1beta1.md +++ b/docs/book/src/topics/crd-changes/v1alpha7-to-v1beta1.md @@ -396,6 +396,9 @@ The field `managedSecurityGroups` is now a pointer to a `ManagedSecurityGroups` Also, we can now add security group rules that authorize traffic from all nodes via `allNodesSecurityGroupRules`. It takes a list of security groups rules that should be applied to selected nodes. The following rule fields are mutually exclusive: `remoteManagedGroups`, `remoteGroupID` and `remoteIPPrefix`. +If none of these fields are set, the rule will have a remote IP prefix of `0.0.0.0/0` per Neutron default. + +```yaml Valid values for `remoteManagedGroups` are `controlplane`, `worker` and `bastion`. Also, `OpenStackCluster.Spec.AllowAllInClusterTraffic` moved under `ManagedSecurityGroups`. diff --git a/pkg/cloud/services/networking/securitygroups.go b/pkg/cloud/services/networking/securitygroups.go index cc41b9423b..5863adc262 100644 --- a/pkg/cloud/services/networking/securitygroups.go +++ b/pkg/cloud/services/networking/securitygroups.go @@ -297,10 +297,6 @@ func getAllNodesRules(remoteManagedGroups map[string]string, allNodesSecurityGro // validateRemoteManagedGroups validates that the remoteManagedGroups target existing managed security groups. func validateRemoteManagedGroups(remoteManagedGroups map[string]string, ruleRemoteManagedGroups []infrav1.ManagedSecurityGroupName) error { - if len(ruleRemoteManagedGroups) == 0 { - return fmt.Errorf("remoteManagedGroups is required") - } - for _, group := range ruleRemoteManagedGroups { if _, ok := remoteManagedGroups[group.String()]; !ok { return fmt.Errorf("remoteManagedGroups: %s is not a valid remote managed security group", group) diff --git a/pkg/cloud/services/networking/securitygroups_test.go b/pkg/cloud/services/networking/securitygroups_test.go index a9f0ca3163..84d6bdecd4 100644 --- a/pkg/cloud/services/networking/securitygroups_test.go +++ b/pkg/cloud/services/networking/securitygroups_test.go @@ -49,19 +49,14 @@ func TestValidateRemoteManagedGroups(t *testing.T) { wantErr: true, }, { - name: "Valid rule with missing remoteManagedGroups", + name: "Valid rule with no remoteManagedGroups", rule: infrav1.SecurityGroupRuleSpec{ - PortRangeMin: ptr.To(22), - PortRangeMax: ptr.To(22), - Protocol: ptr.To("tcp"), + PortRangeMin: ptr.To(22), + PortRangeMax: ptr.To(22), + Protocol: ptr.To("tcp"), + RemoteIPPrefix: ptr.To("0.0.0.0/0"), }, - remoteManagedGroups: map[string]string{ - "self": "self", - "controlplane": "1", - "worker": "2", - "bastion": "3", - }, - wantErr: true, + wantErr: false, }, { name: "Valid rule with remoteManagedGroups", @@ -171,6 +166,70 @@ func TestGetAllNodesRules(t *testing.T) { }, }, }, + { + name: "Valid remoteIPPrefix in a rule", + remoteManagedGroups: map[string]string{ + "controlplane": "1", + "worker": "2", + }, + allNodesSecurityGroupRules: []infrav1.SecurityGroupRuleSpec{ + { + Protocol: ptr.To("tcp"), + PortRangeMin: ptr.To(22), + PortRangeMax: ptr.To(22), + RemoteIPPrefix: ptr.To("0.0.0.0/0"), + }, + }, + wantRules: []resolvedSecurityGroupRuleSpec{ + { + Protocol: "tcp", + PortRangeMin: 22, + PortRangeMax: 22, + RemoteIPPrefix: "0.0.0.0/0", + }, + }, + }, + { + name: "Valid allNodesSecurityGroupRules with no remote parameter", + remoteManagedGroups: map[string]string{ + "controlplane": "1", + "worker": "2", + }, + allNodesSecurityGroupRules: []infrav1.SecurityGroupRuleSpec{ + { + Protocol: ptr.To("tcp"), + PortRangeMin: ptr.To(22), + PortRangeMax: ptr.To(22), + }, + }, + wantRules: []resolvedSecurityGroupRuleSpec{ + { + Protocol: "tcp", + PortRangeMin: 22, + PortRangeMax: 22, + }, + }, + wantErr: false, + }, + { + name: "Invalid allNodesSecurityGroupRules with bastion while remoteManagedGroups does not have bastion", + remoteManagedGroups: map[string]string{ + "controlplane": "1", + "worker": "2", + }, + allNodesSecurityGroupRules: []infrav1.SecurityGroupRuleSpec{ + { + Protocol: ptr.To("tcp"), + PortRangeMin: ptr.To(22), + PortRangeMax: ptr.To(22), + RemoteManagedGroups: []infrav1.ManagedSecurityGroupName{ + "bastion", + }, + }, + }, + wantRules: nil, + wantErr: true, + }, { name: "Invalid allNodesSecurityGroupRules with wrong remoteManagedGroups", remoteManagedGroups: map[string]string{ From 8594acca0c94628d5add0e3382c0f47cc89ba22d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Dulko?= Date: Wed, 15 May 2024 11:13:57 +0200 Subject: [PATCH 3/6] Drop dulek from reviewers Going forward I won't be able to contribute time to both CPO and CAPO, so this commit drops me from CAPO reviewers list. --- OWNERS_ALIASES | 1 - 1 file changed, 1 deletion(-) diff --git a/OWNERS_ALIASES b/OWNERS_ALIASES index e4bda950ca..a0c073e435 100644 --- a/OWNERS_ALIASES +++ b/OWNERS_ALIASES @@ -24,4 +24,3 @@ aliases: - mdbooth cluster-api-openstack-reviewers: - emilienm - - dulek From e81fc275001df64da7df6cd87ea2f649939fd3f0 Mon Sep 17 00:00:00 2001 From: Francois Eleouet Date: Thu, 16 May 2024 10:14:19 +0200 Subject: [PATCH 4/6] Handle errors returned by GetInstanceStatusByName in machine controller These errors were ignored up to now, which could lead controller to attempt to recreate an existing instance under rare circumstances. --- controllers/openstackmachine_controller.go | 25 +++++++++++++--------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/controllers/openstackmachine_controller.go b/controllers/openstackmachine_controller.go index ee923ec812..230ecd5342 100644 --- a/controllers/openstackmachine_controller.go +++ b/controllers/openstackmachine_controller.go @@ -776,17 +776,22 @@ func (r *OpenStackMachineReconciler) getOrCreateInstance(logger logr.Logger, ope } if instanceStatus == nil { // Check if there is an existing instance with machine name, in case where instance ID would not have been stored in machine status - if instanceStatus, err = computeService.GetInstanceStatusByName(openStackMachine, openStackMachine.Name); err == nil { - if instanceStatus != nil { - return instanceStatus, nil - } - if openStackMachine.Status.InstanceID != nil { - logger.Info("Not reconciling machine in failed state. The previously existing OpenStack instance is no longer available") - conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceNotFoundReason, clusterv1.ConditionSeverityError, "virtual machine no longer exists") - openStackMachine.SetFailure(capierrors.UpdateMachineError, errors.New("virtual machine no longer exists")) - return nil, nil - } + instanceStatus, err = computeService.GetInstanceStatusByName(openStackMachine, openStackMachine.Name) + if err != nil { + logger.Info("Unable to get OpenStack instance by name", "name", openStackMachine.Name) + conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceCreateFailedReason, clusterv1.ConditionSeverityError, err.Error()) + return nil, err + } + if instanceStatus != nil { + return instanceStatus, nil } + if openStackMachine.Status.InstanceID != nil { + logger.Info("Not reconciling machine in failed state. The previously existing OpenStack instance is no longer available") + conditions.MarkFalse(openStackMachine, infrav1.InstanceReadyCondition, infrav1.InstanceNotFoundReason, clusterv1.ConditionSeverityError, "virtual machine no longer exists") + openStackMachine.SetFailure(capierrors.UpdateMachineError, errors.New("virtual machine no longer exists")) + return nil, nil + } + instanceSpec, err := machineToInstanceSpec(openStackCluster, machine, openStackMachine, userData) if err != nil { return nil, err From 75ec5ad4985bb916cf0d2db9cdd31463a7d5233f Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Fri, 7 Jun 2024 13:20:01 +0100 Subject: [PATCH 5/6] Add EmilienM as a maintainer --- OWNERS_ALIASES | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OWNERS_ALIASES b/OWNERS_ALIASES index a0c073e435..3c9d48a297 100644 --- a/OWNERS_ALIASES +++ b/OWNERS_ALIASES @@ -19,8 +19,8 @@ aliases: - neolit123 - vincepri cluster-api-openstack-maintainers: + - emilienm - jichenjc - lentzi90 - mdbooth cluster-api-openstack-reviewers: - - emilienm From ade7f7708a21029264d95ecb5b5d481327d150e6 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Thu, 4 Jul 2024 12:00:12 +0100 Subject: [PATCH 6/6] Fix down-conversion of IdentityRef We were not setting IdentityRef.Kind when down-converting an object with no previous version annotation, which results in a validation error. --- api/v1alpha6/openstackcluster_conversion.go | 5 +- api/v1alpha6/openstackmachine_conversion.go | 1 - api/v1alpha6/types_conversion.go | 2 + api/v1alpha7/openstackcluster_conversion.go | 5 +- api/v1alpha7/types_conversion.go | 2 + .../apivalidations/openstackcluster_test.go | 81 +++++++++++++------ .../apivalidations/openstackmachine_test.go | 55 +++++++++++++ test/e2e/suites/apivalidations/suite_test.go | 18 +++++ 8 files changed, 141 insertions(+), 28 deletions(-) diff --git a/api/v1alpha6/openstackcluster_conversion.go b/api/v1alpha6/openstackcluster_conversion.go index 3bbb3f97d8..28108d1648 100644 --- a/api/v1alpha6/openstackcluster_conversion.go +++ b/api/v1alpha6/openstackcluster_conversion.go @@ -364,7 +364,10 @@ func Convert_v1beta1_OpenStackClusterSpec_To_v1alpha6_OpenStackClusterSpec(in *i } out.CloudName = in.IdentityRef.CloudName - out.IdentityRef = &OpenStackIdentityReference{Name: in.IdentityRef.Name} + out.IdentityRef = &OpenStackIdentityReference{} + if err := Convert_v1beta1_OpenStackIdentityReference_To_v1alpha6_OpenStackIdentityReference(&in.IdentityRef, out.IdentityRef, s); err != nil { + return err + } if in.APIServerLoadBalancer != nil { if err := Convert_v1beta1_APIServerLoadBalancer_To_v1alpha6_APIServerLoadBalancer(in.APIServerLoadBalancer, &out.APIServerLoadBalancer, s); err != nil { diff --git a/api/v1alpha6/openstackmachine_conversion.go b/api/v1alpha6/openstackmachine_conversion.go index aed2d3b7d4..47a757bd7d 100644 --- a/api/v1alpha6/openstackmachine_conversion.go +++ b/api/v1alpha6/openstackmachine_conversion.go @@ -373,7 +373,6 @@ func Convert_v1beta1_OpenStackMachineSpec_To_v1alpha6_OpenStackMachineSpec(in *i } if in.IdentityRef != nil { - out.IdentityRef = &OpenStackIdentityReference{Name: in.IdentityRef.Name} out.CloudName = in.IdentityRef.CloudName } diff --git a/api/v1alpha6/types_conversion.go b/api/v1alpha6/types_conversion.go index 041ce9eb3a..30c8618cc8 100644 --- a/api/v1alpha6/types_conversion.go +++ b/api/v1alpha6/types_conversion.go @@ -534,6 +534,8 @@ func Convert_v1alpha6_OpenStackIdentityReference_To_v1beta1_OpenStackIdentityRef func Convert_v1beta1_OpenStackIdentityReference_To_v1alpha6_OpenStackIdentityReference(in *infrav1.OpenStackIdentityReference, out *OpenStackIdentityReference, _ apiconversion.Scope) error { out.Name = in.Name + // Kind will be overwritten during restore if it was previously set to some other value, but if not then some value is still required + out.Kind = "Secret" return nil } diff --git a/api/v1alpha7/openstackcluster_conversion.go b/api/v1alpha7/openstackcluster_conversion.go index 6b4a82a9bf..cfdea6e20d 100644 --- a/api/v1alpha7/openstackcluster_conversion.go +++ b/api/v1alpha7/openstackcluster_conversion.go @@ -361,7 +361,10 @@ func Convert_v1beta1_OpenStackClusterSpec_To_v1alpha7_OpenStackClusterSpec(in *i } out.CloudName = in.IdentityRef.CloudName - out.IdentityRef = &OpenStackIdentityReference{Name: in.IdentityRef.Name} + out.IdentityRef = &OpenStackIdentityReference{} + if err := Convert_v1beta1_OpenStackIdentityReference_To_v1alpha7_OpenStackIdentityReference(&in.IdentityRef, out.IdentityRef, s); err != nil { + return err + } if in.APIServerLoadBalancer != nil { if err := Convert_v1beta1_APIServerLoadBalancer_To_v1alpha7_APIServerLoadBalancer(in.APIServerLoadBalancer, &out.APIServerLoadBalancer, s); err != nil { diff --git a/api/v1alpha7/types_conversion.go b/api/v1alpha7/types_conversion.go index 806e145806..db640e1e95 100644 --- a/api/v1alpha7/types_conversion.go +++ b/api/v1alpha7/types_conversion.go @@ -536,6 +536,8 @@ func Convert_v1alpha7_OpenStackIdentityReference_To_v1beta1_OpenStackIdentityRef func Convert_v1beta1_OpenStackIdentityReference_To_v1alpha7_OpenStackIdentityReference(in *infrav1.OpenStackIdentityReference, out *OpenStackIdentityReference, _ apiconversion.Scope) error { out.Name = in.Name + // Kind will be overwritten during restore if it was previously set to some other value, but if not then some value is still required + out.Kind = "Secret" return nil } diff --git a/test/e2e/suites/apivalidations/openstackcluster_test.go b/test/e2e/suites/apivalidations/openstackcluster_test.go index 80320e167c..76bcbb40bf 100644 --- a/test/e2e/suites/apivalidations/openstackcluster_test.go +++ b/test/e2e/suites/apivalidations/openstackcluster_test.go @@ -19,6 +19,7 @@ package apivalidations import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/onsi/gomega/format" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" @@ -33,16 +34,6 @@ import ( var _ = Describe("OpenStackCluster API validations", func() { var namespace *corev1.Namespace - create := func(obj client.Object) error { - err := k8sClient.Create(ctx, obj) - if err == nil { - DeferCleanup(func() error { - return k8sClient.Delete(ctx, obj) - }) - } - return err - } - BeforeEach(func() { namespace = createNamespace() }) @@ -58,12 +49,12 @@ var _ = Describe("OpenStackCluster API validations", func() { }) It("should allow the smallest permissible cluster spec", func() { - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") }) It("should only allow controlPlaneEndpoint to be set once", func() { By("Creating a bare cluster") - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") By("Setting the control plane endpoint") cluster.Spec.ControlPlaneEndpoint = &clusterv1.APIEndpoint{ @@ -79,12 +70,12 @@ var _ = Describe("OpenStackCluster API validations", func() { It("should allow an empty managed security groups definition", func() { cluster.Spec.ManagedSecurityGroups = &infrav1.ManagedSecurityGroups{} - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") }) It("should default enabled to true if APIServerLoadBalancer is specified without enabled=true", func() { cluster.Spec.APIServerLoadBalancer = &infrav1.APIServerLoadBalancer{} - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") // Fetch the cluster and check the defaulting fetchedCluster := &infrav1.OpenStackCluster{} @@ -95,7 +86,7 @@ var _ = Describe("OpenStackCluster API validations", func() { }) It("should not default APIServerLoadBalancer if it is not specifid", func() { - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") // Fetch the cluster and check the defaulting fetchedCluster := &infrav1.OpenStackCluster{} @@ -116,19 +107,19 @@ var _ = Describe("OpenStackCluster API validations", func() { }, }, } - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") }) It("should not allow bastion.enabled=true without a spec", func() { cluster.Spec.Bastion = &infrav1.Bastion{ Enabled: ptr.To(true), } - Expect(create(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed") + Expect(createObj(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed") }) It("should not allow an empty Bastion", func() { cluster.Spec.Bastion = &infrav1.Bastion{} - Expect(create(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed") + Expect(createObj(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed") }) It("should default bastion.enabled=true", func() { @@ -141,7 +132,7 @@ var _ = Describe("OpenStackCluster API validations", func() { }, }, } - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should not succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should not succeed") // Fetch the cluster and check the defaulting fetchedCluster := &infrav1.OpenStackCluster{} @@ -162,7 +153,7 @@ var _ = Describe("OpenStackCluster API validations", func() { }, FloatingIP: ptr.To("10.0.0.0"), } - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") }) It("should not allow non-IPv4 as bastion floating IP", func() { @@ -176,7 +167,7 @@ var _ = Describe("OpenStackCluster API validations", func() { }, FloatingIP: ptr.To("foobar"), } - Expect(create(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed") + Expect(createObj(cluster)).NotTo(Succeed(), "OpenStackCluster creation should not succeed") }) }) @@ -196,7 +187,7 @@ var _ = Describe("OpenStackCluster API validations", func() { Kind: "FakeKind", Name: "identity-ref", } - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") // Fetch the infrav1 version of the cluster infrav1Cluster := &infrav1.OpenStackCluster{} @@ -218,7 +209,7 @@ var _ = Describe("OpenStackCluster API validations", func() { It("should not enable an explicitly disabled bastion when converting to v1beta1", func() { cluster.Spec.Bastion = &infrav1alpha7.Bastion{Enabled: false} - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") // Fetch the infrav1 version of the cluster infrav1Cluster := &infrav1.OpenStackCluster{} @@ -233,6 +224,26 @@ var _ = Describe("OpenStackCluster API validations", func() { Expect(infrav1Bastion).ToNot(BeNil(), "Bastion should not have been removed") Expect(infrav1Bastion.Enabled).To(Equal(ptr.To(false)), "Bastion should remain disabled") }) + + It("should downgrade cleanly from infrav1", func() { + infrav1Cluster := &infrav1.OpenStackCluster{} + infrav1Cluster.Namespace = namespace.Name + infrav1Cluster.GenerateName = clusterNamePrefix + infrav1Cluster.Spec.IdentityRef.CloudName = "test-cloud" + infrav1Cluster.Spec.IdentityRef.Name = "test-credentials" + Expect(createObj(infrav1Cluster)).To(Succeed(), "infrav1 OpenStackCluster creation should succeed") + + // Just fetching the object as v1alpha6 doesn't trigger + // validation failure, so we first fetch it and then + // patch the object with identical contents. The patch + // triggers a validation failure. + cluster := &infrav1alpha7.OpenStackCluster{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: infrav1Cluster.Name, Namespace: infrav1Cluster.Namespace}, cluster)).To(Succeed(), "OpenStackCluster fetch should succeed") + + setObjectGVK(cluster) + cluster.ManagedFields = nil + Expect(k8sClient.Patch(ctx, cluster, client.Apply, client.FieldOwner("test"), client.ForceOwnership)).To(Succeed(), format.Object(cluster, 4)) + }) }) Context("v1alpha6", func() { @@ -251,7 +262,7 @@ var _ = Describe("OpenStackCluster API validations", func() { Kind: "FakeKind", Name: "identity-ref", } - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") // Fetch the infrav1 version of the cluster infrav1Cluster := &infrav1.OpenStackCluster{} @@ -273,7 +284,7 @@ var _ = Describe("OpenStackCluster API validations", func() { It("should not enable an explicitly disabled bastion when converting to v1beta1", func() { cluster.Spec.Bastion = &infrav1alpha6.Bastion{Enabled: false} - Expect(create(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") + Expect(createObj(cluster)).To(Succeed(), "OpenStackCluster creation should succeed") // Fetch the infrav1 version of the cluster infrav1Cluster := &infrav1.OpenStackCluster{} @@ -288,5 +299,25 @@ var _ = Describe("OpenStackCluster API validations", func() { Expect(infrav1Bastion).ToNot(BeNil(), "Bastion should not have been removed") Expect(infrav1Bastion.Enabled).To(Equal(ptr.To(false)), "Bastion should remain disabled") }) + + It("should downgrade cleanly from infrav1", func() { + infrav1Cluster := &infrav1.OpenStackCluster{} + infrav1Cluster.Namespace = namespace.Name + infrav1Cluster.GenerateName = clusterNamePrefix + infrav1Cluster.Spec.IdentityRef.CloudName = "test-cloud" + infrav1Cluster.Spec.IdentityRef.Name = "test-credentials" + Expect(createObj(infrav1Cluster)).To(Succeed(), "infrav1 OpenStackCluster creation should succeed") + + // Just fetching the object as v1alpha6 doesn't trigger + // validation failure, so we first fetch it and then + // patch the object with identical contents. The patch + // triggers a validation failure. + cluster := &infrav1alpha6.OpenStackCluster{} //nolint:staticcheck + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: infrav1Cluster.Name, Namespace: infrav1Cluster.Namespace}, cluster)).To(Succeed(), "OpenStackCluster fetch should succeed") + + setObjectGVK(cluster) + cluster.ManagedFields = nil + Expect(k8sClient.Patch(ctx, cluster, client.Apply, client.FieldOwner("test"), client.ForceOwnership)).To(Succeed(), format.Object(cluster, 4)) + }) }) }) diff --git a/test/e2e/suites/apivalidations/openstackmachine_test.go b/test/e2e/suites/apivalidations/openstackmachine_test.go index 568a82b8d6..7e428bb374 100644 --- a/test/e2e/suites/apivalidations/openstackmachine_test.go +++ b/test/e2e/suites/apivalidations/openstackmachine_test.go @@ -21,9 +21,14 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/onsi/gomega/format" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + infrav1alpha6 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha6" + infrav1alpha7 "sigs.k8s.io/cluster-api-provider-openstack/api/v1alpha7" infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1" ) @@ -278,4 +283,54 @@ var _ = Describe("OpenStackMachine API validations", func() { Expect(k8sClient.Create(ctx, machine)).NotTo(Succeed(), "Creating a machine with an additional block device with an empty name availability zone should fail") }) }) + + Context("v1alpha7", func() { + It("should downgrade cleanly from infrav1", func() { + infrav1Machine := &infrav1.OpenStackMachine{} + infrav1Machine.Namespace = namespace.Name + infrav1Machine.GenerateName = clusterNamePrefix + infrav1Machine.Spec.IdentityRef = &infrav1.OpenStackIdentityReference{ + CloudName: "test-cloud", + Name: "test-credentials", + } + infrav1Machine.Spec.Image.ID = ptr.To("de9872ee-0c2c-44ed-9414-90163c8b0e0d") + Expect(createObj(infrav1Machine)).To(Succeed(), "infrav1 OpenStackMachine creation should succeed") + + // Just fetching the object as v1alpha6 doesn't trigger + // validation failure, so we first fetch it and then + // patch the object with identical contents. The patch + // triggers a validation failure. + machine := &infrav1alpha7.OpenStackMachine{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: infrav1Machine.Name, Namespace: infrav1Machine.Namespace}, machine)).To(Succeed(), "OpenStackMachine fetch should succeed") + + setObjectGVK(machine) + machine.ManagedFields = nil + Expect(k8sClient.Patch(ctx, machine, client.Apply, client.FieldOwner("test"), client.ForceOwnership)).To(Succeed(), format.Object(machine, 4)) + }) + }) + + Context("v1alpha6", func() { + It("should downgrade cleanly from infrav1", func() { + infrav1Machine := &infrav1.OpenStackMachine{} + infrav1Machine.Namespace = namespace.Name + infrav1Machine.GenerateName = clusterNamePrefix + infrav1Machine.Spec.IdentityRef = &infrav1.OpenStackIdentityReference{ + CloudName: "test-cloud", + Name: "test-credentials", + } + infrav1Machine.Spec.Image.ID = ptr.To("de9872ee-0c2c-44ed-9414-90163c8b0e0d") + Expect(createObj(infrav1Machine)).To(Succeed(), "infrav1 OpenStackMachine creation should succeed") + + // Just fetching the object as v1alpha6 doesn't trigger + // validation failure, so we first fetch it and then + // patch the object with identical contents. The patch + // triggers a validation failure. + machine := &infrav1alpha6.OpenStackMachine{} //nolint:staticcheck + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: infrav1Machine.Name, Namespace: infrav1Machine.Namespace}, machine)).To(Succeed(), "OpenStackMachine fetch should succeed") + + setObjectGVK(machine) + machine.ManagedFields = nil + Expect(k8sClient.Patch(ctx, machine, client.Apply, client.FieldOwner("test"), client.ForceOwnership)).To(Succeed(), format.Object(machine, 4)) + }) + }) }) diff --git a/test/e2e/suites/apivalidations/suite_test.go b/test/e2e/suites/apivalidations/suite_test.go index beac7d11aa..c8cca42d73 100644 --- a/test/e2e/suites/apivalidations/suite_test.go +++ b/test/e2e/suites/apivalidations/suite_test.go @@ -165,3 +165,21 @@ func createNamespace() *corev1.Namespace { By(fmt.Sprintf("Using namespace %s", namespace.Name)) return &namespace } + +func createObj(obj client.Object) error { + err := k8sClient.Create(ctx, obj) + if err == nil { + DeferCleanup(func() error { + return k8sClient.Delete(ctx, obj) + }) + } + return err +} + +func setObjectGVK(obj runtime.Object) { + gvk, unversioned, err := testScheme.ObjectKinds(obj) + Expect(unversioned).To(BeFalse(), "Object is considered unversioned") + Expect(err).ToNot(HaveOccurred(), "Error fetching gvk for Object") + Expect(gvk).To(HaveLen(1), "Object should have only one gvk") + obj.GetObjectKind().SetGroupVersionKind(gvk[0]) +}