From f0f00ff9b9b184a306676050feb8d5bc6e8f0443 Mon Sep 17 00:00:00 2001 From: David Eads Date: Wed, 22 Jul 2020 10:08:37 -0400 Subject: [PATCH] update CVO to inject internal loadbalancer for use by the CVO pod During upgrades and kube-apiserver rollouts, the localhost connection can become unavailable for multiple minutes. Using the internal loadbalancer prevents the outage and lease loss. --- ...luster-version-operator_03_deployment.yaml | 4 +- lib/resourcebuilder/apps.go | 57 ++++++- lib/resourcebuilder/podspec.go | 75 +++++++++ lib/resourcebuilder/podspec_test.go | 156 ++++++++++++++++++ 4 files changed, 282 insertions(+), 10 deletions(-) diff --git a/install/0000_00_cluster-version-operator_03_deployment.yaml b/install/0000_00_cluster-version-operator_03_deployment.yaml index 29949ffcb..3b3ae8d1b 100644 --- a/install/0000_00_cluster-version-operator_03_deployment.yaml +++ b/install/0000_00_cluster-version-operator_03_deployment.yaml @@ -45,9 +45,9 @@ spec: name: serving-cert readOnly: true env: - - name: KUBERNETES_SERVICE_PORT # allows CVO to communicate with apiserver directly on same host. + - name: KUBERNETES_SERVICE_PORT # allows CVO to communicate with apiserver directly on same host. Is substituted with port from infrastructures.status.apiServerInternalURL if available. value: "6443" - - name: KUBERNETES_SERVICE_HOST # allows CVO to communicate with apiserver directly on same host. + - name: KUBERNETES_SERVICE_HOST # allows CVO to communicate with apiserver directly on same host. Is substituted with hostname from infrastructures.status.apiServerInternalURL if available. value: "127.0.0.1" - name: NODE_NAME valueFrom: diff --git a/lib/resourcebuilder/apps.go b/lib/resourcebuilder/apps.go index b9ad2a3d4..da72f81bb 100644 --- a/lib/resourcebuilder/apps.go +++ b/lib/resourcebuilder/apps.go @@ -3,6 +3,8 @@ package resourcebuilder import ( "context" "fmt" + "net" + "net/url" "strings" configv1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" @@ -22,18 +24,20 @@ import ( ) type deploymentBuilder struct { - client *appsclientv1.AppsV1Client - proxyGetter configv1.ProxiesGetter - raw []byte - modifier MetaV1ObjectModifierFunc - mode Mode + client *appsclientv1.AppsV1Client + proxyGetter configv1.ProxiesGetter + infrastructureGetter configv1.InfrastructuresGetter + raw []byte + modifier MetaV1ObjectModifierFunc + mode Mode } func newDeploymentBuilder(config *rest.Config, m lib.Manifest) Interface { return &deploymentBuilder{ - client: appsclientv1.NewForConfigOrDie(withProtobuf(config)), - proxyGetter: configv1.NewForConfigOrDie(config), - raw: m.Raw, + client: appsclientv1.NewForConfigOrDie(withProtobuf(config)), + proxyGetter: configv1.NewForConfigOrDie(config), + infrastructureGetter: configv1.NewForConfigOrDie(config), + raw: m.Raw, } } @@ -72,6 +76,43 @@ func (b *deploymentBuilder) Do(ctx context.Context) error { } } + // if we detect the CVO deployment we need to replace the KUBERNETES_SERVICE_HOST env var with the internal load + // balancer to be resilient to kube-apiserver rollouts that cause the localhost server to become non-responsive for + // multiple minutes. + if deployment.Namespace == "openshift-cluster-version" && deployment.Name == "cluster-version-operator" { + infrastructureConfig, err := b.infrastructureGetter.Infrastructures().Get("cluster", metav1.GetOptions{}) + // not found just means that we don't have infrastructure configuration yet, so we should tolerate not found and avoid substitution + if err != nil && !errors.IsNotFound(err) { + return err + } + if !errors.IsNotFound(err) { + lbURL, err := url.Parse(infrastructureConfig.Status.APIServerInternalURL) + if err != nil { + return err + } + // if we have any error and have empty strings, substitution below will do nothing and leave the manifest specified value + // errors can happen when the port is not specified, in which case we have a host and we write that into the env vars + lbHost, lbPort, err := net.SplitHostPort(lbURL.Host) + if err != nil { + if strings.Contains(err.Error(), "missing port in address") { + lbHost = lbURL.Host + lbPort = "" + } else { + return err + } + } + err = updatePodSpecWithInternalLoadBalancerKubeService( + &deployment.Spec.Template.Spec, + []string{"cluster-version-operator"}, + lbHost, + lbPort, + ) + if err != nil { + return err + } + } + } + _, updated, err := resourceapply.ApplyDeployment(b.client, deployment) if err != nil { return err diff --git a/lib/resourcebuilder/podspec.go b/lib/resourcebuilder/podspec.go index 105d268d2..c2fc516a1 100644 --- a/lib/resourcebuilder/podspec.go +++ b/lib/resourcebuilder/podspec.go @@ -45,3 +45,78 @@ func updatePodSpecWithProxy(podSpec *corev1.PodSpec, containerNames []string, ht return nil } + +// updatePodSpecWithInternalLoadBalancerKubeService mutates the input podspec by setting the KUBERNETES_SERVICE_HOST to the internal +// loadbalancer endpoint and the KUBERNETES_SERVICE_PORT to the specified port +func updatePodSpecWithInternalLoadBalancerKubeService(podSpec *corev1.PodSpec, containerNames []string, internalLoadBalancerHost, internalLoadBalancerPort string) error { + hasInternalLoadBalancer := len(internalLoadBalancerHost) > 0 + if !hasInternalLoadBalancer { + return nil + } + + for _, containerName := range containerNames { + found := false + for i := range podSpec.Containers { + if podSpec.Containers[i].Name != containerName { + continue + } + found = true + + podSpec.Containers[i].Env = setKubeServiceValue(podSpec.Containers[i].Env, internalLoadBalancerHost, internalLoadBalancerPort) + } + for i := range podSpec.InitContainers { + if podSpec.InitContainers[i].Name != containerName { + continue + } + found = true + + podSpec.InitContainers[i].Env = setKubeServiceValue(podSpec.Containers[i].Env, internalLoadBalancerHost, internalLoadBalancerPort) + } + + if !found { + return fmt.Errorf("requested injection for non-existent container: %q", containerName) + } + } + + return nil +} + +// setKubeServiceValue replaces values if they are present and adds them if they are not +func setKubeServiceValue(in []corev1.EnvVar, internalLoadBalancerHost, internalLoadBalancerPort string) []corev1.EnvVar { + ret := []corev1.EnvVar{} + + portVal := "443" + if len(internalLoadBalancerPort) != 0 { + portVal = internalLoadBalancerPort + } + + foundPort := false + foundHost := false + for j := range in { + ret = append(ret, *in[j].DeepCopy()) + if ret[j].Name == "KUBERNETES_SERVICE_PORT" { + foundPort = true + ret[j].Value = portVal + } + if ret[j].Name == "KUBERNETES_SERVICE_HOST" { + foundHost = true + ret[j].Value = internalLoadBalancerHost + } + } + + if !foundPort { + ret = append(ret, corev1.EnvVar{ + Name: "KUBERNETES_SERVICE_PORT", + Value: portVal, + }) + } + + if !foundHost { + ret = append(ret, corev1.EnvVar{ + Name: "KUBERNETES_SERVICE_HOST", + Value: internalLoadBalancerHost, + }) + } + + return ret +} diff --git a/lib/resourcebuilder/podspec_test.go b/lib/resourcebuilder/podspec_test.go index 458b7f6bc..b435bcd39 100644 --- a/lib/resourcebuilder/podspec_test.go +++ b/lib/resourcebuilder/podspec_test.go @@ -110,3 +110,159 @@ func TestUpdatePodSpecWithProxy(t *testing.T) { }) } } + +func TestUpdatePodSpecWithInternalLoadBalancerKubeService(t *testing.T) { + tests := []struct { + name string + + input *corev1.PodSpec + containerNames []string + lbHost, lbPort string + + expectedErr string + expected *corev1.PodSpec + }{ + { + name: "no lbhost val", + input: &corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "foo", + }, + }, + }, + expected: &corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "foo", + }, + }, + }, + }, + { + name: "host and port, add to container and mutate", + containerNames: []string{"foo", "init-foo"}, + lbHost: "lbhost-val", + lbPort: "lbport-val", + input: &corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Name: "init-foo", + Env: []corev1.EnvVar{ + {Name: "KUBERNETES_SERVICE_PORT", Value: "oldport-val"}, + {Name: "KUBERNETES_SERVICE_HOST", Value: "oldhost-val"}, + }, + }, + { + Name: "init-bar", + }, + }, + Containers: []corev1.Container{ + { + Name: "foo", + }, + { + Name: "bar", + }, + }, + }, + expected: &corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Name: "init-foo", + Env: []corev1.EnvVar{ + {Name: "KUBERNETES_SERVICE_PORT", Value: "lbport-val"}, + {Name: "KUBERNETES_SERVICE_HOST", Value: "lbhost-val"}, + }, + }, + { + Name: "init-bar", + }, + }, + Containers: []corev1.Container{ + { + Name: "foo", + Env: []corev1.EnvVar{ + {Name: "KUBERNETES_SERVICE_PORT", Value: "lbport-val"}, + {Name: "KUBERNETES_SERVICE_HOST", Value: "lbhost-val"}, + }, + }, + { + Name: "bar", + }, + }, + }, + }, + { + name: "lbhost only, add to container and mutate", + containerNames: []string{"foo", "init-foo"}, + lbHost: "lbhost-val", + input: &corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Name: "init-foo", + Env: []corev1.EnvVar{ + {Name: "KUBERNETES_SERVICE_PORT", Value: "oldport-val"}, + {Name: "KUBERNETES_SERVICE_HOST", Value: "oldhost-val"}, + }}, + { + Name: "init-bar", + }, + }, + Containers: []corev1.Container{ + { + Name: "foo", + }, + { + Name: "bar", + }, + }, + }, + expected: &corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Name: "init-foo", + Env: []corev1.EnvVar{ + {Name: "KUBERNETES_SERVICE_PORT", Value: "443"}, + {Name: "KUBERNETES_SERVICE_HOST", Value: "lbhost-val"}, + }, + }, + { + Name: "init-bar", + }, + }, + Containers: []corev1.Container{ + { + Name: "foo", + Env: []corev1.EnvVar{ + {Name: "KUBERNETES_SERVICE_PORT", Value: "443"}, + {Name: "KUBERNETES_SERVICE_HOST", Value: "lbhost-val"}, + }, + }, + { + Name: "bar", + }, + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + err := updatePodSpecWithInternalLoadBalancerKubeService(test.input, test.containerNames, test.lbHost, test.lbPort) + switch { + case err == nil && len(test.expectedErr) == 0: + case err != nil && len(test.expectedErr) == 0: + t.Fatal(err) + case err == nil && len(test.expectedErr) != 0: + t.Fatal(err) + case err != nil && len(test.expectedErr) != 0 && err.Error() != test.expectedErr: + t.Fatal(err) + } + + if !reflect.DeepEqual(test.input, test.expected) { + t.Error(diff.ObjectDiff(test.input, test.expected)) + } + }) + } +}