Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Add support for selecting policies based on namespace labels #157

Closed
wants to merge 2 commits into from

Conversation

stefanprodan
Copy link
Member

@stefanprodan stefanprodan commented Apr 23, 2021

Problem

In the current implementation, users are forced into placing all the image automation objects in the same namespace because the ImageUpdateAutomation selects only the ImagePolicies in the same namespace as itself. This means users have to copy the image pull secrets from their original namespaces into flux-system or copy the Git auth secret in all apps namespaces.

@vladlosev explains why this limitation is a blocker in #151

In our company we have multiple semi-independent teams working on different
projects in our cluster and a single infrastructure team managing the
infrastructure for them, including the cluster itself. For the purposes of
data protection we have to keep the teams isolated, so each team has a
namespace it controls, and the infrastructure team runs the system namespaces,
including flux-system, where the repository and image automation CRDs are
defined. The individual teams control the exact set of images they generate
and push and the image selection criteria for deployment so we want to allow
them to manage the ImageRepository and ImageUpdatePolicy in their
namespaces. But with the GitRepository and ImageUpdateAutomation residing
in flux-system, they can't, unless ImageUpdateAutomation can handle policies
from other namespaces.

Many other users face similar problems as seen in #85.

Proposed solution

To allow an ImageUpdateAutomation to apply ImagePolicies across the cluster, a label selector can be used to specify which namespaces the controller should look for policies.

type ImageUpdateAutomationSpec struct {
	// PolicyNamespaceSelector is used for selecting policies in
	// all namespaces that match the list of labels and their values.
	// When not specified, it defaults to selecting the policies
	// present in the same namespace as the ImageUpdateAutomation.
	// +optional
	PolicyNamespaceSelector *PolicyNamespaceSelector `json:"policyNamespaceSelector,omitempty"`
}

type PolicyNamespaceSelector struct {
	MatchLabels map[string]string `json:"matchLabels,omitempty"`
}

The PolicyNamespaceSelector is optional, when not specified the controller behaves like before, when only the ImageUpdateAutomation namespace is taking into considerations when selecting the image policies.

In regards to multi-tenancy, this feature doesn't present any risks as no sensitive information is fetched from other namespaces.

Example

Assuming you have two app namespaces each with an image policy:

apiVersion: v1
kind: Namespace
metadata:
  labels:
    flux-auto: enabled
  name: app1
---
apiVersion: image.toolkit.fluxcd.io/v1alpha2
kind: ImagePolicy
metadata:
  name: app1
  namespace: app1
spec:
  imageRepositoryRef:
    name: app1
  policy:
    semver:
      range: 1.0.x
---
apiVersion: v1
kind: Namespace
metadata:
  labels:
    flux-auto: enabled
  name: app2
---
apiVersion: image.toolkit.fluxcd.io/v1alpha2
kind: ImagePolicy
metadata:
  name: app2
  namespace: app2
spec:
  imageRepositoryRef:
    name: app2
  policy:
    semver:
      range: 1.0.x

Instead of creating a pair of ImageUpdateAutomation/GitRepository in each namespace and copy the Git auth secret, you would use the namespace label flux-auto: enabled to select the policies in the ImageUpdateAutomation (that's in flux-system namespace):

apiVersion: image.toolkit.fluxcd.io/v1alpha2
kind: ImageUpdateAutomation
metadata:
  name: flux-system
  namespace: flux-system
spec:
  interval: 1m0s
  sourceRef:
    kind: GitRepository
    name: flux-system
  git:
    commit:
      author:
        email: [email protected]
        name: fluxcdbot
  update:
    path: ./clusters/my-cluster
    strategy: Setters
  policyNamespaceSelector:
    matchLabels:
      flux-auto: enabled

Feedback & Testing

Please comment on this PR and let us know your thoughts about this feature.

To install the controller with the policyNamespaceSelector capability:

  1. install the controllers
flux install --components-extra=image-reflector-controller,image-automation-controller
  1. apply the CRD from this branch
kubectl apply -f https://raw.githubusercontent.com/fluxcd/image-automation-controller/policy-namespace-selector/config/crd/bases/image.toolkit.fluxcd.io_imageupdateautomations.yaml
  1. allow the controller to list namespaces
cat << EOF | kubectl apply -f -
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: crd-controller-list-ns
rules:
- apiGroups:
  - ""
  resources:
  - namespaces
  verbs:
  - get
  - list
  - watch
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: crd-controller-list-ns
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: crd-controller-list-ns
subjects:
  - kind: ServiceAccount
    name: image-automation-controller
    namespace: flux-system
EOF
  1. deploy image-automation-controller AMD64 build of this branch
kubectl -n flux-system set image deployment/image-automation-controller \
manager=docker.io/stefanprodan/image-automation-controller:policy-selector.1

@stefanprodan stefanprodan added the enhancement New feature or request label Apr 23, 2021
@stefanprodan stefanprodan requested review from a team, squaremo and hiddeco and removed request for a team April 23, 2021 11:16
PolicyNamespaceSelector is used for selecting policies in all namespaces that match the list of labels and their values. When not specified, it defaults to selecting the policies present in the same namespace as the ImageUpdateAutomation.

Signed-off-by: Stefan Prodan <[email protected]>
@rverma-dev
Copy link

It would be nice if we have a construct for scan-all or something equivalent which can enable the updation Policy to all namespaces.

Copy link

@vladlosev vladlosev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this change! This will work fine for our situation, although there is a drawback. In order to suspend or enable image update automation for a given namespace, one has to add or remove a label to the namespace itself. We restrict our teams' access to modify the definitions of namespaces they operate in, in order to ensure the security related annotations (for kiam or kubernetes-external-secrets, for example) stay intact. So the teams will not be able to suspend/re-enable image automation updates on their own. It can be an obstacle, but not an insurmountable one - one can write a simple controller to watch for some condition within the namespaces and update the namespace labels accordingly.

controllers/imageupdateautomation_controller.go Outdated Show resolved Hide resolved
@stefanprodan
Copy link
Member Author

stefanprodan commented Apr 23, 2021

So the teams will not be able to suspend/re-enable image automation updates on their own.

They can, either by suspending the ImageRepositories or by setting a fix tag in the ImagePolicies (rollback procedure).

@stealthybox
Copy link
Member

stealthybox commented Apr 27, 2021

At my first glance, this proposal seems reasonable and enables behavior that platform admins would otherwise need a custom controller for.

I think it composes well with the myriad of mechanisms we've gone over for Source Policy. (fluxcd/flux2#582 (comment))
Whatever we do there will likely apply to the client that fetches these cross-Namespace ImagePolicies.

@hiddeco
Copy link
Member

hiddeco commented Apr 27, 2021

Any reason flux-auto: enabled does not follow the fluxcd.io domain standard we use for all other labels and annotations?

@stefanprodan
Copy link
Member Author

Any reason flux-auto: enabled does not follow the fluxcd.io domain standard

Can be anything, it's up to the user to decide what labels to use, flux-auto is just an example.

@stefanprodan
Copy link
Member Author

stefanprodan commented Apr 29, 2021

will likely apply to the client that fetches these cross-Namespace ImageRepositories

@stealthybox that's not the case, the ImageRepositories and their secrets are not fetched cross-namespaces, this RCF is about fetching ImagePolicies. There is no leaking of secrets nor any disclosure of sensitive informations besides the image tag. If the owner of the ImageUpdateAutomation has no write access to the upstream repo, the image tags are not displayed, so I think this RCF is 💯 safe without any impersonation needed.

@stealthybox
Copy link
Member

stealthybox commented Apr 29, 2021

Sorry @stefanprodan
I misspoke -- I've changed the comment to say cross NS ImagePolicies instead of repos.

You're likely right that the behavior is inconsequential, but I'd consider using an impersonated user on that client for auditing and to prevent any information disclosure

@davisford
Copy link

Hi, we tested this out in one of our dev clusters, and it appears to be working. Would love to see this get merged. Is there a chance it will be accepted as a solution?

@monotek
Copy link

monotek commented May 4, 2021

I have tested the pr now and it works. Thanks :)

Maybe the other way around, to enable all namespaces by default, and disabling namespaces explicitly where auto update is not wanted, would be a nice option too.

@squaremo
Copy link
Member

squaremo commented May 5, 2021

In regards to multi-tenancy, this feature doesn't present any risks as no sensitive information is fetched from other namespaces.

No secrets are fetched from other namespaces, but who is to say that image names are not sensitive? If I've understood the implementation here: given access to create an automation in any namespace, I can exfiltrate image names for image repositories that I don't have access to, from any other namespace.

To allow an ImageUpdateAutomation to apply ImagePolicies across the cluster, a label selector can be used to specify which namespaces the controller should look for policies.

This is different from the grant of access as proposed in .. I can't find it .. source access lists. In that model, the bearer of access (GitRepository) grants access to resources in other namespaces. In this design, access is claimed by resources in other namespaces. For that reason, I don't think this is consistent with Flux2's multi-tenant model, after all.

It would be nice if we have a construct for scan-all or something equivalent

@rverma-nsl The implementation here has a special meaning for a missing .spec.policyNamespaceSelector, but leaves interpretation of the matchLabels value to controller-runtime. This interpretation is encoded in the Kubernetes API machinery lib, which I assume controller-runtime relies on -- a missing selector means "match nothing", and an empty selector means "match everything". So you can select all namespaces like this:

apiVersion: image.toolkit.fluxcd.io/v1alpha2
kind: ImageUpdateAutomation
metadata:
  name: flux-system
  namespace: flux-system
spec:
  interval: 1m0s
  sourceRef:
    kind: GitRepository
    name: flux-system
  git:
    commit:
      author:
        email: [email protected]
        name: fluxcdbot
  update:
    path: ./clusters/my-cluster
    strategy: Setters
  policyNamespaceSelector:
    matchLabels: {}

@stefanprodan
Copy link
Member Author

I can exfiltrate image names for image repositories that I don't have access to, from any other namespace.

@squaremo how would you do this if the image URL is not in your repo? The only way to view the image names is in the commit message, but if an image is not in Git then it will not get printed I guess.

@squaremo
Copy link
Member

squaremo commented May 5, 2021

@squaremo how would you do this if the image URL is not in your repo? The only way to view the image names is in the commit message, but if an image is not in Git then it will not get printed I guess.

Set up a git repo with any resource YAML in it, give a field an update marker naming the policy you want to read, then make an ImageUpdateAutomation object pointing at the git repo and at the namespace. (It's always possible -- and especially at present -- that I am missing something...)

@stefanprodan
Copy link
Member Author

give a field an update marker naming the policy you want to read

Who would you know the policy name if you don't have access to the namespaces where those are?

@squaremo
Copy link
Member

squaremo commented May 5, 2021

[How] would you know the policy name if you don't have access to the namespaces where those are?

You might have to guess it. The point is, it turns security by access control into security by obscurity. Would it be acceptable, say, for secrets in any namespace to be accessible to anyone who can guess the right name? (rhetorical question)

@stefanprodan
Copy link
Member Author

The point is, it turns security by access control into security by obscurity.

Ok closing this, back to the drawing board.

@squaremo
Copy link
Member

squaremo commented May 5, 2021

@davisford @monotek Would it work equally well for you if you could refer to a GitRepository in another namespace?

@davisford
Copy link

davisford commented May 5, 2021

@squaremo I'm puzzled by why we would require a GitRepository resource in different namespaces. For me, I only use one repo, but I have like 15 namespaces, so I have to copy all the things into every namespace and it just seems un-necessary.

To me, a GitRepository would be better served by remaining as a singular resource in the flux-system namespace -- the same namespace the flux controllers are in, but let the ImagePolicy in other namespaces refer to it. We tried to set this up by creating a ClusterRole / RoleBinding, but did not get it working.

Just as a side-note, because the controllers don't yet implement native auth against cloud image repositories (we use ECR), I also have to run the CronJob to refresh the secret in every single namespace too.

@monotek
Copy link

monotek commented May 5, 2021

@squaremo
this would mean i would need to have a ImageUpdateAutomation, referencing the GitRepository, in every namespace i have ImagePolicy and ImageRepository in?

I think it would work too, but feels a bit odd, defining several ImageUpdateAutomation which are referencing the same GitRepository over and over again.

@squaremo
Copy link
Member

squaremo commented May 5, 2021

I'm puzzled by why we would require a GitRepository resource in different namespaces

@davisford This PR would have let you create an ImageUpdateAutomation object that lives in the same namespace as its GitRepository, but examines policies in arbitrary other namespaces. I am asking whether the following alternative would work in your case: you can create an ImageUpdateAutomation that lives in the same namespace as the policies, and refers to a single GitRepository.

I think it would work too, but feels a bit odd, defining several ImageUpdateAutomation which are referencing the same GitRepository over and over again.

@monotek Perhaps it feels odd given your use case, but for better or worse this situation is the norm in Kubernetes. For example, if you use the same image repository in different namespaces, you have to provide an imagePullSecret in each of those namespaces.

Specifically with ImageUpdateAutomation objects, it seems more likely to me that each namespace will have its own parameters for automation (e.g., path, push branch, commit author), even if they all target the same git repository. The trade-off is that you can't have a "bulk automation", which applies the same parameters to a whole set of namespaces. This is why I asked the question, to see where you stood on that trade-off :-)

@monotek
Copy link

monotek commented May 6, 2021

Having different committers per team/namespace might indeed be helpful :)

@gws
Copy link

gws commented May 7, 2021

@squaremo

Would it work equally well for you if you could refer to a GitRepository in another namespace?

This would work well for my use case. A Kustomization spec.sourceRef can refer to a GitRepository from another namespace, and I expected ImageUpdateAutomation to work the same way.

@davisford
Copy link

@squaremo

@davisford This PR would have let you create an ImageUpdateAutomation object that lives in the same namespace as its GitRepository, but examines policies in arbitrary other namespaces. I am asking whether the following alternative would work in your case: you can create an ImageUpdateAutomation that lives in the same namespace as the policies, and refers to a single GitRepository.

It would work -- having a separate ImageUpdateAutomation object per namespace seems fine. That would allow proper control per namespace. My only real gripe at this point is having to run the CronJob to refresh the ECR secret per namespace as well. I think it is on the roadmap for the flux team to implement native auth with the various cloud repos so I think eventually that would go away, but it is not ideal to run so many cron jobs which are all doing the same thing.

@stefanprodan
Copy link
Member Author

it is not ideal to run so many cron jobs which are all doing the same thing.

The current solution is to run the CronJob in the flux-system namespace or some other admin only NS, then use Kyverno or other solutions to replicate the secret in all the namespaces that you want. See https://kyverno.io/docs/writing-policies/generate/#generating-resources-into-existing-namespaces

@stealthybox
Copy link
Member

stealthybox commented May 25, 2021

it is not ideal to run so many cron jobs which are all doing the same thing.

@davisford
The Deployment/CronJob + ServiceAccount can be updated to publish the one ImagePullSecret to multiple Namespaces.

@stefansedich
Copy link

@stefanprodan are we likely to see any workable solution to this in the near future with this solution being dropped, or will copying the ImageUpdateAutomation to the other namespaces be the workaround we should go with as we migrate to Flux2?

@stefanprodan stefanprodan deleted the policy-namespace-selector branch June 22, 2021 10:59
@stefanprodan
Copy link
Member Author

New RFC here fluxcd/image-reflector-controller#162

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants