Merge pull request #1501 from ching-kuo/add_ovn_lb_support

k8s-ci-robot · web-flow · commit a1be2929d720 · 2023-03-24T02:54:31.000-07:00
✨Add Octavia OVN Provider Support
diff --git a/api/v1alpha3/conversion_test.go b/api/v1alpha3/conversion_test.go
@@ -234,6 +234,7 @@ func TestFuzzyConversion(t *testing.T) {
 				v1alpha7Cluster.Spec.AllowAllInClusterTraffic = false
 				v1alpha7Cluster.Spec.DisableAPIServerFloatingIP = false
 				v1alpha7Cluster.Spec.APIServerLoadBalancer.AllowedCIDRs = nil
+				v1alpha7Cluster.Spec.APIServerLoadBalancer.Provider = ""
 				if v1alpha7Cluster.Spec.Bastion != nil {
 					v1alpha7Cluster.Spec.Bastion.Instance.ImageUUID = ""
 					v1alpha7Cluster.Spec.Bastion.Instance.Ports = nil
diff --git a/api/v1alpha4/conversion_test.go b/api/v1alpha4/conversion_test.go
@@ -335,6 +335,7 @@ func TestFuzzyConversion(t *testing.T) {
 				v1alpha7Cluster.ObjectMeta.Annotations = map[string]string{}
 
 				v1alpha7Cluster.Spec.APIServerLoadBalancer.AllowedCIDRs = nil
+				v1alpha7Cluster.Spec.APIServerLoadBalancer.Provider = ""
 
 				v1alpha7Cluster.Spec.ControlPlaneOmitAvailabilityZone = false
 
@@ -413,6 +414,7 @@ func TestFuzzyConversion(t *testing.T) {
 				v1alpha7ClusterTemplate.ObjectMeta.Annotations = map[string]string{}
 
 				v1alpha7ClusterTemplate.Spec.Template.Spec.APIServerLoadBalancer.AllowedCIDRs = nil
+				v1alpha7ClusterTemplate.Spec.Template.Spec.APIServerLoadBalancer.Provider = ""
 
 				v1alpha7ClusterTemplate.Spec.Template.Spec.ControlPlaneOmitAvailabilityZone = false
 
diff --git a/api/v1alpha5/conversion.go b/api/v1alpha5/conversion.go
@@ -235,3 +235,8 @@ func Convert_Slice_v1alpha7_Network_To_Slice_v1alpha5_Network(in *[]infrav1.Netw
 func Convert_v1alpha5_OpenStackMachineSpec_To_v1alpha7_OpenStackMachineSpec(in *OpenStackMachineSpec, out *infrav1.OpenStackMachineSpec, s conversion.Scope) error {
 	return autoConvert_v1alpha5_OpenStackMachineSpec_To_v1alpha7_OpenStackMachineSpec(in, out, s)
 }
+
+func Convert_v1alpha7_APIServerLoadBalancer_To_v1alpha5_APIServerLoadBalancer(in *infrav1.APIServerLoadBalancer, out *APIServerLoadBalancer, s conversion.Scope) error {
+	// Provider have been added in v1alpha7 but have no equivalent in v1alpha5
+	return autoConvert_v1alpha7_APIServerLoadBalancer_To_v1alpha5_APIServerLoadBalancer(in, out, s)
+}
diff --git a/api/v1alpha5/zz_generated.conversion.go b/api/v1alpha5/zz_generated.conversion.go
diff --git a/api/v1alpha6/conversion.go b/api/v1alpha6/conversion.go
@@ -235,3 +235,8 @@ func (r *OpenStackMachineTemplateList) ConvertFrom(srcRaw ctrlconversion.Hub) er
 func Convert_v1alpha6_OpenStackMachineSpec_To_v1alpha7_OpenStackMachineSpec(in *OpenStackMachineSpec, out *infrav1.OpenStackMachineSpec, s conversion.Scope) error {
 	return autoConvert_v1alpha6_OpenStackMachineSpec_To_v1alpha7_OpenStackMachineSpec(in, out, s)
 }
+
+func Convert_v1alpha7_APIServerLoadBalancer_To_v1alpha6_APIServerLoadBalancer(in *infrav1.APIServerLoadBalancer, out *APIServerLoadBalancer, s conversion.Scope) error {
+	// Provider have been added in v1alpha7 but have no equivalent in v1alpha6
+	return autoConvert_v1alpha7_APIServerLoadBalancer_To_v1alpha6_APIServerLoadBalancer(in, out, s)
+}
diff --git a/api/v1alpha6/conversion_test.go b/api/v1alpha6/conversion_test.go
@@ -19,8 +19,11 @@ package v1alpha6
 import (
 	"testing"
 
+	fuzz "github.com/google/gofuzz"
 	"github.com/onsi/gomega"
+	"k8s.io/apimachinery/pkg/api/apitesting/fuzzer"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	runtimeserializer "k8s.io/apimachinery/pkg/runtime/serializer"
 	utilconversion "sigs.k8s.io/cluster-api/util/conversion"
 	ctrlconversion "sigs.k8s.io/controller-runtime/pkg/conversion"
 
@@ -35,16 +38,33 @@ func TestFuzzyConversion(t *testing.T) {
 		delete(obj.GetAnnotations(), utilconversion.DataAnnotation)
 	}
 
+	fuzzerFuncs := func(_ runtimeserializer.CodecFactory) []interface{} {
+		return []interface{}{
+			func(v1alpha7Cluster *infrav1.OpenStackCluster, c fuzz.Continue) {
+				c.FuzzNoCustom(v1alpha7Cluster)
+
+				v1alpha7Cluster.Spec.APIServerLoadBalancer.Provider = ""
+			},
+			func(v1alpha7ClusterTemplate *infrav1.OpenStackClusterTemplate, c fuzz.Continue) {
+				c.FuzzNoCustom(v1alpha7ClusterTemplate)
+
+				v1alpha7ClusterTemplate.Spec.Template.Spec.APIServerLoadBalancer.Provider = ""
+			},
+		}
+	}
+
 	t.Run("for OpenStackCluster", utilconversion.FuzzTestFunc(utilconversion.FuzzTestFuncInput{
 		Hub:              &infrav1.OpenStackCluster{},
 		Spoke:            &OpenStackCluster{},
 		HubAfterMutation: ignoreDataAnnotation,
+		FuzzerFuncs:      []fuzzer.FuzzerFuncs{fuzzerFuncs},
 	}))
 
 	t.Run("for OpenStackClusterTemplate", utilconversion.FuzzTestFunc(utilconversion.FuzzTestFuncInput{
 		Hub:              &infrav1.OpenStackClusterTemplate{},
 		Spoke:            &OpenStackClusterTemplate{},
 		HubAfterMutation: ignoreDataAnnotation,
+		FuzzerFuncs:      []fuzzer.FuzzerFuncs{fuzzerFuncs},
 	}))
 
 	t.Run("for OpenStackMachine", utilconversion.FuzzTestFunc(utilconversion.FuzzTestFuncInput{
diff --git a/api/v1alpha6/zz_generated.conversion.go b/api/v1alpha6/zz_generated.conversion.go
diff --git a/api/v1alpha7/types.go b/api/v1alpha7/types.go
@@ -323,6 +323,8 @@ type APIServerLoadBalancer struct {
 	AdditionalPorts []int `json:"additionalPorts,omitempty"`
 	// AllowedCIDRs restrict access to all API-Server listeners to the given address CIDRs.
 	AllowedCIDRs []string `json:"allowedCidrs,omitempty"`
+	// Octavia Provider Used to create load balancer
+	Provider string `json:"provider,omitempty"`
 }
 
 // ValueSpec represents a single value_spec key-value pair.
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml
@@ -6174,6 +6174,9 @@ spec:
                     description: Enabled defines whether a load balancer should be
                       created.
                     type: boolean
+                  provider:
+                    description: Octavia Provider Used to create load balancer
+                    type: string
                 type: object
               apiServerPort:
                 description: APIServerPort is the port on which the listener on the
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclustertemplates.yaml
@@ -2274,6 +2274,9 @@ spec:
                             description: Enabled defines whether a load balancer should
                               be created.
                             type: boolean
+                          provider:
+                            description: Octavia Provider Used to create load balancer
+                            type: string
                         type: object
                       apiServerPort:
                         description: APIServerPort is the port on which the listener
diff --git a/pkg/cloud/services/loadbalancer/loadbalancer.go b/pkg/cloud/services/loadbalancer/loadbalancer.go
@@ -40,9 +40,8 @@ import (
 )
 
 const (
-	networkPrefix               string = "k8s-clusterapi"
-	kubeapiLBSuffix             string = "kubeapi"
-	defaultLoadBalancerProvider string = "amphora"
+	networkPrefix   string = "k8s-clusterapi"
+	kubeapiLBSuffix string = "kubeapi"
 )
 
 const loadBalancerProvisioningStatusActive = "ACTIVE"
@@ -64,13 +63,18 @@ func (s *Service) ReconcileLoadBalancer(openStackCluster *infrav1.OpenStackClust
 		return err
 	}
 
-	// As mostly all LoadBalancer features are only supported on "amphora" we explicitly set the provider
-	// in the LoadBalancer create call to make sure to get the desired features - even if multiple providers exist.
-	var lbProvider string
-	for _, v := range providers {
-		if v.Name == defaultLoadBalancerProvider {
-			lbProvider = v.Name
-			break
+	// Choose the selected provider if it is set in cluster spec, if not, omit the field and Octavia will use the default provider.
+	lbProvider := ""
+	if openStackCluster.Spec.APIServerLoadBalancer.Provider != "" {
+		for _, v := range providers {
+			if v.Name == openStackCluster.Spec.APIServerLoadBalancer.Provider {
+				lbProvider = v.Name
+				break
+			}
+		}
+		if lbProvider == "" {
+			record.Warnf(openStackCluster, "OctaviaProviderNotFound", "Provider specified for Octavia not found.")
+			record.Eventf(openStackCluster, "OctaviaProviderNotFound", "Provider %s specified for Octavia not found, using the default provider.", openStackCluster.Spec.APIServerLoadBalancer.Provider)
 		}
 	}
 
@@ -124,7 +128,7 @@ func (s *Service) ReconcileLoadBalancer(openStackCluster *infrav1.OpenStackClust
 			return err
 		}
 
-		pool, err := s.getOrCreatePool(openStackCluster, lbPortObjectsName, listener.ID, lb.ID)
+		pool, err := s.getOrCreatePool(openStackCluster, lbPortObjectsName, listener.ID, lb.ID, lb.Provider)
 		if err != nil {
 			return err
 		}
@@ -300,7 +304,7 @@ func validateIPs(openStackCluster *infrav1.OpenStackCluster, definedCIDRs []stri
 	return marshaledCIDRs
 }
 
-func (s *Service) getOrCreatePool(openStackCluster *infrav1.OpenStackCluster, poolName, listenerID, lbID string) (*pools.Pool, error) {
+func (s *Service) getOrCreatePool(openStackCluster *infrav1.OpenStackCluster, poolName, listenerID, lbID string, lbProvider string) (*pools.Pool, error) {
 	pool, err := s.checkIfPoolExists(poolName)
 	if err != nil {
 		return nil, err
@@ -312,10 +316,16 @@ func (s *Service) getOrCreatePool(openStackCluster *infrav1.OpenStackCluster, po
 
 	s.scope.Logger().Info(fmt.Sprintf("Creating load balancer pool for listener %q", listenerID), "name", poolName, "lb-id", lbID)
 
+	method := pools.LBMethodRoundRobin
+
+	if lbProvider == "ovn" {
+		method = pools.LBMethodSourceIpPort
+	}
+
 	poolCreateOpts := pools.CreateOpts{
 		Name:       poolName,
 		Protocol:   "TCP",
-		LBMethod:   pools.LBMethodRoundRobin,
+		LBMethod:   method,
 		ListenerID: listenerID,
 		Tags:       openStackCluster.Spec.Tags,
 	}
@@ -356,6 +366,13 @@ func (s *Service) getOrCreateMonitor(openStackCluster *infrav1.OpenStackCluster,
 		Timeout:        5,
 	}
 	monitor, err = s.loadbalancerClient.CreateMonitor(monitorCreateOpts)
+	// Skip creating monitor if it is not supported by Octavia provider
+	if capoerrors.IsNotImplementedError(err) {
+		record.Warnf(openStackCluster, "SkippedCreateMonitor", "Health monitors are not supported with the current Octavia provider.")
+		record.Eventf(openStackCluster, "SkippedCreateMonitor", "Health Monitor is not created as it's not implemented with the current Octavia provider.")
+		return nil
+	}
+
 	if err != nil {
 		record.Warnf(openStackCluster, "FailedCreateMonitor", "Failed to create monitor %s: %v", monitorName, err)
 		return err
diff --git a/pkg/utils/errors/errors.go b/pkg/utils/errors/errors.go
@@ -86,3 +86,14 @@ func IsConflict(err error) bool {
 
 	return false
 }
+
+func IsNotImplementedError(err error) bool {
+	var errUnexpectedResponseCode gophercloud.ErrUnexpectedResponseCode
+	if errors.As(err, &errUnexpectedResponseCode) {
+		if errUnexpectedResponseCode.Actual == http.StatusNotImplemented {
+			return true
+		}
+	}
+
+	return false
+}

Original file line number	Diff line number	Diff line change
`@@ -235,3 +235,8 @@ func Convert_Slice_v1alpha7_Network_To_Slice_v1alpha5_Network(in *[]infrav1.Netw`
`235`	`235`	`func Convert_v1alpha5_OpenStackMachineSpec_To_v1alpha7_OpenStackMachineSpec(in OpenStackMachineSpec, out infrav1.OpenStackMachineSpec, s conversion.Scope) error {`
`236`	`236`	`return autoConvert_v1alpha5_OpenStackMachineSpec_To_v1alpha7_OpenStackMachineSpec(in, out, s)`
`237`	`237`	`}`
	`238`	`+`
	`239`	`+func Convert_v1alpha7_APIServerLoadBalancer_To_v1alpha5_APIServerLoadBalancer(in infrav1.APIServerLoadBalancer, out APIServerLoadBalancer, s conversion.Scope) error {`
	`240`	`+ // Provider have been added in v1alpha7 but have no equivalent in v1alpha5`
	`241`	`+ return autoConvert_v1alpha7_APIServerLoadBalancer_To_v1alpha5_APIServerLoadBalancer(in, out, s)`
	`242`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -235,3 +235,8 @@ func (r *OpenStackMachineTemplateList) ConvertFrom(srcRaw ctrlconversion.Hub) er`
`235`	`235`	`func Convert_v1alpha6_OpenStackMachineSpec_To_v1alpha7_OpenStackMachineSpec(in OpenStackMachineSpec, out infrav1.OpenStackMachineSpec, s conversion.Scope) error {`
`236`	`236`	`return autoConvert_v1alpha6_OpenStackMachineSpec_To_v1alpha7_OpenStackMachineSpec(in, out, s)`
`237`	`237`	`}`
	`238`	`+`
	`239`	`+func Convert_v1alpha7_APIServerLoadBalancer_To_v1alpha6_APIServerLoadBalancer(in infrav1.APIServerLoadBalancer, out APIServerLoadBalancer, s conversion.Scope) error {`
	`240`	`+ // Provider have been added in v1alpha7 but have no equivalent in v1alpha6`
	`241`	`+ return autoConvert_v1alpha7_APIServerLoadBalancer_To_v1alpha6_APIServerLoadBalancer(in, out, s)`
	`242`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -323,6 +323,8 @@ type APIServerLoadBalancer struct {`
`323`	`323`	AdditionalPorts []int `json:"additionalPorts,omitempty"`
`324`	`324`	`// AllowedCIDRs restrict access to all API-Server listeners to the given address CIDRs.`
`325`	`325`	AllowedCIDRs []string `json:"allowedCidrs,omitempty"`
	`326`	`+ // Octavia Provider Used to create load balancer`
	`327`	+ Provider string `json:"provider,omitempty"`
`326`	`328`	`}`
`327`	`329`
`328`	`330`	`// ValueSpec represents a single value_spec key-value pair.`