From 7748a2ce4bd7aec326b56286516857aeb7b9e354 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Fri, 10 Nov 2023 14:02:11 +0100 Subject: [PATCH] OCPBUGS-23149, OCPBUGS-23131: fix IRONIC_EXTERNAL_URL_V6 IRONIC_EXTERNAL_URL_V6 is currently calculated incorrectly: 1) It uses the API VIP, but the Ironic's httpd is not proxied in the same way as Ironic itself and thus may not be available there. 2) It uses an external IP even when a provisioning network is enabled and VirtualMediaViaExternalNetwork is false. This change fixes both issues, while also cleaning up the provisioning utils to ensure all functions are dualstack-aware. The split between the BMO and Metal3 pods is an implicit requirement of this change. This change does NOT fix the fact that ICC is not configured correctly for deploying IPv6-only nodes from a dualstack cluster. --- provisioning/baremetal_config_test.go | 3 +- provisioning/baremetal_pod.go | 3 +- provisioning/bmo_pod_test.go | 14 +++++- provisioning/image_customization.go | 2 +- provisioning/ironic_proxy.go | 5 +- provisioning/utils.go | 72 +++++++++++++-------------- 6 files changed, 54 insertions(+), 45 deletions(-) diff --git a/provisioning/baremetal_config_test.go b/provisioning/baremetal_config_test.go index 2c0876490..5457fbdba 100644 --- a/provisioning/baremetal_config_test.go +++ b/provisioning/baremetal_config_test.go @@ -115,6 +115,7 @@ type provisioningBuilder struct { } const testProvisioningIP = "172.30.20.3" +const testProvisioningIPv6 = "fd2e:6f44:5dd8:b856::2" func managedProvisioning() *provisioningBuilder { return &provisioningBuilder{ @@ -134,7 +135,7 @@ func managedIPv6Provisioning() *provisioningBuilder { return &provisioningBuilder{ metal3iov1alpha1.ProvisioningSpec{ ProvisioningInterface: "eth0", - ProvisioningIP: "fd2e:6f44:5dd8:b856::2", + ProvisioningIP: testProvisioningIPv6, ProvisioningNetworkCIDR: "fd2e:6f44:5dd8:b856::0/80", ProvisioningMacAddresses: []string{"34:b3:2d:81:f8:fb", "34:b3:2d:81:f8:fc", "34:b3:2d:81:f8:fd"}, ProvisioningDHCPRange: "fd2e:6f44:5dd8:b856::10,fd2e:6f44:5dd8:b856::ff", diff --git a/provisioning/baremetal_pod.go b/provisioning/baremetal_pod.go index b931f02cb..b9f96c7ef 100644 --- a/provisioning/baremetal_pod.go +++ b/provisioning/baremetal_pod.go @@ -268,8 +268,7 @@ func setIronicExternalIp(name string, config *metal3iov1alpha1.ProvisioningSpec) } func setIronicExternalUrl(info *ProvisioningInfo) (corev1.EnvVar, error) { - ironicIPs, err := getServerInternalIPs(info.OSClient) - + ironicIPs, err := GetRealIronicIPs(info) if err != nil { return corev1.EnvVar{}, fmt.Errorf("Failed to get Ironic IP when setting external url: %w", err) } diff --git a/provisioning/bmo_pod_test.go b/provisioning/bmo_pod_test.go index 34a813658..31eb06840 100644 --- a/provisioning/bmo_pod_test.go +++ b/provisioning/bmo_pod_test.go @@ -83,12 +83,22 @@ func TestNewBMOContainers(t *testing.T) { expectedContainers []corev1.Container }{ { - name: "ManagedSpec", + name: "ManagedSpec with IPv4", config: managedProvisioning().build(), expectedContainers: []corev1.Container{ withEnv( containers["metal3-baremetal-operator"], - envWithValue("IRONIC_EXTERNAL_URL_V6", "https://[fd2e:6f44:5dd8:c956::16]:6183"), + ), + }, + sshkey: "sshkey", + }, + { + name: "ManagedSpec with IPv6", + config: managedIPv6Provisioning().build(), + expectedContainers: []corev1.Container{ + withEnv( + containers["metal3-baremetal-operator"], + envWithValue("IRONIC_EXTERNAL_URL_V6", fmt.Sprintf("https://[%s]:6183", testProvisioningIPv6)), ), }, sshkey: "sshkey", diff --git a/provisioning/image_customization.go b/provisioning/image_customization.go index 7b8011a39..061d90895 100644 --- a/provisioning/image_customization.go +++ b/provisioning/image_customization.go @@ -240,7 +240,7 @@ func newImageCustomizationDeployment(info *ProvisioningInfo, ironicIPs []string, } func EnsureImageCustomizationDeployment(info *ProvisioningInfo) (updated bool, err error) { - ironicIPs, inspectorIPs, err := GetIronicIPs(*info) + ironicIPs, inspectorIPs, err := GetIronicIPs(info) if err != nil { return false, fmt.Errorf("unable to determine Ironic's IP to pass to the machine-image-customization-controller: %w", err) } diff --git a/provisioning/ironic_proxy.go b/provisioning/ironic_proxy.go index d2ff259f6..deecad7bf 100644 --- a/provisioning/ironic_proxy.go +++ b/provisioning/ironic_proxy.go @@ -107,13 +107,14 @@ func createContainerIronicProxy(ironicIP string, images *Images) corev1.Containe } func newIronicProxyPodTemplateSpec(info *ProvisioningInfo) (*corev1.PodTemplateSpec, error) { - ironicIP, err := getPodHostIP(info.Client.CoreV1(), info.Namespace) + ironicIPs, err := getPodIPs(info.Client.CoreV1(), info.Namespace) if err != nil { return nil, errors.Wrap(err, "cannot figure out the upstream IP for ironic proxy") } containers := []corev1.Container{ - createContainerIronicProxy(ironicIP, info.Images), + // Even in a dual-stack environment, we don't really care which IP address to use since both are accessible internally. + createContainerIronicProxy(ironicIPs[0], info.Images), } tolerations := []corev1.Toleration{ diff --git a/provisioning/utils.go b/provisioning/utils.go index 120f52ded..083c301e2 100644 --- a/provisioning/utils.go +++ b/provisioning/utils.go @@ -2,6 +2,7 @@ package provisioning import ( "context" + "errors" "fmt" "net" @@ -55,14 +56,27 @@ func getPod(podClient coreclientv1.PodsGetter, targetNamespace string) (corev1.P return pods[0], nil } -func getPodHostIP(podClient coreclientv1.PodsGetter, targetNamespace string) (string, error) { +// getPodIPs returns pod IPs for the Metal3 pod (and thus Ironic and its httpd). +func getPodIPs(podClient coreclientv1.PodsGetter, targetNamespace string) (ips []string, err error) { pod, err := getPod(podClient, targetNamespace) if err != nil { - return "", err + return nil, err + } + // NOTE(dtantsur): we can use PodIPs here because with host networking they're identical to HostIP + for _, ip := range pod.Status.PodIPs { + if ip.IP != "" { + ips = append(ips, ip.IP) + } + } + if len(ips) == 0 { + // This is basically a safeguard to be able to assume that the returned slice is not empty later on + err = errors.New("the metal3 pod does not have any podIP's yet") } - return pod.Status.HostIP, nil + return } +// getServerInternalIPs returns virtual IPs on which Kubernetes is accessible. +// These are the IPs on which the proxied services (currently Ironic and Inspector) should be accessed by external consumers. func getServerInternalIPs(osclient osclientset.Interface) ([]string, error) { infra, err := osclient.ConfigV1().Infrastructures().Get(context.Background(), "cluster", metav1.GetOptions{}) if err != nil { @@ -99,20 +113,26 @@ func getServerInternalIPs(osclient osclientset.Interface) ([]string, error) { } } -func GetIronicIPs(info ProvisioningInfo) (ironicIPs []string, inspectorIPs []string, err error) { - var podIP string +// GetRealIronicIPs returns the actual IPs on which Ironic is accessible without a proxy. +// The provisioning IP is used when present and not disallowed for virtual media via configuration. +func GetRealIronicIPs(info *ProvisioningInfo) ([]string, error) { config := info.ProvConfig.Spec - if config.ProvisioningNetwork != metal3iov1alpha1.ProvisioningNetworkDisabled && !config.VirtualMediaViaExternalNetwork { - podIP = config.ProvisioningIP - } else { - podIP, err = getPodHostIP(info.Client.CoreV1(), info.Namespace) - if err != nil { - return - } + return []string{config.ProvisioningIP}, nil } - if UseIronicProxy(&config) { + return getPodIPs(info.Client.CoreV1(), info.Namespace) +} + +// GetIronicIPs returns Ironic IPs for external consumption, potentially behind an HA proxy. +// Without a proxy, the provisioning IP is used when present and not disallowed for virtual media via configuration. +func GetIronicIPs(info *ProvisioningInfo) (ironicIPs []string, inspectorIPs []string, err error) { + podIPs, err := GetRealIronicIPs(info) + if err != nil { + return + } + + if UseIronicProxy(&info.ProvConfig.Spec) { ironicIPs, err = getServerInternalIPs(info.OSClient) if err != nil { err = fmt.Errorf("error fetching internalIPs: %w", err) @@ -120,38 +140,16 @@ func GetIronicIPs(info ProvisioningInfo) (ironicIPs []string, inspectorIPs []str } if ironicIPs == nil { - ironicIPs = []string{podIP} + ironicIPs = podIPs } } else { - ironicIPs = []string{podIP} + ironicIPs = podIPs } inspectorIPs = ironicIPs // keep returning separate variables for future enhancements return ironicIPs, inspectorIPs, err } -func GetPodIP(podClient coreclientv1.PodsGetter, targetNamespace string, networkType NetworkStackType) (string, error) { - pod, err := getPod(podClient, targetNamespace) - if err != nil { - return "", err - } - - for _, podIP := range pod.Status.PodIPs { - if networkType == NetworkStackDual { - return podIP.IP, nil - } - ip := net.ParseIP(podIP.IP) - if networkType == NetworkStackV6 && ip.To4() == nil { - return podIP.IP, nil - } - if networkType == NetworkStackV4 && ip.To4() != nil { - return podIP.IP, nil - } - } - - return "", fmt.Errorf("Pod doesn't have an IP address of the requested type") -} - func IpOptionForProvisioning(config *metal3iov1alpha1.ProvisioningSpec, networkStack NetworkStackType) string { var optionValue string ip := net.ParseIP(config.ProvisioningIP)