Merge pull request #4 from tumblr/gabe/fix-annotation-matching

improve error handling for matching annotations

Dockerfile

@@ -15,12 +15,11 @@ RUN make test all
 FROM alpine:latest
 ENV TLS_PORT=9443 \
     LIFECYCLE_PORT=9000 \
-    CONFIG=./conf/sidecars.yaml \
     TLS_CERT_FILE=/var/lib/secrets/cert.crt \
     TLS_KEY_FILE=/var/lib/secrets/cert.key
 RUN apk --no-cache add ca-certificates bash
 COPY --from=0 /src/bin/k8s-sidecar-injector /bin/k8s-sidecar-injector
-COPY ./conf ./conf
+COPY ./conf /conf
 COPY ./entrypoint.sh /bin/entrypoint.sh
 ENTRYPOINT ["entrypoint.sh"]
 EXPOSE $TLS_PORT $LIFECYCLE_PORT

cmd/main.go

@@ -129,7 +129,7 @@ func main() {
 				}
 				glog.V(1).Infof("got %d updated InjectionConfigs from reconciliation", len(updatedInjectionConfigs))
 
-				newInjectionConfigs := make([]config.InjectionConfig, len(updatedInjectionConfigs)+len(cfg.Injections))
+				newInjectionConfigs := make([]*config.InjectionConfig, len(updatedInjectionConfigs)+len(cfg.Injections))
 				{
 					i := 0
 					for k := range cfg.Injections {

entrypoint.sh

@@ -2,7 +2,7 @@
 set -e
 LIFECYCLE_PORT="${LIFECYCLE_PORT:-9000}"
 TLS_PORT="${TLS_PORT:-9443}"
-CONFIG_DIR="${CONFIG_DIR:-conf/}"
+CONFIG_DIR="${CONFIG_DIR:-/conf}"
 TLS_CERT_FILE="${TLS_CERT_FILE:-/var/lib/secrets/cert.crt}"
 TLS_KEY_FILE="${TLS_KEY_FILE:-/var/lib/secrets/cert.key}"
 CONFIGMAP_LABELS="${CONFIGMAP_LABELS:-app=k8s-sidecar-injector}"

internal/pkg/config/config.go

@@ -7,6 +7,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"strings"
 	"sync"
 
 	"github.com/ghodss/yaml"
@@ -39,8 +40,8 @@ type InjectionConfig struct {
 // Config is a struct indicating how a given injection should be configured
 type Config struct {
 	sync.RWMutex
-	AnnotationNamespace string                     `yaml:"annotationnamespace"`
-	Injections          map[string]InjectionConfig `yaml:"injections"`
+	AnnotationNamespace string                      `yaml:"annotationnamespace"`
+	Injections          map[string]*InjectionConfig `yaml:"injections"`
 }
 
 // String returns a string representation of the config
@@ -50,10 +51,10 @@ func (c *InjectionConfig) String() string {
 
 // ReplaceInjectionConfigs will take a list of new InjectionConfigs, and replace the current configuration with them.
 // this blocks waiting on being able to update the configs in place.
-func (c *Config) ReplaceInjectionConfigs(replacementConfigs []InjectionConfig) {
+func (c *Config) ReplaceInjectionConfigs(replacementConfigs []*InjectionConfig) {
 	c.Lock()
 	defer c.Unlock()
-	c.Injections = map[string]InjectionConfig{}
+	c.Injections = map[string]*InjectionConfig{}
 	for _, r := range replacementConfigs {
 		c.Injections[r.Name] = r
 	}
@@ -64,25 +65,26 @@ func (c *Config) ReplaceInjectionConfigs(replacementConfigs []InjectionConfig) {
 func (c *Config) HasInjectionConfig(key string) bool {
 	c.RLock()
 	defer c.RUnlock()
-	_, ok := c.Injections[key]
+	_, ok := c.Injections[strings.ToLower(key)]
 	return ok
 }
 
 // GetInjectionConfig returns the InjectionConfig given a requested key
 func (c *Config) GetInjectionConfig(key string) (*InjectionConfig, error) {
 	c.RLock()
 	defer c.RUnlock()
-	i, ok := c.Injections[key]
+	k := strings.ToLower(key)
+	i, ok := c.Injections[k]
 	if !ok {
 		return nil, fmt.Errorf("no injection config found for annotation %s", key)
 	}
-	return &i, nil
+	return i, nil
 }
 
 // LoadConfigDirectory loads all configs in a directory and returns the Config
 func LoadConfigDirectory(path string) (*Config, error) {
 	cfg := Config{
-		Injections: map[string]InjectionConfig{},
+		Injections: map[string]*InjectionConfig{},
 	}
 	glob := filepath.Join(path, "*.yaml")
 	matches, err := filepath.Glob(glob)
@@ -95,7 +97,7 @@ func LoadConfigDirectory(path string) (*Config, error) {
 			glog.Errorf("Error reading injection config from %s: %v", p, err)
 			return nil, err
 		}
-		cfg.Injections[c.Name] = *c
+		cfg.Injections[c.Name] = c
 	}
 
 	if len(cfg.Injections) == 0 {

internal/pkg/config/watcher/watcher.go

@@ -135,7 +135,7 @@ func mapStringStringToLabelSelector(m map[string]string) string {
 }
 
 // Get fetches all matching ConfigMaps
-func (c *K8sConfigMapWatcher) Get() (cfgs []config.InjectionConfig, err error) {
+func (c *K8sConfigMapWatcher) Get() (cfgs []*config.InjectionConfig, err error) {
 	glog.V(1).Infof("Fetching ConfigMaps...")
 	clist, err := c.client.ConfigMaps(c.Namespace).List(metav1.ListOptions{
 		LabelSelector: mapStringStringToLabelSelector(c.ConfigMapLabels),
@@ -156,16 +156,16 @@ func (c *K8sConfigMapWatcher) Get() (cfgs []config.InjectionConfig, err error) {
 }
 
 // InjectionConfigsFromConfigMap parse items in a configmap into a list of InjectionConfigs
-func InjectionConfigsFromConfigMap(cm v1.ConfigMap) ([]config.InjectionConfig, error) {
-	ics := []config.InjectionConfig{}
+func InjectionConfigsFromConfigMap(cm v1.ConfigMap) ([]*config.InjectionConfig, error) {
+	ics := []*config.InjectionConfig{}
 	for name, payload := range cm.Data {
 		glog.V(3).Infof("Parsing %s/%s:%s into InjectionConfig", cm.ObjectMeta.Namespace, cm.ObjectMeta.Name, name)
 		ic, err := config.LoadInjectionConfig(strings.NewReader(payload))
 		if err != nil {
 			return nil, fmt.Errorf("error parsing ConfigMap %s item %s into injection config: %s", cm.ObjectMeta.Name, name, err.Error())
 		}
 		glog.V(2).Infof("Loaded InjectionConfig %s from ConfigMap %s:%s", ic.Name, cm.ObjectMeta.Name, name)
-		ics = append(ics, *ic)
+		ics = append(ics, ic)
 	}
 	return ics, nil
 }

pkg/server/errors.go

@@ -0,0 +1,37 @@
+package server
+
+import (
+	"fmt"
+)
+
+var (
+	// ErrSkipIgnoredNamespace ...
+	ErrSkipIgnoredNamespace = fmt.Errorf("Skipping pod in ignored namespace")
+	// ErrSkipAlreadyInjected ...
+	ErrSkipAlreadyInjected = fmt.Errorf("Skipping pod that has already been injected")
+	// ErrMissingRequestAnnotation ...
+	ErrMissingRequestAnnotation = fmt.Errorf("Missing injection request annotation")
+	// ErrRequestedSidecarNotFound ...
+	ErrRequestedSidecarNotFound = fmt.Errorf("Requested sidecar not found in configuration")
+)
+
+// GetErrorReason returns a string description for a given error, for use
+// when reporting "reason" in metrics
+func GetErrorReason(err error) string {
+	var reason string
+	switch err {
+	case ErrSkipIgnoredNamespace:
+		reason = "ignored_namespace"
+	case ErrSkipAlreadyInjected:
+		reason = "already_injected"
+	case ErrMissingRequestAnnotation:
+		reason = "no_annotation"
+	case ErrRequestedSidecarNotFound:
+		reason = "missing_config"
+	case nil:
+		reason = ""
+	default:
+		reason = "unknown_error"
+	}
+	return reason
+}

pkg/server/webhook.go

@@ -19,6 +19,11 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/serializer"
 )
 
+const (
+	// StatusInjected is the annotation value for /status that indicates an injection was already performed on this pod
+	StatusInjected = "injected"
+)
+
 var (
 	runtimeScheme = runtime.NewScheme()
 	codecs        = serializer.NewCodecFactory(runtimeScheme)
@@ -120,12 +125,12 @@ func applyDefaultsWorkaround(containers []corev1.Container, volumes []corev1.Vol
 }
 
 // Check whether the target resoured need to be mutated
-func (whsvr *WebhookServer) requiredMutation(ignoredList []string, metadata *metav1.ObjectMeta) string {
-	// skip special kubernete system namespaces
+func (whsvr *WebhookServer) getSidecarConfigurationRequested(ignoredList []string, metadata *metav1.ObjectMeta) (string, error) {
+	// skip special kubernetes system namespaces
 	for _, namespace := range ignoredList {
 		if metadata.Namespace == namespace {
-			glog.Infof("Skip mutation for %v in ignorednamespace: %v", metadata.Name, metadata.Namespace)
-			return ""
+			glog.Infof("Pod %s/%s should skip injection due to ignored namespace", metadata.Name, metadata.Namespace)
+			return "", ErrSkipIgnoredNamespace
 		}
 	}
 
@@ -134,20 +139,29 @@ func (whsvr *WebhookServer) requiredMutation(ignoredList []string, metadata *met
 		annotations = map[string]string{}
 	}
 
-	status := annotations[whsvr.Config.AnnotationNamespace+"/status"]
+	statusAnnotationKey := whsvr.Config.AnnotationNamespace + "/status"
+	requestAnnotationKey := whsvr.Config.AnnotationNamespace + "/request"
+
+	status, ok := annotations[statusAnnotationKey]
+	if ok && strings.ToLower(status) == StatusInjected {
+		glog.Infof("Pod %s/%s annotation %s=%s indicates injection already satisfied, skipping", metadata.Namespace, metadata.Name, statusAnnotationKey, status)
+		return "", ErrSkipAlreadyInjected
+	}
 
 	// determine whether to perform mutation based on annotation for the target resource
-	requestedInjection := ""
-	if strings.ToLower(status) != "injected" {
-		requestedInjection = strings.ToLower(annotations[whsvr.Config.AnnotationNamespace+"/request"])
-		if !whsvr.Config.HasInjectionConfig(requestedInjection) {
-			glog.Errorf("Mutation policy for %v/%v: status:%q requested injection: %s was not in configuration, skipping", metadata.Namespace, metadata.Name, status, requestedInjection)
-			return ""
-		}
+	requestedInjection, ok := annotations[requestAnnotationKey]
+	if !ok {
+		glog.Infof("Pod %s/%s annotation %s is missing, skipping injection", metadata.Namespace, metadata.Name, statusAnnotationKey)
+		return "", ErrMissingRequestAnnotation
+	}
+	injectionKey := strings.ToLower(requestedInjection)
+	if !whsvr.Config.HasInjectionConfig(requestedInjection) {
+		glog.Errorf("Mutation policy for pod %s/%s: requested injection %s was not in configuration, skipping", metadata.Namespace, metadata.Name, requestedInjection)
+		return requestedInjection, ErrRequestedSidecarNotFound
 	}
 
-	glog.Infof("Mutation policy for %v/%v: status:%q injection:%s", metadata.Namespace, metadata.Name, status, requestedInjection)
-	return requestedInjection
+	glog.Infof("Pod %s/%s annotation %s=%s requesting sidecar config %s", metadata.Namespace, metadata.Name, requestAnnotationKey, requestedInjection, injectionKey)
+	return injectionKey, nil
 }
 
 func setEnvironment(target []corev1.Container, addedEnv []corev1.EnvVar) (patch []patchOperation) {
@@ -364,22 +378,23 @@ func (whsvr *WebhookServer) mutate(ar *v1beta1.AdmissionReview) *v1beta1.Admissi
 		}
 	}
 
-	glog.Infof("AdmissionReview for Kind=%v, Namespace=%v Name=%v (%v) UID=%v patchOperation=%v UserInfo=%v",
+	glog.Infof("AdmissionReview for Kind=%s, Namespace=%s Name=%s (%s) UID=%s patchOperation=%s UserInfo=%s",
 		req.Kind, req.Namespace, req.Name, pod.Name, req.UID, req.Operation, req.UserInfo)
 
 	// determine whether to perform mutation
-	injectionKey := whsvr.requiredMutation(ignoredNamespaces, &pod.ObjectMeta)
-	if injectionKey == "" {
-		glog.Infof("Skipping mutation for %s/%s because no injection request in annotations", pod.Namespace, pod.Name)
-		injectionCounter.With(prometheus.Labels{"status": "skipped", "reason": "no_annotation", "requested": injectionKey}).Inc()
+	injectionKey, err := whsvr.getSidecarConfigurationRequested(ignoredNamespaces, &pod.ObjectMeta)
+	if err != nil {
+		glog.Infof("Skipping mutation of %s/%s: %v", pod.Namespace, pod.Name, err)
+		reason := GetErrorReason(err)
+		injectionCounter.With(prometheus.Labels{"status": "skipped", "reason": reason, "requested": injectionKey}).Inc()
 		return &v1beta1.AdmissionResponse{
 			Allowed: true,
 		}
 	}
 
 	injectionConfig, err := whsvr.Config.GetInjectionConfig(injectionKey)
 	if err != nil {
-		glog.Errorf("Error getting injection config for %s, so we will fail open: %s", injectionConfig, err.Error())
+		glog.Errorf("Error getting injection config %s, permitting launch of pod with no sidecar injected: %s", injectionConfig, err.Error())
 		// dont prevent pods from launching! just return allowed
 		injectionCounter.With(prometheus.Labels{"status": "skipped", "reason": "missing_config", "requested": injectionKey}).Inc()
 		return &v1beta1.AdmissionResponse{
@@ -389,7 +404,7 @@ func (whsvr *WebhookServer) mutate(ar *v1beta1.AdmissionReview) *v1beta1.Admissi
 
 	// Workaround: https://github.com/kubernetes/kubernetes/issues/57982
 	applyDefaultsWorkaround(injectionConfig.Containers, injectionConfig.Volumes)
-	annotations := map[string]string{config.InjectionStatusAnnotation: "injected"}
+	annotations := map[string]string{config.InjectionStatusAnnotation: StatusInjected}
 	patchBytes, err := createPatch(&pod, injectionConfig, annotations)
 	if err != nil {
 		injectionCounter.With(prometheus.Labels{"status": "error", "reason": "patching_error", "requested": injectionKey}).Inc()

pkg/server/webhook_test.go

@@ -12,15 +12,27 @@ import (
 )
 
 var (
-	sidecars    = "test/fixtures/sidecars"
-	obj1        = "test/fixtures/k8s/object1.yaml"
-	obj2        = "test/fixtures/k8s/object2.yaml"
-	env1        = "test/fixtures/k8s/env1.yaml"
-	obj3Missing = "test/fixtures/k8s/object3-missing.yaml"
-	obj4        = "test/fixtures/k8s/object4.yaml"
-	obj5        = "test/fixtures/k8s/object5.yaml"
+	sidecars = "test/fixtures/sidecars"
+
+	// all these configs are deserialized into metav1.ObjectMeta structs
+	obj1             = "test/fixtures/k8s/object1.yaml"
+	obj2             = "test/fixtures/k8s/object2.yaml"
+	env1             = "test/fixtures/k8s/env1.yaml"
+	obj3Missing      = "test/fixtures/k8s/object3-missing.yaml"
+	obj4             = "test/fixtures/k8s/object4.yaml"
+	obj5             = "test/fixtures/k8s/object5.yaml"
+	ignoredNamespace = "test/fixtures/k8s/ignored-namespace-pod.yaml"
+	badSidecar       = "test/fixtures/k8s/bad-sidecar.yaml"
+
+	testIgnoredNamespaces = []string{"ignore-me"}
 )
 
+type expectedSidecarConfiguration struct {
+	configuration   string
+	expectedSidecar string
+	expectedError   error
+}
+
 func TestLoadConfig(t *testing.T) {
 	expectedNumInjectionConfigs := 4
 	c, err := config.LoadConfigDirectory(sidecars)
@@ -46,16 +58,19 @@ func TestLoadConfig(t *testing.T) {
 	}
 
 	// load some objects that are k8s metadata objects
-	objects := map[string]string{
-		obj1:        "sidecar-test",
-		obj2:        "complex-sidecar",
-		env1:        "env1",
-		obj3Missing: "", // this one is missing any annotations :)
-		obj4:        "", // this one is already injected, so it should not get injected again
-		obj5:        "volume-mounts",
+	tests := []expectedSidecarConfiguration{
+		{configuration: obj1, expectedSidecar: "sidecar-test"},
+		{configuration: obj2, expectedSidecar: "complex-sidecar"},
+		{configuration: env1, expectedSidecar: "env1"},
+		{configuration: obj3Missing, expectedSidecar: "", expectedError: ErrMissingRequestAnnotation}, // this one is missing any annotations :)
+		{configuration: obj4, expectedSidecar: "", expectedError: ErrSkipAlreadyInjected},             // this one is already injected, so it should not get injected again
+		{configuration: obj5, expectedSidecar: "volume-mounts"},
+		{configuration: ignoredNamespace, expectedSidecar: "", expectedError: ErrSkipIgnoredNamespace},
+		{configuration: badSidecar, expectedSidecar: "this-doesnt-exist", expectedError: ErrRequestedSidecarNotFound},
 	}
-	for f, k := range objects {
-		data, err := ioutil.ReadFile(f)
+
+	for _, test := range tests {
+		data, err := ioutil.ReadFile(test.configuration)
 		if err != nil {
 			t.Errorf("unable to load object metadata yaml: %v", err)
 			t.Fail()
@@ -66,9 +81,13 @@ func TestLoadConfig(t *testing.T) {
 			t.Errorf("unable to unmarshal object metadata yaml: %v", err)
 			t.Fail()
 		}
-		key := s.requiredMutation([]string{}, obj)
-		if key != k {
-			t.Errorf("%s: expected required mutation key to be %v but was %v instead", f, k, key)
+		key, err := s.getSidecarConfigurationRequested(testIgnoredNamespaces, obj)
+		if err != test.expectedError {
+			t.Errorf("%s: error %v did not match %v", test.configuration, err, test.expectedError)
+			t.Fail()
+		}
+		if key != test.expectedSidecar {
+			t.Errorf("%s: expected sidecar to be %v but was %v instead", test.configuration, test.expectedSidecar, key)
 			t.Fail()
 		}
 	}

test/fixtures/k8s/bad-sidecar.yaml

@@ -0,0 +1,4 @@
+name: bad-sidecar
+namespace: "test-namespace"
+annotations:
+  "injector.unittest.com/request": "this-doesnt-exist"

test/fixtures/k8s/ignored-namespace-pod.yaml

@@ -0,0 +1,4 @@
+name: ignored-namespace
+namespace: "ignore-me"
+annotations:
+  "injector.unittest.com/request": "volume-mounts"