Merge pull request #239 from sadasu/sadasu-2040671

Bug 2040671: Fix the way the network stack is determined

config/rbac/role.yaml

@@ -82,6 +82,14 @@ rules:
   - infrastructures/status
   verbs:
   - get
+- apiGroups:
+  - config.openshift.io
+  resources:
+  - networks
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - config.openshift.io
   resources:

controllers/provisioning_controller.go

@@ -19,7 +19,6 @@ import (
 	"context"
 	"fmt"
 	"net"
-	"net/url"
 	"os"
 	"strings"
 	"time"
@@ -35,6 +34,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/klog/v2"
+	utilnet "k8s.io/utils/net"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -84,6 +84,7 @@ type ensureFunc func(*provisioning.ProvisioningInfo) (bool, error)
 
 // +kubebuilder:rbac:groups=config.openshift.io,resources=proxies,verbs=get;list;watch
 // +kubebuilder:rbac:groups=config.openshift.io,resources=infrastructures,verbs=get;list;watch
+// +kubebuilder:rbac:groups=config.openshift.io,resources=networks,verbs=get;list;watch
 // +kubebuilder:rbac:groups=security.openshift.io,resources=securitycontextconstraints,verbs=use
 // +kubebuilder:rbac:groups=config.openshift.io,resources=clusteroperators;clusteroperators/status,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=config.openshift.io,resources=infrastructures;infrastructures/status,verbs=get
@@ -395,42 +396,29 @@ func (r *ProvisioningReconciler) deleteMetal3Resources(info *provisioning.Provis
 	return nil
 }
 
-func networkStack(ips []net.IP) provisioning.NetworkStackType {
+func (r *ProvisioningReconciler) networkStackFromServiceNetwork(ctx context.Context) (provisioning.NetworkStackType, error) {
 	ns := provisioning.NetworkStackType(0)
-	for _, ip := range ips {
-		if ip.IsLoopback() {
-			continue
-		}
-		if ip.To4() != nil {
-			ns |= provisioning.NetworkStackV4
-		} else {
-			ns |= provisioning.NetworkStackV6
-		}
-	}
-	return ns
-}
-
-func (r *ProvisioningReconciler) apiServerInternalHost(ctx context.Context) (string, error) {
-	infra, err := r.OSClient.ConfigV1().Infrastructures().Get(ctx, "cluster", metav1.GetOptions{})
+	network, err := r.OSClient.ConfigV1().Networks().Get(ctx, "cluster", metav1.GetOptions{})
 	if err != nil {
-		return "", errors.Wrap(err, "unable to read Infrastructure CR")
+		return ns, errors.Wrap(err, "unable to read Network CR")
 	}
 
-	if infra.Status.APIServerInternalURL == "" {
-		return "", errors.Wrap(err, "invalid APIServerInternalURL in Infrastructure CR")
-	}
-
-	apiServerInternalURL, err := url.Parse(infra.Status.APIServerInternalURL)
-	if err != nil {
-		return "", errors.Wrap(err, "unable to parse API Server Internal URL")
+	serviceNetwork := network.Status.ServiceNetwork
+	if len(serviceNetwork) == 0 {
+		serviceNetwork = network.Spec.ServiceNetwork
 	}
-
-	host, _, err := net.SplitHostPort(apiServerInternalURL.Host)
-	if err != nil {
-		return "", err
+	for _, snet := range serviceNetwork {
+		_, cidr, err := net.ParseCIDR(snet)
+		if err != nil {
+			return ns, errors.Wrapf(err, "could not parse Service Network %s", snet)
+		}
+		if utilnet.IsIPv6CIDR(cidr) {
+			ns |= provisioning.NetworkStackV6
+		} else {
+			ns |= provisioning.NetworkStackV4
+		}
 	}
-
-	return host, nil
+	return ns, nil
 }
 
 func (r *ProvisioningReconciler) updateProvisioningMacAddresses(ctx context.Context, provConfig *metal3iov1alpha1.Provisioning) error {
@@ -481,18 +469,13 @@ func (r *ProvisioningReconciler) SetupWithManager(mgr ctrl.Manager) error {
 			}
 		}
 	}
-	apiInt, err := r.apiServerInternalHost(ctx)
-	if err != nil {
-		return errors.Wrap(err, "could not get internal APIServer")
-	}
 
-	ips, err := net.LookupIP(apiInt)
+	r.NetworkStack, err = r.networkStackFromServiceNetwork(ctx)
 	if err != nil {
-		return errors.Wrap(err, "could not lookupIP for internal APIServer: "+apiInt)
+		return fmt.Errorf("unable to determine network stack from service network: %w", err)
 	}
 
-	r.NetworkStack = networkStack(ips)
-	klog.InfoS("Network stack calculation", "APIServerInternalHost", apiInt, "NetworkStack", r.NetworkStack)
+	klog.InfoS("Network stack calculation", "NetworkStack", r.NetworkStack)
 
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&metal3iov1alpha1.Provisioning{}).

controllers/provisioning_controller_test.go

@@ -2,7 +2,6 @@ package controllers
 
 import (
 	"context"
-	"net"
 	"reflect"
 	"testing"
 
@@ -81,78 +80,107 @@ func TestProvisioning(t *testing.T) {
 	}
 }
 
-func TestNetworkStack(t *testing.T) {
-	tests := []struct {
-		name    string
-		ips     []net.IP
-		want    provisioning.NetworkStackType
-		wantErr bool
+func TestNetworkStackFromServiceNetwork(t *testing.T) {
+	testCases := []struct {
+		name                 string
+		networkCR            *configv1.Network
+		expectedError        bool
+		expectedNetworkStack provisioning.NetworkStackType
 	}{
 		{
-			name: "v4 basic",
-			ips:  []net.IP{net.ParseIP("192.168.0.1")},
-			want: provisioning.NetworkStackV4,
-		},
-		{
-			name: "v4 in v6 format: basic",
-			ips:  []net.IP{net.ParseIP("::FFFF:192.168.0.1")},
-			want: provisioning.NetworkStackV4,
+			name: "StatusIPv4",
+			networkCR: &configv1.Network{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "Network",
+					APIVersion: "config.openshift.io/v1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "cluster",
+				},
+				Status: configv1.NetworkStatus{
+					ServiceNetwork: []string{"172.30.0.0/16"},
+				},
+			},
+			expectedError:        false,
+			expectedNetworkStack: provisioning.NetworkStackV4,
 		},
 		{
-			name: "v6: basic",
-			ips:  []net.IP{net.ParseIP("2001:db8::68")},
-			want: provisioning.NetworkStackV6,
+			name: "SpecIPv6",
+			networkCR: &configv1.Network{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "Network",
+					APIVersion: "config.openshift.io/v1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "cluster",
+				},
+				Spec: configv1.NetworkSpec{
+					ServiceNetwork: []string{"fd02::/112"},
+				},
+				Status: configv1.NetworkStatus{
+					ServiceNetwork: []string{},
+				},
+			},
+			expectedError:        false,
+			expectedNetworkStack: provisioning.NetworkStackV6,
 		},
 		{
-			name: "dual: basic",
-			ips:  []net.IP{net.ParseIP("2001:db8::68"), net.ParseIP("192.168.0.1")},
-			want: provisioning.NetworkStackDual,
+			name: "StatusDualStack",
+			networkCR: &configv1.Network{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "Network",
+					APIVersion: "config.openshift.io/v1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "cluster",
+				},
+				Status: configv1.NetworkStatus{
+					ServiceNetwork: []string{"172.30.0.0/16", "fd02::/112"},
+				},
+			},
+			expectedError:        false,
+			expectedNetworkStack: provisioning.NetworkStackDual,
 		},
 		{
-			name: "v6: with v4 local",
-			ips:  []net.IP{net.ParseIP("2001:db8::68"), net.ParseIP("127.0.0.1")},
-			want: provisioning.NetworkStackV6,
+			name: "SpecDualStack",
+			networkCR: &configv1.Network{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       "Network",
+					APIVersion: "config.openshift.io/v1",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "cluster",
+				},
+				Spec: configv1.NetworkSpec{
+					ServiceNetwork: []string{"172.30.0.0/16", "fd02::/112"},
+				},
+				Status: configv1.NetworkStatus{
+					ServiceNetwork: []string{},
+				},
+			},
+			expectedError:        false,
+			expectedNetworkStack: provisioning.NetworkStackDual,
 		},
 	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			got := networkStack(tt.ips)
-			if !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("networkStack() = %v, want %v", got, tt.want)
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Logf("Testing tc : %s", tc.name)
+
+			r := &ProvisioningReconciler{
+				Scheme:   setUpSchemeForReconciler(),
+				OSClient: fakeconfigclientset.NewSimpleClientset(tc.networkCR),
 			}
+			ns, err := r.networkStackFromServiceNetwork(context.TODO())
+			if !tc.expectedError && err != nil {
+				t.Errorf("unexpected error: %v", err)
+				return
+			}
+			assert.Equal(t, tc.expectedNetworkStack, ns, "network stack results did not match")
+			return
 		})
 	}
 }
 
-func TestAPIServerInternalHost(t *testing.T) {
-	infra := &configv1.Infrastructure{
-		TypeMeta: metav1.TypeMeta{
-			Kind:       "Infrastructure",
-			APIVersion: "config.openshift.io/v1",
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Name: "cluster",
-		},
-		Status: configv1.InfrastructureStatus{
-			APIServerInternalURL: "https://api-int.ostest.test.metalkube.org:6443",
-		},
-	}
-	want := "api-int.ostest.test.metalkube.org"
-
-	r := &ProvisioningReconciler{
-		Scheme:   setUpSchemeForReconciler(),
-		OSClient: fakeconfigclientset.NewSimpleClientset(infra),
-	}
-	got, err := r.apiServerInternalHost(context.TODO())
-	if err != nil {
-		t.Errorf("ProvisioningReconciler.apiServerInternalHost() error = %v", err)
-		return
-	}
-	if got != want {
-		t.Errorf("ProvisioningReconciler.apiServerInternalHost() = %v, want %v", got, want)
-	}
-}
-
 func TestUpdateProvisioningMacAddresses(t *testing.T) {
 	sc := setUpSchemeForReconciler()
 	objects := []runtime.Object{

go.mod

@@ -23,6 +23,7 @@ require (
 	sigs.k8s.io/controller-runtime v0.10.2
 	sigs.k8s.io/controller-tools v0.4.1
 	sigs.k8s.io/kustomize/kustomize/v3 v3.9.4
+	sigs.k8s.io/yaml v1.2.0
 )
 
 replace github.com/metal3-io/baremetal-operator => github.com/openshift/baremetal-operator v0.0.0-20210527161605-4e331bfd4b1d

manifests/0000_31_cluster-baremetal-operator_05_rbac.yaml

@@ -145,6 +145,14 @@ rules:
   - infrastructures/status
   verbs:
   - get
+- apiGroups:
+  - config.openshift.io
+  resources:
+  - networks
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - config.openshift.io
   resources:

provisioning/baremetal_pod.go

@@ -324,7 +324,7 @@ func ipOptionForExternal(info *ProvisioningInfo) string {
 	case NetworkStackV6:
 		optionValue = "ip=dhcp6"
 	case NetworkStackDual:
-		optionValue = ""
+		optionValue = "ip=dhcp,dhcp6"
 	}
 	return optionValue
 }

provisioning/baremetal_pod_test.go

@@ -491,7 +491,7 @@ func TestIPOptionForExternal(t *testing.T) {
 		},
 		{
 			ns:   NetworkStackDual,
-			want: "",
+			want: "ip=dhcp,dhcp6",
 		},
 	}
 	for _, tt := range tests {