feat: add support for multi-cluster with the same API Url #10897

denysvitali · 2022-10-11T14:00:13Z

This PR changes the way ArgoCD uses the clusters.

Rationale

For some use cases (including ours), an user might want to use the same Kubernetes API Server multiple times, with a different set of credentials.

Let me start with an example.

Given:

A Kubernetes Cluster with the Kubernetes API Server URL https://kubernetes.corp.example.com and three namespaces
- dev,
- staging
- prod
Three different accounts (credentials) that are scoped to their respective namespaces:
- acc-dev
- acc-staging
- acc-prod
An ArgoCD deployment running the latest version as of today (v2.5.3)

It is currently not possible to set-up three accounts in a way that they all use the same API Server URL, due to the fact that ArgoCD internally (and in the UI) prioritizes the API Server URL over the Cluster Name.

When viewing the Cluster details in ArgoCD UI, the UI jumps between the 3 clusters (because there is a 1s automatic refresh and the API returns the first match in a non-deterministic way):
https://argocd.corp.example.com/settings/clusters/https%3A%2F%2Fkubernetes.corp.example.com

This PR fixes all of these issues by using only the cluster name (in-cluster, cluster-dev, cluster-staging, cluster-prod) as an identifier for both the sync and the UI.

This PR closes #9606.
Comments and modifications are really appreciated! Thanks!

Checklist:

codecov · 2022-10-11T14:30:53Z

Codecov Report

Base: 46.88% // Head: 46.85% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (125c30a) compared to base (981a1f1).
Patch coverage: 44.11% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10897      +/-   ##
==========================================
- Coverage   46.88%   46.85%   -0.03%     
==========================================
  Files         240      240              
  Lines       39911    39936      +25     
==========================================
+ Hits        18712    18714       +2     
- Misses      19306    19328      +22     
- Partials     1893     1894       +1

Impacted Files	Coverage Δ
cmd/argocd/commands/admin/cluster.go	`0.00% <0.00%> (ø)`
controller/appcontroller.go	`52.33% <0.00%> (ø)`
controller/cache/cache.go	`19.48% <0.00%> (ø)`
server/application/application.go	`28.61% <0.00%> (ø)`
server/application/terminal.go	`15.76% <0.00%> (ø)`
util/db/helmrepository.go	`0.00% <0.00%> (ø)`
util/settings/settings.go	`48.87% <ø> (ø)`
util/db/cluster.go	`56.62% <32.00%> (-2.87%)`	⬇️
util/argo/argo.go	`61.76% <36.36%> (-0.65%)`	⬇️
server/cluster/cluster.go	`37.98% <50.00%> (ø)`
... and 4 more

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 · 2022-11-30T14:43:56Z

We've updated the PR and consider the feature to be completed. Can we please have a review?

Ping @crenshaw-dev @alexmt @jessesuen

This is the first commit towards addressing argoproj#9606 Signed-off-by: Denys Vitali <denys.vitali@swisscom.com>

Signed-off-by: Denys Vitali <denys.vitali@swisscom.com>

…ange the proxy origin Signed-off-by: Denys Vitali <denys.vitali@swisscom.com>

Signed-off-by: Denys Vitali <denys.vitali@swisscom.com>

denysvitali · 2023-01-05T16:27:55Z

@crenshaw-dev , firendly ping :)

leptitchriss · 2023-01-18T22:25:38Z

Thank you @denysvitali for this fix, we have the exact same problem and believe this will solve our multi-tenancy problem.

denysvitali · 2023-01-19T06:36:35Z

@jannfis @alexmt friendly ping for a review (:

jannfis

Hi @denysvitali, thanks a lot for trying to tackle this issue and for your patience. There's a lot of PR backlog right now, we're very sorry about the delay.

I have a few concerns with this PR as-is, because as far as I can tell, it makes the cluster name a mandatory field. If this assumption is correct, it means that the PR will introduce a majorly breaking change, because the cluster name is currently an optional property of any cluster connection and may not be set in many configurations.

Also, what effects will this have on the cluster cache? Currently, we use the cluster's URL as the primary key for cluster caches. I have not seen a change to this logic in the PR, so I assume that this still holds true. Because many of the cluster settings are defined within the cluster cache, I have the concern that this ends up in a scenario where we have multiple clusters with the same URL but a different name, but only a single cluster cache configuration?

jannfis · 2023-01-19T14:10:57Z

docs/developer-guide/code-contributions.md

@@ -2,12 +2,19 @@

 ## Preface

-The Argo CD project continuously grows, both in terms of features and community size. It gets adopted by more and more organisations which entrust Argo CD to handle their critical production workloads. Thus, we need to take great care with any changes that affect compatibility, performance, scalability, stability and security of Argo CD. For this reason, every new feature or larger enhancement must be properly designed and discussed before it gets accepted into the code base.
+The Argo CD project continuously grows, both in terms of features and community size. 


In this file, have there been any changes except line breaks?

Can you please revert the changes of the line breaks?

Sure! You're correct I've only changed the line breaks (:

jannfis · 2023-01-19T14:11:51Z

docs/developer-guide/debugging-remote-environment.md

@@ -2,22 +2,27 @@

 In this guide, we will describe how to debug a remote ArgoCD environment with [Telepresence](https://telepresence.io/).

-Telepresence allows you to connect & debug a service deployed in a remote environment and to "cherry-pick" one service to run locally, staying connected to the remote cluster. This will:
+Telepresence allows you to connect & debug a service deployed in a remote environment and to "cherry-pick" 


Same as previous comment - please revert changes, at least the line breaks.

jannfis · 2023-01-19T14:12:16Z

docs/developer-guide/running-locally.md

@@ -2,11 +2,14 @@

 ## Run Argo CD outside of Kubernetes

-During development, it might be viable to run Argo CD outside of a Kubernetes cluster. This will greatly speed up development, as you don't have to constantly build, push and install new Argo CD Docker images with your latest changes.
+During development, it might be viable to run Argo CD outside a Kubernetes cluster. 


Please revert line break changes.

jannfis · 2023-01-19T14:26:32Z

server/server.go

@@ -850,7 +850,7 @@ func (a *ArgoCDServer) newHTTPServer(ctx context.Context, port int, grpcWebHandl
 	// NOTE: if a marshaller option is not supplied, grpc-gateway will default to the jsonpb from
 	// golang/protobuf. Which does not support types such as time.Time. gogo/protobuf does support
 	// time.Time, but does not support custom UnmarshalJSON() and MarshalJSON() methods. Therefore
-	// we use our own Marshaler
+	// we use our own Marshaller


IMHO, this is just a difference between BE and AE, both spellings (single vs double l) are actually valid.

jannfis · 2023-01-19T14:36:31Z

ui/src/app/settings/components/cluster-details/cluster-details.tsx

-        <DataLoader ref={loaderRef} input={server} load={(url: string) => timer(0, 1000).pipe(mergeMap(() => from(services.clusters.get(url, ''))))}>
+        <DataLoader ref={loaderRef} input={server} load={(name: string) => timer(0, 5000).pipe(mergeMap(() => from(services.clusters.getByName(name))))}>


Does this change mean, cluster's name field is now mandatory?

Indeed, because the Cluster API URL is not unique anymore, so we use the Cluster Name.
We could eventually make it unique without breaking compatibility by relying on Kubernetes name+namespace uniqueness by following what Traefik does: encoding the name into something like resource-name@namespace

jannfis · 2023-01-20T21:10:38Z

@denysvitali So I've taken this PR to this weeks contributor's meeting to discuss it with fellow maintainers. While we all think this feature is worth pursuing and eventually want it implemented, we feel that it may require some more thought and design.

We appreciate the work you already put into this, but would like to kindly ask you to submit an enhancement proposal which captures all the important design decisions before we progress on the code any further. After we have agreed on how the change will look like, we'd be more than happy to see the contribution to the code made by you, and will help you bring this PR to a state where it eventually be merged.

WDYT of this?

denysvitali · 2023-01-20T21:21:23Z

Sounds awesome! I'll do that!
Thank you for bringing it up!

denysvitali · 2023-03-07T17:09:53Z

FYI, just created proposal 004.
You can check #12755

yellow-straw-hat · 2023-05-24T19:37:03Z

Hello @jannfis, are there any updates about this ?

This becomes a security concern in our organization. We need to add n clusters having the same url but with a different configuration (each project team works on a different namespace)

Many companies chose the kubernetes namespace isolation feature to manage authorization for each team. And imo argocd should be flexible enough to follow the k8s logic.

Interaze · 2023-06-21T02:20:25Z

Would love to see this get approved

blakepettersson · 2023-06-30T12:06:47Z

#14255 might be of interest to everyone that's checking this PR

denysvitali · 2023-06-30T12:14:10Z

#14255 could indeed be a solution for us because at the moment we're trying to adapt the concept of "clusters" to allow us to do exactly that - using different service accounts for different namespaces.

With that proposal, instead of tackling the problem from the cluster perspective (trust me, it's ugly, we have N+1 clusters for N namespaces and the control plane on another K8s cluster) we solve it by specifying which service accounts are used for the sync.

I second that proposall, as it seems to solve another problem that my proposal has: multiple ArgoCD clusters to represent the same Kubernetes cluster.

If everyone in this thread agrees, we can simply close this proposal in favor of #14255.
I'll still have to read the proposal in depth, but the general idea seems a better implementation of my solution :)

Interaze · 2023-06-30T14:43:27Z

I've found a solution for my problem in scoping namespaces to the project. If we could have something that allows users to move away from the "one credential to rule all" I'd be all for it

YDamree · 2023-07-12T16:07:44Z

Hello,
Is there any chance to merge this PR on the main branch some day please ?
It would be really helpful for our project :)
Thanks a lot in advance

blakepettersson · 2023-07-12T17:24:30Z

@YDamree I'd say that the chances are quite low, TBH. A practical implementation of #14255 is more likely to be merged, and unless the spec changes significantly I'd say it's relatively straightforward to implement.

ButtonDev · 2024-09-06T16:08:10Z

is there any plan to merge this? i have the same issue

denysvitali force-pushed the feature/add-multi-cluster-serverurl-support branch from 3fbc391 to f0101ae Compare November 30, 2022 14:20

Denys Vitali added 7 commits November 30, 2022 16:56

feat: add initial support for multi-cluster with the same API Url

c6f4596

This is the first commit towards addressing argoproj#9606 Signed-off-by: Denys Vitali <denys.vitali@swisscom.com>

docs: improve code-contributions documentation (limit column size)

c92825d

Signed-off-by: Denys Vitali <denys.vitali@swisscom.com>

refactor: fix spelling mistakes and comments

8590374

Signed-off-by: Denys Vitali <denys.vitali@swisscom.com>

style: correct code comments

60bd745

Signed-off-by: Denys Vitali <denys.vitali@swisscom.com>

refactor: adapt code to fix deprecation warnings

50ff491

Signed-off-by: Denys Vitali <denys.vitali@swisscom.com>

feat: adapt ui settings to be Cluster.name centric

708edf8

Signed-off-by: Denys Vitali <denys.vitali@swisscom.com>

feat: allow developers to specify ARGOCD_API_CHANGE_ORIGIN=true to ch…

c4d3fd4

…ange the proxy origin Signed-off-by: Denys Vitali <denys.vitali@swisscom.com>

denysvitali force-pushed the feature/add-multi-cluster-serverurl-support branch from f0101ae to c4d3fd4 Compare November 30, 2022 15:57

fix: rename SecretMaperValidation into SecretMapperValidation

f711628

Signed-off-by: Denys Vitali <denys.vitali@swisscom.com>

denysvitali force-pushed the feature/add-multi-cluster-serverurl-support branch from 76aeb31 to f711628 Compare November 30, 2022 16:15

fix: eslint --fix

125c30a

Signed-off-by: Denys Vitali <denys.vitali@swisscom.com>

jannfis reviewed Jan 19, 2023

View reviewed changes

denysvitali mentioned this pull request Mar 7, 2023

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

Open

blakepettersson mentioned this pull request Aug 12, 2023

Allow multiple clusters pointing to the same URL #15027

Open

ljupchokotev mentioned this pull request Aug 29, 2023

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

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for multi-cluster with the same API Url #10897

feat: add support for multi-cluster with the same API Url #10897

denysvitali commented Oct 11, 2022 •

edited

Loading

codecov bot commented Oct 11, 2022 •

edited

Loading

denysvitali commented Nov 30, 2022

denysvitali commented Jan 5, 2023

leptitchriss commented Jan 18, 2023

denysvitali commented Jan 19, 2023

jannfis left a comment

jannfis Jan 19, 2023

denysvitali Jan 19, 2023

jannfis Jan 19, 2023

jannfis Jan 19, 2023

jannfis Jan 19, 2023

jannfis Jan 19, 2023

denysvitali Jan 19, 2023

jannfis commented Jan 20, 2023

denysvitali commented Jan 20, 2023

denysvitali commented Mar 7, 2023

yellow-straw-hat commented May 24, 2023

Interaze commented Jun 21, 2023

blakepettersson commented Jun 30, 2023

denysvitali commented Jun 30, 2023

Interaze commented Jun 30, 2023

YDamree commented Jul 12, 2023 •

edited

Loading

blakepettersson commented Jul 12, 2023

ButtonDev commented Sep 6, 2024

		<DataLoader ref={loaderRef} input={server} load={(url: string) => timer(0, 1000).pipe(mergeMap(() => from(services.clusters.get(url, ''))))}>
		<DataLoader ref={loaderRef} input={server} load={(name: string) => timer(0, 5000).pipe(mergeMap(() => from(services.clusters.getByName(name))))}>

feat: add support for multi-cluster with the same API Url #10897

Are you sure you want to change the base?

feat: add support for multi-cluster with the same API Url #10897

Conversation

denysvitali commented Oct 11, 2022 • edited Loading

Rationale

Checklist:

codecov bot commented Oct 11, 2022 • edited Loading

Codecov Report

denysvitali commented Nov 30, 2022

denysvitali commented Jan 5, 2023

leptitchriss commented Jan 18, 2023

denysvitali commented Jan 19, 2023

jannfis left a comment

Choose a reason for hiding this comment

jannfis Jan 19, 2023

Choose a reason for hiding this comment

denysvitali Jan 19, 2023

Choose a reason for hiding this comment

jannfis Jan 19, 2023

Choose a reason for hiding this comment

jannfis Jan 19, 2023

Choose a reason for hiding this comment

jannfis Jan 19, 2023

Choose a reason for hiding this comment

jannfis Jan 19, 2023

Choose a reason for hiding this comment

denysvitali Jan 19, 2023

Choose a reason for hiding this comment

jannfis commented Jan 20, 2023

denysvitali commented Jan 20, 2023

denysvitali commented Mar 7, 2023

yellow-straw-hat commented May 24, 2023

Interaze commented Jun 21, 2023

blakepettersson commented Jun 30, 2023

denysvitali commented Jun 30, 2023

Interaze commented Jun 30, 2023

YDamree commented Jul 12, 2023 • edited Loading

blakepettersson commented Jul 12, 2023

ButtonDev commented Sep 6, 2024

denysvitali commented Oct 11, 2022 •

edited

Loading

codecov bot commented Oct 11, 2022 •

edited

Loading

YDamree commented Jul 12, 2023 •

edited

Loading