feat: enable metadata to be set on namespaces

[blakepettersson]

This builds upon the work that @pasha-codefresh did in #10288.

The main differences between this PR and the previous one is that we use
SSA to diff between different versions of the namespace, as well as
having a slightly different API in gitops-engine for setting the
namespace modifier.

We now also set the ownership of the namespace in ArgoCD (if managedNamespaceMetadata is set).

Fix #4628
Fix #6215
Fix #7799

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).

[codecov]

Codecov Report

Base: 45.60% // Head: 45.63% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (d76f316) compared to base (a773b1e).
Patch coverage: 84.37% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10672      +/-   ##
==========================================
+ Coverage   45.60%   45.63%   +0.03%     
==========================================
  Files         237      238       +1     
  Lines       28933    28953      +20     
==========================================
+ Hits        13194    13214      +20     
+ Misses      13922    13920       -2     
- Partials     1817     1819       +2     

Impacted Files	Coverage Δ
pkg/apis/application/v1alpha1/types.go	54.84% <ø> (ø)
controller/sync.go	55.21% <83.33%> (+1.15%)	⬆️
controller/sync_namespace.go	85.00% <85.00%> (ø)

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[blakepettersson]

This needs argoproj/gitops-engine#465 to first be merged

[crenshaw-dev]

@blakepettersson thanks so much for this!

Can you add docs? Specifically I'd be interested in:

• field documented in docs/operator-manual/application.yaml
• explanation of what happens if I add this field but do not set CreateNamespace=true
• explanation of what happens if I add this field, but the application manifests contain a conflicting definition of the namespace
• explanation of what happens if I change the metadata in the Application manifest after the namespace is created

I think that covers all the edge cases.

[leoluz]

Thanks for this!
Please check my comments below and here as I believe that we need to better handle the conflict cases and not just replace labels and annotations in namespaces. Maybe returning an error?

[leoluz]

This would replace any existing labels with the provided ones. We discussed about raising an error whenever a conflict is found.

[leoluz]

This would replace any existing annotations with the provided ones. We discussed about raising an error whenever a conflict is found.

[blakepettersson]

@leoluz what are your thoughts on my comment in #10288?

The more I'm thinking about this, the more I think we should disallow updates to namespaces given any conflict. My current line of thinking is

Emit a warning when detecting a conflict. At the moment (locally) it's a SharedResourceWarning but can be made into something else.
Do not sync any changes that would have otherwise happened to the namespace.

Thoughts on this @leoluz @crenshaw-dev @gazab?

My thinking is that in case of a conflict on a namespace, it seems a bit heavy-handed to block the whole sync of an application. On the other hand, as you say, it's not fully desirable to just overwrite annotations and labels on a given namespace.

That's why I'm thinking "emit a warning, and don't modify the namespace, but do carry on with the other manifests in the meantime".

This leads to another question; how would we best handle / tell users how to migrate existing namespaces to use managedNamespaceMetadata? For someone that is upgrading to use this feature, what would the suggested course of action be?

To prevent causing an error (or warning), I'm guessing we'll need to note that a user will first need to manually set the resource tracking label (or annotation)?

[leoluz]

To avoid replacing existing labels/annotations from namespaces we could use the new server-side apply feature to allow k8s handle creating the proper patch.
The idea is that you don’t have to retrieve the whole object from k8s then mutate it and try to apply it back. You can provide a partial manifest and kubernetes will take care of updating just the required portion.

To enable SSA apply flag in Argo CD is just a matter of adding this annotation in the namespace:

argocd.argoproj.io/sync-options: ServerSideApply=true

This way k8s will figure out the labels/annotations that are managed by Argo CD and leave existing ones intact

[blakepettersson]

I think the Server Side Apply approach is generally solid (thanks again for your help @leoluz!). There seems to be a few caveats though, none of which should be a major showstopper but more as an FYI. Before pushing my changes, I just want to gather your thoughts/opinions on the following:

1. Adopting an existing namespace and then removing an "adopted" field.

From what I can see with the SSA approach, if ArgoCD "adopts" an existing namespace and we would then want to remove a preexisting field from the adopted namespace, we'd first need to change its value, and then remove it in two separate syncs.

So, imagine we have a pre-existing namespace as below:

apiVersion: v1
kind: Namespace
metadata:
  name: foobar
  annotations:
    foo: bar
    abc: 123

If we want to manage the foobar namespace with ArgoCD and to then also remove the foo: bar annotation, in managedNamespaceMetadata we'd need to first rename the foo value:

apiVersion: argoproj.io/v1alpha1
kind: Application
spec:
  syncPolicy:
    managedNamespaceMetadata:
      annotations:
        abc: 123 # adding this is informational with SSA; this would be sticking around in any case until we set a new value
        foo: remove-me
    syncOptions:
      - CreateNamespace=true

Once that has been synced, we're ok to remove foo

apiVersion: argoproj.io/v1alpha1
kind: Application
spec:
  syncPolicy:
    managedNamespaceMetadata:
      annotations:
        abc: 123 # adding this is informational with SSA; this would be sticking around in any case until we set a new value
    syncOptions:
      - CreateNamespace=true

2. Metadata seems to be overwritten regardless when having a k8s manifest + managedNamespaceMetadata.

It doesn't seem like SSA has any effect if managedNamespaceMetadata and an ArgoCD application has a k8s manifest for the same namespace. This could potentially be an issue with the E2E tests I'm running, but from what I can see the k8s manifest will always take precedence over what's in managedNamespaceMetadata.

For the first issue that's a matter of documenting the behaviour. As for the second issue we'll need to raise an error and prevent the syncing from happening. WDYT @leoluz @crenshaw-dev?

[leoluz]

@blakepettersson

Adopting an existing namespace and then removing an "adopted" field.

That is correct. The example you gave is great and can be added to a future documentation 👍🏻

Metadata seems to be overwritten regardless when having a k8s manifest + managedNamespaceMetadata.

I think that is expected. ManagedNamespaces will run as pre-sync hooks. This means that if a namespace is provided as part of the application's manifests, it will override what was defined in the managedNamespaceMetadata. In my opinion it is a matter of documenting this behaviour clearly.

[blakepettersson]

Alright, so now I've modified the namespace sync to make use of SSA, and updated the docs accordingly. I've also squashed my commits since it started looking a bit messy.

I still need to figure out what's going on with the integration tests, they seem a bit flaky 🤔

[leoluz]

@blakepettersson I merged the gitops-engine PR and updated the go.mod file in this PR. However the build is failing with some e2e test error. Can you please double check if this isn't a real problem? Maybe it is a good idea to update this branch with current master as well.

[blakepettersson]

@leoluz there were some unrelated failures before, I've seen at least the first one to have been a bit flaky in the past:

time="2022-11-03T21:03:37Z" level=error msg="`../../dist/argocd app sync test-immutable-change --timeout 10 --prune --plaintext --server 127.0.0.1:8088 --auth-token *** --insecure` failed exit status 1: time=\"2022-11-03T21:03:37Z\" level=fatal msg=\"Operation has completed with phase: Failed\"" execID=bd820
    app_management_test.go:379: failed expectation: error
--- FAIL: TestImmutableChange (2.95s)

and

app_management_ns_test.go:149: 
  Error Trace:	app_management_ns_test.go:149
        	          consequences.go:47
        	          app_management_ns_test.go:146  
  Error:      	"Get \"https://10.1.0.15:10250/containerLogs/argocd-e2e--test-namespaced-get-logs-allow-switch-on-ns-sljzu/guestbook-ui-6df84db565-bbv6j/guestbook-ui?timestamps=true\": EOF" does not contain "Hi"
        	Test:       	TestNamespacedGetLogsAllowSwitchOnNS

All the tests pass now in any case 👍

[blakepettersson]

Thanks again @leoluz! 🎉🎉

[lukpep]

was this included in the 2.5.2 version just released? Docs don't mention it but since it was merged to master then ... maybe ;-)

[leoluz]

was this included in the 2.5.2 version just released? Docs don't mention it but since it was merged to master then ... maybe ;-)

@lukpep this is targeting 2.6.
I updated the parent issue as well as this PR pointing it to right milestone to avoid this confusion.