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

docs: add proposal 004-cluster-name-as-id #12755

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

denysvitali
Copy link

This is a proposal to start using Cluster Names as the identifier in ArgoCD, switching away from the current indexing by Cluster API URL (which might be duplicated, see the examples).

This is related to PR #10897.

@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (0f500a5) 47.79% compared to head (fd62108) 47.79%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #12755   +/-   ##
=======================================
  Coverage   47.79%   47.79%           
=======================================
  Files         246      246           
  Lines       41968    41968           
=======================================
  Hits        20058    20058           
  Misses      19910    19910           
  Partials     2000     2000           
Impacted Files Coverage Δ
util/settings/settings.go 49.36% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@denysvitali
Copy link
Author

Ping @jannfis / @crenshaw-dev, this is related to what you asked for #10897


## Proposal

- Indexing is performed on `name@namespace` (for example `cluster-a@argo-cd`)
Copy link
Member

@blakepettersson blakepettersson May 28, 2023

Choose a reason for hiding this comment

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

Could the default not be to still be using the cluster url? If we were to add a namespace field to the cluster secret, on subsequent lookups with db.GetCluster we could add a namespace parameter.

m.db.GetCluster(context.Background(), app.Spec.Destination.Server, app.Spec.Destination.Namespace)

In GetCluster we could retrieve all the clusters by server and determine if any of them has a matching namespace field in the secret. If any of the cluster secrets has a matching namespace attribute that cluster would be returned.

If none of the cluster secrets in the list has a namespace attribute, the same behaviour as today would happen (i.e to return the first entry from the cluster secret list).

If any of the cluster secrets has a namespace attribute which does not match the namespace passed in GetCluster, those secrets gets excluded from the cluster secret list. Then the first entry in the list gets returned (i.e same behaviour as today, but clusters with namespaces set are excluded from selection).

The challenge that remains is how to handle caching...

Copy link
Author

Choose a reason for hiding this comment

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

This will still come with limitations imposed by the fact that we're indexing by API URL.

Ideally, you should be able to connect N times to the same "cluster" (Kubernetes API) with a different set of credentials to perform different actions - maybe on the exact same namespace.

If you just assume that adding a "namespace" discriminant is enough, I'm sure other corner cases will pop-up.

I think the basic idea of having 1 Cluster = 1 API URL might be wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, you should be able to connect N times to the same "cluster" (Kubernetes API) with a different set of credentials to perform different actions - maybe on the exact same namespace.

If you just assume that adding a "namespace" discriminant is enough, I'm sure other corner cases will pop-up.

In the context of Argo CD, do you have any concrete use cases where having separate cluster credentials per namespace would not be enough?

This in theory could be extended to cover more dimensions than just namespaces, but IMO I don't see a reason why this shouldn't be enough.

Copy link
Author

Choose a reason for hiding this comment

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

One that I can think of at the moment, but we haven't used yet, is the ability to differentiate between the two operations:

  • Read state of the cluster (R/O credentials)
  • Write state to the cluster (R/W credentials)

Now, this might be nitpicking again, but it just shows how the assumption "one credential per namespace" could be wrong.

Copy link
Member

Choose a reason for hiding this comment

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

We'll have to agree to disagree 😄 In case @crenshaw-dev or @jannfis does not chime in before then, I'd recommend you to join the contributors meeting on Thursday (17.15 CET) where we can discuss this in more detail.

Copy link
Author

Choose a reason for hiding this comment

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

Don't worry, I'm not strongly opinionated towards my approach - I just find it might be more flexible for the future to have them indexed by name instead.

As long as our use case is covered (the one I've opened a bug about), I'm all good 😊.

Copy link
Member

Choose a reason for hiding this comment

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

There was a discussion about this proposal a bit more than a week ago; the gist of that discussion was that having a more fleshed out proposal (and implementation) of #7689 would be preferable (@alexmt can give more context to give as to why).

Choose a reason for hiding this comment

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

We have a multi-tenant cluster where tenants have a "deployer" service account in their namespaces. These tenants want to use ArgoCD to deploy to their respective namespaces, but this is currently impossible due to the issue described.

The cluster is being run by a separate team than the team that managed ArgoCD, so it is not allowed to make ArgoCD a cluster-admin on the cluster and allow everyone to deploy to their namespaces.

As long as ArgoCD can match on the name of the cluster, then this would be a non-issue as we can add the same cluster as different "ArgoCD clusters" and applications would point to the correct cluster name. Maybe "destination" is a better work than "cluster" in this case :)

@blakepettersson
Copy link
Member

@denysvitali #14255 might be of interest to you

@chetan-rns
Copy link
Member

chetan-rns commented Nov 8, 2023

Hi @denysvitali @blakepettersson, I agree that impersonation could be used when working with multiple credentials for the same cluster. But I think it can coexist with the solution mentioned in this proposal.

Impersonation is more admin-focused where admins have to manage the cluster secrets and also map different service accounts to the right destination. In contrast, multiple cluster secrets follow the "self-service" approach with each user/tenant managing their cluster secret. I'm thinking of a broader use case that can be achieved with this proposal like clusters in any namespace where users can create and manage cluster secrets in a "non-argocd" namespace with less admin intervention. This would be similar to the existing Application in any namespace feature.

For example, let us consider two tenants A and B who want to deploy resources to an external cluster with restricted permissions. They can create cluster secrets in their tenant/non-argocd namespace along with their Application CRs and Argo CD would access the cluster using the restricted credentials.

kind: Secret
metadata:
  labels:
    argocd.argoproj.io/secret-type: cluster
  name: test-A                              
  namespace: user-tenant-A        # Cluster created in tenant A's namespace
type: Opaque 
data: 
# Credentials for namespace A of the external cluster

---
kind: Secret
metadata:
  labels:
    argocd.argoproj.io/secret-type: cluster
  name: test-B                              
  namespace: user-tenant-B        # Cluster created in tenant B's namespace
type: Opaque 
data: 
# Credentials for namespace B of the external cluster                 

This is just a gist and I understand that we need to delve deeper here. But we will run into the same problem of duplicate cluster URLs. So, changing the way Argo CD maps a cluster to a cache could be beneficial in enabling tenants to manage cluster secrets independently without admin intervention.


Currently, the clusters are identified by their API URL. Pretty much everywhere in the codebase, we assume that the API
URL is unique within an ArgoCD instance, and thus this is used as an identifier.
With this proposal, we suggest to switch to a (Namespaced) Cluster Name oriented approach, so that one can use the same API URL multiple times
Copy link
Member

Choose a reason for hiding this comment

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

When we say Cluster Name, does it mean the name of the cluster specified in the .data.name of the secret? Or is it the name of the secret resource metadata.name? Since .data.name is not a mandatory field it could be difficult to check for the uniqueness of the key. However, the secret name is mandatory, and combining it with the namespace will result in a unique key.


As we assume the `name@namespace` are unique within a cluster, and they're enforced by Kubernetes already (you can't have two resources of the same type in the same namespace), the upgrade strategy is pretty straight-forward.

Since we're not breaking any functionality for the exsiting users,
Copy link
Member

Choose a reason for hiding this comment

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

I agree that this wouldn't break any existing features. But we are changing the cache key and I wonder how it would affect the users when they upgrade. Would it invalidate the existing caches and result in the re-syncing of resources?

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

Successfully merging this pull request may close these issues.

4 participants