Merge pull request #2382 from antoineco/tokenize-ann-secret

Accept ns/name Secret reference in annotations

docs/user-guide/annotations.md

@@ -16,11 +16,11 @@ The following annotations are supported:
 |[nginx.ingress.kubernetes.io/auth-realm](#authentication)|string|
 |[nginx.ingress.kubernetes.io/auth-secret](#authentication)|string|
 |[nginx.ingress.kubernetes.io/auth-type](#authentication)|basic or digest|
-|[nginx.ingress.kubernetes.io/auth-tls-secret](#certificate-authentication)|string|
+|[nginx.ingress.kubernetes.io/auth-tls-secret](#client-certificate-authentication)|string|
-|[nginx.ingress.kubernetes.io/auth-tls-verify-depth](#certificate-authentication)|number|
+|[nginx.ingress.kubernetes.io/auth-tls-verify-depth](#client-certificate-authentication)|number|
-|[nginx.ingress.kubernetes.io/auth-tls-verify-client](#certificate-authentication)|string|
+|[nginx.ingress.kubernetes.io/auth-tls-verify-client](#client-certificate-authentication)|string|
-|[nginx.ingress.kubernetes.io/auth-tls-error-page](#certificate-authentication)|string|
+|[nginx.ingress.kubernetes.io/auth-tls-error-page](#client-certificate-authentication)|string|
-|[nginx.ingress.kubernetes.io/auth-tls-pass-certificate-to-upstream](#certificate-authentication)|"true" or "false"|
+|[nginx.ingress.kubernetes.io/auth-tls-pass-certificate-to-upstream](#client-certificate-authentication)|"true" or "false"|
 |[nginx.ingress.kubernetes.io/auth-url](#external-authentication)|string|
 |[nginx.ingress.kubernetes.io/base-url-scheme](#rewrite)|string|
 |[nginx.ingress.kubernetes.io/client-body-buffer-size](#client-body-buffer-size)|string|
@@ -108,8 +108,8 @@ Indicates the [HTTP Authentication Type: Basic or Digest Access Authentication](
 nginx.ingress.kubernetes.io/auth-secret: secretName
 ```
 
-The name of the secret that contains the usernames and passwords with access to the `path`s defined in the Ingress Rule.
+The name of the Secret that contains the usernames and passwords which are granted access to the `path`s defined in the Ingress rules.
-The secret must be created in the same namespace as the Ingress rule.
+This annotation also accepts the alternative form "namespace/secretName", in which case the Secret lookup is performed in the referenced namespace instead of the Ingress namespace.
 
 ```
 nginx.ingress.kubernetes.io/auth-realm: "realm string"
@@ -161,7 +161,8 @@ The annotations are:
 nginx.ingress.kubernetes.io/auth-tls-secret: secretName
 ```
 
-The name of the secret that contains the full Certificate Authority chain `ca.crt` that is enabled to authenticate against this ingress. It's composed of namespace/secretName.
+The name of the Secret that contains the full Certificate Authority chain `ca.crt` that is enabled to authenticate against this Ingress.
+This annotation also accepts the alternative form "namespace/secretName", in which case the Secret lookup is performed in the referenced namespace instead of the Ingress namespace.
 
 ```
 nginx.ingress.kubernetes.io/auth-tls-verify-depth

internal/ingress/controller/store/store.go

@@ -538,12 +538,12 @@ func (s *k8sStore) updateSecretIngressMap(ing *extensions.Ingress) {
 		"auth-tls-secret",
 	}
 	for _, ann := range secretAnnotations {
-		secrName, err := parser.GetStringAnnotation(ann, ing)
+		secrKey, err := objectRefAnnotationNsKey(ann, ing)
 		if err != nil {
+			glog.Errorf("error reading secret reference in annotation %q: %s", ann, err)
 			continue
 		}
-		if secrName != "" {
+		if secrKey != "" {
-			secrKey := fmt.Sprintf("%v/%v", ing.Namespace, secrName)
 			refSecrets = append(refSecrets, secrKey)
 		}
 	}
@@ -552,6 +552,25 @@ func (s *k8sStore) updateSecretIngressMap(ing *extensions.Ingress) {
 	s.secretIngressMap.Insert(key, refSecrets...)
 }
 
+// objectRefAnnotationNsKey returns an object reference formatted as a
+// 'namespace/name' key from the given annotation name.
+func objectRefAnnotationNsKey(ann string, ing *extensions.Ingress) (string, error) {
+	annValue, err := parser.GetStringAnnotation(ann, ing)
+	if annValue == "" {
+		return "", err
+	}
+
+	secrNs, secrName, err := cache.SplitMetaNamespaceKey(annValue)
+	if secrName == "" {
+		return "", err
+	}
+
+	if secrNs == "" {
+		return fmt.Sprintf("%v/%v", ing.Namespace, secrName), nil
+	}
+	return annValue, nil
+}
+
 // syncSecrets synchronizes data from all Secrets referenced by the given
 // Ingress with the local store and file system.
 func (s k8sStore) syncSecrets(ing *extensions.Ingress) {

internal/ingress/controller/store/store_test.go

@@ -19,6 +19,7 @@ package store
 import (
 	"fmt"
 	"os"
+	"sync"
 	"sync/atomic"
 	"testing"
 	"time"
@@ -31,8 +32,10 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/client-go/kubernetes"
+	"k8s.io/client-go/tools/cache"
 
 	"k8s.io/ingress-nginx/internal/file"
+	"k8s.io/ingress-nginx/internal/ingress/annotations/parser"
 	"k8s.io/ingress-nginx/test/e2e/framework"
 )
 
@@ -653,3 +656,88 @@ func newFS(t *testing.T) file.Filesystem {
 	}
 	return fs
 }
+
+// newStore creates a new mock object store for tests which do not require the
+// use of Informers.
+func newStore(t *testing.T) *k8sStore {
+	fs, err := file.NewFakeFS()
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	return &k8sStore{
+		listers: &Lister{
+			// add more listers if needed
+			Ingress: IngressLister{cache.NewStore(cache.MetaNamespaceKeyFunc)},
+		},
+		sslStore:         NewSSLCertTracker(),
+		filesystem:       fs,
+		updateCh:         channels.NewRingChannel(10),
+		mu:               new(sync.Mutex),
+		secretIngressMap: NewObjectRefMap(),
+	}
+}
+
+func TestUpdateSecretIngressMap(t *testing.T) {
+	s := newStore(t)
+
+	ingTpl := &v1beta1.Ingress{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test",
+			Namespace: "testns",
+		},
+	}
+	s.listers.Ingress.Add(ingTpl)
+
+	t.Run("with TLS secret", func(t *testing.T) {
+		ing := ingTpl.DeepCopy()
+		ing.Spec = v1beta1.IngressSpec{
+			TLS: []v1beta1.IngressTLS{{SecretName: "tls"}},
+		}
+		s.listers.Ingress.Update(ing)
+		s.updateSecretIngressMap(ing)
+
+		if l := s.secretIngressMap.Len(); !(l == 1 && s.secretIngressMap.Has("testns/tls")) {
+			t.Errorf("Expected \"testns/tls\" to be the only referenced Secret (got %d)", l)
+		}
+	})
+
+	t.Run("with annotation in simple name format", func(t *testing.T) {
+		ing := ingTpl.DeepCopy()
+		ing.ObjectMeta.SetAnnotations(map[string]string{
+			parser.GetAnnotationWithPrefix("auth-secret"): "auth",
+		})
+		s.listers.Ingress.Update(ing)
+		s.updateSecretIngressMap(ing)
+
+		if l := s.secretIngressMap.Len(); !(l == 1 && s.secretIngressMap.Has("testns/auth")) {
+			t.Errorf("Expected \"testns/auth\" to be the only referenced Secret (got %d)", l)
+		}
+	})
+
+	t.Run("with annotation in namespace/name format", func(t *testing.T) {
+		ing := ingTpl.DeepCopy()
+		ing.ObjectMeta.SetAnnotations(map[string]string{
+			parser.GetAnnotationWithPrefix("auth-secret"): "otherns/auth",
+		})
+		s.listers.Ingress.Update(ing)
+		s.updateSecretIngressMap(ing)
+
+		if l := s.secretIngressMap.Len(); !(l == 1 && s.secretIngressMap.Has("otherns/auth")) {
+			t.Errorf("Expected \"otherns/auth\" to be the only referenced Secret (got %d)", l)
+		}
+	})
+
+	t.Run("with annotation in invalid format", func(t *testing.T) {
+		ing := ingTpl.DeepCopy()
+		ing.ObjectMeta.SetAnnotations(map[string]string{
+			parser.GetAnnotationWithPrefix("auth-secret"): "ns/name/garbage",
+		})
+		s.listers.Ingress.Update(ing)
+		s.updateSecretIngressMap(ing)
+
+		if l := s.secretIngressMap.Len(); l != 0 {
+			t.Errorf("Expected 0 referenced Secret (got %d)", l)
+		}
+	})
+}