feat: Ability to create custom labels for namespaces created with syncOptions CreateNamespace

[pasha-codefresh]

Closes #4628
Closes #6215
Closes #7799

[todaywasawesome]

Linked PR in GitOps engine. argoproj/gitops-engine#443

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.

Project coverage is 45.90%. Comparing base (f84d828) to head (58f4835).
Report is 2612 commits behind head on master.

Files with missing lines	Patch %	Lines
controller/sync.go	0.00%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10288      +/-   ##
==========================================
- Coverage   45.92%   45.90%   -0.02%     
==========================================
  Files         229      229              
  Lines       28278    28290      +12     
==========================================
  Hits        12987    12987              
- Misses      13518    13530      +12     
  Partials     1773     1773              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[leoluz]

Maybe better providing the name here as well?
Or even make this struct just have a ObjectMeta reference in it?

something like:

type CreateNamespaceMetadata struct {
	metav1.ObjectMeta `json:"metadata"`

[crenshaw-dev]

The name is inferred from the resource referencing the to-be-created namespace.

[leoluz]

As discussed a few scenarios that need to be addressed and documented:

• ArgoCD needs to correctly maintain the namespace state as defined in the new CreateNamespaceMetadata. This means that it needs to handle create, update and deletes. It is basically similar thing that we do during the reconciliation but with the difference that the desired state doesn't come from git in this case.

• Define what will be the behaviour if the user provides the namespace with specific state in git and also defines the Application resource with a different state. What will be the correct behaviour in this case?

• There are some use-cases where users want to define the namespace name dynamically. Are we supporting this use-case as part of this PR? (I can't find the comment where the user asked about this feature.. Let's leave it out for now for simplification.)

[crenshaw-dev]

There are some use-cases where users want to define the namespace name dynamically. Are we supporting this use-case as part of this PR?

Can you link those use cases? afaik the namespace names should always be taken from the resources in the sync operation.

[leoluz]

As discussed with @pre in the original ticket please consider changing the terminology from CreateNamespaceMetadata to ManagedNamespaceMetadata.

More details here and here.

[rcarre]

Many thanks for this enhancement.
Today we are facing SharedResourceWarning since 2 applications manage the same namespace to be created in a specific Rancher project by the means of here enclosed manifest. As this namespace is the only shared resource, it is not worth your while to create a specific application for shared resource.

apiVersion: v1
kind: Namespace
metadata:
  annotations:
    field.cattle.io/projectId: ${ARGOCD_ENV_RANCHER_PROJECT_ID}
  name: ${ARGOCD_APP_NAMESPACE}

Thanks to this PR, we expect to trigger syncOptions: - CreateNamespace=true with specific annotations built from env variable

      source:
        plugin:
          name: argocd-vault-plugin-envsubst
          env:
             - name: RANCHER_PROJECT_ID
             - value: ${ARGOCD_ENV_RANCHER_DEFAULT_PROJECT_ID}
      syncPolicy:
        syncOptions: 
          - CreateNamespace=true

[crenshaw-dev]

@pasha-codefresh do you still have time to move this forwards?

[blakepettersson]

I'd be happy to carry the torch if @pasha-codefresh doesn't have time before the cutting of 2.5 😄

[crenshaw-dev]

@blakepettersson that would be awesome! Hoping to cut RC1 next week, but we'll see how multi-source apps goes. :-)

[blakepettersson]

I see that the required changes to implement this was reverted with argoproj/gitops-engine#455, why's that @leoluz @pasha-codefresh @crenshaw-dev?

[leoluz]

I see that the required changes to implement this was reverted with argoproj/gitops-engine#455, why's that @leoluz @pasha-codefresh @crenshaw-dev?

@blakepettersson That was reverted because it was breaking Argo CD build as it was introducing a breaking change.

[blakepettersson]

@leoluz can you give some examples on what you mean here?

Define what will be the behaviour if the user provides the namespace with specific state in git and also defines the Application resource with a different state. What will be the correct behaviour in this case?

[gazab]

@leoluz can you give some examples on what you mean here?

Define what will be the behaviour if the user provides the namespace with specific state in git and also defines the Application resource with a different state. What will be the correct behaviour in this case?

I guess he means what should happen if the Application contains a Namespace resource for the managed namespace. Something like this:

syncPolicy: 
  managedNamespaceMetadata:
    annotations: 
      annotation1: x1
      annotation2: y1
    labels: 
      label1: z
      label2: v

together with:

apiVersion: v1
kind: Namespace
metadata:
  name: same-as-application-namespace
  annotations:
    annotation1: x2
    annotation2: y2
    annotation3: new_annotation_value

What values should the namespace end up with?

[leoluz]

I guess he means what should happen if the Application contains a Namespace resource for the managed namespace. Something like this:

Exactly. Tks for clarifying that with examples @gazab !
I am tempted to say that Argo CD should raise a conflict error in this case.

Another case is, the namespace resource is not provided with the application but already exists in the cluster with conflicting labels/annotations values. We need to define what would be this behaviour and document it.

[blakepettersson]

Exactly. Tks for clarifying that with examples @gazab !
I am tempted to say that Argo CD should raise a conflict error in this case.

Indeed, thanks @gazab 😄. IMO at the very least there definitely should be some kind of warning if that happens. Currently when the same k8s resource is managed by two separate Argo applications a SharedResourceWarning is emitted; it would make sense for something like that to be emitted when the same application is attempting to create the same namespace both through k8s manifests and managedNamespaceMetadata.

Another (complementary?) option could be to say "k8s manifest wins" (with a warning), something like

1. Namespace is created with managedNamespaceMetadata and gets synced first
2. Then the application manifests gets applied, overwriting whatever has been done with managedNamespaceMetadata.

Another case is, the namespace resource is not provided with the application but already exists in the cluster with conflicting labels/annotations values. We need to define what would be this behaviour and document it.

Today, if an existing k8s resource gets adopted by an ArgoCD application, it would be overwritten by whatever ArgoCD applies right? It would make sense in that case to be consistent and apply the same behaviour for namespaces as well.

[leoluz]

Today, if an existing k8s resource gets adopted by an ArgoCD application, it would be overwritten by whatever ArgoCD applies right? It would make sense in that case to be consistent and apply the same behaviour for namespaces as well.

@blakepettersson the difference is that in this case it is the same actor (Argo CD Controller) conflicting with itself. The managedFields would be updated with the same manager. However, the overriding logic you described doesn't look bad and I wouldn't be opposed to that behaviour as long as it is documented.

[blakepettersson]

I might need another pair of eyes to check that I'm not missing something 😄

In #10672, I added some E2E test cases to verify that

1. For a namespace that is not originally managed by ArgoCD, the metadata in managedNamespaceMetadata gets applied on the preexisting namespace
2. For a namespace which has conflicting metadata in managedNamespaceMetadata and as a k8s manifest, the k8s manifest data will overwrite the data which is in managedNamespaceMetadata

I still need to add warnings and/or errors for these conflicts, that's next on my list.

[blakepettersson]

Another (complementary?) option could be to say "k8s manifest wins" (with a warning), something like

Namespace is created with managedNamespaceMetadata and gets synced first
Then the application manifests gets applied, overwriting whatever has been done with managedNamespaceMetadata.

I still need to add warnings and/or errors for these conflicts, that's next on my list.

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

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

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

[crenshaw-dev]

What's the downside to just silently accepting whatever is in the manifests?

[blakepettersson]

To be fair, I don't really see any downside apart from the fact that it may surprise some users 😄 Doing that would keep this simple, and is inline with how manifests are currently synced.

[leoluz]

What's the downside to just silently accepting whatever is in the manifests?

@crenshaw-dev @blakepettersson I don't think we should do that. Existing namespaces might hold crucial labels and annotations that are necessary for the correct operation of user's clusters. Argo CD shouldn't just replace all annotations and labels as it would certainly break some k8s setups.

I think it would be better continue with further discussions in @blakepettersson PR (#10672)

I am closing this for now.