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

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

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

Conversation

agaudreault
Copy link
Member

@agaudreault agaudreault commented Dec 16, 2024

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
Copy link

bunnyshell bot commented Dec 16, 2024

🔴 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

Copy link

codecov bot commented Dec 16, 2024

Codecov Report

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

Project coverage is 53.29%. Comparing base (ef55ba5) to head (9bd1f1f).
Report is 9 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.17%   53.29%   -1.89%     
==========================================
  Files         337      337              
  Lines       57057    57076      +19     
==========================================
- Hits        31483    30416    -1067     
- Misses      22870    24010    +1140     
+ Partials     2704     2650      -54     

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

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

Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
@agaudreault agaudreault marked this pull request as draft December 17, 2024 19:39
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!

Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
This reverts commit c96ecd8.
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
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"

@agaudreault agaudreault enabled auto-merge (squash) January 3, 2025 21:32
@agaudreault agaudreault requested a review from rumstead January 3, 2025 21:32
@agaudreault
Copy link
Member Author

@ishitasequeira @terrytangyuan @rumstead lint/test fixed. Let's wait for discussion on the issue to know if we should cherry-pick.

Thanks for the review!

Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

lgtm besides a few nitpicks. One final suggestion would be to change the tests to tabular tests for readability. Will go with your decision on whether that's worth the time.

util/db/cluster.go Outdated Show resolved Hide resolved
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.

Overall LGTM

Signed-off-by: Alexandre Gaudreault <[email protected]>
@agaudreault agaudreault requested a review from a team as a code owner January 17, 2025 20:02
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
@agaudreault agaudreault disabled auto-merge January 17, 2025 21:21
@agaudreault agaudreault enabled auto-merge (squash) January 17, 2025 21:21
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.

Disabling in-cluster does not prevent the usage of in-cluster
5 participants