fix: do not assume container 0 is the function container

Add a utility method for correctly finding the index of the function
Container inside a Deployment spec.

Signed-off-by: Lucas Roesler <[email protected]>

Author: LucasRoesler

pkg/handlers/update.go

@@ -91,14 +91,19 @@ func updateDeploymentSpec(
 	}
 
 	if len(deployment.Spec.Template.Spec.Containers) > 0 {
-		deployment.Spec.Template.Spec.Containers[0].Image = request.Image
+		idx, _ := k8s.FunctionContainer(*deployment)
+		if idx < 0 {
+			return fmt.Errorf("deployment is not a valid function spec"), http.StatusBadRequest
+		}
+
+		deployment.Spec.Template.Spec.Containers[idx].Image = request.Image
 
 		// Disabling update support to prevent unexpected mutations of deployed functions,
 		// since imagePullPolicy is now configurable. This could be reconsidered later depending
 		// on desired behavior, but will need to be updated to take config.
-		//deployment.Spec.Template.Spec.Containers[0].ImagePullPolicy = v1.PullAlways
+		//deployment.Spec.Template.Spec.Containers[idx].ImagePullPolicy = v1.PullAlways
 
-		deployment.Spec.Template.Spec.Containers[0].Env = buildEnvVars(&request)
+		deployment.Spec.Template.Spec.Containers[idx].Env = buildEnvVars(&request)
 
 		factory.ConfigureReadOnlyRootFilesystem(request, deployment)
 		factory.ConfigureContainerUserID(deployment)
@@ -135,7 +140,7 @@ func updateDeploymentSpec(
 			return resourceErr, http.StatusBadRequest
 		}
 
-		deployment.Spec.Template.Spec.Containers[0].Resources = *resources
+		deployment.Spec.Template.Spec.Containers[idx].Resources = *resources
 
 		secrets := k8s.NewSecretsClient(factory.Client)
 		existingSecrets, err := secrets.GetSecrets(functionNamespace, request.Secrets)
@@ -154,8 +159,8 @@ func updateDeploymentSpec(
 			return err, http.StatusBadRequest
 		}
 
-		deployment.Spec.Template.Spec.Containers[0].LivenessProbe = probes.Liveness
-		deployment.Spec.Template.Spec.Containers[0].ReadinessProbe = probes.Readiness
+		deployment.Spec.Template.Spec.Containers[idx].LivenessProbe = probes.Liveness
+		deployment.Spec.Template.Spec.Containers[idx].ReadinessProbe = probes.Readiness
 
 		// compare the annotations from args to the cache copy of the deployment annotations
 		// at this point we have already updated the annotations to the new value, if we

pkg/k8s/function_status.go

@@ -6,6 +6,7 @@ package k8s
 import (
 	types "github.com/openfaas/faas-provider/types"
 	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
 )
 
 // EnvProcessName is the name of the env variable containing the function process
@@ -19,7 +20,7 @@ func AsFunctionStatus(item appsv1.Deployment) *types.FunctionStatus {
 		replicas = uint64(*item.Spec.Replicas)
 	}
 
-	functionContainer := item.Spec.Template.Spec.Containers[0]
+	_, functionContainer := FunctionContainer(item)
 
 	labels := item.Spec.Template.Labels
 	function := types.FunctionStatus{
@@ -69,7 +70,8 @@ func nodeSelectorToConstraint(deploy appsv1.Deployment) []string {
 }
 
 func hasReadOnlyRootFilesystem(function appsv1.Deployment) bool {
-	securityContext := function.Spec.Template.Spec.Containers[0].SecurityContext
+	_, c := FunctionContainer(function)
+	securityContext := c.SecurityContext
 	if securityContext == nil {
 		return false
 	}
@@ -80,3 +82,18 @@ func hasReadOnlyRootFilesystem(function appsv1.Deployment) bool {
 
 	return *securityContext.ReadOnlyRootFilesystem
 }
+
+// FunctionContainer returns the index and spec of the OpenFaaS function container
+// in the deployment. Use this method to safely retrieve the container spec, it protects
+// the controller from potential changes in the deployment spec by other Operators and
+// Admission webhooks in the cluster.
+//
+// idx will be -1 if the function container is not found.
+func FunctionContainer(function appsv1.Deployment) (idx int, c corev1.Container) {
+	for idx, container := range function.Spec.Template.Spec.Containers {
+		if container.Name == function.Name {
+			return idx, container
+		}
+	}
+	return -1, c
+}

pkg/k8s/securityContext.go

@@ -23,11 +23,22 @@ func (f *FunctionFactory) ConfigureContainerUserID(deployment *appsv1.Deployment
 		functionUser = &userID
 	}
 
-	if deployment.Spec.Template.Spec.Containers[0].SecurityContext == nil {
-		deployment.Spec.Template.Spec.Containers[0].SecurityContext = &corev1.SecurityContext{}
+	if deployment == nil {
+		return
 	}
 
-	deployment.Spec.Template.Spec.Containers[0].SecurityContext.RunAsUser = functionUser
+	idx, container := FunctionContainer(*deployment)
+	if idx < 0 {
+		// function container not found
+		// and there is nothing we can do at this point
+		return
+	}
+
+	if container.SecurityContext == nil {
+		deployment.Spec.Template.Spec.Containers[idx].SecurityContext = &corev1.SecurityContext{}
+	}
+
+	deployment.Spec.Template.Spec.Containers[idx].SecurityContext.RunAsUser = functionUser
 }
 
 // ConfigureReadOnlyRootFilesystem will create or update the required settings and mounts to ensure
@@ -39,19 +50,30 @@ func (f *FunctionFactory) ConfigureContainerUserID(deployment *appsv1.Deployment
 //
 // This method is safe for both create and update operations.
 func (f *FunctionFactory) ConfigureReadOnlyRootFilesystem(request types.FunctionDeployment, deployment *appsv1.Deployment) {
-	if deployment.Spec.Template.Spec.Containers[0].SecurityContext != nil {
-		deployment.Spec.Template.Spec.Containers[0].SecurityContext.ReadOnlyRootFilesystem = &request.ReadOnlyRootFilesystem
+	if deployment == nil {
+		return
+	}
+
+	idx, container := FunctionContainer(*deployment)
+	if idx < 0 {
+		// function container not found
+		// and there is nothing we can do at this point
+		return
+	}
+
+	if container.SecurityContext != nil {
+		deployment.Spec.Template.Spec.Containers[idx].SecurityContext.ReadOnlyRootFilesystem = &request.ReadOnlyRootFilesystem
 	} else {
-		deployment.Spec.Template.Spec.Containers[0].SecurityContext = &corev1.SecurityContext{
+		deployment.Spec.Template.Spec.Containers[idx].SecurityContext = &corev1.SecurityContext{
 			ReadOnlyRootFilesystem: &request.ReadOnlyRootFilesystem,
 		}
 	}
 
 	existingVolumes := removeVolume("temp", deployment.Spec.Template.Spec.Volumes)
 	deployment.Spec.Template.Spec.Volumes = existingVolumes
 
-	existingMounts := removeVolumeMount("temp", deployment.Spec.Template.Spec.Containers[0].VolumeMounts)
-	deployment.Spec.Template.Spec.Containers[0].VolumeMounts = existingMounts
+	existingMounts := removeVolumeMount("temp", container.VolumeMounts)
+	deployment.Spec.Template.Spec.Containers[idx].VolumeMounts = existingMounts
 
 	if request.ReadOnlyRootFilesystem {
 		deployment.Spec.Template.Spec.Volumes = append(
@@ -64,7 +86,7 @@ func (f *FunctionFactory) ConfigureReadOnlyRootFilesystem(request types.Function
 			},
 		)
 
-		deployment.Spec.Template.Spec.Containers[0].VolumeMounts = append(
+		deployment.Spec.Template.Spec.Containers[idx].VolumeMounts = append(
 			existingMounts,
 			corev1.VolumeMount{
 				Name:      "temp",

pkg/k8s/securityContext_test.go

@@ -10,6 +10,7 @@ import (
 
 	appsv1 "k8s.io/api/apps/v1"
 	apiv1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func readOnlyRootDisabled(t *testing.T, deployment *appsv1.Deployment) {
@@ -68,6 +69,9 @@ func readOnlyRootEnabled(t *testing.T, deployment *appsv1.Deployment) {
 func Test_configureReadOnlyRootFilesystem_Disabled_To_Disabled(t *testing.T) {
 	f := mockFactory()
 	deployment := &appsv1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "testfunc",
+		},
 		Spec: appsv1.DeploymentSpec{
 			Template: apiv1.PodTemplateSpec{
 				Spec: apiv1.PodSpec{
@@ -91,6 +95,9 @@ func Test_configureReadOnlyRootFilesystem_Disabled_To_Disabled(t *testing.T) {
 func Test_configureReadOnlyRootFilesystem_Disabled_To_Enabled(t *testing.T) {
 	f := mockFactory()
 	deployment := &appsv1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "testfunc",
+		},
 		Spec: appsv1.DeploymentSpec{
 			Template: apiv1.PodTemplateSpec{
 				Spec: apiv1.PodSpec{
@@ -115,6 +122,9 @@ func Test_configureReadOnlyRootFilesystem_Enabled_To_Disabled(t *testing.T) {
 	f := mockFactory()
 	trueValue := true
 	deployment := &appsv1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "testfunc",
+		},
 		Spec: appsv1.DeploymentSpec{
 			Template: apiv1.PodTemplateSpec{
 				Spec: apiv1.PodSpec{
@@ -155,6 +165,9 @@ func Test_configureReadOnlyRootFilesystem_Enabled_To_Enabled(t *testing.T) {
 	f := mockFactory()
 	trueValue := true
 	deployment := &appsv1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "testfunc",
+		},
 		Spec: appsv1.DeploymentSpec{
 			Template: apiv1.PodTemplateSpec{
 				Spec: apiv1.PodSpec{