docs(proposal): decoupling app sync from control plane user w/ impersonation

[anandf]

Proposal for decoupling the application sync process from the Application controller service account.

Addresses issue #7689
Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• The title of the PR conforms to the Toolchain Guide
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• Optional. My organization is added to USERS.md.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.

Please see Contribution FAQs if you have questions about your pull-request.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (55713b3) 49.26% compared to head (d2c3cdf) 49.25%.
Report is 6 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14255      +/-   ##
==========================================
- Coverage   49.26%   49.25%   -0.02%     
==========================================
  Files         274      274              
  Lines       48158    48170      +12     
==========================================
- Hits        23727    23724       -3     
- Misses      22085    22099      +14     
- Partials     2346     2347       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[blakepettersson]

Could we not have the defaultServiceAccountName in the destinations list instead? Then we'd have the possibility of doing something like below:

destinations:
- namespace: guestbook
  server: https://kubernetes.default.svc
  defaultServiceAccountName: foo
- namespace: guestbook-2
  server: https://kubernetes.default.svc
  defaultServiceAccountName: bar
- namespace: other-namespaces*
  server: https://kubernetes.default.svc
  defaultServiceAccountName: bat

[blakepettersson]

With this there shouldn't be a need to allow users to set serviceAccountName directly in the Application CRD. This would mean that this can be more tightly controlled by the Argo CD admins.

[blakepettersson]

This could perhaps be extended by adding allowedServiceAccounts, so that a destinations block could look as below:

destinations:
- namespace: guestbook
  server: https://kubernetes.default.svc
  defaultServiceAccountName: foo
  allowedServiceAccounts: 
  - foo
  - bar
  - bat
- namespace: guestbook-2
  server: https://kubernetes.default.svc
  defaultServiceAccountName: bar
  allowedServiceAccounts: 
  - foo
  - bar
  - bat
- namespace: other-namespaces*
  server: https://kubernetes.default.svc
  defaultServiceAccountName: bat
  allowedServiceAccounts: 
  - foo
  - bar
  - bat

[anandf]

Thanks @blakepettersson for all your review comments and the doc fixes (which I missed) :-)
We too, had similar internal discussions of restricting the service account config only at the AppProject. Allowing it at the Application level was to give more flexibility to users to handle scenarios where there is a need to have 1 service account per Application. But, since we need to restrict the service account specified in the Application, it probably makes sense to have all the control in the AppProject itself. I like the idea of having it part of the destinations array as it allows us to specify 1 per destination target. Let me see what others think about this approach and then make the change in the proposal.

[blakepettersson]

With this there shouldn't be a need to allow users to set serviceAccountName directly in the Application CRD. This would mean that this can be more tightly controlled by the Argo CD admins.

[blakepettersson]

@anandf the proposal as it stands is 👨‍🍳 💋, LGTM! Could you remove the docs/decouple-application-sync-user-using-impersonation doc since it was a bit confusing to follow and the actual proposal is located in docs/proposals/decouple-application-sync-user-using-impersonation?

[crenshaw-dev]

Thanks for the proposal, @gpaul! Most of my questions are just asking for clarification.

[crenshaw-dev]

I think it would be helpful to add a "how-to" example. Basically a step-by-step guide for a user to take advantage of this hypothetical feature. e.g.

1. Create an AppProject for team A
2. Add a destination for team A's cluster, https://team-a.kubernetes.svc
3. Add a serviceAccountName team-a-service-account to the destination
4. Create a cluster config that looks like X and do Y to allow that cluster config to impersonate team-a-service-account on the destination cluster
5. etc...

I'm not super familiar with the mechanics of impersonation, so the more detailed drill-down will help me imagine exactly how this feature will work.

[blakepettersson]

Also, how would the service account itself be managed? I suspect there'll need to be an impersonation opt-out (something like an annotation on the relevant manifest itself perhaps?), otherwise the service account would not be able to be managed in a Gitops-manner.

[anandf]

Have added details on how to install argocd, setting up the feature flag for enabling this feature, create separate namespace, service account and rbac policies for trying out this feature. Let me know

[anandf]

@blakepettersson The namespace, serviceaccount, and the related RBAC policies would be created separately as part of the tenant provisioning process is what I am thinking.

[blakepettersson]

One question that just came to me is what happens if there are multiple potential matches, i.e there are several potential serviceaccounts that gets returned? Does the first match win, or should this be an error?

[anandf]

The one with the closest match should be used. If there is an exact match of namespace and server, then that must be given the highest priority. The section matching with wildcard charcter * must have the lowest priority.

[blakepettersson]

How should negated destinations be ranked? For example if we have a destination looking like

  destinations:
  - namespace: !something*
    server: !https://kubernetes.default.svc
    serviceAccountName: generic-deployer
  - namespace: not-something
    server: https://server-1
    serviceAccountName: some-other-deployer

How should that be ranked?

[anandf]

I am not sure how we should handle negative matches. Can that be prioritized the least ? or is it better to keep it simple and throw an error if there are multiple matches ?

[blakepettersson]

IMO I think it's better to keep it simple and throw an error on multiple matches, WDYT @crenshaw-dev?

[anandf]

Keeping the logic to throw error if there are multiple matches either through regex or an exact match of the destination.

[crenshaw-dev]

I think it might actually be kinda weird to have the destinations field serve two purposes: allowing/denying clusters/namespaces and also associating service account names with clusters.

What if we just add a new field called clusterServiceAccounts?

clusterServiceAccounts:
- server: https://kubernetes.default.svc # ! negation is not allowed here
  serviceAccountName: in-cluster
- server: * # Fallback, first match wins
  serviceAccountName: argocd-default-sa

I don't wanna overload the destinations field with too much functionality. Causes confusion.

[anandf]

Addressed the review comments.

[anandf]

Have added details on how to install argocd, setting up the feature flag for enabling this feature, create separate namespace, service account and rbac policies for trying out this feature. Let me know

[anandf]

@blakepettersson The namespace, serviceaccount, and the related RBAC policies would be created separately as part of the tenant provisioning process is what I am thinking.

[ljupchokotev]

A question on the proposal, how would this help with fixing issues like #15027, #10897, #9606 and #9515. I see this proposal being mentioned as the fix, but I can't figure out how as all examples point to ArgoCD managing the same cluster where it is also deployed and not remote clusters. I think #12755 goes into a better direction to fix the mentioned issues, as long as Argo can differentiate clusters with the same cluster url.

Isn't it also assuming that ArgoCD will have the permission to impersonate all the needed service accounts which might be difficult in multi-tenant clusters where the cluster is managed by a team different than the team managing ArgoCD. The examples in the docs do not really setup the permissions, so am I missing something here? Don't you have to grant the impersonate verb to ArgoCD so it can impersonate other SAs?

[anandf]

A question on the proposal, how would this help with fixing issues like #15027, #10897, #9606 and #9515. I see this proposal being mentioned as the fix, but I can't figure out how as all examples point to ArgoCD managing the same cluster where it is also deployed and not remote clusters. I think #12755 goes into a better direction to fix the mentioned issues, as long as Argo can differentiate clusters with the same cluster url.

I see the issues #15027, #10897, #9606 and #9515 are all about managing multiple cluster URLs with different namespace. Probably one namespace per tenant and each tenant having its own set of cluster secrets.

Though this proposal might not directly address, it might help to remove the need for adding multiple cluster secrets per namespace. You can login to the remote cluster as cluster-admin user, for all the tenants, and for sync operations each tenant (cluster + namespace combination) would use a separate service account. That way you can manage with a single cluster secret and instead control the access to each application or tenant using appropriate RBAC policies. I can add an example of how to manage a remote cluster.

[ljupchokotev]

You can login to the remote cluster as cluster-admin user

That is exactly the problem in our case and I assume for a lot of other people judging by the feedback on the linked issues. I do not own the multi-tenant cluster, so I am not able to get a cluster-admin service account to setup in Argo. Tenants get a service account that has privileges in their own namespace only. Due to this, I don't see this proposal as a solution for that particular issue at least. The proposal in #12755 (or similar) should still be implemented as that allows for least-privilege principle to be implemented in an easy way. Impersonation can still be used as needed, whether with a cluster-admin SA or a namespace SA.

[akram]

As this may be quite complex to comprehend and to implement, and it may degrade user experience, should we really allow regex for destination?

Instead, can it be thought to have an annotation on the destination?

[ishitasequeira]

If we are going to throw an error upon multiple matches, would it make more sense to make it exact match for destination instead of regex?

[anandf]

The idea to support regex was to allow some kind of convention, where a standard service account name is used for all the deployments. In that case, users can configure same service account for all destinations and then ensure that the service account is created for each namespace. For instance use the service account name argocd-deployer for all namespaces for all clusters. Also regex is already supported for namespace field.

[ishitasequeira]

That makes sense. Can we consider the first matching destination only and ignore the rest instead of throwing an error?

[anandf]

If there is a glob pattern match and an exact match, we can give priority to the exact match and not throw any error. But if there are multiple glob pattern matches or multiple exact matches, its better to throw an error and make sure that the admin user corrects the configuration. wdyt ?

[akram]

can we detail here how to get the legacy behaviour or an equivalent of it?
And what is or will be the default behaviour? The one with impersonation or without?

[anandf]

Impersonation would be disabled by default. Users have to opt-in by explicitly enabling this feature. I will add the default behaviour in this statement for better clarity.

[akram]

ok, that was something like this that I was mentioning previously. Is it required for the previous examples as well? isn'it?

[anandf]

When you add a cluster using argocd cluster add without providing any specific service account, ArgoCD creates a service account argocd-manager, ClusterRole called argocd-manager-role with cluster-admin level of access and binds it using a ClusterRoleBinding named argocd-manager-role-binding. That is why it was not required in the previous example.

[ishitasequeira]

If we are going to throw an error upon multiple matches, would it make more sense to make it exact match for destination instead of regex?

[ishitasequeira]

This seems to be same as example 3. Can you be more explicit about the difference in both the examples?

[anandf]

I have modified the title. Copy-Paste error. Example 4 is where the default service account argocd-manager is not created on the remote cluster and instead, only a restrictive service account is provided.

[reegnz]

Additionally supporting full principal name would be preferable, eg. system:serviceaccount:guestbook-stage:guestbook-stage-deployer, eg whatever you would pass on to in the --as flag.

This allows easier understanding of what flag it gets translated to.
Also supports use-cases when you want to put the deployer service accounts in a separate namespace, not the same namespace that you're targeting.
Eg. SA is in 'deployer' namespace, but targeting 'guestbook-stage'.

[anandf]

We want to restrict users from being able to use service account from a different namespace. This is also the convention followed even for pods where you can just specify the serviceAccountName and it will be looked up in the pod's namespace.

[reegnz]

If you go for the pod serviceAccountName analogy then approject should only use SA from the same namespace and use the serviceAccountName field name.

The proposal however seems to follow semantics that are much closer to role binding semantics, which allows for what I described.

Also a single service account could be used to deploy across namespaces, and the cross namespace permissions are more natural to represent in RBAC.

But the proposal currently forces the use of switching between multiple service accounts if an app targets multiple namespaces, and thereby the cross namespace role binding aspect of RBAC is not utilized, instead it bleeds into the spec and the argocd implementation, and argocd has to implement conscious selection of SA per namespace during the sync of a single app.

If you leave that to RBAC (eg single SA, role bindings to that SA per Target namespace) then the implementation becomes trivial as it only requires a single impersonate API call with no additional logic to determine when to switch SA-s within an application sync.

Just using a single impersonation seems less hassle to implement as it delegates these concerns completely to RBAC for the entire sync operation.

[reegnz]

Some other problems I'm finding:

• how does the audit trail in the k8s audit logs look like if a single application deployment touching multiple namespaces is switching between multiple SA-s? IMHO that would be somewhat chaotic.
• how do you control deploying cluster-scoped resources?

From an audit-trail story IMHO it would be a cleaner story if the approject has a clean audit-identity by a 1:1 mapping of AppProject to ServiceAccount (or even better would be App to ServiceAccount).

[reegnz]

Created an example so its clear what particular deployment model I'm talking about:
https://gist.github.com/reegnz/e83a5958be0d92698061c318f893120e

You have some helm chart, that deploys into one or more namespaces, plus it also deploys cluster-scoped resources.
From an audit perspective you likely don't want the deployment of this particular application to be split between multiple principals. Also you want it to be able to deploy clusterscoped resources and control which clusterscoped resources the app is allowed to deploy.

This kind of deployment works today with flux helm controller where you can define a single SA for an individual helm chart to control what service account it can use to deploy, and the semantics of serviceAccountName pick up the SA from the same namespace as the CRD is defined in.

[reegnz]

I guess with this proposal the granularity I'm looking for is likely achieved with a 1:1 mapping of AppProject to App for most use-cases. Except that I'm tied to placing the SA into the target namespace if I'm just targeting one namespace.

However it would still be preferable to not assume the deployer SA is in the same namespace as the application, just as we don't assume the AppProject has to reside in the same namespace.
Ideally deployer SA should be allowed to live next to the AppProject IMHO. Otherwise I feel like RedHat Openshift-like opinionated SA structures start leaking into ArgoCD (which I assume where the proposal assumptions on SA placement is coming from).

One use-case of mine is where I create namespaces from an argocd app, and the same app is deploying quotas limitranges and serviceaccounts into all of the newly created namespaces.
To achieve that use-case I'd need something that doesn't assume which namespace the SA lives in.

tl;dr - the audit-trail for multi-namespace deployments is still highly problematic IMHO, also tying to the target namespace doesn't make sense if there is no target namespace (eg. when bootstrapping namespaces). IMHO if you'd allow for a 'default' serviceAccountName for resources that don't match any of the other configured namespace rules I'd be good.

eg.

defaultDestination:
  namespace: argocd
  server: https://kubernetes.default.svc
  serviceAccountName: my-deployer

[blakepettersson]

how does the audit trail in the k8s audit logs look like if a single application deployment touching multiple namespaces is switching between multiple SA-s? IMHO that would be somewhat chaotic.

Do you have an example of an Application deployment touching multiple namespaces?

also tying to the target namespace doesn't make sense if there is no target namespace (eg. when bootstrapping namespaces)

I'm inclined to agree on this one, I'm having trouble understanding how this would work in a Gitops context to create namespaces which do not already exist.

I'd be for amending the destinations array to take a full principal name instead of a service account name.

[reegnz]

One example of mine is an application that sets up namespaces + their quotas, limitranges, etc. Essentially bootstrapping the namespaces themselves. It wouldn't make sense splitting them into multiple applications TBQH. We also don't have service accounts available inside the namespaces in the beginning, so it's a chicken and egg problem.
I wouldn't want to provision them outside argocd, but manage all with argocd after an initial manual bootstrap of the argocd namespace and argocd deployments.
I also still want to ensure I follow principle of least privilege and when deploying namespaces I still want to enforce that the app only touches namespaces, quotas, limitranges, and likely roles+rolebindings. (maybe clusterrole bindings in case I want to set up an SA to provision cluster-scoped resources in another app).

[reegnz]

Another use-case is, when you provision the really critical parts of your cluster, eg. setting up cluster roles to be used by deployer service accounts. You likely want to treat that as a separate app, because creating clusterroles is a VERY sensitive issue. What namespace do I target with that, it's not a namespaced resource.

I might have separation of duties in my application teams, where a dedicated security team manages the service accounts, roles and rolebindings, and application teams have no business in provisioning them. I might have m:n mapping of apps to namespaces with m<n that the security team manages.

I think not allowing flexibility on using one principal touching multiple namespaces in a one-to-many manner is a problem in itself (eg. from an audit trail perspective it is just unnecessarily complicated to hunt down who deployed what)

The CD tool shouldn't be opinionated about anybody's security model, instead it should provide the most flexibility on how one ties it together with RBAC. Leave the job of being opinionated to the user.

Allowing the basic as semantics we have in kubeconfigs and kubectl for impersonation makes the most sense as it is the least opinionated approach on one's security model.

I can easily say several more examples if someone wants them. In enterprise contexts there's a ton of non-trivial cases and not everyone uses OpenShift-like RBAC with per-namespace deployer SA-s, which this proposal smells like it's opinionated towards (probably unintentionally).

[reegnz]

To leave it flexible, next to serviceAccountName I'd also expose the feature with as and asGroups attributes, to match kubectl flags and kubecontext configuration.

It would make sense to try and match the 1st party kubernetes semantics around impersonation behaviour, it'd be much easier for the user to connect the behaviour to how kubectl behaves.

See https://kubernetes.io/docs/reference/config-api/kubeconfig.v1/#AuthInfo and https://kubernetes.io/docs/reference/kubectl/kubectl/#options