feat: Applications in any namespace #9755
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9755 +/- ##
==========================================
- Coverage 46.18% 46.08% -0.10%
==========================================
Files 226 228 +2
Lines 27581 27858 +277
==========================================
+ Hits 12737 12839 +102
- Misses 13124 13282 +158
- Partials 1720 1737 +17
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
jannfis
force-pushed
the
feat/apps-in-any-namespace
branch
from
9e0a02c
to
b16bfb1
Compare
|
For the tracking label thing for other namespace, maybe mutating webhook can be a better solution, means you don’t need to make any change in existing logic. Especially we use CRDs in such case |
| appName := syncReq.GetName() | ||
| appNs := s.appNamespaceOrDefault(syncReq.GetAppNamespace()) | ||
| appIf := s.appclientset.ArgoprojV1alpha1().Applications(appNs) | ||
| a, err := appIf.Get(ctx, appName, metav1.GetOptions{}) |
There was a problem hiding this comment.
Should we do an "is allowed namespace" check first?
There was a problem hiding this comment.
Maybe "superficial check" as described above.
| appName := rollbackReq.GetName() | ||
| appNs := s.appNamespaceOrDefault(rollbackReq.GetAppNamespace()) | ||
| appIf := s.appclientset.ArgoprojV1alpha1().Applications(appNs) | ||
| a, err := appIf.Get(ctx, appName, metav1.GetOptions{}) |
There was a problem hiding this comment.
Should we do an "is allowed namespace" check first?
There was a problem hiding this comment.
Maybe "superficial check" as described above.
| a, err := s.appclientset.ArgoprojV1alpha1().Applications(s.ns).Get(ctx, *termOpReq.Name, metav1.GetOptions{}) | ||
| appName := termOpReq.GetName() | ||
| appNs := s.appNamespaceOrDefault(termOpReq.GetAppNamespace()) | ||
| a, err := s.appclientset.ArgoprojV1alpha1().Applications(appNs).Get(ctx, appName, metav1.GetOptions{}) |
There was a problem hiding this comment.
Should we do an "is allowed namespace" check first?
There was a problem hiding this comment.
Maybe "superficial check" as described above.
| appName := q.GetName() | ||
| appNs := s.appNamespaceOrDefault(q.GetAppNamespace()) | ||
| appIf := s.appclientset.ArgoprojV1alpha1().Applications(appNs) | ||
| a, err := appIf.Get(ctx, appName, metav1.GetOptions{}) |
There was a problem hiding this comment.
Should we do an "is allowed namespace" check first?
There was a problem hiding this comment.
Maybe "superficial check" as described above.
|
Curious about how this affects other resources (when declared outside of the control plane namespace):
|
Access to clusters and repositories is governed by the AppProject, see above. This PR does not change the scope of secrets for cluster and repositories.
Eventually, users could launch their own ApplicationSet controller(s) in any of the namespaces they're allowed to to generate |
I would expect namespaced AppProjects to have similar restrictions to namespaced apps. i.e., they can only be assigned to applications in the same namespace, can only access secrets from that namespace, etc. So there should be no way to escalate privileges beyond the namespace. Is there any reason that would not work? (if out of scope for this PR, I can understand that.) |
Currently, the AppProject restricts - among other things - what kind of resources (and their scope, i.e. cluster scoped or namespace scoped) an application is allowed to deploy, and where it is allowed to deploy (clusters, and namespaces in those clusters). IMHO, this control must not be given to the user. I'm not sure how we could properly design a governance model around this. But if you have ideas, I'm all ears :) |
|
@crenshaw-dev Thanks for the review! I have addressed almost all of your comments. For the RBAC change (which has not yet been made), I was twisting my head. I see two possible options to extend the RBAC syntax to address applications:
I think option 1 would be the one to go for, as it provides the least amount of surprise. However, there are few problems due to the fact that our glob matcher doesn't treat the forward-slash as separator. So, We could introduce a slightly breaking change here and provide Maybe you have some thoughts on all that? |
| @@ -180,6 +181,7 @@ func NewApplicationCreateCommand(clientOpts *argocdclient.ClientOptions) *cobra. | |||
| if err != nil { | |||
| log.Fatal(err) | |||
| } | |||
| command.Flags().StringVarP(&appNamespace, "app-namespace", "N", "", "Namespace where the application will be created in") | |||
There was a problem hiding this comment.
Will/should this default to "wherever the controller lives"?
cmd/argocd/commands/app.go
Outdated
| errors.CheckError(err) | ||
| resources, err := appIf.ManagedResources(ctx, &applicationpkg.ResourcesQuery{ApplicationName: &appName}) | ||
| resources, err := appIf.ManagedResources(context.Background(), &applicationpkg.ResourcesQuery{ApplicationName: &appName, AppNamespace: &appNs}) |
There was a problem hiding this comment.
Any reason not to stick with ctx?
| @@ -911,7 +925,7 @@ func findandPrintDiff(ctx context.Context, app *argoappv1.Application, resources | |||
| unstructureds = append(unstructureds, obj) | |||
| } | |||
| groupedObjs := groupObjsByKey(unstructureds, liveObjs, app.Spec.Destination.Namespace) | |||
| items = groupObjsForDiff(resources, groupedObjs, items, argoSettings, appName) | |||
| items = groupObjsForDiff(resources, groupedObjs, items, argoSettings, app.Name) | |||
There was a problem hiding this comment.
Why InstanceName above but just Name here?
| @@ -1715,12 +1748,18 @@ func waitOnApplicationStatus(ctx context.Context, acdClient argocdclient.Client, | |||
| // time when the sync status lags behind when an operation completes | |||
| refresh := false | |||
|
|
|||
| appRealName, appNs := argo.ParseAppQualifiedName(appName, "") | |||
There was a problem hiding this comment.
appRealName is kind of an odd name. What about renaming the func param to appQualifiedName and this to appName for consistency with other code?
| // read arguments | ||
| if len(args) == 1 { | ||
| if appName != "" && appName != args[0] { | ||
| return nil, fmt.Errorf("--name argument '%s' does not match app name %s", appName, args[0]) | ||
| } | ||
| appName = args[0] | ||
| } | ||
| appName, appNs := argo.ParseAppQualifiedName(appName, "") |
There was a problem hiding this comment.
Maybe renamed the func param appQualifiedName for readability?
| appName := rollbackReq.GetName() | ||
| appNs := s.appNamespaceOrDefault(rollbackReq.GetAppNamespace()) | ||
| appIf := s.appclientset.ArgoprojV1alpha1().Applications(appNs) | ||
| a, err := appIf.Get(ctx, appName, metav1.GetOptions{}) |
There was a problem hiding this comment.
Maybe "superficial check" as described above.
| a, err := s.appclientset.ArgoprojV1alpha1().Applications(s.ns).Get(ctx, *termOpReq.Name, metav1.GetOptions{}) | ||
| appName := termOpReq.GetName() | ||
| appNs := s.appNamespaceOrDefault(termOpReq.GetAppNamespace()) | ||
| a, err := s.appclientset.ArgoprojV1alpha1().Applications(appNs).Get(ctx, appName, metav1.GetOptions{}) |
There was a problem hiding this comment.
Maybe "superficial check" as described above.
| appName := q.GetName() | ||
| appNs := s.appNamespaceOrDefault(q.GetAppNamespace()) | ||
| appIf := s.appclientset.ArgoprojV1alpha1().Applications(appNs) | ||
| a, err := appIf.Get(ctx, appName, metav1.GetOptions{}) |
There was a problem hiding this comment.
Maybe "superficial check" as described above.
| return nil, err | ||
| } | ||
| if !proj.IsAppNamespacePermitted(app, ns) { | ||
| return nil, fmt.Errorf("application '%s' in namespace '%s' is not allowed to use project '%s'", |
There was a problem hiding this comment.
Maybe add ", because the project does not allow applications in that namespace" or something to clarify why it's disallowed.
I like option 1. Considering the change without adding the separator:
Since the admin has to opt in to adding namespaces, I think we can just add notes to the docs about how RBAC matching works. The behavior above seems fairly intuitive. Am I correct that |
I also agree with option 1. I think |
This part I'm less certain about, because I think it would break anything with fewer than two slashes (i.e. We could auto-upgrade those patterns on the backend:
But there are some patterns which would be trickier to upgrade:
For typical patterns, I think auto-upgrade is easy. For atypical patterns, I think it's a pretty big challenge. @jannfis what's the RBAC pattern for an Application in the argocd namespace (regardless of whether we add the separator)? |
|
We could
|
|
Can I test this with a specific image tag instead of manually compiling the source? |
|
@FireDrunk I think you'll have to compile. I don't believe images in PRs are pushed anywhere. But the |
Signed-off-by: jannfis <jann@mistrust.net>
Signed-off-by: jannfis <jann@mistrust.net>
Signed-off-by: jannfis <jann@mistrust.net>
Signed-off-by: jannfis <jann@mistrust.net>
Signed-off-by: jannfis <jann@mistrust.net>
Signed-off-by: jannfis <jann@mistrust.net>
Signed-off-by: jannfis <jann@mistrust.net>
Signed-off-by: jannfis <jann@mistrust.net>
Signed-off-by: jannfis <jann@mistrust.net>
Signed-off-by: jannfis <jann@mistrust.net>
@jannfis: I currently testing out the new feature. I am hitting the same issue like described by @FireDrunk, I don't understand this. I mean I could extend the ClusterRoleBinding to an additional namespace. But I am asking about the sense. I am using 2.5.1 migrated from 2.4.11 |
|
Problem solved, a restart of the argocd-server pod did the trick. |
|
How can one disable this functionality? Any chance of having a Helm chart option to disable/enable? |
1 similar comment
|
How can one disable this functionality? Any chance of having a Helm chart option to disable/enable? |
|
@pentago this feature is opt-in. If you haven’t configured additional application namespaces, the feature is disabled. |
|
I ask this because in UI on app cards I see argocd/appname and its a bit confusing. If feature is disabled, the app name in web UI should be just appname instead of argocd/appname. Any chance of making it happen? |
|
@pentago ah yes that’s annoying. The fix has been merged, I hope to cut 2.5.3 with that fix Monday morning. |
|
Was wondering if it is possible to support declarative repository connections as secrets in other namespaces with this feature? More info here |
Apps in any Namespace
Description
This change enables Application resources to exist in any namespace allowed by configuration.
The feature is not enabled by default, and has to be explicitly enabled by the administrator (see below).
The change aims to be fully backwards compatible.
This is a rather large change. It comprises changes to the controller's reconciliation logic, the API server as well as changes to the CLI and the UI. I try to outline the changes and design decisions best I can in the below description.
This PR implements proposal #6409 and supersedes the PoC PR #6537
Some terminology
Status
This change is beta-quality and could require some additional testing and eyes.
My suggestion is to merge it early and keep polishing it until 2.5 release. If at time of 2.5 release there still are some known issues, we make sure to document them properly.
Things that work
Current end-to-end test suite runs without any changes being made to the tests. The changes to the e2e test framework are done to support testing applications in namespaces other than the control plane's one.
Initial set of e2e tests with applications in a different namespace have been added (not yet complete)
UI support is not fully finished yet (most things work, needs fine tuning)
CLI support is near complete (end-to-end tests are passing)
Documentation is not yet written, as there may be additional changes derived from the feedback cycles
Known issues
application.resourceTrackingMethodis set toannotation. I'm not yet sure whether I introduced this bug, or if it's a general bug with the tracking method. Edit: This is indeed a bug with the tracking method, not introduced through this change (reproduced it on code from master branch) - will open separate issue for that. Edit 2: Issue for this Argo CD loses track of managed live resource when switching tracking method #9781Things to do
Breaking changes
Change details
Enabling the feature
By default, the feature is disabled. Administrators have to explicitly enable it by giving the
--application-namespacesparameter to the workloads ofargocd-serverandargocd-application-controller.Open questions:
argocd-cmrather than a command line parameter, so list of namespaces can be changed at runtime?Allowed namespaces
The
--application-namespacesparameter takes a comma-separated list of allowed namespaces as glob patterns. The pattern*enables all namespaces on the control plane's cluster. The control plane's namespace (usuallyargocd) is always enabled. It cannot be disabled.For example, setting
--application-namespaces app-team1-*,app-team2-*would allow the controller and the server to handle applications in namespaces starting withapp-team1-andapp-team2-, but not in the namespaceapp-team-3.When the feature is enabled, controller and server are switched from namespaced mode (i.e. "get and watch resources only from the control plane's namespace") to all namespaces mode. One reason is, that to support glob patterns, we don't know the namespaces to operate on at initialisation time, and second we don't want to keep a separate lister or informer for each allowed namespace due to compute and complexity constraints. Instead, we check for enabled namespaces when using the lister.
Only namespaces in the control plane's cluster can host Applications. Hosting
Applicationresources in remote clusters are out of scope for now.If there are multiple instances of Argo CD installed to the same cluster, the behavior when more than one instance specifies same allowed namespace patterns (and have access to those namespaces) is undefined. This scenario should be avoided (needs to be documented).
Application names
Applications will now be referenced to as
namespace/namein the CLI and the UI. Ifnamespaceis omitted, the control plane's namespace (argocdin most installations) will be assumed. That means when Argo CD is installed in the namespaceargocd, the application namesargocd/guestbookandguestbookare semantically the same. When Argo CD is installed in the namespacegitops, likewise the application namesgitops/guestbookandguestbookare semantically the same.A similar syntactic change has been made for the UI. For example, both of the following URLs would now refer to the same application in the UI:
and
For the API, all relevant endpoints now offer the (optional) parameter
applicationNamespaceto specify the source namespace of the Application. If this parameter is not set, the control plane's namespace will be assumed.For example, to retrieve the application
guestbookin the namespacefoovia the REST API, the following call can be madeThe reason for implementing this as query parameter instead of in the path is actually a limitation of grpc-gateway, which does not allow two paths to the same resource.
These changes to referencing also mean that now there may be two or more applications with the same name of
guestbook, when all of them exist in a different namespace. For example:Would all refer to an application named
guestbook, but each of them would be in different namespaces and are unique applications.The tracking label
For applications in the control plane's namespace, nothing changes. That means that the application
guestbookin theargocdnamespace will still be using the valueguestbookin the tracking label. This keeps backwards compatibility and ensures that no resync of resources has to happen when a version containing this change is deployed, or when the feature is being enabled.For applications in other namespaces, the value of the tracking label will be a combination of the namespace and the application name, concatenated with the underscore character. That means the application
guestbookin the namespacefoowill usefoo_guestbookas the tracking label's value. The underscore character was chosen for being unambigious in this context, as it is not allowed in Kubernetes resource name.Because this can lead to values that exceed the maximum length for label values, it is recommended to use annotation method for tracking resources. When the string combined of namespace and application name exceeds the 63 characters limit for label values, Kubernetes throws an error when trying to manage resources of such applications with Argo CD using the
labeltracking method.Project association for applications
AppProjects govern much of the permissions that are granted to an Application. For this reason, Argo CD API RBAC ensures that users can only associate to projects that they are allowed to. A typical Argo CD user has no access to the control plane's namespace in Kubernetes, and is only allowed to create applications using the API, where RBAC is enforced.
The goal of this change is to allow ordinary Argo CD users to manage their Applications in a declarative way, in their own namespaces on the control plane's cluster. But in the declarative way, an Application may associate to any AppProject by just setting the field
.spec.projectto the name of the appropriate project. This might allow users to escalate their privileges on the cluster.In order to prevent this privilege escalation, an Argo CD administrator will have to configure the names of allowed namespaces in the project. For this, a new field has been introduced in the AppProject's spec:
.spec.sourceNamespaces. An Application wanting to associate to a given project must exist in one of the namespaces defined in this field.As an example, when
.spec.sourceNamespacesfor projectprojis set tofoo, the Applicationfoo/appis allowed to associate to projectproj, but the applicationbar/appis not.The
.spec.sourceNamespacesfield supports pattern matching, i.e.would allow
Applicationsin theteam1-sourcenamespace as well as in any namespace starting withteam2-to associate with the project.The check for a namespace to be allowed to associate to a project has been made an integral part of the controller's reconciliation logic. Administrators can revoke the permissions at any time by removing the namespace from the
sourceNamespacesfield of theAppProject, and the controller will then enforce it. When the controller finds an Application with a project association that is not allowed, it will refuse to perform any reconciliation activity.For backwards compatibility, Applications in the control plane's namespace are allowed to associate to any project.
By default, no other namespaces are allowed.
How to test manually
Set
ARGOCD_APPLICATION_NAMESPACESto a list of namespaces that should be allowed by API server and controller and runmake startormake start-local, e.g:ARGOCD_APPLICATION_NAMESPACES="argocd-*" make startCreate a new
AppProjectwhich has.spec.sourceNamespacesset to some allowed namespaces for that project (or modify thedefaultproject).Create a new app in a different namespace, e.g.
argocd app create argocd-test/test-app ...Manage this app using the CLI or the UI
Try different things - different namespaces (including ones that are not allowed, etc), app deletion, declarative app creation in allowed namespaces and not allowed namespaces, etc
Open questions
Closes #3474