diff --git a/pkg/cmd/render/bootstrap_ip.go b/pkg/cmd/render/bootstrap_ip.go index fcee41ed0f..03b3756449 100644 --- a/pkg/cmd/render/bootstrap_ip.go +++ b/pkg/cmd/render/bootstrap_ip.go @@ -137,9 +137,71 @@ func ContainedByCIDR(cidr string) AddressFilter { } } +// normalizeIPv4 removes leading zeros from IPv4 addresses. +// For example, "192.000.002.001" becomes "192.0.2.1". +// Returns the normalized string if it's a valid IPv4, otherwise returns the original. +func normalizeIPv4(ip string) string { + // Try parsing as-is first + if parsed := net.ParseIP(ip); parsed != nil { + return ip + } + + // Check if it looks like an IPv4 with potential leading zeros + parts := make([]string, 0, 4) + octet := "" + dotCount := 0 + + for _, ch := range ip { + if ch == '.' { + if octet == "" { + return ip // Invalid: empty octet + } + // Remove leading zeros from octet + normalized := octet + for len(normalized) > 1 && normalized[0] == '0' { + normalized = normalized[1:] + } + parts = append(parts, normalized) + octet = "" + dotCount++ + } else if ch >= '0' && ch <= '9' { + octet += string(ch) + } else { + return ip // Invalid character + } + } + + // Add the last octet + if octet == "" || dotCount != 3 { + return ip // Invalid format + } + normalized := octet + for len(normalized) > 1 && normalized[0] == '0' { + normalized = normalized[1:] + } + parts = append(parts, normalized) + + result := fmt.Sprintf("%s.%s.%s.%s", parts[0], parts[1], parts[2], parts[3]) + + // Verify it's valid + if net.ParseIP(result) == nil { + return ip // Return original if normalization failed + } + + return result +} + func AddressNotIn(ips ...string) AddressFilter { return func(addr netlink.Addr) bool { - return !slices.Contains(ips, addr.IP.String()) + canonicalAddr := addr.IP.String() + for _, ip := range ips { + normalizedIP := normalizeIPv4(ip) + parsedIP := net.ParseIP(normalizedIP) + if parsedIP != nil && canonicalAddr == parsedIP.String() { + return false + } + } + return true } } diff --git a/pkg/cmd/render/bootstrap_ip_test.go b/pkg/cmd/render/bootstrap_ip_test.go index fad4ab09f2..aecf870628 100644 --- a/pkg/cmd/render/bootstrap_ip_test.go +++ b/pkg/cmd/render/bootstrap_ip_test.go @@ -234,3 +234,194 @@ func withRoute(linkIndex int, dst string, protocol int) LinkRoutes { }, } } + +func TestAddressNotIn(t *testing.T) { + scenarios := []struct { + name string + excludedIPs []string + testAddr string + expected bool + }{ + { + name: "address not in empty exclusion list", + excludedIPs: []string{}, + testAddr: "192.0.2.1", + expected: true, + }, + { + name: "address not in exclusion list", + excludedIPs: []string{"192.0.2.2", "192.0.2.3"}, + testAddr: "192.0.2.1", + expected: true, + }, + { + name: "address in exclusion list", + excludedIPs: []string{"192.0.2.1", "192.0.2.2"}, + testAddr: "192.0.2.1", + expected: false, + }, + { + name: "IPv4 canonicalization - leading zeros", + excludedIPs: []string{"192.000.002.001"}, + testAddr: "192.0.2.1", + expected: false, + }, + { + name: "IPv6 address not in exclusion list", + excludedIPs: []string{"2001:db8::2", "2001:db8::3"}, + testAddr: "2001:db8::1", + expected: true, + }, + { + name: "IPv6 address in exclusion list", + excludedIPs: []string{"2001:db8::1", "2001:db8::2"}, + testAddr: "2001:db8::1", + expected: false, + }, + { + name: "IPv6 canonicalization - expanded form", + excludedIPs: []string{"2001:0db8:0000:0000:0000:0000:0000:0001"}, + testAddr: "2001:db8::1", + expected: false, + }, + { + name: "IPv6 canonicalization - compressed form matches expanded", + excludedIPs: []string{"2001:db8::1"}, + testAddr: "2001:0db8:0000:0000:0000:0000:0000:0001", + expected: false, + }, + { + name: "invalid IP in exclusion list is ignored", + excludedIPs: []string{"not-an-ip", "192.0.2.2"}, + testAddr: "192.0.2.1", + expected: true, + }, + { + name: "multiple exclusions with mixed valid and invalid", + excludedIPs: []string{"invalid", "192.0.2.1", "also-invalid"}, + testAddr: "192.0.2.1", + expected: false, + }, + { + name: "IPv4-mapped IPv6 address", + excludedIPs: []string{"::ffff:192.0.2.1"}, + testAddr: "::ffff:192.0.2.1", + expected: false, + }, + { + name: "single IP exclusion", + excludedIPs: []string{"10.0.0.1"}, + testAddr: "10.0.0.1", + expected: false, + }, + { + name: "many exclusions, address not present", + excludedIPs: []string{"10.0.0.1", "10.0.0.2", "10.0.0.3", "10.0.0.4", "10.0.0.5"}, + testAddr: "10.0.0.100", + expected: true, + }, + { + name: "many exclusions, address present at end", + excludedIPs: []string{"10.0.0.1", "10.0.0.2", "10.0.0.3", "10.0.0.4", "10.0.0.100"}, + testAddr: "10.0.0.100", + expected: false, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + filter := AddressNotIn(scenario.excludedIPs...) + testNetlinkAddr := netlink.Addr{ + IPNet: netlink.NewIPNet(net.ParseIP(scenario.testAddr)), + } + result := filter(testNetlinkAddr) + if result != scenario.expected { + t.Errorf("expected %v, got %v for address %s with exclusions %v", + scenario.expected, result, scenario.testAddr, scenario.excludedIPs) + } + }) + } +} + +func TestNormalizeIPv4(t *testing.T) { + scenarios := []struct { + name string + input string + expected string + }{ + { + name: "IPv4 with leading zeros", + input: "192.000.002.001", + expected: "192.0.2.1", + }, + { + name: "IPv4 without leading zeros", + input: "192.0.2.1", + expected: "192.0.2.1", + }, + { + name: "IPv4 with mixed leading zeros", + input: "010.000.002.100", + expected: "10.0.2.100", + }, + { + name: "IPv4 all zeros", + input: "000.000.000.000", + expected: "0.0.0.0", + }, + { + name: "IPv4 with all leading zeros in last octet", + input: "192.168.1.001", + expected: "192.168.1.1", + }, + { + name: "IPv6 address - should return as-is", + input: "2001:db8::1", + expected: "2001:db8::1", + }, + { + name: "IPv6 with leading zeros - should return as-is", + input: "2001:0db8:0000:0000:0000:0000:0000:0001", + expected: "2001:0db8:0000:0000:0000:0000:0000:0001", + }, + { + name: "invalid IP - should return as-is", + input: "not-an-ip", + expected: "not-an-ip", + }, + { + name: "empty string - should return as-is", + input: "", + expected: "", + }, + { + name: "IPv4 with too few octets - should return as-is", + input: "192.168.1", + expected: "192.168.1", + }, + { + name: "IPv4 with too many octets - should return as-is", + input: "192.168.1.1.1", + expected: "192.168.1.1.1", + }, + { + name: "IPv4 with invalid characters - should return as-is", + input: "192.168.1.a", + expected: "192.168.1.a", + }, + { + name: "IPv4 with out-of-range octet after normalization - should return as-is", + input: "192.168.1.999", + expected: "192.168.1.999", + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + result := normalizeIPv4(scenario.input) + if result != scenario.expected { + t.Errorf("normalizeIPv4(%q) = %q, expected %q", scenario.input, result, scenario.expected) + } + }) + } +} diff --git a/pkg/dnshelpers/util.go b/pkg/dnshelpers/util.go index b821ff8ca7..89d74ef8d7 100644 --- a/pkg/dnshelpers/util.go +++ b/pkg/dnshelpers/util.go @@ -40,7 +40,7 @@ func GetPreferredInternalIPAddressForNodeName(network *configv1.Network, node *c return "", "", err } if isIPv4 { - return currAddress.Address, ipFamily, nil + return CanonicalizeIP(currAddress.Address), ipFamily, nil } case "tcp6": isIPv4, err := IsIPv4(currAddress.Address) @@ -48,7 +48,7 @@ func GetPreferredInternalIPAddressForNodeName(network *configv1.Network, node *c return "", "", err } if !isIPv4 { - return currAddress.Address, ipFamily, nil + return CanonicalizeIP(currAddress.Address), ipFamily, nil } default: return "", "", fmt.Errorf("unexpected ip family: %q", ipFamily) @@ -109,7 +109,7 @@ func IsIPv4(ipString string) (bool, error) { func GetInternalIPAddressesForNodeName(node *corev1.Node) ([]string, error) { addresses := []string{} for _, currAddress := range node.Status.Addresses { - if currAddress.Type == corev1.NodeInternalIP { + if currAddress.Type == corev1.NodeInternalIP && currAddress.Address != "" { addresses = append(addresses, currAddress.Address) } } @@ -120,7 +120,22 @@ func GetInternalIPAddressesForNodeName(node *corev1.Node) ([]string, error) { return addresses, nil } -// GetIPFromAddress takes a client or peer address and returns the IP address (unescaped if IPv6). +// CanonicalizeIP returns the canonical string representation of an IP address. +// This ensures consistent IP comparisons by: +// - Normalizing IPv6 addresses to their compressed form (e.g., "2001:0db8::1" → "2001:db8::1") +// - Converting IPv4-mapped IPv6 addresses to IPv4 (e.g., "::ffff:192.0.2.1" → "192.0.2.1") +// - Preserving IPv4 addresses as-is +// If the input is not a valid IP address, it returns the original string unchanged. +func CanonicalizeIP(ip string) string { + parsed := net.ParseIP(ip) + if parsed == nil { + return ip // Return original if invalid + } + return parsed.String() // Returns canonical representation +} + +// GetIPFromAddress takes a client or peer address and returns the canonical IP address (unescaped if IPv6). +// The returned IP is in canonical form for consistent comparisons. func GetIPFromAddress(address string) (string, error) { u, err := url.Parse(address) if err != nil { @@ -131,5 +146,5 @@ func GetIPFromAddress(address string) (string, error) { return "", fmt.Errorf("failed to split host port: %s: %w", u.Host, err) } - return host, nil + return CanonicalizeIP(host), nil } diff --git a/pkg/etcdcli/etcdcli.go b/pkg/etcdcli/etcdcli.go index f067c02aae..8168be6fff 100644 --- a/pkg/etcdcli/etcdcli.go +++ b/pkg/etcdcli/etcdcli.go @@ -534,10 +534,22 @@ func endpoints(nodeLister corev1listers.NodeLister, } } else { if bootstrapIP, ok := configmap.Annotations[BootstrapIPAnnotationKey]; ok && bootstrapIP != "" { - etcdEndpoints = append(etcdEndpoints, fmt.Sprintf("https://%s", net.JoinHostPort(bootstrapIP, "2379"))) + canonicalIP := dnshelpers.CanonicalizeIP(bootstrapIP) + // Add canonical version + etcdEndpoints = append(etcdEndpoints, fmt.Sprintf("https://%s", net.JoinHostPort(canonicalIP, "2379"))) + // Add non-canonical version for backward compatibility if different + if canonicalIP != bootstrapIP { + etcdEndpoints = append(etcdEndpoints, fmt.Sprintf("https://%s", net.JoinHostPort(bootstrapIP, "2379"))) + } } for _, etcdEndpointIp := range configmap.Data { - etcdEndpoints = append(etcdEndpoints, fmt.Sprintf("https://%s", net.JoinHostPort(etcdEndpointIp, "2379"))) + canonicalIP := dnshelpers.CanonicalizeIP(etcdEndpointIp) + // Add canonical version + etcdEndpoints = append(etcdEndpoints, fmt.Sprintf("https://%s", net.JoinHostPort(canonicalIP, "2379"))) + // Add non-canonical version for backward compatibility if different + if canonicalIP != etcdEndpointIp { + etcdEndpoints = append(etcdEndpoints, fmt.Sprintf("https://%s", net.JoinHostPort(etcdEndpointIp, "2379"))) + } } } diff --git a/pkg/etcdcli/etcdcli_test.go b/pkg/etcdcli/etcdcli_test.go index eea4a1816e..e24009a5f1 100644 --- a/pkg/etcdcli/etcdcli_test.go +++ b/pkg/etcdcli/etcdcli_test.go @@ -73,8 +73,8 @@ func TestEndpointFunc(t *testing.T) { u.FakeNode("2", u.WithMasterLabel(), u.WithNodeInternalIP("fda6:cfed:b298:2514:0000:0000:0000:0001")), }, []string{ - "https://[fda6:cfed:b298:2514:0000:0000:0000:0000]:2379", - "https://[fda6:cfed:b298:2514:0000:0000:0000:0001]:2379", + "https://[fda6:cfed:b298:2514::1]:2379", + "https://[fda6:cfed:b298:2514::]:2379", }, nil, }, diff --git a/pkg/etcdenvvar/envvarcontroller_test.go b/pkg/etcdenvvar/envvarcontroller_test.go index f417d7e051..fc5a50baaa 100644 --- a/pkg/etcdenvvar/envvarcontroller_test.go +++ b/pkg/etcdenvvar/envvarcontroller_test.go @@ -37,7 +37,11 @@ var ( []configv1.FeatureGateName{backendquotahelpers.BackendQuotaFeatureGateName}, []configv1.FeatureGateName{}) - defaultEnvResult = map[string]string{ + defaultEnvResult = getDefaultEnvResult() +) + +func getDefaultEnvResult() map[string]string { + result := map[string]string{ "ALL_ETCD_ENDPOINTS": "https://192.168.2.0:2379,https://192.168.2.1:2379,https://192.168.2.2:2379", "ETCDCTL_CACERT": "/etc/kubernetes/static-pod-certs/configmaps/etcd-all-bundles/server-ca-bundle.crt", "ETCDCTL_CERT": "/etc/kubernetes/static-pod-certs/secrets/etcd-all-certs/etcd-peer-NODE_NAME.crt", @@ -66,7 +70,9 @@ var ( "NODE_master_2_ETCD_URL_HOST": "192.168.2.2", "NODE_master_2_IP": "192.168.2.2", } -) + + return result +} func TestEnvVarController(t *testing.T) { scenarios := []struct { diff --git a/pkg/etcdenvvar/etcd_env.go b/pkg/etcdenvvar/etcd_env.go index e226aca3c0..97f24c148f 100644 --- a/pkg/etcdenvvar/etcd_env.go +++ b/pkg/etcdenvvar/etcd_env.go @@ -389,17 +389,31 @@ func getEtcdEndpoints(configmapLister corev1listers.ConfigMapLister, skipBootstr bootstrapAddress := etcdEndpoints.Annotations["alpha.installer.openshift.io/etcd-bootstrap"] // In some cases we want to exclude the ephemeral bootstrap-etcd member from the endpoint list. if !skipBootstrap && len(bootstrapAddress) > 0 { - if ip := net.ParseIP(bootstrapAddress); ip == nil { + ip := net.ParseIP(bootstrapAddress) + if ip == nil { return "", fmt.Errorf("configmaps/%s contains invalid bootstrap ip address: %s: %v", etcdEndpointName, bootstrapAddress, err) } - etcdURLs = append(etcdURLs, fmt.Sprintf("https://%s", net.JoinHostPort(bootstrapAddress, "2379"))) + canonicalBootstrapIP := ip.String() + // Add canonical version + etcdURLs = append(etcdURLs, fmt.Sprintf("https://%s", net.JoinHostPort(canonicalBootstrapIP, "2379"))) + // Add non-canonical version for backward compatibility if different + if canonicalBootstrapIP != bootstrapAddress { + etcdURLs = append(etcdURLs, fmt.Sprintf("https://%s", net.JoinHostPort(bootstrapAddress, "2379"))) + } } for k := range etcdEndpoints.Data { address := etcdEndpoints.Data[k] - if ip := net.ParseIP(address); ip == nil { + ip := net.ParseIP(address) + if ip == nil { return "", fmt.Errorf("configmaps/%s contains invalid ip address: %s: %v", etcdEndpointName, address, err) } - etcdURLs = append(etcdURLs, fmt.Sprintf("https://%s", net.JoinHostPort(address, "2379"))) + canonicalIP := ip.String() + // Add canonical version + etcdURLs = append(etcdURLs, fmt.Sprintf("https://%s", net.JoinHostPort(canonicalIP, "2379"))) + // Add non-canonical version for backward compatibility if different + if canonicalIP != address { + etcdURLs = append(etcdURLs, fmt.Sprintf("https://%s", net.JoinHostPort(address, "2379"))) + } } sort.Strings(etcdURLs) diff --git a/pkg/operator/ceohelpers/canonical_addresses.go b/pkg/operator/ceohelpers/canonical_addresses.go new file mode 100644 index 0000000000..3c5a0ce8c8 --- /dev/null +++ b/pkg/operator/ceohelpers/canonical_addresses.go @@ -0,0 +1,93 @@ +package ceohelpers + +import ( + corev1 "k8s.io/api/core/v1" + + "github.com/openshift/cluster-etcd-operator/pkg/dnshelpers" +) + +// CanonicalAddress wraps a NodeAddress and provides canonical IP access. +// This ensures consistent IP comparisons throughout the codebase by +// automatically handling IPv6 compression and IPv4-mapped IPv6 conversion. +type CanonicalAddress struct { + Type corev1.NodeAddressType + Address string // Original address as stored in the resource + CanonicalAddress string // Canonicalized form for comparisons +} + +// NewCanonicalAddress creates a CanonicalAddress from a NodeAddress +func NewCanonicalAddress(addr corev1.NodeAddress) CanonicalAddress { + return CanonicalAddress{ + Type: addr.Type, + Address: addr.Address, + CanonicalAddress: dnshelpers.CanonicalizeIP(addr.Address), + } +} + +// GetCanonicalInternalIPs extracts and canonicalizes all internal IP addresses from a Node. +// Returns a slice of canonical IP strings for easy comparison. +func GetCanonicalInternalIPs(node *corev1.Node) []string { + var ips []string + for _, addr := range node.Status.Addresses { + if addr.Type == corev1.NodeInternalIP && addr.Address != "" { + ips = append(ips, dnshelpers.CanonicalizeIP(addr.Address)) + } + } + return ips +} + +// GetCanonicalInternalIPsFromMachine extracts and canonicalizes all internal IP addresses from a Machine. +// Returns a slice of canonical IP strings for easy comparison. +func GetCanonicalInternalIPsFromMachine(addresses []corev1.NodeAddress) []string { + var ips []string + for _, addr := range addresses { + if addr.Type == corev1.NodeInternalIP && addr.Address != "" { + ips = append(ips, dnshelpers.CanonicalizeIP(addr.Address)) + } + } + return ips +} + +// GetCanonicalAddresses converts a slice of NodeAddresses to CanonicalAddresses. +// Useful when you need to preserve both the original and canonical forms. +func GetCanonicalAddresses(addresses []corev1.NodeAddress) []CanonicalAddress { + canonical := make([]CanonicalAddress, len(addresses)) + for i, addr := range addresses { + canonical[i] = NewCanonicalAddress(addr) + } + return canonical +} + +// HasCanonicalInternalIP checks if a node has a specific canonical internal IP. +// Both the node's addresses and the provided IP are canonicalized before comparison. +func HasCanonicalInternalIP(node *corev1.Node, ip string) bool { + canonicalIP := dnshelpers.CanonicalizeIP(ip) + for _, addr := range node.Status.Addresses { + if addr.Type == corev1.NodeInternalIP && addr.Address != "" { + if dnshelpers.CanonicalizeIP(addr.Address) == canonicalIP { + return true + } + } + } + return false +} + +// HasCanonicalInternalIPInMachine checks if a machine has a specific canonical internal IP. +// Both the machine's addresses and the provided IP are canonicalized before comparison. +func HasCanonicalInternalIPInMachine(addresses []corev1.NodeAddress, ip string) bool { + canonicalIP := dnshelpers.CanonicalizeIP(ip) + for _, addr := range addresses { + if addr.Type == corev1.NodeInternalIP && addr.Address != "" { + if dnshelpers.CanonicalizeIP(addr.Address) == canonicalIP { + return true + } + } + } + return false +} + +// IPsEqual compares two IP addresses in canonical form. +// Handles IPv6 compression and IPv4-mapped IPv6 conversion automatically. +func IPsEqual(ip1, ip2 string) bool { + return dnshelpers.CanonicalizeIP(ip1) == dnshelpers.CanonicalizeIP(ip2) +} diff --git a/pkg/operator/ceohelpers/canonical_addresses_test.go b/pkg/operator/ceohelpers/canonical_addresses_test.go new file mode 100644 index 0000000000..a2c70c4bba --- /dev/null +++ b/pkg/operator/ceohelpers/canonical_addresses_test.go @@ -0,0 +1,402 @@ +package ceohelpers + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestNewCanonicalAddress(t *testing.T) { + scenarios := []struct { + name string + address corev1.NodeAddress + expectedCanonical string + }{ + { + name: "IPv4 standard", + address: corev1.NodeAddress{ + Type: corev1.NodeInternalIP, + Address: "192.168.1.1", + }, + expectedCanonical: "192.168.1.1", + }, + { + name: "IPv6 compressed", + address: corev1.NodeAddress{ + Type: corev1.NodeInternalIP, + Address: "2001:db8::1", + }, + expectedCanonical: "2001:db8::1", + }, + { + name: "IPv6 expanded", + address: corev1.NodeAddress{ + Type: corev1.NodeInternalIP, + Address: "2001:0db8:0000:0000:0000:0000:0000:0001", + }, + expectedCanonical: "2001:db8::1", + }, + { + name: "IPv6 localhost expanded", + address: corev1.NodeAddress{ + Type: corev1.NodeInternalIP, + Address: "0:0:0:0:0:0:0:1", + }, + expectedCanonical: "::1", + }, + { + name: "IPv4-mapped IPv6", + address: corev1.NodeAddress{ + Type: corev1.NodeInternalIP, + Address: "::ffff:192.0.2.1", + }, + expectedCanonical: "192.0.2.1", + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + canonical := NewCanonicalAddress(scenario.address) + assert.Equal(t, scenario.expectedCanonical, canonical.CanonicalAddress) + assert.Equal(t, scenario.address.Address, canonical.Address) + assert.Equal(t, scenario.address.Type, canonical.Type) + }) + } +} + +func TestGetCanonicalInternalIPs(t *testing.T) { + scenarios := []struct { + name string + node *corev1.Node + expectedIPs []string + }{ + { + name: "single IPv4", + node: &corev1.Node{ + Status: corev1.NodeStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "192.168.1.1"}, + {Type: corev1.NodeHostName, Address: "node1"}, + }, + }, + }, + expectedIPs: []string{"192.168.1.1"}, + }, + { + name: "dual stack", + node: &corev1.Node{ + Status: corev1.NodeStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "192.168.1.1"}, + {Type: corev1.NodeInternalIP, Address: "2001:db8::1"}, + {Type: corev1.NodeHostName, Address: "node1"}, + }, + }, + }, + expectedIPs: []string{"192.168.1.1", "2001:db8::1"}, + }, + { + name: "IPv6 with expansion canonicalization", + node: &corev1.Node{ + Status: corev1.NodeStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "2001:0db8:0000:0000:0000:0000:0000:0001"}, + {Type: corev1.NodeHostName, Address: "node1"}, + }, + }, + }, + expectedIPs: []string{"2001:db8::1"}, + }, + { + name: "filters out non-internal addresses", + node: &corev1.Node{ + Status: corev1.NodeStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "192.168.1.1"}, + {Type: corev1.NodeExternalIP, Address: "203.0.113.1"}, + {Type: corev1.NodeHostName, Address: "node1"}, + }, + }, + }, + expectedIPs: []string{"192.168.1.1"}, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + ips := GetCanonicalInternalIPs(scenario.node) + assert.Equal(t, scenario.expectedIPs, ips) + }) + } +} + +func TestGetCanonicalInternalIPsFromMachine(t *testing.T) { + scenarios := []struct { + name string + addresses []corev1.NodeAddress + expectedIPs []string + }{ + { + name: "single IPv4", + addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "192.168.1.1"}, + {Type: corev1.NodeHostName, Address: "machine1"}, + }, + expectedIPs: []string{"192.168.1.1"}, + }, + { + name: "dual stack with canonicalization", + addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "192.168.1.1"}, + {Type: corev1.NodeInternalIP, Address: "0:0:0:0:0:0:0:1"}, + }, + expectedIPs: []string{"192.168.1.1", "::1"}, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + ips := GetCanonicalInternalIPsFromMachine(scenario.addresses) + assert.Equal(t, scenario.expectedIPs, ips) + }) + } +} + +func TestHasCanonicalInternalIP(t *testing.T) { + scenarios := []struct { + name string + node *corev1.Node + checkIP string + expected bool + }{ + { + name: "exact match IPv4", + node: &corev1.Node{ + Status: corev1.NodeStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "192.168.1.1"}, + }, + }, + }, + checkIP: "192.168.1.1", + expected: true, + }, + { + name: "IPv6 canonical matches expanded", + node: &corev1.Node{ + Status: corev1.NodeStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "::1"}, + }, + }, + }, + checkIP: "0:0:0:0:0:0:0:1", + expected: true, + }, + { + name: "IPv6 expanded matches canonical", + node: &corev1.Node{ + Status: corev1.NodeStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "2001:0db8:0000:0000:0000:0000:0000:0001"}, + }, + }, + }, + checkIP: "2001:db8::1", + expected: true, + }, + { + name: "no match", + node: &corev1.Node{ + Status: corev1.NodeStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "192.168.1.1"}, + }, + }, + }, + checkIP: "192.168.1.2", + expected: false, + }, + { + name: "ignores external IPs", + node: &corev1.Node{ + Status: corev1.NodeStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeExternalIP, Address: "203.0.113.1"}, + }, + }, + }, + checkIP: "203.0.113.1", + expected: false, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + result := HasCanonicalInternalIP(scenario.node, scenario.checkIP) + assert.Equal(t, scenario.expected, result) + }) + } +} + +func TestHasCanonicalInternalIPInMachine(t *testing.T) { + scenarios := []struct { + name string + addresses []corev1.NodeAddress + checkIP string + expected bool + }{ + { + name: "exact match", + addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "192.168.1.1"}, + }, + checkIP: "192.168.1.1", + expected: true, + }, + { + name: "canonical match", + addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "0:0:0:0:0:0:0:1"}, + }, + checkIP: "::1", + expected: true, + }, + { + name: "no match", + addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "192.168.1.1"}, + }, + checkIP: "192.168.1.2", + expected: false, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + result := HasCanonicalInternalIPInMachine(scenario.addresses, scenario.checkIP) + assert.Equal(t, scenario.expected, result) + }) + } +} + +func TestIPsEqual(t *testing.T) { + scenarios := []struct { + name string + ip1 string + ip2 string + expected bool + }{ + { + name: "IPv4 exact match", + ip1: "192.168.1.1", + ip2: "192.168.1.1", + expected: true, + }, + { + name: "IPv6 canonical and expanded", + ip1: "::1", + ip2: "0:0:0:0:0:0:0:1", + expected: true, + }, + { + name: "IPv6 different compressions", + ip1: "2001:db8::1", + ip2: "2001:0db8:0000:0000:0000:0000:0000:0001", + expected: true, + }, + { + name: "IPv4-mapped IPv6 to IPv4", + ip1: "::ffff:192.0.2.1", + ip2: "192.0.2.1", + expected: true, + }, + { + name: "different IPs", + ip1: "192.168.1.1", + ip2: "192.168.1.2", + expected: false, + }, + { + name: "different IPv6", + ip1: "2001:db8::1", + ip2: "2001:db8::2", + expected: false, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + result := IPsEqual(scenario.ip1, scenario.ip2) + assert.Equal(t, scenario.expected, result) + }) + } +} + +func TestGetCanonicalAddresses(t *testing.T) { + addresses := []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "192.168.1.1"}, + {Type: corev1.NodeInternalIP, Address: "0:0:0:0:0:0:0:1"}, + {Type: corev1.NodeHostName, Address: "node1"}, + } + + canonical := GetCanonicalAddresses(addresses) + + require.Len(t, canonical, 3) + assert.Equal(t, "192.168.1.1", canonical[0].Address) + assert.Equal(t, "192.168.1.1", canonical[0].CanonicalAddress) + assert.Equal(t, "0:0:0:0:0:0:0:1", canonical[1].Address) + assert.Equal(t, "::1", canonical[1].CanonicalAddress) + assert.Equal(t, "node1", canonical[2].Address) + assert.Equal(t, "node1", canonical[2].CanonicalAddress) +} + +// TestCanonicalAddressPreservesOriginal verifies that the original address +// is preserved alongside the canonical form, which is important for backwards +// compatibility and debugging. +func TestCanonicalAddressPreservesOriginal(t *testing.T) { + original := corev1.NodeAddress{ + Type: corev1.NodeInternalIP, + Address: "2001:0db8:0000:0000:0000:0000:0000:0001", + } + + canonical := NewCanonicalAddress(original) + + assert.Equal(t, "2001:0db8:0000:0000:0000:0000:0000:0001", canonical.Address, + "Original address should be preserved") + assert.Equal(t, "2001:db8::1", canonical.CanonicalAddress, + "Canonical form should be compressed") + assert.NotEqual(t, canonical.Address, canonical.CanonicalAddress, + "This test verifies they differ to ensure both are useful") +} + +// TestRealWorldScenario simulates the etcd member removal scenario +// where an existing etcd cluster has a member with a non-canonical IP +func TestRealWorldScenario(t *testing.T) { + // Simulate an etcd member that was registered with expanded IPv6 + etcdMemberIP := "0:0:0:0:0:0:0:1" + + // Simulate a node that reports canonical IPv6 + node := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "master-0"}, + Status: corev1.NodeStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "::1"}, + }, + }, + } + + // Without canonicalization, these wouldn't match + assert.NotEqual(t, etcdMemberIP, node.Status.Addresses[0].Address, + "Raw strings don't match") + + // With canonicalization, they match + assert.True(t, HasCanonicalInternalIP(node, etcdMemberIP), + "Canonical comparison matches") + + assert.True(t, IPsEqual(etcdMemberIP, node.Status.Addresses[0].Address), + "IPsEqual helper works") +} diff --git a/pkg/operator/ceohelpers/common.go b/pkg/operator/ceohelpers/common.go index 027ca86ec5..c469ee0f1f 100644 --- a/pkg/operator/ceohelpers/common.go +++ b/pkg/operator/ceohelpers/common.go @@ -79,7 +79,7 @@ func MemberToNodeInternalIP(member *etcdserverpb.Member) (string, error) { if err != nil { return "", err } - return host, nil + return dnshelpers.CanonicalizeIP(host), nil } // FilterMachinesWithMachineDeletionHook a convenience function for filtering only machines with the machine deletion hook present @@ -131,9 +131,10 @@ func HasMachineDeletionHook(machine *machinev1beta1.Machine) bool { func IndexMachinesByNodeInternalIP(machines []*machinev1beta1.Machine) map[string]*machinev1beta1.Machine { index := map[string]*machinev1beta1.Machine{} for _, machine := range machines { - for _, addr := range machine.Status.Addresses { - if addr.Type == corev1.NodeInternalIP { - index[addr.Address] = machine + canonicalAddrs := GetCanonicalAddresses(machine.Status.Addresses) + for _, addr := range canonicalAddrs { + if addr.Type == corev1.NodeInternalIP && addr.CanonicalAddress != "" { + index[addr.CanonicalAddress] = machine // do not stop on first match // machines can have multiple network interfaces } @@ -165,12 +166,8 @@ func FindMachineByNodeInternalIP(nodeInternalIP string, machineSelector labels.S } for _, machine := range machines { - for _, addr := range machine.Status.Addresses { - if addr.Type == corev1.NodeInternalIP { - if addr.Address == nodeInternalIP { - return machine, nil - } - } + if HasCanonicalInternalIPInMachine(machine.Status.Addresses, nodeInternalIP) { + return machine, nil } } return nil, nil @@ -196,6 +193,7 @@ func VotingMemberIPListSet(ctx context.Context, cli etcdcli.EtcdClient) (sets.St if err != nil { return sets.NewString(), err } + // GetIPFromAddress returns canonical IP, insert directly for consistent set membership currentVotingMemberIPListSet.Insert(ip) } @@ -207,7 +205,9 @@ func VotingMemberIPListSet(ctx context.Context, cli etcdcli.EtcdClient) (sets.St func MemberIPSetFromConfigMap(cm *corev1.ConfigMap) sets.String { memberIPs := sets.NewString() for _, ip := range cm.Data { - memberIPs.Insert(ip) + // Canonicalize to ensure consistent comparison with VotingMemberIPListSet + // Configmap may contain non-canonical IPs from legacy etcd members + memberIPs.Insert(dnshelpers.CanonicalizeIP(ip)) } return memberIPs } diff --git a/pkg/operator/ceohelpers/common_test.go b/pkg/operator/ceohelpers/common_test.go index b57bb74179..4c375c250f 100644 --- a/pkg/operator/ceohelpers/common_test.go +++ b/pkg/operator/ceohelpers/common_test.go @@ -6,11 +6,14 @@ import ( "github.com/stretchr/testify/require" - "k8s.io/apimachinery/pkg/runtime" - + machinev1beta1 "github.com/openshift/api/machine/v1beta1" operatorv1 "github.com/openshift/api/operator/v1" + "github.com/openshift/cluster-etcd-operator/pkg/dnshelpers" u "github.com/openshift/cluster-etcd-operator/pkg/testutils" "github.com/openshift/library-go/pkg/operator/v1helpers" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" ) var ( @@ -228,3 +231,274 @@ func TestCurrentRevision(t *testing.T) { }) } } + +func TestCanonicalizeIP(t *testing.T) { + scenarios := []struct { + name string + input string + expected string + }{ + { + name: "valid IPv4", + input: "192.168.1.10", + expected: "192.168.1.10", + }, + { + name: "valid IPv6 compressed", + input: "2001:db8::1", + expected: "2001:db8::1", + }, + { + name: "IPv6 with leading zeros", + input: "2001:0db8:0000:0000:0000:0000:0000:0001", + expected: "2001:db8::1", + }, + { + name: "IPv6 mixed case", + input: "2001:DB8::1", + expected: "2001:db8::1", + }, + { + name: "IPv4-mapped IPv6 converts to IPv4", + input: "::ffff:192.0.2.1", + expected: "192.0.2.1", + }, + { + name: "IPv4-mapped IPv6 with zeros converts to IPv4", + input: "::ffff:c000:0201", + expected: "192.0.2.1", + }, + { + name: "IPv6 loopback", + input: "::1", + expected: "::1", + }, + { + name: "IPv4 loopback", + input: "127.0.0.1", + expected: "127.0.0.1", + }, + { + name: "invalid IP returns original", + input: "not-an-ip", + expected: "not-an-ip", + }, + { + name: "empty string returns original", + input: "", + expected: "", + }, + { + name: "IPv6 with redundant zeros", + input: "fe80:0000:0000:0000:0202:b3ff:fe1e:8329", + expected: "fe80::202:b3ff:fe1e:8329", + }, + { + name: "IPv6 full form", + input: "2001:0db8:85a3:0000:0000:8a2e:0370:7334", + expected: "2001:db8:85a3::8a2e:370:7334", + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + actual := dnshelpers.CanonicalizeIP(scenario.input) + require.Equal(t, scenario.expected, actual) + }) + } +} + +func TestIndexMachinesByNodeInternalIP(t *testing.T) { + scenarios := []struct { + name string + machines []*machinev1beta1.Machine + expectedIndexKeys []string + expectedMachineNameForKey map[string]string + }{ + { + name: "single machine with IPv4", + machines: []*machinev1beta1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{Name: "machine1"}, + Status: machinev1beta1.MachineStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "192.168.1.10"}, + }, + }, + }, + }, + expectedIndexKeys: []string{"192.168.1.10"}, + expectedMachineNameForKey: map[string]string{ + "192.168.1.10": "machine1", + }, + }, + { + name: "single machine with IPv6 expanded form - should use canonical key only", + machines: []*machinev1beta1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{Name: "machine1"}, + Status: machinev1beta1.MachineStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "2001:0db8:0000:0000:0000:0000:0000:0001"}, + }, + }, + }, + }, + expectedIndexKeys: []string{"2001:db8::1"}, + expectedMachineNameForKey: map[string]string{ + "2001:db8::1": "machine1", + }, + }, + { + name: "single machine with dual-stack IPs", + machines: []*machinev1beta1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{Name: "machine1"}, + Status: machinev1beta1.MachineStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "192.168.1.10"}, + {Type: corev1.NodeInternalIP, Address: "2001:db8::1"}, + }, + }, + }, + }, + expectedIndexKeys: []string{"192.168.1.10", "2001:db8::1"}, + expectedMachineNameForKey: map[string]string{ + "192.168.1.10": "machine1", + "2001:db8::1": "machine1", + }, + }, + { + name: "multiple machines with different IPs", + machines: []*machinev1beta1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{Name: "machine1"}, + Status: machinev1beta1.MachineStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "192.168.1.10"}, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "machine2"}, + Status: machinev1beta1.MachineStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "192.168.1.20"}, + }, + }, + }, + }, + expectedIndexKeys: []string{"192.168.1.10", "192.168.1.20"}, + expectedMachineNameForKey: map[string]string{ + "192.168.1.10": "machine1", + "192.168.1.20": "machine2", + }, + }, + { + name: "machine with multiple network interfaces (same machine, multiple IPs)", + machines: []*machinev1beta1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{Name: "machine1"}, + Status: machinev1beta1.MachineStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "10.0.1.10"}, + {Type: corev1.NodeInternalIP, Address: "10.0.2.10"}, + {Type: corev1.NodeHostName, Address: "node1.example.com"}, + }, + }, + }, + }, + expectedIndexKeys: []string{"10.0.1.10", "10.0.2.10"}, + expectedMachineNameForKey: map[string]string{ + "10.0.1.10": "machine1", + "10.0.2.10": "machine1", + }, + }, + { + name: "IPv4-mapped IPv6 should convert to IPv4", + machines: []*machinev1beta1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{Name: "machine1"}, + Status: machinev1beta1.MachineStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "::ffff:192.0.2.1"}, + }, + }, + }, + }, + expectedIndexKeys: []string{"192.0.2.1"}, + expectedMachineNameForKey: map[string]string{ + "192.0.2.1": "machine1", + }, + }, + { + name: "filters out non-internal IP addresses", + machines: []*machinev1beta1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{Name: "machine1"}, + Status: machinev1beta1.MachineStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: "192.168.1.10"}, + {Type: corev1.NodeExternalIP, Address: "1.2.3.4"}, + {Type: corev1.NodeHostName, Address: "node1.example.com"}, + }, + }, + }, + }, + expectedIndexKeys: []string{"192.168.1.10"}, + expectedMachineNameForKey: map[string]string{ + "192.168.1.10": "machine1", + }, + }, + { + name: "empty address should be filtered out", + machines: []*machinev1beta1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{Name: "machine1"}, + Status: machinev1beta1.MachineStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: ""}, + {Type: corev1.NodeInternalIP, Address: "192.168.1.10"}, + }, + }, + }, + }, + expectedIndexKeys: []string{"192.168.1.10"}, + expectedMachineNameForKey: map[string]string{ + "192.168.1.10": "machine1", + }, + }, + { + name: "empty machine list", + machines: []*machinev1beta1.Machine{}, + expectedIndexKeys: []string{}, + expectedMachineNameForKey: map[string]string{}, + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + index := IndexMachinesByNodeInternalIP(scenario.machines) + + // Verify the number of keys matches expected + require.Equal(t, len(scenario.expectedIndexKeys), len(index), + "index should have exactly %d entries, but has %d", + len(scenario.expectedIndexKeys), len(index)) + + // Verify each expected key exists and points to the correct machine + for ip, expectedMachineName := range scenario.expectedMachineNameForKey { + machine, exists := index[ip] + require.True(t, exists, "expected IP %s to exist in index", ip) + require.NotNil(t, machine, "machine should not be nil for IP %s", ip) + require.Equal(t, expectedMachineName, machine.Name, + "machine name mismatch for IP %s", ip) + } + + // Verify no unexpected keys exist + for ip := range index { + _, expected := scenario.expectedMachineNameForKey[ip] + require.True(t, expected, "unexpected IP %s found in index", ip) + } + }) + } +} diff --git a/pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller.go b/pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller.go index 9741252448..5468e8ccf3 100644 --- a/pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller.go +++ b/pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller.go @@ -362,7 +362,7 @@ func (c *clusterMemberRemovalController) removeMemberWithoutMachine(ctx context. if err != nil { return err } - if memberIP != potentialMemberToRemoveIP { + if !ceohelpers.IPsEqual(memberIP, potentialMemberToRemoveIP) { continue } // a member without a node must be reported as unhealthy @@ -479,7 +479,7 @@ func (c *clusterMemberRemovalController) getNodeForMember(memberInternalIP strin if err != nil { return nil, fmt.Errorf("failed to get internal IP for node: %w", err) } - if memberInternalIP == internalNodeIP { + if ceohelpers.IPsEqual(memberInternalIP, internalNodeIP) { return masterNode, nil } } @@ -526,12 +526,7 @@ func (c *clusterMemberRemovalController) getAllVotingMembers(ctx context.Context } func hasInternalIP(machine *machinev1beta1.Machine, memberInternalIP string) bool { - for _, addr := range machine.Status.Addresses { - if addr.Type == corev1.NodeInternalIP && addr.Address == memberInternalIP { - return true - } - } - return false + return ceohelpers.HasCanonicalInternalIPInMachine(machine.Status.Addresses, memberInternalIP) } func membersEqual(left, right []*etcdserverpb.Member) bool { diff --git a/pkg/operator/machinedeletionhooks/machinedeletionhooks.go b/pkg/operator/machinedeletionhooks/machinedeletionhooks.go index f0e4f21c14..164634869a 100644 --- a/pkg/operator/machinedeletionhooks/machinedeletionhooks.go +++ b/pkg/operator/machinedeletionhooks/machinedeletionhooks.go @@ -137,6 +137,7 @@ func (c *machineDeletionHooksController) attemptToDeleteMachineDeletionHook(ctx if err != nil { return err } + // MemberToNodeInternalIP already returns canonical IP currentMemberIPListSet.Insert(memberIP) } @@ -144,12 +145,11 @@ func (c *machineDeletionHooksController) attemptToDeleteMachineDeletionHook(ctx for _, machinePendingDeletion := range machinesPendingDeletion { // if none of the addresses for a machinePendingDeletion are in the member list, we can safely remove the hook skipNode := false - for _, addr := range machinePendingDeletion.Status.Addresses { - if addr.Type == corev1.NodeInternalIP { - if currentMemberIPListSet.Has(addr.Address) { - skipNode = true - break - } + machineIPs := ceohelpers.GetCanonicalInternalIPsFromMachine(machinePendingDeletion.Status.Addresses) + for _, ip := range machineIPs { + if currentMemberIPListSet.Has(ip) { + skipNode = true + break } } diff --git a/pkg/tlshelpers/hostnames_test.go b/pkg/tlshelpers/hostnames_test.go new file mode 100644 index 0000000000..24663cd546 --- /dev/null +++ b/pkg/tlshelpers/hostnames_test.go @@ -0,0 +1,272 @@ +package tlshelpers + +import ( + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "crypto/x509/pkix" + "math/big" + "net" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestGetServerHostNames(t *testing.T) { + scenarios := []struct { + name string + nodeInternalIPs []string + expectedToContain []string + description string + }{ + { + name: "IPv4 canonical form", + nodeInternalIPs: []string{"192.168.1.1"}, + expectedToContain: []string{ + "192.168.1.1", // raw form + "192.168.1.1", // canonical form (same as raw for standard IPv4) + "localhost", // static entries + "127.0.0.1", // static IPv4 localhost + "::1", // static IPv6 localhost + }, + description: "Standard IPv4 address should include both canonical and raw forms", + }, + { + name: "IPv4 addresses from real nodes are already canonical", + nodeInternalIPs: []string{"10.0.128.5"}, + expectedToContain: []string{ + "10.0.128.5", // Kubernetes always provides properly formatted IPs + }, + description: "Real node IPs from Kubernetes API are always in canonical form", + }, + { + name: "IPv6 compressed form", + nodeInternalIPs: []string{"2001:db8::1"}, + expectedToContain: []string{ + "2001:db8::1", // raw form (compressed) + "2001:db8::1", // canonical form (same) + }, + description: "IPv6 compressed form should be included", + }, + { + name: "IPv6 expanded form", + nodeInternalIPs: []string{"2001:0db8:0000:0000:0000:0000:0000:0001"}, + expectedToContain: []string{ + "2001:0db8:0000:0000:0000:0000:0000:0001", // raw form (expanded) + "2001:db8::1", // canonical form (compressed) + }, + description: "IPv6 expanded form should include both expanded (raw) and compressed (canonical) forms", + }, + { + name: "IPv6 localhost expanded form", + nodeInternalIPs: []string{"0:0:0:0:0:0:0:1"}, + expectedToContain: []string{ + "0:0:0:0:0:0:0:1", // raw form + "::1", // canonical form + }, + description: "IPv6 localhost in expanded form should include both forms for TLS compatibility", + }, + { + name: "IPv6 localhost various forms", + nodeInternalIPs: []string{"0000:0000:0000:0000:0000:0000:0000:0001"}, + expectedToContain: []string{ + "0000:0000:0000:0000:0000:0000:0000:0001", // raw form + "::1", // canonical form + }, + description: "IPv6 localhost with leading zeros should include both forms", + }, + { + name: "multiple IP addresses", + nodeInternalIPs: []string{"192.168.1.1", "2001:db8::1"}, + expectedToContain: []string{ + "192.168.1.1", + "2001:db8::1", + }, + description: "Multiple IPs should all be included", + }, + { + name: "IPv4-mapped IPv6 address", + nodeInternalIPs: []string{"::ffff:192.0.2.1"}, + expectedToContain: []string{ + "::ffff:192.0.2.1", // raw form + "192.0.2.1", // canonical form (converted to IPv4) + }, + description: "IPv4-mapped IPv6 should include both IPv6 and converted IPv4 forms", + }, + { + name: "empty string should be filtered out", + nodeInternalIPs: []string{"192.168.1.1", "", "2001:db8::1"}, + expectedToContain: []string{ + "192.168.1.1", + "2001:db8::1", + "localhost", // verify static entries still present + }, + description: "Empty strings in nodeInternalIPs should be filtered out", + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + hostnames := getServerHostNames(scenario.nodeInternalIPs) + + for _, expected := range scenario.expectedToContain { + found := false + for _, hostname := range hostnames { + if hostname == expected { + found = true + break + } + } + if !found { + t.Errorf("%s: expected hostname %q to be in list, but it wasn't. Got: %v", + scenario.description, expected, hostnames) + } + } + + // Verify static entries are always present + staticEntries := []string{ + "localhost", + "etcd.kube-system.svc", + "etcd.kube-system.svc.cluster.local", + "etcd.openshift-etcd.svc", + "etcd.openshift-etcd.svc.cluster.local", + "127.0.0.1", + "::1", + } + for _, staticEntry := range staticEntries { + found := false + for _, hostname := range hostnames { + if hostname == staticEntry { + found = true + break + } + } + if !found { + t.Errorf("expected static entry %q to be in hostname list, but it wasn't. Got: %v", + staticEntry, hostnames) + } + } + + // Verify no empty strings are ever in the output + for _, hostname := range hostnames { + if hostname == "" { + t.Errorf("found empty string in hostname list, which should be filtered out. Full list: %v", hostnames) + } + } + }) + } +} + +// TestIPCanonicalizeInCertificate verifies that TLS certificate verification +// correctly handles both canonical and non-canonical IP address forms. +// This test demonstrates why we need to include both forms in certificate SANs. +func TestIPCanonicalizeInCertificate(t *testing.T) { + scenarios := []struct { + name string + certIPs []string + connectIP string + shouldMatch bool + description string + }{ + { + name: "IPv6 canonical in cert, canonical connection", + certIPs: []string{"::1"}, + connectIP: "::1", + shouldMatch: true, + description: "Connecting to ::1 with ::1 in cert should work", + }, + { + name: "IPv6 canonical in cert, expanded connection", + certIPs: []string{"::1"}, + connectIP: "0:0:0:0:0:0:0:1", + shouldMatch: true, + description: "Both forms parse to same IP bytes, so they should match", + }, + { + name: "IPv6 expanded in cert, canonical connection", + certIPs: []string{"0:0:0:0:0:0:0:1"}, + connectIP: "::1", + shouldMatch: true, + description: "Both forms parse to same IP bytes, so they should match", + }, + { + name: "IPv6 both forms in cert ensures compatibility", + certIPs: []string{"::1", "0:0:0:0:0:0:0:1"}, + connectIP: "0:0:0:0:0:0:0:1", + shouldMatch: true, + description: "Having both forms in cert provides maximum compatibility", + }, + { + name: "IPv6 different address should not match", + certIPs: []string{"2001:db8::1"}, + connectIP: "2001:db8::2", + shouldMatch: false, + description: "Different IPv6 addresses should not match", + }, + } + + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + // Create a self-signed certificate with the specified IP SANs + cert := createTestCertWithIPs(t, scenario.certIPs) + + // Parse the connection IP + connectIPParsed := net.ParseIP(scenario.connectIP) + require.NotNil(t, connectIPParsed, "connect IP should be valid") + + // Check if the connection IP matches any IP in the certificate + matched := false + for _, certIP := range cert.IPAddresses { + if certIP.Equal(connectIPParsed) { + matched = true + break + } + } + + if matched != scenario.shouldMatch { + t.Errorf("%s: expected match=%v, got match=%v. Cert IPs: %v, Connect IP: %v, Parsed cert IPs: %v", + scenario.description, scenario.shouldMatch, matched, + scenario.certIPs, scenario.connectIP, cert.IPAddresses) + } + }) + } +} + +// createTestCertWithIPs creates a self-signed certificate with the specified IP addresses in SANs +func createTestCertWithIPs(t *testing.T, ipStrings []string) *x509.Certificate { + // Generate a private key + privateKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + + // Parse IP addresses + var ips []net.IP + for _, ipStr := range ipStrings { + ip := net.ParseIP(ipStr) + require.NotNil(t, ip, "IP %q should be valid", ipStr) + ips = append(ips, ip) + } + + // Create certificate template + template := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{ + CommonName: "test-cert", + }, + NotBefore: time.Now(), + NotAfter: time.Now().Add(24 * time.Hour), + KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + IPAddresses: ips, + } + + // Create self-signed certificate + certDER, err := x509.CreateCertificate(rand.Reader, template, template, &privateKey.PublicKey, privateKey) + require.NoError(t, err) + + // Parse the certificate + cert, err := x509.ParseCertificate(certDER) + require.NoError(t, err) + + return cert +} diff --git a/pkg/tlshelpers/tlshelpers.go b/pkg/tlshelpers/tlshelpers.go index 1ceea66e67..414bed402f 100644 --- a/pkg/tlshelpers/tlshelpers.go +++ b/pkg/tlshelpers/tlshelpers.go @@ -60,6 +60,14 @@ func GetServingMetricsSecretNameForNode(nodeName string) string { } func getServerHostNames(nodeInternalIPs []string) []string { + completeIPList := make([]string, 0, len(nodeInternalIPs)) + for _, ip := range nodeInternalIPs { + if ip == "" { + continue + } + // add the canonicalized IP address and normal ip address for backwards compatibility + completeIPList = append(completeIPList, dnshelpers.CanonicalizeIP(ip), ip) + } return append([]string{ "localhost", "etcd.kube-system.svc", @@ -68,8 +76,10 @@ func getServerHostNames(nodeInternalIPs []string) []string { "etcd.openshift-etcd.svc.cluster.local", "127.0.0.1", "::1", - // "0:0:0:0:0:0:0:1" will be automatically collapsed to "::1", so we don't have to add it on top - }, nodeInternalIPs...) + // Note: "0:0:0:0:0:0:0:1" and "::1" parse to the same IP bytes in certificates, + // so TLS verification works with both forms even when only "::1" is listed here. + // For dynamic node IPs above, we include both canonical and raw forms for maximum compatibility. + }, completeIPList...) } func CreateSignerCertRotationBundleConfigMap( diff --git a/pkg/tnf/pkg/pacemaker/statuscollector_test.go b/pkg/tnf/pkg/pacemaker/statuscollector_test.go index 76dfe194d6..ca62818a0f 100644 --- a/pkg/tnf/pkg/pacemaker/statuscollector_test.go +++ b/pkg/tnf/pkg/pacemaker/statuscollector_test.go @@ -3,6 +3,7 @@ package pacemaker import ( "encoding/xml" "fmt" + "net" "os" "path/filepath" "strings" @@ -337,9 +338,30 @@ func TestBuildClusterStatus_NodeIPExtraction(t *testing.T) { } // containsAddress checks if a PacemakerNodeAddress slice contains a specific address +// using canonical IP comparison func containsAddress(addresses []pacmkrv1.PacemakerNodeAddress, addr string) bool { + canonicalAddr := net.ParseIP(addr) + if canonicalAddr == nil { + // Not a valid IP, fall back to string comparison + for _, a := range addresses { + if a.Address == addr { + return true + } + } + return false + } + canonicalAddrStr := canonicalAddr.String() + for _, a := range addresses { - if a.Address == addr { + addressIP := net.ParseIP(a.Address) + if addressIP == nil { + // Not a valid IP, do string comparison + if a.Address == addr { + return true + } + continue + } + if addressIP.String() == canonicalAddrStr { return true } }