swap caching for non-caching reads of Secrets (#72)

In order to allow the ClusterRole associated with the ServiceAccount of
a controller started in Cluster Mode to require Read/List/Watch
permissions on Secrets in ALL Kubernetes Namespaces, we need to change
the way that Secret values are read in the resource reconciler.

When a "normal" client-go Kubernetes client is created and a single
watch namespace is *not* provided, then whenever that client is used to
read object information, a new SharedInformer cache is created for that
Kind of resource across *all* Namespaces.

This SharedInformer cache sets up listers and watchers on all resources
of that Kind of resource across all Namespaces and thus the ClusterRole
associated with the ServiceAccount of the controller needs permissions
to get, list and watch those resources across all Namespaces.

For Secret resources, this permission to list/watch Secrets across all
Namespaces is too broad for many security-conscious organizations. For
these organizations, they wish to restrict the ACK controller to only
watch Secret resources that are in specific Namespaces that the ACK
custom resources are created in.

In order to fulfill this security requirement, we needed to make a
relatively small change to the resource reconciler's
SecretValueFromReference method to use the non-caching apiReader part of
the Kubernetes client.

The other changes included in this patch revolve around documentation
and renaming the environment variable used to determine the Namespace
that the CARM ConfigMap resides in.

Previously, the environment variable controlling the Namespace the
`ack-role-account-map` ConfigMap was in was called `K8S_NAMESPACE`. I
thought this name was overly broad and confusing, considering we have a
`--watch-namespace` CLI flag for determining if the ACK controller is
started in "Cluster Mode" or "Namespace Mode", and the generic
`K8S_NAMESPACE` environment variable name was being confused with this
entirely unrelated CLI flag. I have thus renamed `K8S_NAMESPACE` to
`ACK_SYSTEM_NAMESPACE` to more accurately reflect what the variable
configures. I have left support for falling back to the old, now
deprecated `K8S_NAMESPACE` name, however.

Finally, I removed the hard-coded string `ack-system` comparison in the
CARM Account cache and now have it checking the ConfigMap's Namespace
against the value of the `ACK_SYSTEM_NAMESPACE` environment variable.

Note that I have run RDS controller local kind tests against this
version of the ACK runtime to ensure there are no nasty surprises and
all e2e tests are passing properly.

Issue: aws-controllers-k8s/community#1173

Signed-off-by: Jay Pipes <[email protected]>

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

Author: jaypipes

pkg/runtime/cache/account.go

@@ -57,7 +57,7 @@ func resourceMatchACKRoleAccountsConfigMap(raw interface{}) bool {
 func (c *AccountCache) Run(clientSet kubernetes.Interface, stopCh <-chan struct{}) {
 	informer := informersv1.NewConfigMapInformer(
 		clientSet,
-		currentNamespace,
+		ackSystemNamespace,
 		informerResyncPeriod,
 		k8scache.Indexers{},
 	)

pkg/runtime/cache/cache.go

@@ -14,33 +14,48 @@
 package cache
 
 import (
-	"os"
 	"time"
 
 	"github.com/go-logr/logr"
+	"github.com/jaypipes/envutil"
 	kubernetes "k8s.io/client-go/kubernetes"
 )
 
 const (
-	// defaultNamespace is the default namespace to use if the environment
-	// variable NAMESPACE is not found. The NAMESPACE variable is injected
-	// using the kubernetes downward api.
-	defaultNamespace = "ack-system"
+	// envVarACKSystemNamespace is the string key for the environment variable
+	// storing the Kubernetes Namespace we use for ConfigMaps and other ACK
+	// system configuration needs.
+	envVarACKSystemNamespace = "ACK_SYSTEM_NAMESPACE"
+
+	// envVarDeprecatedK8sNamespace is the string key for the old, deprecated
+	// environment variable storing the Kubernetes Namespace we use for
+	// ConfigMaps and other ACK system configuration needs.
+	envVarDeprecatedK8sNamespace = "K8S_NAMESPACE"
+
+	// defaultACKSystemNamespace is the namespace we look up the CARM account
+	// map ConfigMap in if the environment variable ACK_SYSTEM_NAMESPACE is not
+	// found.
+	defaultACKSystemNamespace = "ack-system"
 
 	// informerDefaultResyncPeriod is the period at which ShouldResync
 	// is considered.
+	// NOTE(jaypipes): setting this to zero means we are telling the client-go
+	// caching system not to set up resyncs with an authoritative state source
+	// (i.e. a Kubernetes API server) on a periodic basis.
 	informerResyncPeriod = 0 * time.Second
 )
 
-// currentNamespace is the namespace in which the current service
-// controller Pod is running
-var currentNamespace string
+// ackSystemNamespace is the namespace in which we look up ACK system
+// configuration (ConfigMaps, etc)
+var ackSystemNamespace string
 
 func init() {
-	currentNamespace = os.Getenv("K8S_NAMESPACE")
-	if currentNamespace == "" {
-		currentNamespace = defaultNamespace
-	}
+	ackSystemNamespace = envutil.WithDefault(
+		envVarACKSystemNamespace, envutil.WithDefault(
+			envVarDeprecatedK8sNamespace,
+			defaultACKSystemNamespace,
+		),
+	)
 }
 
 // Caches is used to interact with the different caches

pkg/runtime/cache/namespace.go

@@ -76,12 +76,13 @@ func NewNamespaceCache(log logr.Logger) *NamespaceCache {
 	}
 }
 
-// isIgnoredNamespace returns true if an object is of type corev1.Namespace
-// and its metadata name is one of 'ack-system', 'kube-system' or 'kube-public'
+// isIgnoredNamespace returns true if an object is of type corev1.Namespace and
+// its metadata name is the ACK system namespace, 'kube-system' or
+// 'kube-public'
 func isIgnoredNamespace(raw interface{}) bool {
 	object, ok := raw.(*corev1.Namespace)
 	return ok &&
-		(object.ObjectMeta.Name == "ack-system" ||
+		(object.ObjectMeta.Name == ackSystemNamespace ||
 			object.ObjectMeta.Name == "kube-system" ||
 			object.ObjectMeta.Name == "kube-public")
 }

pkg/runtime/reconciler.go

@@ -111,7 +111,7 @@ func (r *reconciler) SecretValueFromReference(
 		Name:      ref.Name,
 	}
 	var secret corev1.Secret
-	if err := r.kc.Get(ctx, nsn, &secret); err != nil {
+	if err := r.apiReader.Get(ctx, nsn, &secret); err != nil {
 		return "", ackerr.SecretNotFound
 	}