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.

Sign up for GitHub

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

Jump to bottom

fix(in-cluster): do not allow the cluster to be used when disabled #21208

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

fix(in-cluster): do not allow the cluster to be used when disabled #21208

wants to merge 11 commits into from
+371 −120

Conversation

agaudreault
Copy link
Member

@agaudreault agaudreault commented Dec 16, 2024 •
edited
Loading

Closes #21207

The ApplicationSet code is still using the in-cluster, even when disabled. However, the apps will fail in Argo CD, which is expected. See #10473

Depends on #21225

Existing applications:
image

New applications creation (API):
image

@agaudreault agaudreault requested a review from a team as a code owner December 16, 2024 22:40
@bunnyshell Bunnyshell
Copy link

bunnyshell bot commented Dec 16, 2024 •
edited
Loading

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

  • 🔵 /bns:start to start the environment
  • 🚀 /bns:deploy to redeploy the environment
  • ❌ /bns:delete to remove the environment

@codecov Codecov
Copy link

codecov bot commented Dec 16, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 73.52941% with 9 lines in your changes missing coverage. Please review.

Project coverage is 55.19%. Comparing base (46bfc10) to head (7fad375).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
util/db/cluster.go 73.52% 6 Missing and 3 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #21208      +/-   ##
==========================================
+ Coverage   55.03%   55.19%   +0.16%     
==========================================
  Files         338      338              
  Lines       57075    57187     +112     
==========================================
+ Hits        31410    31567     +157     
+ Misses      22968    22919      -49     
- Partials     2697     2701       +4

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

rumstead
rumstead suggested changes Dec 17, 2024
View reviewed changes
Copy link
Member

@rumstead rumstead left a comment

Choose a reason for hiding this comment

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

Could you fix the linting errors? It seems that there is a race condition being caught. The stack traces look like a similar problem I fixed before - https://github.com/argoproj/argo-cd/pull/20805/files

@agaudreault
typo
eb4a528 
Signed-off-by: Alexandre Gaudreault <[email protected]>
@agaudreault
linter
ca03d10 
Signed-off-by: Alexandre Gaudreault <[email protected]>
@agaudreault agaudreault marked this pull request as draft December 17, 2024 19:39
@agaudreault agaudreault mentioned this pull request Dec 17, 2024
Merged
terrytangyuan
terrytangyuan reviewed Dec 18, 2024
View reviewed changes
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Good catch. Thanks!

@agaudreault
e2e
0a72dd8 
Signed-off-by: Alexandre Gaudreault <[email protected]>
@agaudreault
fix e2e
35d6ec0 
Signed-off-by: Alexandre Gaudreault <[email protected]>
@agaudreault
unquote CLI error
c96ecd8 
Signed-off-by: Alexandre Gaudreault <[email protected]>
@agaudreault
Revert "unquote CLI error"
224869d 
This reverts commit c96ecd8.
@agaudreault
just match the quoted result
7fad375 
Signed-off-by: Alexandre Gaudreault <[email protected]>
@agaudreault
Merge remote-tracking branch 'upstream/master' into fix-in-cluster-di…
d5a0c64 
…sable

Signed-off-by: Alexandre Gaudreault <[email protected]>
@agaudreault
Copy link
Member Author

Cherry pick in 2.13 will require #20805 and #21225

@agaudreault agaudreault marked this pull request as ready for review December 18, 2024 16:37
ishitasequeira
ishitasequeira reviewed Dec 18, 2024
View reviewed changes
Copy link
Member

@ishitasequeira ishitasequeira left a comment

Choose a reason for hiding this comment

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

The changes LGTM
@agaudreault can you fix the CI failures?

@agaudreault
Copy link
Member Author

Current documentation seems to indicate that this setting is to disable the cluster. In every other cases, the behavior when a cluster is removed is for the apps to go in the unknown status and prevent sync from an invalid destination.

Removing a cluster

Run argocd cluster rm context-name.

This removes the cluster with the specified name.

!!!note "in-cluster cannot be removed"
The in-cluster cluster cannot be removed with this. If you want to disable the in-cluster configuration, you need to update your argocd-cm ConfigMap. Set cluster.inClusterEnabled to "false"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ishitasequeira ishitasequeira ishitasequeira left review comments

@terrytangyuan terrytangyuan terrytangyuan left review comments

@rumstead rumstead rumstead requested changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disabling in-cluster does not prevent the usage of in-cluster
4 participants
@agaudreault @terrytangyuan @rumstead @ishitasequeira