Fix policy remove

**What**
- Fix bug that was not comparing updated annotations to the previous
  deployment annotations, this resulted in not removing tolerations
  correctly
- Also, make add/remove behave as a patch, so that we can merge
  multiple policies, instead of just the last policy overwriting
  the previous policies
- Update helm chart RBAC to allow GET on ConfigMaps

Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>

chart/openfaas/templates/controller-rbac.yaml

@@ -73,6 +73,12 @@ rules:
       - get
       - list
       - watch
+  - apiGroups:
+      - ""
+    resources:
+      - configmaps
+    verbs:
+      - get
 ---
 apiVersion: rbac.authorization.k8s.io/v1beta1
 kind: ClusterRoleBinding
@@ -153,6 +159,12 @@ rules:
       - get
       - list
       - watch
+  - apiGroups:
+      - ""
+    resources:
+      - configmaps
+    verbs:
+      - get
 ---
 apiVersion: rbac.authorization.k8s.io/v1beta1
 kind: RoleBinding

handlers/update.go

@@ -121,6 +121,9 @@ func updateDeploymentSpec(
 		// deployment.Labels = labels
 		deployment.Spec.Template.ObjectMeta.Labels = labels
 
+		// store the current annotations so that we can diff the annotations
+		// and determine which policies need to be removed
+		currentAnnotations := deployment.Annotations
 		deployment.Annotations = annotations
 		deployment.Spec.Template.Annotations = annotations
 		deployment.Spec.Template.ObjectMeta.Annotations = annotations
@@ -163,14 +166,12 @@ func updateDeploymentSpec(
 		deployment.Spec.Template.Spec.Containers[0].LivenessProbe = probes.Liveness
 		deployment.Spec.Template.Spec.Containers[0].ReadinessProbe = probes.Readiness
 
-		var annotations map[string]string
-		if request.Annotations != nil {
-			annotations = *request.Annotations
-		}
-
-		// if a policy is removed ... how do we remove the consequences?
 		policies := k8s.NewConfigMapPolicyClient(factory.Client)
-		toRemove := k8s.PoliciesToRemove(annotations, deployment.Annotations)
+
+		// 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
+		// compare to that it will produce an empty list
+		toRemove := k8s.PoliciesToRemove(annotations, currentAnnotations)
 		policyList, err := policies.Get(functionNamespace, toRemove...)
 		if err != nil {
 			return err, http.StatusBadRequest

k8s/policies.go

@@ -17,12 +17,12 @@ import (
 const PolicyAnnotationKey = "com.openfaas.policies"
 
 // PolicyClient defines the interface for CRUD operations on policies
-// and applying faaas-netes policies to function Deployments.
+// and applying faas-netes policies to function Deployments.
 type PolicyClient interface {
 	Get(namespace string, names ...string) ([]Policy, error)
 }
 
-// Policy defined kubernetest specific api extensions that can be predefined and applied
+// Policy is and openfaas api extensions that can be predefined and applied
 // to functions by annotating them with `com.openfaas/policy: name1,name2`
 type Policy struct {
 	// If specified, the function's pod tolerations.
@@ -35,16 +35,27 @@ type Policy struct {
 // override preceding Policies with overlapping configurations.
 func (p Policy) Apply(deployment *appsv1.Deployment) *appsv1.Deployment {
 	if len(p.Tolerations) > 0 {
-		deployment.Spec.Template.Spec.Tolerations = p.Tolerations
+		deployment.Spec.Template.Spec.Tolerations = append(deployment.Spec.Template.Spec.Tolerations, p.Tolerations...)
 	}
 	return deployment
 }
 
 // Remove is the inverse of Apply, removing the mutations that the Policy would have applied
 func (p Policy) Remove(deployment *appsv1.Deployment) *appsv1.Deployment {
-	if reflect.DeepEqual(deployment.Spec.Template.Spec.Tolerations, p.Tolerations) {
-		deployment.Spec.Template.Spec.Tolerations = nil
+
+	for _, policyToleration := range p.Tolerations {
+		// filter the existing tolerations and then update the deployment
+		// filter without allocation implementation from
+		// https://github.com/golang/go/wiki/SliceTricks#filtering-without-allocating
+		newTolerations := deployment.Spec.Template.Spec.Tolerations[:0]
+		for _, toleration := range deployment.Spec.Template.Spec.Tolerations {
+			if !reflect.DeepEqual(policyToleration, toleration) {
+				newTolerations = append(newTolerations, toleration)
+			}
+		}
+		deployment.Spec.Template.Spec.Tolerations = newTolerations
 	}
+
 	return deployment
 }
 

k8s/policies_test.go

@@ -154,14 +154,20 @@ func Test_TolerationsPolicy_Apply(t *testing.T) {
 }
 
 func Test_TolerationsPolicy_Remove(t *testing.T) {
-	expectedTolerations := []corev1.Toleration{
+	tolerations := []corev1.Toleration{
 		{
 			Key:      "foo",
 			Value:    "fooValue",
 			Operator: apiv1.TolerationOpEqual,
 		},
 	}
-	p := Policy{Tolerations: expectedTolerations}
+	nonPolicyToleration := corev1.Toleration{
+		Key:      "second-key",
+		Value:    "anotherValue",
+		Operator: apiv1.TolerationOpEqual,
+	}
+
+	p := Policy{Tolerations: tolerations}
 
 	basicDeployment := &appsv1.Deployment{
 		Spec: appsv1.DeploymentSpec{
@@ -170,15 +176,18 @@ func Test_TolerationsPolicy_Remove(t *testing.T) {
 					Containers: []apiv1.Container{
 						{Name: "testfunc", Image: "alpine:latest"},
 					},
-					Tolerations: expectedTolerations,
+					Tolerations: append(tolerations, nonPolicyToleration),
 				},
 			},
 		},
 	}
 
 	basicDeployment = p.Remove(basicDeployment)
-	if basicDeployment.Spec.Template.Spec.Tolerations != nil {
-		t.Fatalf("expected nil, got %v", basicDeployment.Spec.Template.Spec.Tolerations)
+
+	got := basicDeployment.Spec.Template.Spec.Tolerations
+	expected := []corev1.Toleration{nonPolicyToleration}
+	if !reflect.DeepEqual(got, expected) {
+		t.Fatalf("expected %v, got %v", expected, got)
 	}
 }