From 86635bea8d7552e6a0bb2eb859659989c0ff8dbe Mon Sep 17 00:00:00 2001 From: Maxmilian Geberl Date: Wed, 21 Dec 2022 16:21:45 +0100 Subject: [PATCH] add tests, docs and fix lint --- README.md | 7 + .../targetcontroller/service_controller.go | 2 +- .../service_controller_test.go | 154 ++++++++++++++++++ .../loadbalancer/loadbalancer_controller.go | 5 +- .../loadbalancer_controller_test.go | 12 +- .../loadbalancermachine_controller.go | 2 +- internal/helper/errors.go | 2 +- internal/helper/openstack/client.go | 1 + internal/openstack/client.go | 21 ++- internal/openstack/testing/client.go | 2 +- 10 files changed, 197 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 80d0342e..c64947d3 100644 --- a/README.md +++ b/README.md @@ -194,6 +194,13 @@ metadata: # If this is set to a different network ID than defined as default in the yawol-cloud-controller # the default from the yawol-cloud-controller will be added to the additionalNetworks. yawol.stackit.cloud/defaultNetworkID: "OS-networkID" + # If set to true it do not add the default network ID from + # the yawol-cloud-controller to the additionalNetworks. + yawol.stackit.cloud/skipCloudControllerDefaultNetworkID: "false" + # Overwrites the projectID which is set by the secret. + # If not set the settings from the secret binding will be used. + # This field is immutable and can not be changed after the service is created. + yawol.stackit.cloud/projectID: "OS-ProjectID" # Overwrites the openstack floating network for the loadbalancer. yawol.stackit.cloud/floatingNetworkID: "OS-floatingNetID" # Override the default OpenStack availability zone. diff --git a/controllers/yawol-cloud-controller/targetcontroller/service_controller.go b/controllers/yawol-cloud-controller/targetcontroller/service_controller.go index 99c574d2..5f122345 100644 --- a/controllers/yawol-cloud-controller/targetcontroller/service_controller.go +++ b/controllers/yawol-cloud-controller/targetcontroller/service_controller.go @@ -240,7 +240,7 @@ func (r *ServiceReconciler) reconcileInfrastructure( } if !reflect.DeepEqual(newInfra.ProjectID, lb.Spec.Infrastructure.ProjectID) { - return helper.ErrProjectIsImmutable + return kubernetes.SendErrorAsEvent(r.Recorder, helper.ErrProjectIsImmutable, svc) } if !reflect.DeepEqual(newInfra, lb.Spec.Infrastructure) { diff --git a/controllers/yawol-cloud-controller/targetcontroller/service_controller_test.go b/controllers/yawol-cloud-controller/targetcontroller/service_controller_test.go index b4e31758..894c13f1 100644 --- a/controllers/yawol-cloud-controller/targetcontroller/service_controller_test.go +++ b/controllers/yawol-cloud-controller/targetcontroller/service_controller_test.go @@ -2108,5 +2108,159 @@ var _ = Describe("Check loadbalancer reconcile", func() { return fmt.Errorf("additionalNetwork are still set %v", lb.Spec.Infrastructure.AdditionalNetworks) }, time.Second*5, time.Millisecond*500).Should(Succeed()) }) + + It("should update the skipCloudControllerDefaultNetworkID field", func() { + By("creating a service without skipCloudControllerDefaultNetworkID") + service := v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "service-test31", + Namespace: "default", + Annotations: map[string]string{ + yawolv1beta1.ServiceDefaultNetworkID: "default-networkID", + }, + }, + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + Name: "port1", + Protocol: v1.ProtocolTCP, + Port: 12345, + TargetPort: intstr.IntOrString{IntVal: 12345}, + NodePort: 31031, + }, + }, + Type: "LoadBalancer", + }} + Expect(k8sClient.Create(ctx, &service)).Should(Succeed()) + + By("checking that defaultNetworkID is in additional networks") + Eventually(func() error { + err := k8sClient.Get(ctx, types.NamespacedName{Name: "default--service-test31", Namespace: "default"}, &lb) + if err != nil { + return err + } + if len(lb.Spec.Infrastructure.AdditionalNetworks) == 1 && + lb.Spec.Infrastructure.AdditionalNetworks[0].NetworkID == *testInfraDefaults.NetworkID { + return nil + } + return fmt.Errorf("additionalNetwork is not set with defaultNetwork %v", lb.Spec.Infrastructure.AdditionalNetworks) + }, time.Second*5, time.Millisecond*500).Should(Succeed()) + + By("update svc to add skipCloudControllerDefaultNetworkID") + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: service.Name, Namespace: service.Namespace}, &service)).Should(Succeed()) + service.ObjectMeta.Annotations = map[string]string{ + yawolv1beta1.ServiceDefaultNetworkID: "default-networkID", + yawolv1beta1.ServiceSkipCloudControllerDefaultNetworkID: "true", + } + Expect(k8sClient.Update(ctx, &service)).Should(Succeed()) + + By("check if default network is not present in additionalNetworks") + Eventually(func() error { + err := k8sClient.Get(ctx, types.NamespacedName{Name: "default--service-test31", Namespace: "default"}, &lb) + if err != nil { + return err + } + if len(lb.Spec.Infrastructure.AdditionalNetworks) == 0 { + return nil + } + return fmt.Errorf("additionalNetworks are not correct %v", lb.Spec.Infrastructure.AdditionalNetworks) + }, time.Second*5, time.Millisecond*500).Should(Succeed()) + + By("update svc to disable skipCloudControllerDefaultNetworkID") + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: service.Name, Namespace: service.Namespace}, &service)).Should(Succeed()) + service.ObjectMeta.Annotations = map[string]string{ + yawolv1beta1.ServiceDefaultNetworkID: "default-networkID", + } + Expect(k8sClient.Update(ctx, &service)).Should(Succeed()) + + By("checking that defaultNetworkID is in additional networks") + Eventually(func() error { + err := k8sClient.Get(ctx, types.NamespacedName{Name: "default--service-test31", Namespace: "default"}, &lb) + if err != nil { + return err + } + if len(lb.Spec.Infrastructure.AdditionalNetworks) == 1 && + lb.Spec.Infrastructure.AdditionalNetworks[0].NetworkID == *testInfraDefaults.NetworkID { + return nil + } + return fmt.Errorf("additionalNetwork is not set with defaultNetwork %v", lb.Spec.Infrastructure.AdditionalNetworks) + }, time.Second*5, time.Millisecond*500).Should(Succeed()) + }) + + It("should be possible to add a project - but it is immutable", func() { + By("creating a service with different project") + service := v1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "service-test32", + Namespace: "default", + Annotations: map[string]string{ + yawolv1beta1.ServiceDefaultProjectID: "otherProject", + }, + }, + Spec: v1.ServiceSpec{ + Ports: []v1.ServicePort{ + { + Name: "port1", + Protocol: v1.ProtocolTCP, + Port: 12345, + TargetPort: intstr.IntOrString{IntVal: 12345}, + NodePort: 31032, + }, + }, + Type: "LoadBalancer", + }} + Expect(k8sClient.Create(ctx, &service)).Should(Succeed()) + + By("checking that projectID is set") + Eventually(func() error { + err := k8sClient.Get(ctx, types.NamespacedName{Name: "default--service-test32", Namespace: "default"}, &lb) + if err != nil { + return err + } + if lb.Spec.Infrastructure.ProjectID != nil && + *lb.Spec.Infrastructure.ProjectID == "otherProject" { + return nil + } + return fmt.Errorf("projectID is not set in infrastructure %v", lb.Spec.Infrastructure.ProjectID) + }, time.Second*5, time.Millisecond*500).Should(Succeed()) + + By("update svc with other project - should not work") + Expect(k8sClient.Get(ctx, client.ObjectKey{Name: service.Name, Namespace: service.Namespace}, &service)).Should(Succeed()) + service.ObjectMeta.Annotations = map[string]string{ + yawolv1beta1.ServiceDefaultProjectID: "otherProject2", + } + Expect(k8sClient.Update(ctx, &service)).Should(Succeed()) + + By("check for error event") + Eventually(func() error { + eventList := v1.EventList{} + err := k8sClient.List(ctx, &eventList) + if err != nil { + return err + } + for _, event := range eventList.Items { + if event.InvolvedObject.Name == "service-test32" && + event.InvolvedObject.Kind == "Service" && + event.Type == v1.EventTypeWarning && + strings.Contains(event.Message, helper.ErrProjectIsImmutable.Error()) { + return nil + } + } + return helper.ErrNoEventFound + }, time.Second*5, time.Millisecond*500).Should(Succeed()) + + By("checking that projectID is still the ID from the creation") + Eventually(func() error { + err := k8sClient.Get(ctx, types.NamespacedName{Name: "default--service-test32", Namespace: "default"}, &lb) + if err != nil { + return err + } + if lb.Spec.Infrastructure.ProjectID != nil && + *lb.Spec.Infrastructure.ProjectID == "otherProject" { + return nil + } + return fmt.Errorf("projectID is not correctly set in infrastructure %v", lb.Spec.Infrastructure.ProjectID) + }, time.Second*5, time.Millisecond*500).Should(Succeed()) + }) }) }) diff --git a/controllers/yawol-controller/loadbalancer/loadbalancer_controller.go b/controllers/yawol-controller/loadbalancer/loadbalancer_controller.go index a1e3ac45..25f3cdfa 100644 --- a/controllers/yawol-controller/loadbalancer/loadbalancer_controller.go +++ b/controllers/yawol-controller/loadbalancer/loadbalancer_controller.go @@ -374,7 +374,7 @@ func (r *Reconciler) assignOrCreateFIP( return nil } -func (r *Reconciler) reconcilePort( +func (r *Reconciler) reconcilePort( //nolint: gocyclo // TODO reduce complexity in future ctx context.Context, req ctrl.Request, lb *yawolv1beta1.LoadBalancer, @@ -1106,7 +1106,6 @@ func (r *Reconciler) deletePorts( var requeue bool - //nolint: dupl // we can't extract this code because of generics if lb.Status.PortID != nil { port, err := openstackhelper.GetPortByID(ctx, portClient, *lb.Status.PortID) if err != nil { @@ -1132,6 +1131,7 @@ func (r *Reconciler) deletePorts( } // clean up orphan ports + //nolint: dupl // we can't extract this code because of generics if lb.Status.PortName != nil { portName := *lb.Status.PortName var portList []ports.Port @@ -1221,6 +1221,7 @@ func (r *Reconciler) deleteSecGroups( } // clean up orphan secgroups + //nolint: dupl // we can't extract this code because of generics if lb.Status.SecurityGroupName != nil { secGroupName := *lb.Status.SecurityGroupName var secGroupList []groups.SecGroup diff --git a/controllers/yawol-controller/loadbalancer/loadbalancer_controller_test.go b/controllers/yawol-controller/loadbalancer/loadbalancer_controller_test.go index a7aa5c4d..e041f005 100644 --- a/controllers/yawol-controller/loadbalancer/loadbalancer_controller_test.go +++ b/controllers/yawol-controller/loadbalancer/loadbalancer_controller_test.go @@ -103,12 +103,14 @@ var _ = Describe("loadbalancer controller", func() { g.Expect(act.Status.PortID).ToNot(BeNil()) g.Expect(act.Status.PortName).ToNot(BeNil()) + g.Expect(act.Status.PortIP).ToNot(BeNil()) + g.Expect(*act.Status.PortIP).ToNot(Equal("")) + g.Expect(act.Status.FloatingID).To(BeNil()) g.Expect(act.Status.FloatingName).To(BeNil()) g.Expect(act.Status.ExternalIP).ToNot(BeNil()) g.Expect(*act.Status.ExternalIP).ToNot(Equal("")) - return nil }) }) @@ -118,11 +120,17 @@ var _ = Describe("loadbalancer controller", func() { It("should swap to an internal lb", func() { By("checking that the lb gets created with an public ip") hopefully(lbNN, func(g Gomega, act LB) error { + g.Expect(act.Status.PortID).ToNot(BeNil()) + g.Expect(act.Status.PortName).ToNot(BeNil()) + + g.Expect(act.Status.PortIP).ToNot(BeNil()) + g.Expect(*act.Status.PortIP).ToNot(Equal("")) + g.Expect(act.Status.FloatingID).ToNot(BeNil()) g.Expect(act.Status.FloatingName).ToNot(BeNil()) g.Expect(act.Status.ExternalIP).ToNot(BeNil()) - + g.Expect(*act.Status.ExternalIP).ToNot(Equal("")) return nil }) diff --git a/controllers/yawol-controller/loadbalancermachine/loadbalancermachine_controller.go b/controllers/yawol-controller/loadbalancermachine/loadbalancermachine_controller.go index 46592bf7..2babd4cc 100644 --- a/controllers/yawol-controller/loadbalancermachine/loadbalancermachine_controller.go +++ b/controllers/yawol-controller/loadbalancermachine/loadbalancermachine_controller.go @@ -341,7 +341,7 @@ func (r *LoadBalancerMachineReconciler) reconcileRoleBinding( return nil } -func (r *LoadBalancerMachineReconciler) reconcilePort( +func (r *LoadBalancerMachineReconciler) reconcilePort( //nolint: gocyclo // TODO reduce complexity in future ctx context.Context, osClient os.Client, req ctrl.Request, diff --git a/internal/helper/errors.go b/internal/helper/errors.go index dfef32a4..b98f5fb5 100644 --- a/internal/helper/errors.go +++ b/internal/helper/errors.go @@ -65,5 +65,5 @@ var ( ErrCouldNotParseSourceRange = errors.New("could not parse LoadBalancerSourceRange") ErrListingChildLBMs = errors.New("unable to list child loadbalancerMachines") ErrUnsupportedProtocol = errors.New("unsupported protocol used (TCP and UDP is supported)") - ErrProjectIsImmutable = errors.New("project id is immutable, cant be changed after inital creation") + ErrProjectIsImmutable = errors.New("project id is immutable, cant be changed after initial creation") ) diff --git a/internal/helper/openstack/client.go b/internal/helper/openstack/client.go index bd3a1278..11f9700d 100644 --- a/internal/helper/openstack/client.go +++ b/internal/helper/openstack/client.go @@ -3,6 +3,7 @@ package openstack import ( "context" "fmt" + yawolv1beta1 "github.com/stackitcloud/yawol/api/v1beta1" "github.com/stackitcloud/yawol/internal/openstack" diff --git a/internal/openstack/client.go b/internal/openstack/client.go index 7959d395..abbff7da 100644 --- a/internal/openstack/client.go +++ b/internal/openstack/client.go @@ -43,7 +43,12 @@ type OSClient struct { // username="itmyuser" // password="suupersecret" // region="eu01" -func (r *OSClient) Configure(iniBytes []byte, overwrite OSClientOverwrite, timeout time.Duration, promCounter *prometheus.CounterVec) error { +func (r *OSClient) Configure( + iniBytes []byte, + overwrite OSClientOverwrite, + timeout time.Duration, + promCounter *prometheus.CounterVec, +) error { r.ini = iniBytes r.timeout = timeout r.promCounter = promCounter @@ -163,7 +168,12 @@ func (r *OSClient) KeyPairClient(ctx context.Context) (KeyPairClient, error) { return client.Configure(r.computeV2, r.timeout, r.promCounter), nil } -func createNetworkV2FromIni(ctx context.Context, iniData []byte, overwrite OSClientOverwrite, timeout time.Duration) (*gophercloud.ServiceClient, error) { +func createNetworkV2FromIni( + ctx context.Context, + iniData []byte, + overwrite OSClientOverwrite, + timeout time.Duration, +) (*gophercloud.ServiceClient, error) { provider, opts, err := getProvider(ctx, iniData, overwrite, timeout) if err != nil { return nil, err @@ -177,7 +187,12 @@ func createNetworkV2FromIni(ctx context.Context, iniData []byte, overwrite OSCli return client, nil } -func createComputeV2FromIni(ctx context.Context, iniData []byte, overwrite OSClientOverwrite, timeout time.Duration) (*gophercloud.ServiceClient, error) { +func createComputeV2FromIni( + ctx context.Context, + iniData []byte, + overwrite OSClientOverwrite, + timeout time.Duration, +) (*gophercloud.ServiceClient, error) { provider, opts, err := getProvider(ctx, iniData, overwrite, timeout) if err != nil { return nil, err diff --git a/internal/openstack/testing/client.go b/internal/openstack/testing/client.go index b93cc3cb..83d487dd 100644 --- a/internal/openstack/testing/client.go +++ b/internal/openstack/testing/client.go @@ -39,7 +39,7 @@ type MockClient struct { ServerGroupClientObj openstack.ServerGroupClient } -func (r *MockClient) Configure(ini []byte, overwrite openstack.OSClientOverwrite, timeout time.Duration, promCounter *prometheus.CounterVec) error { +func (r *MockClient) Configure(_ []byte, _ openstack.OSClientOverwrite, _ time.Duration, _ *prometheus.CounterVec) error { r.StoredValues = make(map[string]interface{}) return nil }