Merge pull request #18468 from mfojtik/limit-openshift-ca-data

Automatic merge from submit-queue.

apps: do not inject oversized env vars into deployer pod

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1535375

@deads2k @smarterclayton this will allow injecting reasonable sized vars into deployer, we can start deprecation process for `OPENSHIFT_CA_DATA` in 3.10?

Author: openshift-merge-robot

pkg/apps/controller/deployer/deployer_controller.go

@@ -43,6 +43,10 @@ import (
 // In most cases, we shouldn't need to retry up to maxRetryCount...
 const maxRetryCount = 15
 
+// maxInjectedEnvironmentAllowedSize represents maximum size of a value of environment variable
+// that we will inject to a container. The default is 128Kb.
+const maxInjectedEnvironmentAllowedSize = 1000 * 128
+
 // fatalError is an error which can't be retried.
 type fatalError string
 
@@ -463,6 +467,12 @@ func (c *DeploymentController) makeDeployerContainer(strategy *appsapi.Deploymen
 		if set.Has(env.Name) {
 			continue
 		}
+		// TODO: The size of environment value should be probably validated in k8s api validation
+		//       as when the env var size is more than 128kb the execve calls will fail.
+		if len(env.Value) > maxInjectedEnvironmentAllowedSize {
+			glog.Errorf("failed to inject %s environment variable as the size exceed %d bytes", env.Name, maxInjectedEnvironmentAllowedSize)
+			continue
+		}
 		environment = append(environment, env)
 	}
 

pkg/apps/controller/deployer/deployer_controller_test.go

@@ -116,6 +116,48 @@ func okContainer() *v1.Container {
 	}
 }
 
+func TestMakeDeployerContainer(t *testing.T) {
+	randSeq := func(n int) string {
+		b := make([]byte, n)
+		for i := range b {
+			b[i] = '0'
+		}
+		return string(b)
+	}
+	client := &fake.Clientset{}
+	controller := okDeploymentController(client, nil, nil, true, v1.PodUnknown)
+	strategy := appstest.OkRollingStrategy()
+	tests := []struct {
+		name        string
+		environment []v1.EnvVar
+		expected    []v1.EnvVar
+	}{
+		{
+			name:        "simple var should be injected",
+			environment: []v1.EnvVar{{Name: "FOO", Value: "BAR"}},
+			expected:    []v1.EnvVar{{Name: "FOO", Value: "BAR"}},
+		},
+		{
+			name:        "big vars should not be injected",
+			environment: []v1.EnvVar{{Name: "FOO", Value: randSeq(1001 * 128)}},
+			expected:    []v1.EnvVar{},
+		},
+	}
+	for _, test := range tests {
+		controller.environment = test.environment
+		container := controller.makeDeployerContainer(&strategy)
+		if len(test.expected) == 0 {
+			if len(container.Env) > 0 {
+				t.Errorf("%s: expected no variables in container env vars, got %#v", test.name, container.Env)
+			}
+			continue
+		}
+		if !reflect.DeepEqual(container.Env, test.expected) {
+			t.Errorf("%s: expected env vars (%#v) does match container env vars (%#v)", test.name, test.expected, container.Env)
+		}
+	}
+}
+
 // TestHandle_createPodOk ensures that a deployer pod created in response
 // to a new deployment is valid.
 func TestHandle_createPodOk(t *testing.T) {