From e3d8c21effb86e84babaa6db9ab36aaf151767e0 Mon Sep 17 00:00:00 2001 From: dkokkino Date: Mon, 17 Mar 2025 16:06:11 +0100 Subject: [PATCH 1/5] Prevent default API and Ingress VIP generation for user-managed load balancers - Previously, when API and Ingress VIPs were not specified, default values were automatically generated for user-managed load balancers.This was unintended behavior. Now, if the user does not explicitly provide API and Ingress VIPs, a fatal error is thrown instead. --- pkg/types/openstack/defaults/platform.go | 54 +++++++++++++----------- 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/pkg/types/openstack/defaults/platform.go b/pkg/types/openstack/defaults/platform.go index fb87c12aa84..df7c65faf0c 100644 --- a/pkg/types/openstack/defaults/platform.go +++ b/pkg/types/openstack/defaults/platform.go @@ -6,6 +6,7 @@ import ( "github.com/apparentlymart/go-cidr/cidr" + configv1 "github.com/openshift/api/config/v1" "github.com/openshift/installer/pkg/types" "github.com/openshift/installer/pkg/types/openstack" ) @@ -25,32 +26,37 @@ func SetPlatformDefaults(p *openstack.Platform, n *types.Networking) { p.Cloud = DefaultCloudName } } - // APIVIP returns the internal virtual IP address (VIP) put in front - // of the Kubernetes API server for use by components inside the - // cluster. The DNS static pods running on the nodes resolve the - // api-int record to APIVIP. - if len(p.APIVIPs) == 0 && p.DeprecatedAPIVIP == "" { - vip, err := cidr.Host(&n.MachineNetwork[0].CIDR.IPNet, 5) - if err != nil { - // This will fail validation and abort the install - p.APIVIPs = []string{fmt.Sprintf("could not derive API VIP from machine networks: %s", err.Error())} - } else { - p.APIVIPs = []string{vip.String()} + + // When using user-managed loadbalancer do not generate default API and Ingress VIPs + if p.LoadBalancer.Type != configv1.LoadBalancerTypeUserManaged { + // APIVIP returns the internal virtual IP address (VIP) put in front + // of the Kubernetes API server for use by components inside the + // cluster. The DNS static pods running on the nodes resolve the + // api-int record to APIVIP. + if len(p.APIVIPs) == 0 && p.DeprecatedAPIVIP == "" { + vip, err := cidr.Host(&n.MachineNetwork[0].CIDR.IPNet, 5) + if err != nil { + // This will fail validation and abort the install + p.APIVIPs = []string{fmt.Sprintf("could not derive API VIP from machine networks: %s", err.Error())} + } else { + p.APIVIPs = []string{vip.String()} + } } - } - // IngressVIP returns the internal virtual IP address (VIP) put in - // front of the OpenShift router pods. This provides the internal - // accessibility to the internal pods running on the worker nodes, - // e.g. `console`. The DNS static pods running on the nodes resolve - // the wildcard apps record to IngressVIP. - if len(p.IngressVIPs) == 0 && p.DeprecatedIngressVIP == "" { - vip, err := cidr.Host(&n.MachineNetwork[0].CIDR.IPNet, 7) - if err != nil { - // This will fail validation and abort the install - p.IngressVIPs = []string{fmt.Sprintf("could not derive Ingress VIP from machine networks: %s", err.Error())} - } else { - p.IngressVIPs = []string{vip.String()} + // IngressVIP returns the internal virtual IP address (VIP) put in + // front of the OpenShift router pods. This provides the internal + // accessibility to the internal pods running on the worker nodes, + // e.g. `console`. The DNS static pods running on the nodes resolve + // the wildcard apps record to IngressVIP. + if len(p.IngressVIPs) == 0 && p.DeprecatedIngressVIP == "" { + vip, err := cidr.Host(&n.MachineNetwork[0].CIDR.IPNet, 7) + if err != nil { + // This will fail validation and abort the install + p.IngressVIPs = []string{fmt.Sprintf("could not derive Ingress VIP from machine networks: %s", err.Error())} + } else { + p.IngressVIPs = []string{vip.String()} + } } } + } From 8931ff740beed99e9f10c70c52ad994a2497d061 Mon Sep 17 00:00:00 2001 From: dkokkino Date: Fri, 21 Mar 2025 10:01:11 +0100 Subject: [PATCH 2/5] Add default load balancer if none is specified -If no load balancer is provided, a default OpenShift load balancer is now assigned automatically. This ensures proper handling and avoids misconfigurations. --- pkg/types/openstack/defaults/platform.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/types/openstack/defaults/platform.go b/pkg/types/openstack/defaults/platform.go index df7c65faf0c..3bbec5cff5a 100644 --- a/pkg/types/openstack/defaults/platform.go +++ b/pkg/types/openstack/defaults/platform.go @@ -26,6 +26,12 @@ func SetPlatformDefaults(p *openstack.Platform, n *types.Networking) { p.Cloud = DefaultCloudName } } + // When there is no loadbalancer present create a default Openshift managed loadbalancer + if p.LoadBalancer == nil { + p.LoadBalancer = &configv1.OpenStackPlatformLoadBalancer{ + Type: configv1.LoadBalancerTypeOpenShiftManagedDefault, + } + } // When using user-managed loadbalancer do not generate default API and Ingress VIPs if p.LoadBalancer.Type != configv1.LoadBalancerTypeUserManaged { From c9bfb8533e0df731eef937123451aaa2e1ddeed7 Mon Sep 17 00:00:00 2001 From: dkokkino Date: Tue, 25 Mar 2025 13:28:04 +0100 Subject: [PATCH 3/5] Add unit tests for OpenStack platform defaults Adds unit tests in platform_test.py to verify OpenStack platform defaults. Covers cases such as: - Assigning a default OpenShift-managed load balancer when none is specified - Handling user-managed load balancers with and without VIPs - Ensuring correct API and Ingress VIP assignments --- pkg/types/openstack/defaults/platform_test.go | 120 ++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 pkg/types/openstack/defaults/platform_test.go diff --git a/pkg/types/openstack/defaults/platform_test.go b/pkg/types/openstack/defaults/platform_test.go new file mode 100644 index 00000000000..4ea552c3ddd --- /dev/null +++ b/pkg/types/openstack/defaults/platform_test.go @@ -0,0 +1,120 @@ +package defaults + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + configv1 "github.com/openshift/api/config/v1" + "github.com/openshift/installer/pkg/ipnet" + "github.com/openshift/installer/pkg/types" + "github.com/openshift/installer/pkg/types/openstack" +) + +func validPlatform() *openstack.Platform { + return &openstack.Platform{ + Cloud: "test-cloud", + ExternalNetwork: "test-network", + } +} + +func validNetworking() *types.Networking { + return &types.Networking{ + NetworkType: "OVNKubernetes", + MachineNetwork: []types.MachineNetworkEntry{{ + CIDR: *ipnet.MustParseCIDR("10.0.0.0/16"), + }}, + } +} + +func TestSetPlatformDefaults(t *testing.T) { + cases := []struct { + name string + platform *openstack.Platform + config *types.InstallConfig + networking *types.Networking + expectedLB configv1.PlatformLoadBalancerType + expectedAPIVIPs []string + expectedIngressVIPs []string + }{ + { + name: "No load balancer provided", + platform: func() *openstack.Platform { + p := validPlatform() + return p + }(), + networking: validNetworking(), + expectedAPIVIPs: []string{"10.0.0.5"}, + expectedIngressVIPs: []string{"10.0.0.7"}, + expectedLB: "OpenShiftManagedDefault", + }, + { + name: "Default Openshift Managed load balancer VIPs provided", + platform: func() *openstack.Platform { + p := validPlatform() + p.LoadBalancer = &configv1.OpenStackPlatformLoadBalancer{ + Type: configv1.LoadBalancerTypeOpenShiftManagedDefault, + } + p.APIVIPs = []string{"10.0.0.2"} + p.IngressVIPs = []string{"10.0.0.3"} + return p + }(), + networking: validNetworking(), + expectedAPIVIPs: []string{"10.0.0.2"}, + expectedIngressVIPs: []string{"10.0.0.3"}, + expectedLB: "OpenShiftManagedDefault", + }, + { + name: "Default Openshift Managed load balancer no VIPs provided", + platform: func() *openstack.Platform { + p := validPlatform() + p.LoadBalancer = &configv1.OpenStackPlatformLoadBalancer{ + Type: configv1.LoadBalancerTypeOpenShiftManagedDefault, + } + return p + }(), + networking: validNetworking(), + expectedAPIVIPs: []string{"10.0.0.5"}, + expectedIngressVIPs: []string{"10.0.0.7"}, + expectedLB: "OpenShiftManagedDefault", + }, + { + name: "User managed load balancer VIPs provided", + platform: func() *openstack.Platform { + p := validPlatform() + p.LoadBalancer = &configv1.OpenStackPlatformLoadBalancer{ + Type: configv1.LoadBalancerTypeUserManaged, + } + p.APIVIPs = []string{"192.168.100.10"} + p.IngressVIPs = []string{"192.168.100.11"} + return p + }(), + networking: validNetworking(), + expectedAPIVIPs: []string{"192.168.100.10"}, + expectedIngressVIPs: []string{"192.168.100.11"}, + expectedLB: "UserManaged", + }, + { + name: "User managed load balancer no VIPs provided", + platform: func() *openstack.Platform { + p := validPlatform() + p.LoadBalancer = &configv1.OpenStackPlatformLoadBalancer{ + Type: configv1.LoadBalancerTypeUserManaged, + } + return p + }(), + networking: validNetworking(), + expectedAPIVIPs: []string(nil), + expectedIngressVIPs: []string(nil), + expectedLB: "UserManaged", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + SetPlatformDefaults(tc.platform, tc.networking) + assert.Equal(t, tc.expectedLB, tc.platform.LoadBalancer.Type, "unexpected loadbalancer") + assert.Equal(t, tc.expectedAPIVIPs, tc.platform.APIVIPs, "unexpected APIVIPs") + assert.Equal(t, tc.expectedIngressVIPs, tc.platform.IngressVIPs, "unexpected IngressVIPs") + }) + } +} From 02744627a3d082d9063720f8aa8181c27b979367 Mon Sep 17 00:00:00 2001 From: dkokkino Date: Fri, 28 Mar 2025 15:18:06 +0100 Subject: [PATCH 4/5] Edit manifest test - lb-default-stable: As a default load balancer is now being assigned when one is not provided this test needs to be updated to reflect that change - lb-unmanaged: I made changes to how the defaults are set. If the load balancer is user-managed VIPs will not automatically be assigned anymore. This change needs to be reflected in this test by adding a apiVIPs and ingressVIPs value to the install-config --- .../lb-default-stable/test_cluster-infra.py | 9 +++++++-- .../manifest-tests/lb-unmanaged/install-config.yaml | 4 ++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/scripts/openstack/manifest-tests/lb-default-stable/test_cluster-infra.py b/scripts/openstack/manifest-tests/lb-default-stable/test_cluster-infra.py index 217f2bf4e85..72533810027 100755 --- a/scripts/openstack/manifest-tests/lb-default-stable/test_cluster-infra.py +++ b/scripts/openstack/manifest-tests/lb-default-stable/test_cluster-infra.py @@ -20,8 +20,13 @@ def setUp(self): self.cluster_infra = yaml.load(f, Loader=yaml.FullLoader) def test_load_balancer(self): - """Assert that the Cluster infrastructure object does not contain the LoadBalancer configuration.""" - self.assertNotIn("loadBalancer", self.cluster_infra["status"]["platformStatus"]["openstack"]) + """Assert that the Cluster infrastructure object contains the default LoadBalancer configuration.""" + self.assertIn("loadBalancer", self.cluster_infra["status"]["platformStatus"]["openstack"]) + + loadBalancer = self.cluster_infra["status"]["platformStatus"]["openstack"]["loadBalancer"] + + self.assertIn("type", loadBalancer) + self.assertEqual("OpenShiftManagedDefault", loadBalancer["type"]) if __name__ == '__main__': diff --git a/scripts/openstack/manifest-tests/lb-unmanaged/install-config.yaml b/scripts/openstack/manifest-tests/lb-unmanaged/install-config.yaml index f900dca5973..3828c0e55ab 100644 --- a/scripts/openstack/manifest-tests/lb-unmanaged/install-config.yaml +++ b/scripts/openstack/manifest-tests/lb-unmanaged/install-config.yaml @@ -30,4 +30,8 @@ platform: cloud: ${OS_CLOUD} loadBalancer: type: UserManaged + apiVIPs: + - 10.0.128.10 + ingressVIPs: + - 10.0.128.20 pullSecret: ${PULL_SECRET} From 37ba673cbfe50b5d7c7d88887faa70153f7e907d Mon Sep 17 00:00:00 2001 From: Thuan Vo Date: Mon, 12 May 2025 18:44:48 -0700 Subject: [PATCH 5/5] OCPBUGS-56079: apply fixes for yaml-lint errors Notes: there are other yaml-lint warnings but they've been there for a long time. This commit only focuses on the errors introduced during 4.19 cycle. Conflicts: .golangci.yaml NOTE(stephenfin): Changes to .golangci.yaml were removed because they don't apply here. --- .../openstack/manifest-tests/lb-unmanaged/install-config.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/openstack/manifest-tests/lb-unmanaged/install-config.yaml b/scripts/openstack/manifest-tests/lb-unmanaged/install-config.yaml index 3828c0e55ab..ad0e989b307 100644 --- a/scripts/openstack/manifest-tests/lb-unmanaged/install-config.yaml +++ b/scripts/openstack/manifest-tests/lb-unmanaged/install-config.yaml @@ -31,7 +31,7 @@ platform: loadBalancer: type: UserManaged apiVIPs: - - 10.0.128.10 + - 10.0.128.10 ingressVIPs: - - 10.0.128.20 + - 10.0.128.20 pullSecret: ${PULL_SECRET}