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

Allow rollbacks when auto-sync is enabled and user does not have "applications update" permission #20600

Open
fffinkel opened this issue Oct 30, 2024 · 2 comments · May be fixed by fffinkel/argo-cd#2 or #20671
Labels
component:application-controller component:rbac Issues related to Openshift and Racher enhancement New feature or request

Comments

@fffinkel
Copy link

Summary

We would like to allow users to use the rollback feature with apps that have auto-sync enabled, when those users do not have permission to update applications.

Motivation

We currently deploy apps from manifests in a git repository that engineers do not have write access to. For compliance purposes, only the build pipeline can write to this repo, and we do not allow engineers to edit manifests in Argo CD. Furthermore, we enable auto-sync so those apps stay up to date with the latest builds of those apps.

We would like engineers to be able to use the rollback feature, however we cannot do so without granting engineers "update" permissions to their apps. Granting update is required for allowing the engineer to disable auto-sync, but also means that the engineer can edit any of their app's manifests in Argo CD, which does not fit within our compliance requirements.

A possible solution to this problem would be to allow update on applications themselves, while denying edit on any sub-resources. There was a change to allow for fine-grained update/delete RBAC enforcement, however this change does not work for us for two reasons:

The first reason is that it's not actually possible to refer to the Application kind as a sub-resource. The only way to grant update permissions to an Application kind is to also grant update to all of it's sub-resources, meaning we cannot explicitly allow updates for just the Application kind.

p, test-user, applications, update, */*, allow

The second reason is that "allow" on an "update" overrides a deny on sub-resources. As noted in the RBAC documentation, this means we cannot do something like:

p, test-user, applications, update, */*, allow
p, test-user, applications, update/*/Deployment/*, */*, deny

Proposal

We propose two possible solutions to this problem:

  • Fix RBAC to prioritize deny over allow when considering sub-resources, allowing us to allow "update" but deny "update///*"
  • Allow referencing the Application kind directly, letting us specifically allow updates for it

Etc

Slack thread: https://cloud-native.slack.com/archives/C01TSERG0KZ/p1729785863995189

Related issue: #18124

Related fix: 4905911

@fffinkel fffinkel added the enhancement New feature or request label Oct 30, 2024
@todaywasawesome todaywasawesome added component:application-controller component:rbac Issues related to Openshift and Racher labels Oct 31, 2024
@todaywasawesome
Copy link
Contributor

todaywasawesome commented Oct 31, 2024

Another approach to this currently would be to use a sync window to stop autosync, at which point your team would be able to rollback without it being overwritten. When you're ready to resume normal operation, delete the sync window.

https://argo-cd.readthedocs.io/en/stable/user-guide/sync_windows/

@agaudreault
Copy link
Member

Planned for 3.0 #19988

I think it is reasonable to make this configurable behind a feature flag, and we remove that feature flag in 3.0.

fffinkel added a commit to fffinkel/argo-cd that referenced this issue Nov 4, 2024
Change applications resource RBAC to use fine-grained update/delete
enforcement by default. This allows us to enforce RBAC on the
application itself, separately from the sub-resources related to it.

See also GitHub argoproj#18124, argoproj#20600
fffinkel added a commit to fffinkel/argo-cd that referenced this issue Nov 5, 2024
A breaking change was introduced in a previous commit that is planned to
be a part of the next major version of Argo CD (v3) where it's okay to
introduce breaking changes. We want this feature before we hit v3, so
we add a config setting that allows us to explicitly turn this new v3
behavior on in v2. The current v2 behavior is the default, so this
change will not affect folks who do not explicitly opt in.

GitHub argoproj#19988, argoproj#20600
fffinkel added a commit to fffinkel/argo-cd that referenced this issue Nov 5, 2024
Change applications resource RBAC to use fine-grained update/delete
enforcement by default. This allows us to enforce RBAC on the
application itself, separately from the sub-resources related to it.

See also GitHub argoproj#18124, argoproj#20600
fffinkel added a commit to fffinkel/argo-cd that referenced this issue Nov 5, 2024
A breaking change was introduced in a previous commit that is planned to
be a part of the next major version of Argo CD (v3) where it's okay to
introduce breaking changes. We want this feature before we hit v3, so
we add a config setting that allows us to explicitly turn this new v3
behavior on in v2. The current v2 behavior is the default, so this
change will not affect folks who do not explicitly opt in.

GitHub argoproj#19988, argoproj#20600
fffinkel added a commit to fffinkel/argo-cd that referenced this issue Nov 5, 2024
…oj#19988)

Change applications resource RBAC to use fine-grained update/delete
enforcement by default. This allows us to enforce RBAC on the
application itself, separately from the sub-resources related to it.

(see also argoproj#18124, argoproj#20600)
fffinkel added a commit to fffinkel/argo-cd that referenced this issue Nov 5, 2024
A breaking change was introduced in a previous commit that is planned to
be a part of the next major version of Argo CD (v3) where it's okay to
introduce breaking changes. We want this feature before we hit v3, so
we add a config setting that allows us to explicitly turn this new v3
behavior on in v2. The current v2 behavior is the default, so this
change will not affect folks who do not explicitly opt in.

This commit to add the gating code is added separately so it will be
easy to either cherry pick that pervious commit or revert this one.

(see also argoproj#18124, argoproj#20600)
fffinkel added a commit to fffinkel/argo-cd that referenced this issue Nov 5, 2024
…oj#19988)

Change applications resource RBAC to use fine-grained update/delete
enforcement by default. This allows us to enforce RBAC on the
application itself, separately from the sub-resources related to it.

(see also argoproj#18124, argoproj#20600)

Signed-off-by: Matt Finkel <[email protected]>
fffinkel added a commit to fffinkel/argo-cd that referenced this issue Nov 5, 2024
A breaking change was introduced in a previous commit that is planned to
be a part of the next major version of Argo CD (v3) where it's okay to
introduce breaking changes. We want this feature before we hit v3, so
we add a config setting that allows us to explicitly turn this new v3
behavior on in v2. The current v2 behavior is the default, so this
change will not affect folks who do not explicitly opt in.

This commit to add the gating code is added separately so it will be
easy to either cherry pick that pervious commit or revert this one.

(see also argoproj#18124, argoproj#20600)

Signed-off-by: Matt Finkel <[email protected]>
@fffinkel fffinkel linked a pull request Nov 5, 2024 that will close this issue
14 tasks
fffinkel added a commit to fffinkel/argo-cd that referenced this issue Nov 5, 2024
…oj#19988)

Change applications resource RBAC to use fine-grained update/delete
enforcement by default. This allows us to enforce RBAC on the
application itself, separately from the sub-resources related to it.

(see also argoproj#18124, argoproj#20600)

Signed-off-by: Matt Finkel <[email protected]>
fffinkel added a commit to fffinkel/argo-cd that referenced this issue Nov 5, 2024
A breaking change was introduced in a previous commit that is planned to
be a part of the next major version of Argo CD (v3) where it's okay to
introduce breaking changes. We want this feature before we hit v3, so
we add a config setting that allows us to explicitly turn this new v3
behavior on in v2. The current v2 behavior is the default, so this
change will not affect folks who do not explicitly opt in.

This commit to add the gating code is added separately so it will be
easy to either cherry pick that pervious commit or revert this one.

(see also argoproj#18124, argoproj#19988)

Signed-off-by: Matt Finkel <[email protected]>
fffinkel added a commit to fffinkel/argo-cd that referenced this issue Nov 5, 2024
A breaking change was introduced in a previous commit that is planned to
be a part of the next major version of Argo CD (v3) where it's okay to
introduce breaking changes. We want this feature before we hit v3, so
we add a config setting that allows us to explicitly turn this new v3
behavior on in v2. The current v2 behavior is the default, so this
change will not affect folks who do not explicitly opt in.

This commit to add the gating code is added separately so it will be
easy to either cherry pick that pervious commit or revert this one.

(see also argoproj#18124, argoproj#19988)

Signed-off-by: Matt Finkel <[email protected]>
fffinkel added a commit to fffinkel/argo-cd that referenced this issue Nov 5, 2024
…oj#19988)

Change applications resource RBAC to use fine-grained update/delete
enforcement by default. This allows us to enforce RBAC on the
application itself, separately from the sub-resources related to it.

(see also argoproj#18124, argoproj#20600)

Signed-off-by: Matt Finkel <[email protected]>
fffinkel added a commit to fffinkel/argo-cd that referenced this issue Nov 5, 2024
A breaking change was introduced in a previous commit that is planned to
be a part of the next major version of Argo CD (v3) where it's okay to
introduce breaking changes. We want this feature before we hit v3, so
we add a config setting that allows us to explicitly turn this new v3
behavior on in v2. The current v2 behavior is the default, so this
change will not affect folks who do not explicitly opt in.

This commit to add the gating code is added separately so it will be
easy to either cherry pick that pervious commit or revert this one.

(see also argoproj#18124, argoproj#19988)

Signed-off-by: Matt Finkel <[email protected]>
@fffinkel fffinkel linked a pull request Nov 5, 2024 that will close this issue
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:application-controller component:rbac Issues related to Openshift and Racher enhancement New feature or request
Projects
None yet
3 participants