Skip to content
This repository was archived by the owner on Oct 6, 2025. It is now read-only.

Conversation

@pjiang-dev
Copy link
Contributor

@pjiang-dev pjiang-dev commented Jun 2, 2025

fixes argoproj/argo-cd#23214

When performing a Sync with ServerSideApply we will check if there is a kubectl-client-side-apply manager and run a client side apply with the field manager kubectl-client-side-apply. This will make the last-applied-confiugration field to be owned by kubectl-client-side-apply and then once the Sync with ServerSideApply executes it will trigger the auto-migration of fields to argocd-controller which is described in kubernetes/kubernetes#112905

We need this in cases where a user created their resource with kubectl apply -f in the command line then later used argocd to manage it with SSA.

There will be a PR in argo-cd to support configurable field managers to migration from as well as option to disable this feature entirely

@pjiang-dev pjiang-dev requested a review from a team as a code owner June 2, 2025 19:19
Signed-off-by: Peter Jiang <[email protected]>
@pjiang-dev pjiang-dev changed the title fix auto migrate kubectl-client-side-apply fields for SSA fix: auto migrate kubectl-client-side-apply fields for SSA Jun 2, 2025
Signed-off-by: Peter Jiang <[email protected]>
@codecov
Copy link

codecov bot commented Jun 2, 2025

Codecov Report

Attention: Patch coverage is 51.61290% with 30 lines in your changes missing coverage. Please review.

Project coverage is 53.42%. Comparing base (8849c3f) to head (d6f1083).
Report is 48 commits behind head on master.

Files with missing lines Patch % Lines
pkg/sync/sync_context.go 51.61% 30 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #727      +/-   ##
==========================================
- Coverage   54.26%   53.42%   -0.84%     
==========================================
  Files          64       64              
  Lines        6164     6560     +396     
==========================================
+ Hits         3345     3505     +160     
- Misses       2549     2777     +228     
- Partials      270      278       +8     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@blakepettersson
Copy link
Member

@pjiang-dev @crenshaw-dev could we not do something similar as kubernetes/kubernetes#124191 where we'd force ownership of all fields, given that we opt-in to this behavior by adding another sync option? Something like OverwriteFieldManagers=true?

@pjiang-dev
Copy link
Contributor Author

@pjiang-dev @crenshaw-dev could we not do something similar as kubernetes/kubernetes#124191 where we'd force ownership of all fields, given that we opt-in to this behavior by adding another sync option? Something like OverwriteFieldManagers=true?

@blakepettersson That PR works by manipulating managed fields and adds the 'last-applied-configuration' to all field managers before doing SSA. Kubernetes recommends against doing so and also i don't know what side affects this may have. I think its safer to use what k8s already provides. Also some users may want to keep certain field managers.

However, i do think what we can do is provide an additional annotation with SSA that allows users to specify their own field manager to migrate fields. This would replace the 'kubectl-client-side-apply' field manager in this PR with their own with something like this:
argocd.argoproj.io/sync-options: ServerSideApply=true, OverwriteFieldManager=<your-field-manager>
this way they can have the flexibility to migrate whichever field manager they choose. We would still want to keep kubectl-client-side-apply as the default for this though.

@blakepettersson
Copy link
Member

However, i do think what we can do is provide an additional annotation with SSA that allows users to specify their own field manager to migrate fields. This would replace the 'kubectl-client-side-apply' field manager in this PR with their own with something like this

Sounds good to me! Could we have that option for multiple field managers, e.g

 argocd.argoproj.io/sync-options: ServerSideApply=true, OverwriteFieldManagers=<your-field-manager-1>,<your-field-manager-1>

(the default still being kubectl-client-side-apply)

@pjiang-dev
Copy link
Contributor Author

pjiang-dev commented Jun 4, 2025

provide an additional annotation with SSA that allows users to specify their own field manager to migrate fields. This would replace the 'kubectl-client-side-apply' field manager in this PR with their own with something like this:
argocd.argoproj.io/sync-options: ServerSideApply=true, OverwriteFieldManager=<your-field-manager>
this way they can have the flexibility to migrate whichever field manager they choose. We would still want to keep kubectl-client-side-apply as the default for this though.

Unfortunately, we cannot do multiple field managers at once because 'last-applied-configuration' can only belong to one field manager at a time without manipulating managed fields. I think it won't be too much burden for users to just change the manager and re-sync if they have multiple managers though.

@pjiang-dev pjiang-dev requested a review from leoluz June 5, 2025 21:54
@pjiang-dev
Copy link
Contributor Author

For this PR i will leave as is and create a follow up PR to make the field manager to migrate fields configurable to the user

Copy link
Contributor

@leoluz leoluz left a comment

Choose a reason for hiding this comment

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

For this PR i will leave as is and create a follow up PR to make the field manager to migrate fields configurable to the user

I think the only thing missing in this PR is the ability to turn this feature on/off

@pjiang-dev
Copy link
Contributor Author

pjiang-dev commented Jun 6, 2025

For this PR i will leave as is and create a follow up PR to make the field manager to migrate fields configurable to the user

I think the only thing missing in this PR is the ability to turn this feature on/off

Added option to disable this migration using this annotation in argocd:

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: my-app
spec:
  syncPolicy:
    syncOptions:
      - DisableClientSideApplyMigration=true  # Feature disabled

Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Peter Jiang <[email protected]>
@sonarqubecloud
Copy link

Copy link
Contributor

@leoluz leoluz left a comment

Choose a reason for hiding this comment

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

LGTM

@leoluz leoluz changed the title fix: auto migrate kubectl-client-side-apply fields for SSA feat: auto migrate kubectl-client-side-apply fields for SSA Jun 12, 2025
@leoluz leoluz merged commit 69dfa70 into argoproj:master Jun 12, 2025
5 checks passed
RoelofKuijpers pushed a commit to RoelofKuijpers/gitops-engine that referenced this pull request Jul 29, 2025
…#727)

* feat: auto migrate kubectl-client-side-apply fields for SSA

Signed-off-by: Peter Jiang <[email protected]>

* fix master version

Signed-off-by: Peter Jiang <[email protected]>

* run gofumpt

Signed-off-by: Peter Jiang <[email protected]>

* Propagate sync error instead of logging

Signed-off-by: Peter Jiang <[email protected]>

* allow enable/disable of CSA migration using annotation

Signed-off-by: Peter Jiang <[email protected]>

* fix linting

Signed-off-by: Peter Jiang <[email protected]>

* Refactor to allow for multiple managers and disable option

Signed-off-by: Peter Jiang <[email protected]>

* remove commentj

Signed-off-by: Peter Jiang <[email protected]>

* refactor

Signed-off-by: Peter Jiang <[email protected]>

* fix test

Signed-off-by: Peter Jiang <[email protected]>

* Add docs for client side apply migration

Signed-off-by: Peter Jiang <[email protected]>

* Edit comment

Signed-off-by: Peter Jiang <[email protected]>

---------

Signed-off-by: Peter Jiang <[email protected]>
Signed-off-by: Roelof Kuijpers <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Server-Side Apply prevents field deletion owned by other managers

4 participants