Don't rollout revision until three etcd endpoints are listed

Hold revision rollouts until we collect three etcd endpoints (localhost,
local IP and any other). This ensures that kube-apiserver has another
etcd to connect to when local instance is being reconfigured.

This functionality is skipped in control plane uses single replica
topology

Author: Vadim Rutkovsky

pkg/operator/starter.go

@@ -267,6 +267,7 @@ func RunOperator(ctx context.Context, controllerContext *controllercmd.Controlle
 		kubeInformersForNamespaces,
 		kubeClient,
 		startupmonitorreadiness.IsStartupMonitorEnabledFunction(configInformers.Config().V1().Infrastructures().Lister(), operatorClient),
+		notOnSingleReplicaTopology,
 		controllerContext.EventRecorder,
 	)
 

pkg/operator/targetconfigcontroller/targetconfigcontroller.go

@@ -48,7 +48,8 @@ type TargetConfigController struct {
 	kubeClient      kubernetes.Interface
 	configMapLister corev1listers.ConfigMapLister
 
-	isStartupMonitorEnabledFn func() (bool, error)
+	isStartupMonitorEnabledFn    func() (bool, error)
+	notOnSingleReplicaTopologyFn func() bool
 }
 
 func NewTargetConfigController(
@@ -58,15 +59,17 @@ func NewTargetConfigController(
 	kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces,
 	kubeClient kubernetes.Interface,
 	isStartupMonitorEnabledFn func() (bool, error),
+	notOnSingleReplicaTopologyFn func() bool,
 	eventRecorder events.Recorder,
 ) factory.Controller {
 	c := &TargetConfigController{
-		targetImagePullSpec:       targetImagePullSpec,
-		operatorImagePullSpec:     operatorImagePullSpec,
-		operatorClient:            operatorClient,
-		kubeClient:                kubeClient,
-		configMapLister:           kubeInformersForNamespaces.ConfigMapLister(),
-		isStartupMonitorEnabledFn: isStartupMonitorEnabledFn,
+		targetImagePullSpec:          targetImagePullSpec,
+		operatorImagePullSpec:        operatorImagePullSpec,
+		operatorClient:               operatorClient,
+		kubeClient:                   kubeClient,
+		configMapLister:              kubeInformersForNamespaces.ConfigMapLister(),
+		isStartupMonitorEnabledFn:    isStartupMonitorEnabledFn,
+		notOnSingleReplicaTopologyFn: notOnSingleReplicaTopologyFn,
 	}
 
 	return factory.New().WithInformers(
@@ -100,7 +103,8 @@ func (c TargetConfigController) sync(ctx context.Context, syncContext factory.Sy
 	}
 
 	// block until config is observed and specific paths are present
-	if err := isRequiredConfigPresent(operatorSpec.ObservedConfig.Raw); err != nil {
+	isNotOnSingleReplicaTopology := c.notOnSingleReplicaTopologyFn()
+	if err := isRequiredConfigPresent(operatorSpec.ObservedConfig.Raw, isNotOnSingleReplicaTopology); err != nil {
 		syncContext.Recorder().Warning("ConfigMissing", err.Error())
 		return err
 	}
@@ -116,7 +120,7 @@ func (c TargetConfigController) sync(ctx context.Context, syncContext factory.Sy
 	return nil
 }
 
-func isRequiredConfigPresent(config []byte) error {
+func isRequiredConfigPresent(config []byte, isNotSingleNode bool) error {
 	if len(config) == 0 {
 		return fmt.Errorf("no observedConfig")
 	}
@@ -148,6 +152,16 @@ func isRequiredConfigPresent(config []byte) error {
 		if configValString, ok := configVal.(string); ok && len(configValString) == 0 {
 			return fmt.Errorf("%v empty in config", strings.Join(requiredPath, "."))
 		}
+
+		if len(requiredPath) == 2 && requiredPath[0] == "apiServerArguments" && requiredPath[1] == "etcd-servers" && isNotSingleNode {
+			configValSlice, ok := configVal.([]interface{})
+			if !ok {
+				return fmt.Errorf("%v is not a slice", strings.Join(requiredPath, "."))
+			}
+			if len(configValSlice) < 3 {
+				return fmt.Errorf("%v has less than three endpoints: %v", strings.Join(requiredPath, "."), configValSlice)
+			}
+		}
 	}
 	return nil
 }

pkg/operator/targetconfigcontroller/targetconfigcontroller_test.go

@@ -1,6 +1,7 @@
 package targetconfigcontroller
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 
@@ -110,7 +111,7 @@ func TestIsRequiredConfigPresent(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			actual := isRequiredConfigPresent([]byte(test.config))
+			actual := isRequiredConfigPresent([]byte(test.config), false)
 			switch {
 			case actual == nil && len(test.expectedError) == 0:
 			case actual == nil && len(test.expectedError) != 0:
@@ -237,3 +238,101 @@ func TestManageTemplate(t *testing.T) {
 		})
 	}
 }
+
+func TestIsRequiredConfigPresentEtcdEndpoints(t *testing.T) {
+	configTemplate := `{
+		 "servingInfo": {
+		   "namedCertificates": [
+		     {
+		       "certFile": "/etc/kubernetes/static-pod-certs/secrets/localhost-serving-cert-certkey/tls.crt",
+		       "keyFile": "/etc/kubernetes/static-pod-certs/secrets/localhost-serving-cert-certkey/tls.key"
+		     }
+		   ]
+		 },
+		 "admission": {"pluginConfig": { "network.openshift.io/RestrictedEndpointsAdmission": {}}},
+		 "apiServerArguments": {
+		   "etcd-servers": %s
+		 }
+		}
+		`
+	tests := []struct {
+		name            string
+		etcdServers     string
+		expectedError   string
+		isNotSingleNode bool
+	}{
+		{
+			name:          "nil-storage-urls",
+			etcdServers:   "null",
+			expectedError: "apiServerArguments.etcd-servers null in config",
+		},
+		{
+			name:          "missing-storage-urls",
+			etcdServers:   "[]",
+			expectedError: "apiServerArguments.etcd-servers empty in config",
+		},
+		{
+			name:          "empty-string-storage-urls",
+			etcdServers:   `""`,
+			expectedError: "apiServerArguments.etcd-servers empty in config",
+		},
+		{
+			name:            "one-etcd-server",
+			etcdServers:     `[ "val" ]`,
+			isNotSingleNode: true,
+			expectedError:   "apiServerArguments.etcd-servers has less than three endpoints",
+		},
+		{
+			name:            "one-etcd-server-sno",
+			etcdServers:     `[ "val" ]`,
+			isNotSingleNode: false,
+		},
+		{
+			name:            "two-etcd-servers",
+			etcdServers:     `[ "val1", "val2" ]`,
+			isNotSingleNode: true,
+			expectedError:   "apiServerArguments.etcd-servers has less than three endpoints",
+		},
+		{
+			name:            "two-etcd-servers-sno",
+			etcdServers:     `[ "val1", "val2" ]`,
+			isNotSingleNode: false,
+		},
+		{
+			name:            "three-etcd-servers",
+			etcdServers:     `[ "val1", "val2", "val3" ]`,
+			isNotSingleNode: true,
+		},
+		{
+			name:            "three-etcd-servers-sno",
+			etcdServers:     `[ "val1", "val2", "val3" ]`,
+			isNotSingleNode: false,
+		},
+		{
+			name:            "four-etcd-servers",
+			etcdServers:     `[ "val1", "val2", "val3", "val4" ]`,
+			isNotSingleNode: true,
+		},
+		{
+			name:            "four-etcd-servers-sno",
+			etcdServers:     `[ "val1", "val2", "val3", "val4" ]`,
+			isNotSingleNode: false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			config := fmt.Sprintf(configTemplate, test.etcdServers)
+			actual := isRequiredConfigPresent([]byte(config), test.isNotSingleNode)
+			switch {
+			case actual == nil && len(test.expectedError) == 0:
+			case actual == nil && len(test.expectedError) != 0:
+				t.Fatal(actual)
+			case actual != nil && len(test.expectedError) == 0:
+				t.Fatal(actual)
+			case actual != nil && len(test.expectedError) != 0 && !strings.Contains(actual.Error(), test.expectedError):
+				t.Fatal(actual)
+			}
+		})
+	}
+}