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

proposal: Applications outside argocd namespace #6409

Merged
merged 4 commits into from Aug 8, 2022

Conversation

jannfis
Copy link
Member

@jannfis jannfis commented Jun 5, 2021

This is a proposal to enable Application resources to exist outside the Argo CD control plane's namespace.

Should be considered an alternative to #6405

Related issues:

Signed-off-by: jannfis jann@mistrust.net

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

Signed-off-by: jannfis <jann@mistrust.net>
@codecov
Copy link

codecov bot commented Jun 5, 2021

Codecov Report

Merging #6409 (a50f20d) into master (c0f16fb) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6409   +/-   ##
=======================================
  Coverage   45.98%   45.98%           
=======================================
  Files         226      226           
  Lines       27413    27413           
=======================================
  Hits        12605    12605           
  Misses      13094    13094           
  Partials     1714     1714           
Impacted Files Coverage Δ
util/settings/settings.go 51.42% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: jannfis <jann@mistrust.net>
## Open Questions [optional]

* The major open question is, how to name `Application`s in a scenario where
the K8s resource's name isn't unique anymore.
Copy link
Contributor

Choose a reason for hiding this comment

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

K8s resource's name isn't unique anymore.

Is this the resource that the user is deploying from her git repo?

Copy link
Member Author

@jannfis jannfis Jun 17, 2021

Choose a reason for hiding this comment

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

This means the name of the Application resource, whether created from Git or in another way. Where as before uniqueness was ensured by Kubernetes because no two Applications could share the same name in the same namespace, now possibly an application named foo could exist in namespace ns-foo, and another application named foo could exist in namespace ns-bar.

@sbose78
Copy link
Contributor

sbose78 commented Jun 17, 2021

I've gone through the whole proposal and it looks like a fairly simple enhancement to the existing paradigm!

@jannfis
Copy link
Member Author

jannfis commented Sep 2, 2021

@jessesuen @alexmt Can we please revisit this topic? I think it'd be an important feature to have.

@tiagomeireles
Copy link

FWIW I really look forward to seeing this ship.

@bloodsper
Copy link
Contributor

looking for this feature

it makes "apps of apps" such a mess if want to have tenant namespaces but having the application definition elsewhere from the actual deployments

@m-yosefpor
Copy link
Contributor

Thanks. This is indeed a great feature which we are looking for. Right now we are trying to do a similar thing with a ton of hacks:

We have written a k8s controller to watch for Application CRs in all namespaces except for argocd ns, and create a new corresponding app CR in argocd ns (using parts of uid for avoiding duplication). Also we have written a validation webhook to enforce and reject App CRs created in other ns, by checking and validating the .spec.project of apps based on a a label on namespaces which identifies the argo project, e.g. ns1 with the label argo.snappcloud.io/project: proj1 will cause creating any Application CR with .spec.project != proj1 in ns1 namespace to be rejected.

field to associate themselves to the `AppProject` named `some-project`.
In the above example, the Application `some-app` would be allowed to associate
to the AppProject `some-project`, but the Application `other-app` would be
invalid.
Copy link
Contributor

@m-yosefpor m-yosefpor Nov 5, 2021

Choose a reason for hiding this comment

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

One thing that is not clear in this section is that how do you define invalid? In the implementation details you have mentioned aborting reconcile, but it is not clear to users and is only discoverable in the controller logs. Should it be rejecting via a validation webhook? or letting the creation to happen, and introduce a new status for Application as Invalid and maybe also have some events?

I think the former might be more clear and less error prone for users, especially as rejection can happen with a clear message, while the latter requires users to notice that the app status is invalid and search for the reason in the events or in .status.description.

Also it is unclear in this proposal how updates to appProj is handled (e.g. when we revoke acccess with removing some soureNamespaces)? I think we should immediately recheck all affected applications (all apps created in sourceNamespaces section of updated appproj) and update their status to invalid if required.

The downside of validationwebhook is that, when an app is created in a ns which initially was allowed in sourceNamespaces, we can not notify users that it is no longer valid, and the reconcile will only fail and might confuse them.

Copy link
Member Author

Choose a reason for hiding this comment

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

One thing that is not clear in this section is that how do you define invalid? In the implementation details you have mentioned aborting reconcile, but it is not clear to users and is only discoverable in the controller logs.

This would be reflected in the Application's .status field, similar to how a failed sync is reflected right now. Since it's the application controller who is updating the .status of an Application, it's also the application controller who will be enforcing whether an application will be reconciled or not. This check is happening constantly before reconciling any app, so when you remove a source namespace from the AppProject, it would not be allowed to sync anymore.

```

* `Applications` created imperatively (e.g. through Argo CD API via UI or
CLI) will keep being created in the control plane namespace, and Argo CD
Copy link
Contributor

Choose a reason for hiding this comment

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

So what if users define apps in new ns, in this case they can create with kubectl -f file.yaml and they are not able to create the same manifest with argocd app create -f file.yaml right? wouldn't it be confusing?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the corresponding command would then be argocd app create -n <namespace> -f file.yaml (or specfiying the target namespace in the YAML). The API would then show this application as <namespace>/<appname> instead of just <appname>.

of the second application would become `barns/some-app`. This would have the
advantage to instantly have a clue about _where_ the application is coming from,
without having to resort to the Kubernetes API to inspect `.spec.namespace`
field of the app. The notation may also use a hyphen (`-`) or underscore (`_`)
Copy link
Contributor

Choose a reason for hiding this comment

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

a - should not be used as it is allowed in k8s names. See the two following conflicting examples:

name: hello
namespace: hi-all
---
name: hello-hi
namespace: all

both will be hello-hi-all if separator be a dash.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Thinking about this for a while :) Will come back to it later.

@jannfis
Copy link
Member Author

jannfis commented Nov 12, 2021

Thanks @m-yosefpor for reviewing and picking up the discussion.

Copy link
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

LGTM

We've been going back and forth on this proposal for many months. Recently all maintainers agreed this should be implemented.

I think we should merge proposal,start implementing it and handle implementation details as we go.

@jessesuen , @jannfis , @wtam2018 let me know if you have any objections please.

@m-yosefpor
Copy link
Contributor

LGTM

We've been going back and forth on this proposal for many months. Recently all maintainers agreed this should be implemented.

I think we should merge proposal,start implementing it and handle implementation details as we go.

@jessesuen , @jannfis , @wtam2018 let me know if you have any objections please.

I have commented some notes on this proposal. @jannfis has replied them with valid answers. However I think it would be great if those answers are somehow mentioned in the proposal as well.

Also I think the hyphen separator mentioned in #6409 (comment) should be deleted due to described duplication error.

@mkaiserincomm
Copy link

Will RBAC be addressed? When I create an ArgoCD Application via the UI, I am constrained by the RBAC rules effecting my user. If I create an ArgoCD application by creating a YAML for an Application object and applying, it will not know my ArgoCD user and I suspect will not enforce RBAC. My use case is that multiple teams share a cluster. I want teams to be able to use the App of Apps pattern, but I want them to be restricted into only creating applications within the projects their RBAC allows and only deploying to clusters which their RBAC allows.

@Avni-Sharma
Copy link

Hi all, Was just curious and wanted to know where are we on this?

@jannfis jannfis mentioned this pull request Jun 22, 2022
6 tasks
@Paulo-B
Copy link

Paulo-B commented Jul 12, 2022

Hello, any news when this feature will be released?

@crenshaw-dev
Copy link
Collaborator

crenshaw-dev commented Jul 13, 2022

@Paulo-B it's expected for 2.5 in August. The PR is under review.

@FireDrunk
Copy link

@jannfis I very much agree with the concerns of @mkaiserincomm. There is a significant risk involved if people can create Applications linking to any project in their own namespace.
This would grant them permissions by-proxy that they should not have according to the RBAC rules.

I think that some explicit configuration is required to allow this.
One idea would be to only allow Applications in namespaces that are explicitly linked to Projects that explicitly contain configuration to read Application objects from that namespace.

For example:

apiVersion: argoproj.io/v1alpha1
kind: AppProject
metadata:
  name: cicd
  finalizers:
    - resources-finalizer.argocd.argoproj.io
spec:
  description: Project CICD owned by Team CICD

  destinations:
    - namespace: 'cicd'
      allowApplications: true
      server: 'https://kubernetes.default.svc'
    - namespace: 'cicd-*'
      allowApplications: false #default
      server: 'https://kubernetes.default.svc'

  sourceRepos:
    - '*'

This configuration would trigger ArgoCD to read any Application CRD's from the cicd namespace, and explicitly 'parent' them into the CICD project regardless of the project property in the Application Resource.

@jannfis
Copy link
Member Author

jannfis commented Jul 28, 2022

@FireDrunk This problem is solved with the new sourceNamespaces property in the AppProject. This defines a list of namespaces where Applications are allowed to exist in order to associate to the given project.

For example, with an AppProject foobar that has

spec:
  sourceNamespaces:
  - ns-foo
  - ns-bar

Then an Application created in either namespace ns-foo or ns-bar may set their own .spec.project to foobar, thus associating to the project. If an Application is created in namespace ns-baz and has .spec.project set to foobar, the application controller will not reconcile this application.

@FireDrunk
Copy link

@FireDrunk This problem is solved with the new sourceNamespaces property in the AppProject. This defines a list of namespaces where Applications are allowed to exist in order to associate to the given project.

For example, with an AppProject foobar that has

spec:
  sourceNamespaces:
  - ns-foo
  - ns-bar

Then an Application created in either namespace ns-foo or ns-bar may set their own .spec.project to foobar, thus associating to the project. If an Application is created in namespace ns-baz and has .spec.project set to foobar, the application controller will not reconcile this application.

That looks like a perfect alternative! I would be happy to test this functionality, since we are anxiously awaiting these kinds of improvements!

Is there any kind of 'event' or notification stream or any kind of monitoring solution to make it easy to find these kinds of mishaps?

@crenshaw-dev
Copy link
Collaborator

(The PR for testing if you want @FireDrunk :-) #9755)

@jannfis jannfis force-pushed the proposal/applications-namespaces branch from 3014451 to a50f20d Compare August 8, 2022 08:50
@jannfis jannfis merged commit ca08f4d into argoproj:master Aug 8, 2022
@jannfis
Copy link
Member Author

jannfis commented Aug 8, 2022

The PR #9755 implementing this proposal will be merged soon eventually.

ashutosh16 pushed a commit to ashutosh16/argo-cd that referenced this pull request Aug 11, 2022
* proposal: Applications outside argocd namespace

Signed-off-by: jannfis <jann@mistrust.net>

* Typos and add another use-case

Signed-off-by: jannfis <jann@mistrust.net>

* Update 003-applications-outside-argocd-namespace.md

Signed-off-by: jannfis <jann@mistrust.net>
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.

None yet