fix(k8s): fix builtdns annotations

- Make override of the port work
- Fix from `kuma.io/builtindnsport` to `kuma.io/builtin-dns-port`
- Fix from `kuma.io/builtindns` to `kuma.io/builtin-dns`
- Rewrite `annotations.go` to make it easier to use defaults
- Provide a standard way to mark pod annotations as deprecated and to log about them

Signed-off-by: Charly Molter <charly.molter@konghq.com>

UPGRADE.md

@@ -10,8 +10,10 @@ does not have any particular instructions.
 
 ### Helm
 
-Under `cni.image`, the default values for `repository` and `registry` have been
+- Under `cni.image`, the default values for `repository` and `registry` have been
 changed to agree with the other `image` values.
+- We are deprecating `kuma.io/builtindns` and `kuma.io/builtindnsport` annotations in favour or the clearer `kuma.io/builtin-dns` and `kuma.io/builtin-dns-port` the code is backward compatible
+but you should migrate (a warning is present on the log if you are using the deprecated version).
 
 ## Upgrade to `1.7.x`
 

pkg/plugins/runtime/k8s/containers/factory.go

@@ -67,18 +67,12 @@ func (i *DataplaneProxyFactory) proxyConcurrencyFor(annotations map[string]strin
 
 func (i *DataplaneProxyFactory) envoyAdminPort(annotations map[string]string) (uint32, error) {
 	adminPort, _, err := metadata.Annotations(annotations).GetUint32(metadata.KumaEnvoyAdminPort)
-	if err != nil {
-		return 0, err
-	}
-	return adminPort, nil
+	return adminPort, err
 }
 
 func (i *DataplaneProxyFactory) drainTime(annotations map[string]string) (time.Duration, error) {
-	drainTime, exists := metadata.Annotations(annotations).GetString(metadata.KumaSidecarDrainTime)
-	if !exists {
-		return i.ContainerConfig.DrainTime, nil
-	}
-	return time.ParseDuration(drainTime)
+	r, _, err := metadata.Annotations(annotations).GetDurationWithDefault(i.ContainerConfig.DrainTime, metadata.KumaSidecarDrainTime)
+	return r, err
 }
 
 func (i *DataplaneProxyFactory) NewContainer(
@@ -250,7 +244,7 @@ func (i *DataplaneProxyFactory) sidecarEnvVars(mesh string, podAnnotations map[s
 	}
 
 	// override defaults and cfg env vars with annotations
-	annotationEnvVars, err := metadata.Annotations(podAnnotations).GetMap(metadata.KumaSidecarEnvVarsAnnotation)
+	annotationEnvVars, _, err := metadata.Annotations(podAnnotations).GetMap(metadata.KumaSidecarEnvVarsAnnotation)
 	if err != nil {
 		return nil, err
 	}

pkg/plugins/runtime/k8s/controllers/pod_converter.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"regexp"
-	"strings"
 
 	"github.com/pkg/errors"
 	kube_core "k8s.io/api/core/v1"
@@ -112,12 +111,12 @@ func (p *PodConverter) dataplaneFor(
 			RedirectPortOutbound:  outboundPort,
 			RedirectPortInboundV6: inboundPortV6,
 		}
-		if services, _ := annotations.GetString(metadata.KumaDirectAccess); services != "" {
-			dataplane.Networking.TransparentProxying.DirectAccessServices = strings.Split(services, ",")
+		if directAccessServices, exist := annotations.GetList(metadata.KumaDirectAccess); exist {
+			dataplane.Networking.TransparentProxying.DirectAccessServices = directAccessServices
 		}
-		if reachableServicesRaw, exist := annotations.GetString(metadata.KumaTransparentProxyingReachableServicesAnnotation); exist {
-			reachableServices = strings.Split(reachableServicesRaw, ",")
-			dataplane.Networking.TransparentProxying.ReachableServices = reachableServices
+		if reachableServicesValue, exist := annotations.GetList(metadata.KumaTransparentProxyingReachableServicesAnnotation); exist {
+			dataplane.Networking.TransparentProxying.ReachableServices = reachableServicesValue
+			reachableServices = reachableServicesValue
 		}
 	}
 
@@ -237,10 +236,7 @@ func MetricsAggregateFor(pod *kube_core.Pod) ([]*mesh_proto.PrometheusAggregateM
 		} else {
 			enabled = true
 		}
-		path, exist := metadata.Annotations(pod.Annotations).GetString(fmt.Sprintf(metadata.KumaMetricsPrometheusAggregatePath, app))
-		if !exist {
-			path = "/metrics"
-		}
+		path, _ := metadata.Annotations(pod.Annotations).GetStringWithDefault("/metrics", fmt.Sprintf(metadata.KumaMetricsPrometheusAggregatePath, app))
 		port, exist, err := metadata.Annotations(pod.Annotations).GetUint32(fmt.Sprintf(metadata.KumaMetricsPrometheusAggregatePort, app))
 		if err != nil {
 			return nil, err

pkg/plugins/runtime/k8s/metadata/annotations.go

@@ -1,8 +1,10 @@
 package metadata
 
 import (
+	"fmt"
 	"strconv"
 	"strings"
+	"time"
 
 	"github.com/pkg/errors"
 )
@@ -69,9 +71,12 @@ const (
 	// KumaMetricsPrometheusPath to override `Mesh`-wide default path
 	KumaMetricsPrometheusPath = "prometheus.metrics.kuma.io/path"
 
+	// TODO add issue for deprecation
+	KumaBuiltinDNSDeprecated     = "kuma.io/builtindns"
+	KumaBuiltinDNSPortDeprecated = "kuma.io/builtindnsport"
 	// KumaBuiltinDNS the sidecar will use its builtin DNS
-	KumaBuiltinDNS     = "kuma.io/builtindns"
-	KumaBuiltinDNSPort = "kuma.io/builtindnsport"
+	KumaBuiltinDNS     = "kuma.io/builtin-dns"
+	KumaBuiltinDNSPort = "kuma.io/builtin-dns-port"
 
 	KumaTrafficExcludeInboundPorts  = "traffic.kuma.io/exclude-inbound-ports"
 	KumaTrafficExcludeOutboundPorts = "traffic.kuma.io/exclude-outbound-ports"
@@ -102,6 +107,27 @@ const (
 	KumaMetricsPrometheusAggregatePattern = "^prometheus.metrics.kuma.io/aggregate-([a-zA-Z0-9-]+)-(port|path|enabled)$"
 )
 
+var PodAnnotationDeprecations = []Deprecation{
+	NewReplaceByDeprecation(KumaBuiltinDNSDeprecated, KumaBuiltinDNS),
+	NewReplaceByDeprecation(KumaBuiltinDNSPortDeprecated, KumaBuiltinDNSPort),
+	{
+		Key:     KumaSidecarInjectionAnnotation,
+		Message: "WARNING: you are using kuma.io/sidecar-injection as annotation. Please migrate it to label to have strong guarantee that application can only start with sidecar",
+	},
+}
+
+type Deprecation struct {
+	Key     string
+	Message string
+}
+
+func NewReplaceByDeprecation(old, new string) Deprecation {
+	return Deprecation{
+		Key:     old,
+		Message: fmt.Sprintf("'%s' is being replaced by: '%s'", old, new),
+	}
+}
+
 // Annotations that are being automatically set by the Kuma Sidecar Injector.
 const (
 	KumaSidecarInjectedAnnotation                      = "kuma.io/sidecar-injected"
@@ -137,56 +163,123 @@ const (
 
 type Annotations map[string]string
 
-func (a Annotations) GetEnabled(key string) (bool, bool, error) {
-	value, ok := a[key]
-	if !ok {
-		return false, false, nil
-	}
-	switch value {
-	case AnnotationEnabled, AnnotationTrue:
-		return true, true, nil
-	case AnnotationDisabled, AnnotationFalse:
-		return false, true, nil
-	default:
-		return false, true, errors.Errorf("annotation \"%s\" has wrong value \"%s\"", key, value)
+func (a Annotations) GetEnabled(keys ...string) (bool, bool, error) {
+	return a.GetEnabledWithDefault(false, keys...)
+}
+func (a Annotations) GetEnabledWithDefault(def bool, keys ...string) (bool, bool, error) {
+	v, exists, err := a.getWithDefault(def, func(key, value string) (interface{}, error) {
+		switch value {
+		case AnnotationEnabled, AnnotationTrue:
+			return true, nil
+		case AnnotationDisabled, AnnotationFalse:
+			return false, nil
+		default:
+			return false, errors.Errorf("annotation \"%s\" has wrong value \"%s\"", key, value)
+		}
+	}, keys...)
+	if err != nil {
+		return def, exists, err
 	}
+	return v.(bool), exists, nil
 }
 
-func (a Annotations) GetUint32(key string) (uint32, bool, error) {
-	value, ok := a[key]
-	if !ok {
-		return 0, false, nil
-	}
-	u, err := strconv.ParseUint(value, 10, 32)
+func (a Annotations) GetUint32(keys ...string) (uint32, bool, error) {
+	return a.GetUint32WithDefault(0, keys...)
+}
+func (a Annotations) GetUint32WithDefault(def uint32, keys ...string) (uint32, bool, error) {
+	v, exists, err := a.getWithDefault(def, func(key string, value string) (interface{}, error) {
+		u, err := strconv.ParseUint(value, 10, 32)
+		if err != nil {
+			return 0, errors.Errorf("failed to parse annotation %q: %s", key, err.Error())
+		}
+		return uint32(u), nil
+	}, keys...)
 	if err != nil {
-		return 0, true, errors.Errorf("failed to parse annotation %q: %s", key, err.Error())
+		return def, exists, err
 	}
-	return uint32(u), true, nil
+	return v.(uint32), exists, nil
 }
 
-func (a Annotations) GetString(key string) (string, bool) {
-	value, ok := a[key]
-	if !ok {
-		return "", false
+func (a Annotations) GetString(keys ...string) (string, bool) {
+	return a.GetStringWithDefault("", keys...)
+}
+func (a Annotations) GetStringWithDefault(def string, keys ...string) (string, bool) {
+	v, exists, _ := a.getWithDefault(def, func(key string, value string) (interface{}, error) {
+		return value, nil
+	}, keys...)
+	return v.(string), exists
+}
+
+func (a Annotations) GetDurationWithDefault(def time.Duration, keys ...string) (time.Duration, bool, error) {
+	v, exists, err := a.getWithDefault(def, func(key string, value string) (interface{}, error) {
+		return time.ParseDuration(value)
+	}, keys...)
+	if err != nil {
+		return def, exists, err
 	}
-	return value, true
+	return v.(time.Duration), exists, err
+}
+
+func (a Annotations) GetList(keys ...string) ([]string, bool) {
+	return a.GetListWithDefault(nil, keys...)
+}
+func (a Annotations) GetListWithDefault(def []string, keys ...string) ([]string, bool) {
+	defCopy := []string{}
+	defCopy = append(defCopy, def...)
+	v, exists, _ := a.getWithDefault(defCopy, func(key string, value string) (interface{}, error) {
+		r := strings.Split(value, ",")
+		var res []string
+		for _, v := range r {
+			if v != "" {
+				res = append(res, v)
+			}
+		}
+		return res, nil
+	}, keys...)
+	return v.([]string), exists
 }
 
 // GetMap returns map from annotation. Example: "kuma.io/sidecar-env-vars: TEST1=1;TEST2=2"
-func (a Annotations) GetMap(key string) (map[string]string, error) {
-	value, ok := a[key]
-	if !ok {
-		return nil, nil
+func (a Annotations) GetMap(keys ...string) (map[string]string, bool, error) {
+	return a.GetMapWithDefault(map[string]string{}, keys...)
+}
+func (a Annotations) GetMapWithDefault(def map[string]string, keys ...string) (map[string]string, bool, error) {
+	defCopy := make(map[string]string, len(def))
+	for k, v := range def {
+		defCopy[k] = v
+	}
+	v, exists, err := a.getWithDefault(defCopy, func(key string, value string) (interface{}, error) {
+		result := map[string]string{}
+
+		pairs := strings.Split(value, ";")
+		for _, pair := range pairs {
+			kvSplit := strings.Split(pair, "=")
+			if len(kvSplit) != 2 {
+				return nil, errors.Errorf("invalid format. Map in %q has to be provided in the following format: key1=value1;key2=value2", key)
+			}
+			result[kvSplit[0]] = kvSplit[1]
+		}
+		return result, nil
+	}, keys...)
+	if err != nil {
+		return def, exists, err
 	}
-	result := map[string]string{}
+	return v.(map[string]string), exists, nil
+}
 
-	pairs := strings.Split(value, ";")
-	for _, pair := range pairs {
-		kvSplit := strings.Split(pair, "=")
-		if len(kvSplit) != 2 {
-			return nil, errors.Errorf("invalid format. Map in %q has to be provided in the following format: key1=value1;key2=value2", key)
+func (a Annotations) getWithDefault(def interface{}, fn func(string, string) (interface{}, error), keys ...string) (interface{}, bool, error) {
+	res := def
+	exists := false
+	for _, k := range keys {
+		v, ok := a[k]
+		if ok {
+			exists = true
+			r, err := fn(k, v)
+			if err != nil {
+				return nil, exists, err
+			}
+			res = r
 		}
-		result[kvSplit[0]] = kvSplit[1]
 	}
-	return result, nil
+	return res, exists, nil
 }

pkg/plugins/runtime/k8s/metadata/annotations_test.go

@@ -56,6 +56,72 @@ var _ = Describe("Kubernetes Annotations", func() {
 		})
 	})
 
+	Describe("withDefaultUint", func() {
+		It("not set annotations", func() {
+			res, exists, err := metadata.Annotations(map[string]string{}).GetUint32WithDefault(23, "foo")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(exists).To(BeFalse())
+			Expect(res).To(Equal(uint32(23)))
+		})
+		It("use last key entry", func() {
+			res, exists, err := metadata.Annotations(map[string]string{"foo": "43", "bar": "25"}).GetUint32WithDefault(23, "foo", "bar")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(exists).To(BeTrue())
+			Expect(res).To(Equal(uint32(25)))
+		})
+		It("use one key entry", func() {
+			res, exists, err := metadata.Annotations(map[string]string{"foo": "24"}).GetUint32WithDefault(23, "foo")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(exists).To(BeTrue())
+			Expect(res).To(Equal(uint32(24)))
+		})
+		It("bad value", func() {
+			_, exists, err := metadata.Annotations(map[string]string{"foo": "few"}).GetUint32WithDefault(32, "foo")
+			Expect(err).To(HaveOccurred())
+			Expect(exists).To(BeTrue())
+		})
+	})
+	Describe("withDefaultEnabled", func() {
+		It("not set annotations", func() {
+			res, exists, err := metadata.Annotations(map[string]string{}).GetEnabledWithDefault(false, "foo")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(exists).To(BeFalse())
+			Expect(res).To(BeFalse())
+		})
+		It("use last key entry", func() {
+			res, exists, err := metadata.Annotations(map[string]string{"foo": "enabled", "bar": "disabled"}).GetEnabledWithDefault(true, "foo", "bar")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(exists).To(BeTrue())
+			Expect(res).To(BeFalse())
+		})
+		It("use one key entry", func() {
+			res, exists, err := metadata.Annotations(map[string]string{"foo": "disabled"}).GetEnabledWithDefault(true, "foo")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(exists).To(BeTrue())
+			Expect(res).To(BeFalse())
+		})
+		It("bad value", func() {
+			res, exists, err := metadata.Annotations(map[string]string{"foo": "few"}).GetEnabledWithDefault(true, "foo")
+			Expect(err).To(HaveOccurred())
+			Expect(exists).To(BeTrue())
+			Expect(res).To(BeTrue())
+		})
+	})
+	Describe("withDefaultString", func() {
+		It("not set annotations", func() {
+			res, _ := metadata.Annotations(map[string]string{}).GetStringWithDefault("def", "foo")
+			Expect(res).To(Equal("def"))
+		})
+		It("use last key entry", func() {
+			res, _ := metadata.Annotations(map[string]string{"foo": "enabled", "bar": "disabled"}).GetStringWithDefault("", "foo", "bar")
+			Expect(res).To(Equal("disabled"))
+		})
+		It("use one key entry", func() {
+			res, _ := metadata.Annotations(map[string]string{"foo": "disabled"}).GetStringWithDefault("", "foo")
+			Expect(res).To(Equal("disabled"))
+		})
+	})
+
 	Context("GetUint32()", func() {
 		It("should parse value to uint32", func() {
 			// given
@@ -87,11 +153,12 @@ var _ = Describe("Kubernetes Annotations", func() {
 			}
 
 			// when
-			m, err := metadata.Annotations(annotations).GetMap("key1")
+			m, exists, err := metadata.Annotations(annotations).GetMap("key1")
 
 			// then
 			Expect(err).ToNot(HaveOccurred())
 			Expect(m).To(Equal(map[string]string{"TEST1": "1", "TEST2": "2"}))
+			Expect(exists).To(BeTrue())
 		})
 
 		It("should return error if value has wrong format", func() {
@@ -101,10 +168,11 @@ var _ = Describe("Kubernetes Annotations", func() {
 			}
 
 			// when
-			_, err := metadata.Annotations(annotations).GetMap("key1")
+			_, exists, err := metadata.Annotations(annotations).GetMap("key1")
 
 			// then
 			Expect(err).To(MatchError(`invalid format. Map in "key1" has to be provided in the following format: key1=value1;key2=value2`))
+			Expect(exists).To(BeTrue())
 		})
 	})
 })