Prevent unintended usage of "replace"

[mattenklicker]

Summary

Make it possible to prevent the unintended usage of "replace" in UI sync options.

Motivation

To update resources by "kubectl replace" is dangerous under certain conditions. It already has a warning sign, but some might want to disable the usage completely. See #9767 also. It is very present in UI sync options and unfortunately it is likely impossible to block "replace" through kubernetes RBAC or other measures like OPA.

Proposal

• Create config flag to make the "replace" box grey out or disappear in UI sync options
• Alternatively: Make it a RBAC control.

[fhopfensperger]

I am also looking for something like this. It would be nice to be able to customize the sync options per application.

[pranchals]

Implementing RBAC for Sync Options would be good addition and will enforce stricter access controls and define precise permissions with regards synchronisation actions within an Argo CD Application.

Is there any update on this being implemented ?

[conman2305]

I'll +1 this - I want to give users the ability to sync their apps, but doing so currently also means they can totally wipe it out. It'd be nice to implement fine-grained controls for the sync permission like there is for update and delete: https://argo-cd.readthedocs.io/en/stable/operator-manual/rbac/#fine-grained-permissions-for-updatedelete-action

[jeremyrajan]

Is there a way around it? Can I give sync permissions but don't allow replace?

[todaywasawesome]

@jeremyrajan not today

[andrii-korotkov-verkada]

I'm planning to work on this an implement an option in the config which allows to hide the button. UI would query that data from the backend and not render a button at all if configured so.

[andrii-korotkov-verkada]

Extra UI friction may not be as helpful, since people may ignore it anyways as they can ignore the warning icon near the checkbox.

[andrii-korotkov-verkada]

@todaywasawesome, which configmap is best for putting the option in? Is it argocd-cm or some other?

[andrii-korotkov-verkada]

Seems like RBAC already has a support for rules about resource deletion https://argo-cd.readthedocs.io/en/stable/operator-manual/rbac/#fine-grained-permissions-for-updatedelete-action. Somebody brought this up during the contributors meeting, that checking for resource deletion permission can be done. Given possibility to customize like above, I think it will solve the problem.

[andrii-korotkov-verkada]

@todaywasawesome, @crenshaw-dev, @nitishfy, @agaudreault, do you know where's the code which checks for sync permissions, i.e. if the rbac allows to sync an application? I've tried to check in SyncAppState, but only found checks for group/kind and destination.

[andrii-korotkov-verkada]

Btw, does sync with prune checks for deletion permissions? If not, should it be added as well?

[crenshaw-dev]

I think the proposal needs to be more clearly/specifically defined. For example, I think an RBAC-based system checking for delete permission has some problems we'd need to overcome: #20720 (review)

[andrii-korotkov-verkada]

@crenshaw-dev, @agaudreault, continuing the conversation here instead of the PR. To summarize:

The current proposal:

• Partially use delete permissions RBAC to deny some of syncs with replace if the replace option is specified from UI/CLI.
• Don't use these if the replace is only configured via application/resource annotations (i.e. leave as it is now).
• If whole apps are allowed to be deleted, sync with replace is allowed.
• If whole apps are not allowed to be deleted, resource list for sync is not provided and any resource is allowed to be deleted, sync with replace is allowed.
• If whole apps are not allowed to be deleted, resource list for sync is provided and the corresponding resources are allowed to be deleted, sync with replace is allowed.
• Otherwise, sync with replace is denied.
• Use documentation to talk about this behavior.

Some caveats:

• The approach doesn't go all the way of configuring sync with replace permissions and has some inconsistency, e.g. when using annotations for replace during sync.
• Delete is not the same as replace or prune, so permissions for delete are under a higher scrutiny.
• There can be cases of potential over-permissioning due to a new model.

Feel free to summarize the things I've missed.

[andrii-korotkov-verkada]

Based on some other things discussed, maybe indeed coming up with a new permission action like prune as complementary for delete is good. This would apply to sync with replace and sync with prune, both full and partial.

[agaudreault]

I think the solution with the least caveats would be to add sync-replace and sync-prune as new actions.

To support partial-sync, these actions could either be expressed with sync or sync/*, in the same fashion as the fine-grained delete/update.

So you would have

1. Implement feature flag to turn on fine-grained sync. (Following items assume feature flag is on)
2. When user triggers a basic app sync, permissions are validated on sync
3. When user triggers a partial sync, permission on each resources are validated against sync/<resource>
4. When user triggers a basic app sync with replace, permissions are validated on sync-replace
5. When user triggers a partial sync with replace, permission on each resource is validated against sync-replace/<resource>
6. When user triggers a basic app sync with prune, permissions are validated on sync-prune
7. When user triggers a partial sync with prune, permission on each resources are validated against sync-prune/<resource>
8. A sync with replace and prune checks both rbac action.

This way there's only 1 rbac check done in the server based on the request options. If app or individual resources are configured with annotations, then it is not a user action and does not need to go through rbac.

[crenshaw-dev]

Yep I think I agree with all the above.

But given the cognitive and maintenance costs of new RBAC verbs, and given that the use case seems primarily for accident avoidance rather than security constraints, I think we should seriously consider non-RBAC mitigations.

[andrii-korotkov-verkada]

I like the idea of separate RBAC permission, though it'd add a bit of configuration and maintenance overhead.

Though people would still probably have to give broad sync with replace permissions, if they wanna be able to click a checkbox in UI for the whole app.

Application controller can also check RBAC when it knows the diff and actual resources to be replaced. But it'd be inconsistent with how server RBAC checks work (give immediate feedback).

If we configure on app or app project level, we need some granularity on resource types, since sometimes people would want replace to update immutable fields.

Where RBAC is making it easier is when you can configure some kind of power user permission allowing to do sync with replace, but make it not granted by default and be requestable through Opal for example (or whatever system is used).

[andrii-korotkov-verkada]

There's a tricky part with the annotations approach. Allowing disabling auto sync via UI requires applications update permission turned on. But this is wider than desired, allowing people to add annotations they want. So it'd add friction, but won't prevent people from doing sync with replace.

[andrii-korotkov-verkada]

@crenshaw-dev, I think the new verbs for RBAC may not be that bad from a technical perspective. But cognitive one may be more impactful.

[andrii-korotkov-verkada]

Can you guys share more of your experience for a summary of valid use cases of sync with replace, please? E.g. how often would it make sense to do it for a whole application? How are people supposed to do sync for most of resources, but sync with replace for a small subset?

[andrii-korotkov-verkada]

Here are some thoughts on how the sync with replace can and supposed to work.

Here are some properties (I think):

• Most resources don't need to be synced with replace.
• Sync with replace is needed to "update" immutable fields on some resources. However, when immutable fields remain unchanged, those resources don't need a sync with replace.

Thus, a need for sync with replace shouldn't be a property of the resource or application, since it'd be an overkill. It's a property of a specific update that needs to be done.

I think we should have a sync with a conditional replace instead, i.e. only replace if apply returned errors about immutable fields. This kind of operation would not really have the same level of danger as the current sync with replace, and perhaps we don't need any RBAC or non-RBAC changes as we need with the current sync with replace.

What do you think?