ROX-26039: Create tenant argoCd app #2009

ludydoo · 2024-09-02T17:36:41Z

Description

Adds the support for deploying tenant-resources through ArgoCD rather than the built-in tenant-resources chart. The CentralReconciler will

Create an ArgoCD application if the flag is enabled for a tenant
Delete the ArgoCD application if the flag is disabled (or do nothing)

Environment variables have been added that represent the default tenant-resources application source. Currently, it defaults to


Repo	`github.com/stackrox/acscs-manifests`
Path	`tenant-resources`
Ref	`HEAD`

Because this is an opt-in feature, at this point deploying this will have no consequence on the int/stage/prod environments.

The acscs-manifests repository is private. But ArgoCD should have access to it thanks to https://github.com/stackrox/acs-fleet-manager-aws-config/pull/257

A further step in the development of the feature is to enable the local development support for ArgoCD: https://github.com/stackrox/acs-fleet-manager/pull/2074/files

Checklist (Definition of Done)

Test manual

TODO: Add manual testing efforts

# To run tests locally run:
make db/teardown db/setup db/migrate
make ocm/setup
make verify lint binary test test/integration

kovayur

🔥

openshift-ci · 2024-10-16T11:26:26Z

New changes are detected. LGTM label has been removed.

johannes94

I don't like being the "Process-driven block a PR because of formal issues guy", but for a change like this, I'd expect more information in the PR description. For instance how you've tested the changes and also a functional description of what you expect / not expect to work after this PR.

johannes94 · 2024-10-16T11:54:33Z

fleetshard/config/config.go

+	DefaultTenantArgoCdAppSourceRepoURL string `env:"TENANT_ARGOCD_APP_SOURCE_REPO_URL_DEFAULT" envDefault:"https://github.com/stackrox/acscs-manifests.git"`
+	DefaultTenantArgoCdAppSourceRef     string `env:"TENANT_ARGOCD_APP_SOURCE_REF_DEFAULT" envDefault:"HEAD"`
+	DefaultTenantArgoCdAppSourcePath    string `env:"TENANT_ARGOCD_APP_SOURCE_PATH_DEFAULT" envDefault:"tenant-resources"`


The naming of the Env variables compared to the field is inconsistent. The "DEFAULT" should be at the start of the variable name.

Looking at the other parts of this PR I wonder if we even need that "Default" prefix. Currently it's never set to a "non-default" value. Is that going to come with a future PR?

Other thing to mention is that even if we can override it on a per tenant basis, on the CentralReconciler and CentralReconcilerOptions struct it shouldn't be called Default* anymore if the override is applied on that level.

I named it default in the sense of it being the default if there are no per-tenant overrides. You are recommending dropping the DEFAULT here ?

For this specific env variables I'd recommend to make the naming consistent. The default should be either a prefix or a suffix, for instance: Env Variable: TENANT_ARGOCD_APP_SOURCE_REF_DEFAULT should be field TenantArgoCdAppSourceRefDefault not DefaultTenantArgoCdAppSourceRef

For the CentralReconciler / CentralReconcilerOptions I don't see any logic for overriding that values on a per-tenant basis, so I'm questioning if this is going to be added in follow up PRs or if the "Default" prefix is unnecessary.

The ability to override the ArgoCD application source (repo/path/ref) will be added later

I'll rename the env var to be consistent

johannes94 · 2024-10-16T11:57:20Z

fleetshard/pkg/central/reconciler/reconciler.go

+	return nil
+}
+
+func (r *CentralReconciler) getArgoCDApplication(remoteCentral private.ManagedCentral) (*argocd.Application, error) {


nit: The method name was confusing me. I expected this function to send a GET requests to argoCd as opposed to creating the *argocd.Application object from a remoteCentral.

johannes94 · 2024-10-16T12:05:58Z

fleetshard/pkg/central/reconciler/reconciler.go

+	} else {
+
+		{
+			// This little part would only happen if we enable, then disable ArgoCD for a tenant


In my understanding this part would execute on every reconciliation where remoteCentral.Spec.ArgoCd.Enabled != true, am I wrong here? If so could you explain why?

Maybe the function would be NOOP in that case but it would go through this code path.

Not every reconciliation, only if the hashSum has been changed.
This would run if

ArgoCD enabled for a tenant

Then ArgoCD disabled for a tenant

This would basically clean up the ArgoCD application

Not every reconciliation, only if the hashSum has been changed.

True, forgot about that part.

Still I think this code path would run even if ArgoCD was never enabled for a tenant. Not only in the situation described above, which makes the statement of the comment wrong.

Ah I see what you mean. Will change to

This little part handles the case where we enable, then disable ArgoCD for a tenant

ludydoo · 2024-10-16T14:58:02Z

https://github.com/stackrox/acs-fleet-manager/pull/2074/files

You're right. After some thought, I believe it would make sense to merge both https://github.com/stackrox/acs-fleet-manager/pull/2074/files and this PR together. Otherwise it might break the local development setup (because argoCD CRDs would not be present). Though it is easier to review separately.

Edit: suspicion confirmed by the failing e2e test.

1 runtime.go:249] Unexpected error occurred rhacs-cs7r0jv6t49s7391gqdg/e2e-b8f0c67c874f629f: unable to delete ArgoCD application for central rhacs-cs7r0jv6t49s7391gqdg/e2e-b8f0c67c874f629f: getting ArgoCD application: no matches for kind "Application" in version "argoproj.io/v1alpha1"

ludydoo · 2024-10-29T06:42:15Z

/retest

ludydoo · 2024-10-29T08:56:55Z

/retest

ludydoo · 2024-10-30T08:16:21Z

/retest

ludydoo · 2024-10-30T16:35:29Z

/retest

ludydoo · 2024-10-31T06:24:27Z

/retest

ludydoo · 2024-10-31T17:32:43Z

/retest

ludydoo temporarily deployed to development September 2, 2024 17:36 — with GitHub Actions Inactive

openshift-ci bot added the approved label Sep 2, 2024

ludydoo temporarily deployed to development September 2, 2024 17:51 — with GitHub Actions Inactive

ludydoo force-pushed the ROX-26038-argocd-tenant-flag branch from cb7795a to 2f08d57 Compare September 4, 2024 12:10

ludydoo force-pushed the ROX-26039-create-argocd-tenant-app branch from f42e55c to a1ac500 Compare September 4, 2024 12:47

ludydoo temporarily deployed to development September 4, 2024 12:47 — with GitHub Actions Inactive

Base automatically changed from ROX-26038-argocd-tenant-flag to main October 14, 2024 09:58

ludydoo force-pushed the ROX-26039-create-argocd-tenant-app branch from a1ac500 to 3635f24 Compare October 16, 2024 08:47

ludydoo temporarily deployed to development October 16, 2024 08:47 — with GitHub Actions Inactive

ludydoo requested review from kovayur, kurlov and johannes94 October 16, 2024 08:48

ludydoo temporarily deployed to development October 16, 2024 08:54 — with GitHub Actions Inactive

ludydoo temporarily deployed to development October 16, 2024 09:14 — with GitHub Actions Inactive

kovayur approved these changes Oct 16, 2024

View reviewed changes

openshift-ci bot assigned kovayur Oct 16, 2024

openshift-ci bot added the lgtm label Oct 16, 2024

ludydoo force-pushed the ROX-26039-create-argocd-tenant-app branch from e3a4736 to e4d10fa Compare October 16, 2024 11:26

openshift-ci bot removed the lgtm label Oct 16, 2024

ludydoo temporarily deployed to development October 16, 2024 11:26 — with GitHub Actions Inactive

johannes94 requested changes Oct 16, 2024

View reviewed changes

openshift-ci bot assigned johannes94 Oct 16, 2024

ludydoo force-pushed the ROX-26039-create-argocd-tenant-app branch from e4d10fa to a614365 Compare October 22, 2024 08:30

ludydoo temporarily deployed to development October 22, 2024 08:30 — with GitHub Actions Inactive

ludydoo changed the base branch from main to lc/ROX-26523-dev-argocd October 22, 2024 08:30

ludydoo temporarily deployed to development October 22, 2024 08:33 — with GitHub Actions Inactive

ludydoo temporarily deployed to development October 22, 2024 10:43 — with GitHub Actions Inactive

ludydoo temporarily deployed to development October 23, 2024 08:13 — with GitHub Actions Inactive

ludydoo force-pushed the ROX-26039-create-argocd-tenant-app branch from 5724a64 to cee658d Compare October 28, 2024 15:13

ludydoo temporarily deployed to development October 28, 2024 15:13 — with GitHub Actions Inactive

ROX-26039: fix

a71c53c

ludydoo temporarily deployed to development October 31, 2024 08:20 — with GitHub Actions Inactive

ROX-26039: fix

492cc70

ludydoo temporarily deployed to development October 31, 2024 08:30 — with GitHub Actions Inactive

ROX-26039: fix

31a1656

ludydoo temporarily deployed to development October 31, 2024 10:00 — with GitHub Actions Inactive

ROX-26039: fix

6a88ce2

ludydoo temporarily deployed to development October 31, 2024 11:51 — with GitHub Actions Inactive

ROX-26039: fix

bd2f731

ludydoo temporarily deployed to development October 31, 2024 13:56 — with GitHub Actions Inactive

ROX-26039: fix

09fdeb8

ludydoo temporarily deployed to development October 31, 2024 15:24 — with GitHub Actions Inactive

ROX-26039: fix

53fdb52

ludydoo temporarily deployed to development October 31, 2024 15:29 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROX-26039: Create tenant argoCd app #2009

ROX-26039: Create tenant argoCd app #2009

ludydoo commented Sep 2, 2024 •

edited

Loading

kovayur left a comment

openshift-ci bot commented Oct 16, 2024

johannes94 left a comment

johannes94 Oct 16, 2024

ludydoo Oct 16, 2024

johannes94 Oct 16, 2024

ludydoo Oct 16, 2024

ludydoo Oct 16, 2024

johannes94 Oct 16, 2024 •

edited

Loading

johannes94 Oct 16, 2024

ludydoo Oct 16, 2024

johannes94 Oct 16, 2024 •

edited

Loading

ludydoo Oct 16, 2024

ludydoo commented Oct 16, 2024 •

edited

Loading

ludydoo commented Oct 29, 2024

ludydoo commented Oct 29, 2024

ludydoo commented Oct 30, 2024

ludydoo commented Oct 30, 2024

ludydoo commented Oct 31, 2024

ludydoo commented Oct 31, 2024

ROX-26039: Create tenant argoCd app #2009

Are you sure you want to change the base?

ROX-26039: Create tenant argoCd app #2009

Conversation

ludydoo commented Sep 2, 2024 • edited Loading

Description

Checklist (Definition of Done)

Test manual

kovayur left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 16, 2024

johannes94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannes94 Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannes94 Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ludydoo commented Oct 16, 2024 • edited Loading

ludydoo commented Oct 29, 2024

ludydoo commented Oct 29, 2024

ludydoo commented Oct 30, 2024

ludydoo commented Oct 30, 2024

ludydoo commented Oct 31, 2024

ludydoo commented Oct 31, 2024

ludydoo commented Sep 2, 2024 •

edited

Loading

johannes94 Oct 16, 2024 •

edited

Loading

johannes94 Oct 16, 2024 •

edited

Loading

ludydoo commented Oct 16, 2024 •

edited

Loading