From 0701cb8a1df628624f36e33bfd74adb5da15ade3 Mon Sep 17 00:00:00 2001 From: Jay Pipes Date: Mon, 14 Feb 2022 13:28:58 -0500 Subject: [PATCH] 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 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --- pkg/runtime/cache/account.go | 2 +- pkg/runtime/cache/cache.go | 39 +++++++++++++++++++++++----------- pkg/runtime/cache/namespace.go | 7 +++--- pkg/runtime/reconciler.go | 2 +- 4 files changed, 33 insertions(+), 17 deletions(-) diff --git a/pkg/runtime/cache/account.go b/pkg/runtime/cache/account.go index 22a9c8e..812df66 100644 --- a/pkg/runtime/cache/account.go +++ b/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{}, ) diff --git a/pkg/runtime/cache/cache.go b/pkg/runtime/cache/cache.go index 5b5b0d4..cbf0bb4 100644 --- a/pkg/runtime/cache/cache.go +++ b/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 diff --git a/pkg/runtime/cache/namespace.go b/pkg/runtime/cache/namespace.go index 44454d2..e147913 100644 --- a/pkg/runtime/cache/namespace.go +++ b/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") } diff --git a/pkg/runtime/reconciler.go b/pkg/runtime/reconciler.go index 20b241e..7b2df57 100644 --- a/pkg/runtime/reconciler.go +++ b/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 }