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

feat: (PoC) Applications outside argocd namespace #6537

Closed
wants to merge 5 commits into from

Conversation

jannfis
Copy link
Member

@jannfis jannfis commented Jun 23, 2021

PoC for apps outside argocd namespace

Description

This PR is a PoC to prove that the proposal created at #6409 actually can be implemented with changes that are not too intrusive.

Despite the PR looks big (>=48 files changes), most of the changes are about how things (e.g. application names) are referenced (e.g. application name). There's not much changes to complexity imho.

This shall not be merged. It serves merely as a demonstration.

Major changes

Applications can now be created and managed in every namespace

The limitation is that it is a namespace on the Argo CD's control plane cluster. Application controller and API server must have access to these namespaces.

AppProject restricts the namespaces

The AppProject CRD has been extended by .spec.sourceNamespaces, which is a list of namespaces where Applications may exist and are allowed to associate to the AppProject.

If an Application is created in namespace some-ns and sets .spec.project to some-project, then some-project must specify some-ns in the .spec.sourceNamespaces list. This is a safeguard against applications associating to any project to elevate their privileges.

If the installation namespace of the Application is not in the .spec.sourceNamespaces list of the AppProject it references, Application resource still may be created, but the application controller will not reconcile it.

To keep backward compatibility, the Argo CD control plane's namespace (e.g. argocd) is implicitly allowed and does not need to be specified in .spec.sourceNamespaces, because it is assumed that only Argo CD administrators have access to this namespace (and also can control AppProject resources).

Referencing Applications on the CLI and API

The CLI and API have been changed to accept application names in the format <namespace>/<name> to allow working with applications in different namespaces. For backwards compatibility Applications in the control plane's namespace (e.g. argocd), can be referenced in a non namespaced way (e.g. just <name>).

For example, consider the following two applications:

  • some-app in the argocd namespace
  • some-app in the other-ns namespace.

Both applications have the same name (some-app) but can be referenced to independently:

  • argocd app get some-app and argocd app get argocd/some-app are semantically the same and would retrieve information about the application some-app in the argocd namespace
  • argocd app get other-ns/some-app would retrieve information about the application some-app in the other-ns namespace

This syntax is valid for all CLI commands that work with Application resources except argocd app create. To create an application using the CLI, the --app-namespace parameter can be specified to denote in which namespace the Application resource should be created:

argocd app create some-app --app-namespace other-ns ...

Also, if argocd app create is used in conjunction with the -f parameter to create the application from a manifest, the .metadata.namespace value in the manifest is considered for being the namespace in which to create the application.

Change of instance label value on resources

In order to uniquely identify applications with the same name in different namespaces, the value of the instance label (app.kubernetes.io/instance) has been changed from <appname> to <namespace>_<appName>. As a consequence, in this PoC the length of <namespace> and <appName> combined may not exceed 62 characters (plus one for the underscore).

This may be overcome with changes as proposed in #6425.

Also, if you test this change with an existing Argo CD installation, all of your Applications will become out of sync and need to be re-synced to reflect the new value.

Unsupported in this PoC

Web UI

The web UI has not seen any changes and also was not tested. Most likely, it will not work.

Documentation

Nothing has been documented yet ¯_(ツ)_/¯

Limiting the namespaces to watch

Currently, all namespaces are being watched on the cluster for Application resources and there's no way to limit that. This may change in a future incarnation of this change.

Insufficient permissions for API server

I have not included any manifest changes. If you build an image from this, and install to a cluster, you may give argocd-server additional permissions to applications.argoproj.io in all namespaces.

Testing

End-to-End tests

There are no dedicated end-to-end tests yet for this. However, current end-to-end tests run successfully.

The PoC has adapted the end-to-end test suite to also run the existing end-to-end tests with all Application resources installed to a dedicated namespace different than the control plane's namespace. In order to run these tests with Applications managed in another namespace, do the following:

  1. Start end-to-end test server components (Argo CD will use argocd-e2e namespace for control plane):

    make start-e2e
  2. In another shell, create dedicated namespace for Application resources and run the tests, instructing the test framework to use argocd-e2e-src as application namespace:

    kubectl create ns argocd-e2e-src
    make test-e2e ARGOCD_E2E_K3S=true ARGOCD_E2E_APP_NAMESPACE=argocd-e2e-src

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

jannfis commented Jun 23, 2021

FYI @alexmt @jessesuen @sbose78

jannfis added 3 commits June 23, 2021 12:08
Signed-off-by: jannfis <jann@mistrust.net>
Signed-off-by: jannfis <jann@mistrust.net>
Signed-off-by: jannfis <jann@mistrust.net>
@codecov
Copy link

codecov bot commented Jun 23, 2021

Codecov Report

Merging #6537 (1ba8f47) into master (2541fa0) will increase coverage by 0.02%.
The diff coverage is 52.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6537      +/-   ##
==========================================
+ Coverage   41.11%   41.13%   +0.02%     
==========================================
  Files         153      153              
  Lines       20304    20378      +74     
==========================================
+ Hits         8348     8383      +35     
- Misses      10798    10829      +31     
- Partials     1158     1166       +8     
Impacted Files Coverage Δ
cmd/argocd/commands/app.go 0.00% <0.00%> (ø)
controller/cache/cache.go 10.74% <0.00%> (ø)
pkg/apis/application/v1alpha1/types.go 57.71% <0.00%> (-0.18%) ⬇️
util/argo/argo.go 59.63% <14.28%> (-2.12%) ⬇️
cmd/util/app.go 35.79% <33.33%> (-0.03%) ⬇️
server/application/application.go 32.81% <50.00%> (+0.04%) ⬆️
server/server.go 54.74% <50.00%> (-0.22%) ⬇️
reposerver/repository/repository.go 60.86% <57.14%> (-0.14%) ⬇️
controller/state.go 67.69% <71.42%> (ø)
controller/appcontroller.go 53.49% <77.08%> (+0.22%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2541fa0...1ba8f47. Read the comment docs.

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

sbose78 commented Jun 24, 2021

"The limitation is that it is a namespace on the Argo CD's control plane cluster. Application controller and API server must have access to these namespaces"

I wouldn't call it a limitation, rather it's a constraint.

@Ziphone
Copy link

Ziphone commented Aug 31, 2021

What is the status here, @jannfis? Do you consider implementing something similar soon?

@jannfis
Copy link
Member Author

jannfis commented Sep 2, 2021

@Ziphone My plan is to bring the PoC into a ready state and have it integrated with Argo CD. But we had some discussions about this, and had no final conclusion yet.

/cc @alexmt @jessesuen

@jannfis
Copy link
Member Author

jannfis commented Jun 22, 2022

This is superseded by #9755

@jannfis jannfis closed this Jun 22, 2022
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.

3 participants