fix: validates CONTROL_PLANE_ENDPOINT_IP and NUTANIX_ENDPOINT are dis…

…tinct (#1002) **What problem does this PR solve?**: webhook errors out if NUTANIX_ENDPOINT IP is same as PC IP. It only implements dumb check which compares PC IP with control-plane IP. **Which issue(s) this PR fixes**: Fixes # https://jira.nutanix.com/browse/NCN-102626
nutanix-cloud-native · Dec 18, 2024 · 6e96d3c · 6e96d3c
1 parent ce6738a
commit 6e96d3c
Show file tree

Hide file tree

Showing 6 changed files with 388 additions and 17 deletions.
diff --git a/api/v1alpha1/common_types.go b/api/v1alpha1/common_types.go
@@ -77,3 +77,15 @@ type LocalObjectReference struct {
 	// +kubebuilder:validation:MinLength=1
 	Name string `json:"name"`
 }
+
+func (s ControlPlaneEndpointSpec) VirtualIPAddress() string {
+	// If specified, use the virtual IP address and/or port,
+	// otherwise fall back to the control plane endpoint host and port.
+	if s.VirtualIPSpec != nil &&
+		s.VirtualIPSpec.Configuration != nil &&
+		s.VirtualIPSpec.Configuration.Address != "" {
+		return s.VirtualIPSpec.Configuration.Address
+	}
+
+	return s.Host
+}
diff --git a/api/v1alpha1/common_types_test.go b/api/v1alpha1/common_types_test.go
@@ -0,0 +1,67 @@
+// Copyright 2023 Nutanix. All rights reserved.
+// SPDX-License-Identifier: Apache-2.0
+package v1alpha1
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestVirtualIPAddress(t *testing.T) {
+	tests := []struct {
+		name     string
+		spec     ControlPlaneEndpointSpec
+		expected string
+	}{
+		{
+			name: "Virtual IP specified",
+			spec: ControlPlaneEndpointSpec{
+				VirtualIPSpec: &ControlPlaneVirtualIPSpec{
+					Configuration: &ControlPlaneVirtualIPConfiguration{
+						Address: "192.168.1.1",
+					},
+				},
+				Host: "192.168.1.2",
+			},
+			expected: "192.168.1.1",
+		},
+		{
+			name: "VirtualIPSpec struct not specified",
+			spec: ControlPlaneEndpointSpec{
+				VirtualIPSpec: nil,
+				Host:          "192.168.1.2",
+			},
+			expected: "192.168.1.2",
+		},
+		{
+			name: "ControlPlaneVirtualIPConfiguration struct not specified",
+			spec: ControlPlaneEndpointSpec{
+				VirtualIPSpec: &ControlPlaneVirtualIPSpec{
+					Configuration: nil,
+				},
+				Host: "192.168.1.2",
+			},
+			expected: "192.168.1.2",
+		},
+		{
+			name: "Virtual IP specified as empty string",
+			spec: ControlPlaneEndpointSpec{
+				VirtualIPSpec: &ControlPlaneVirtualIPSpec{
+					Configuration: &ControlPlaneVirtualIPConfiguration{
+						Address: "",
+					},
+				},
+				Host: "192.168.1.2",
+			},
+			expected: "192.168.1.2",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := tt.spec.VirtualIPAddress()
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}
diff --git a/api/v1alpha1/nutanix_clusterconfig_types.go b/api/v1alpha1/nutanix_clusterconfig_types.go
@@ -5,6 +5,7 @@ package v1alpha1
 
 import (
 	"fmt"
+	"net/netip"
 	"net/url"
 	"strconv"
 )
@@ -76,3 +77,17 @@ func (s NutanixPrismCentralEndpointSpec) ParseURL() (string, uint16, error) {
 
 	return hostname, uint16(port), nil
 }
+
+func (s NutanixPrismCentralEndpointSpec) ParseIP() (netip.Addr, error) {
+	pcHostname, _, err := s.ParseURL()
+	if err != nil {
+		return netip.Addr{}, err
+	}
+
+	pcIP, err := netip.ParseAddr(pcHostname)
+	if err != nil {
+		return netip.Addr{}, fmt.Errorf("error parsing Prism Central IP: %w", err)
+	}
+
+	return pcIP, nil
+}
diff --git a/api/v1alpha1/nutanix_clusterconfig_types_test.go b/api/v1alpha1/nutanix_clusterconfig_types_test.go
@@ -0,0 +1,132 @@
+// Copyright 2024 Nutanix. All rights reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package v1alpha1
+
+import (
+	"fmt"
+	"net/netip"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestParseURL(t *testing.T) {
+	tests := []struct {
+		name         string
+		spec         NutanixPrismCentralEndpointSpec
+		expectedHost string
+		expectedPort uint16
+		expectedErr  error
+	}{
+		{
+			name: "Valid URL with port",
+			spec: NutanixPrismCentralEndpointSpec{
+				URL: "https://192.168.1.1:9440",
+			},
+			expectedHost: "192.168.1.1",
+			expectedPort: 9440,
+			expectedErr:  nil,
+		},
+		{
+			name: "Valid URL without port",
+			spec: NutanixPrismCentralEndpointSpec{
+				URL: "https://192.168.1.1",
+			},
+			expectedHost: "192.168.1.1",
+			expectedPort: 9440,
+			expectedErr:  nil,
+		},
+		{
+			name: "Invalid URL",
+			spec: NutanixPrismCentralEndpointSpec{
+				URL: "invalid-url",
+			},
+			expectedHost: "",
+			expectedPort: 0,
+			expectedErr: fmt.Errorf(
+				"error parsing Prism Central URL: parse %q: invalid URI for request",
+				"invalid-url",
+			),
+		},
+		{
+			name: "Invalid port",
+			spec: NutanixPrismCentralEndpointSpec{
+				URL: "https://192.168.1.1:invalid-port",
+			},
+			expectedHost: "",
+			expectedPort: 0,
+			expectedErr: fmt.Errorf(
+				"error parsing Prism Central URL: parse %q: invalid port %q after host",
+				"https://192.168.1.1:invalid-port",
+				":invalid-port",
+			),
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			host, port, err := tt.spec.ParseURL()
+			if tt.expectedErr != nil {
+				require.EqualError(t, err, tt.expectedErr.Error())
+			} else {
+				require.NoError(t, err)
+			}
+			assert.Equal(t, tt.expectedHost, host)
+			assert.Equal(t, tt.expectedPort, port)
+		})
+	}
+}
+
+func TestParseIP(t *testing.T) {
+	tests := []struct {
+		name        string
+		spec        NutanixPrismCentralEndpointSpec
+		expectedIP  netip.Addr
+		expectedErr error
+	}{
+		{
+			name: "Valid IP",
+			spec: NutanixPrismCentralEndpointSpec{
+				URL: "https://192.168.1.1:9440",
+			},
+			expectedIP:  netip.MustParseAddr("192.168.1.1"),
+			expectedErr: nil,
+		},
+		{
+			name: "Invalid URL",
+			spec: NutanixPrismCentralEndpointSpec{
+				URL: "invalid-url",
+			},
+			expectedIP: netip.Addr{},
+			expectedErr: fmt.Errorf(
+				"error parsing Prism Central URL: parse %q: invalid URI for request",
+				"invalid-url",
+			),
+		},
+		{
+			name: "Invalid IP",
+			spec: NutanixPrismCentralEndpointSpec{
+				URL: "https://invalid-ip:9440",
+			},
+			expectedIP: netip.Addr{},
+			expectedErr: fmt.Errorf(
+				"error parsing Prism Central IP: ParseAddr(%q): unable to parse IP",
+				"invalid-ip",
+			),
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ip, err := tt.spec.ParseIP()
+			if tt.expectedErr != nil {
+				require.EqualError(t, err, tt.expectedErr.Error())
+			} else {
+				require.NoError(t, err)
+			}
+			assert.Equal(t, tt.expectedIP, ip)
+		})
+	}
+}
diff --git a/pkg/webhook/cluster/nutanix_validator.go b/pkg/webhook/cluster/nutanix_validator.go
@@ -7,8 +7,8 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"net"
 	"net/http"
+	"net/netip"
 
 	v1 "k8s.io/api/admission/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
@@ -70,15 +70,23 @@ func (a *nutanixValidator) validate(
 		)
 	}
 
-	if clusterConfig.Nutanix != nil &&
-		clusterConfig.Addons != nil {
-		// Check if Prism Central IP is in MetalLB Load Balancer IP range.
-		if err := validatePrismCentralIPNotInLoadBalancerIPRange(
+	if clusterConfig.Nutanix != nil {
+		if err := validatePrismCentralIPDoesNotEqualControlPlaneIP(
 			clusterConfig.Nutanix.PrismCentralEndpoint,
-			clusterConfig.Addons.ServiceLoadBalancer,
+			clusterConfig.Nutanix.ControlPlaneEndpoint,
 		); err != nil {
 			return admission.Denied(err.Error())
 		}
+
+		if clusterConfig.Addons != nil {
+			// Check if Prism Central IP is in MetalLB Load Balancer IP range.
+			if err := validatePrismCentralIPNotInLoadBalancerIPRange(
+				clusterConfig.Nutanix.PrismCentralEndpoint,
+				clusterConfig.Addons.ServiceLoadBalancer,
+			); err != nil {
+				return admission.Denied(err.Error())
+			}
+		}
 	}
 
 	return admission.Allowed("")
@@ -96,14 +104,10 @@ func validatePrismCentralIPNotInLoadBalancerIPRange(
 		return nil
 	}
 
-	pcHostname, _, err := pcEndpoint.ParseURL()
+	pcIP, err := pcEndpoint.ParseIP()
 	if err != nil {
-		return err
-	}
-
-	pcIP := net.ParseIP(pcHostname)
-	// PC URL can contain IP/FQDN, so compare only if PC is an IP address.
-	if pcIP == nil {
+		// If it's not able to parse IP correctly then, ignore the error as
+		// we want to compare only IP addresses.
 		return nil
 	}
 
@@ -131,3 +135,35 @@ func validatePrismCentralIPNotInLoadBalancerIPRange(
 
 	return nil
 }
+
+// validatePrismCentralIPDoesNotEqualControlPlaneIP checks if Prism Central and Control Plane IP are same,
+// error out if they are same.
+// It strictly compares IP addresses(no FQDN) and doesn't involve any network calls.
+func validatePrismCentralIPDoesNotEqualControlPlaneIP(
+	pcEndpoint v1alpha1.NutanixPrismCentralEndpointSpec,
+	controlPlaneEndpointSpec v1alpha1.ControlPlaneEndpointSpec,
+) error {
+	controlPlaneVIP, err := netip.ParseAddr(controlPlaneEndpointSpec.VirtualIPAddress())
+	if err != nil {
+		// If controlPlaneEndpointIP is a hostname, we cannot compare it with PC IP
+		// so return directly.
+		return nil
+	}
+
+	pcIP, err := pcEndpoint.ParseIP()
+	if err != nil {
+		// If it's not able to parse IP correctly then, ignore the error as
+		// we want to compare only IP addresses.
+		return nil
+	}
+
+	if pcIP.Compare(controlPlaneVIP) == 0 {
+		errMsg := fmt.Sprintf(
+			"Prism Central and control plane endpoint cannot have the same IP %q",
+			pcIP.String(),
+		)
+		return errors.New(errMsg)
+	}
+
+	return nil
+}