swap caching for non-caching reads of Secrets #72

jaypipes · 2022-02-13T16:35:27Z

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 jaypipes@gmail.com

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

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 <jaypipes@gmail.com>

vijtrip2

Looks good!

We should also update the community CARM docs to let customers know about these new environment variables, right?

a-hilaly · 2022-02-14T01:34:13Z

pkg/runtime/cache/namespace.go

 	object, ok := raw.(*corev1.Namespace)
 	return ok &&
-		(object.ObjectMeta.Name == "ack-system" ||
+		(object.ObjectMeta.Name == ackSystemNamespace ||


a-hilaly · 2022-02-14T01:38:39Z

pkg/runtime/cache/cache.go

+	// 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"


We should also change K8S_NAMESPACE to ACK_SYSTEM_NAMESPACE in helm/deployment.yaml and config/controller/deployment.yaml

Yeah, I've got a patch for that to code-generator. I'll submit it once we release the runtime (since I will pin code-generator to that runtime version)

jaypipes · 2022-02-14T14:21:46Z

Looks good!

We should also update the community CARM docs to let customers know about these new environment variables, right?

Yep. I did that here: aws-controllers-k8s/community#1178

vijtrip2 · 2022-02-14T18:28:16Z

/lgtm

ack-bot · 2022-02-14T18:28:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, jaypipes, vijtrip2

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [A-Hilaly,jaypipes,vijtrip2]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This is the patch that corresponds to changes made to ACK runtime to rename the `K8S_NAMESPACE` environment variable to `ACK_SYSTEM_NAMESPACE` to better represent what this variable referred to. Related: aws-controllers-k8s/runtime#72 Related: aws-controllers-k8s/community#1178 Issue: aws-controllers-k8s/community#1173 Signed-off-by: Jay Pipes <jaypipes@gmail.com>

This is the patch that corresponds to changes made to ACK runtime to rename the `K8S_NAMESPACE` environment variable to `ACK_SYSTEM_NAMESPACE` to better represent what this variable referred to. Related: aws-controllers-k8s/runtime#72 Related: aws-controllers-k8s/community#1178 Issue: aws-controllers-k8s/community#1173 Signed-off-by: Jay Pipes <jaypipes@gmail.com> By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-bot requested review from RedbackThomson and vijtrip2 February 13, 2022 16:35

ack-bot added the approved label Feb 13, 2022

vijtrip2 approved these changes Feb 13, 2022

View reviewed changes

a-hilaly approved these changes Feb 14, 2022

View reviewed changes

a-hilaly reviewed Feb 14, 2022

View reviewed changes

ack-bot assigned vijtrip2 Feb 14, 2022

ack-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2022

ack-bot merged commit 0701cb8 into aws-controllers-k8s:main Feb 14, 2022

jaypipes mentioned this pull request Feb 15, 2022

K8S_NAMESPACE -> ACK_SYSTEM_NAMESPACE aws-controllers-k8s/code-generator#290

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

swap caching for non-caching reads of Secrets #72

swap caching for non-caching reads of Secrets #72

Uh oh!

jaypipes commented Feb 13, 2022

Uh oh!

vijtrip2 left a comment

Uh oh!

a-hilaly Feb 14, 2022

Uh oh!

a-hilaly Feb 14, 2022 •

edited

Loading

Uh oh!

jaypipes Feb 14, 2022

Uh oh!

jaypipes commented Feb 14, 2022

Uh oh!

vijtrip2 commented Feb 14, 2022

Uh oh!

ack-bot commented Feb 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

swap caching for non-caching reads of Secrets #72

swap caching for non-caching reads of Secrets #72

Uh oh!

Conversation

jaypipes commented Feb 13, 2022

Uh oh!

vijtrip2 left a comment

Choose a reason for hiding this comment

Uh oh!

a-hilaly Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

a-hilaly Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaypipes Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

jaypipes commented Feb 14, 2022

Uh oh!

vijtrip2 commented Feb 14, 2022

Uh oh!

ack-bot commented Feb 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

a-hilaly Feb 14, 2022 •

edited

Loading