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

Add the ability to add annotations to all automatically created namespaces #4628

Closed
insanemal opened this issue Oct 21, 2020 · 16 comments · Fixed by #10672
Closed

Add the ability to add annotations to all automatically created namespaces #4628

insanemal opened this issue Oct 21, 2020 · 16 comments · Fixed by #10672
Labels
enhancement New feature or request
Milestone

Comments

@insanemal
Copy link

Summary

Currently Argo CD can automatically create namespaces. We use Rancher UI. These namespaces do not have the RancherUI annotation for Rancher Projects. It would be nice to be able to define on the Project or even application level annotations that get added to automatically created Namespaces.

Motivation

When using ArgoCD and Rancher UI

Proposal

I think it could be a project level mapping. That way you could map Argo Projects to Rancher Projects. But possibly with App level overrides. It is really about annotating the namespace.

@insanemal insanemal added the enhancement New feature or request label Oct 21, 2020
@KarstenSiemer
Copy link
Contributor

KarstenSiemer commented Jul 23, 2021

Any update or workarounds for this? This is a great feature and this is the somewhat first question you ask yourself when using it. Especially because kustomize removed the ability to add pre- and suffixes to namespaces.

@insanemal
Copy link
Author

Yeah, it's still an issue for us. It would be super great for other reasons as well. Using Filebeat/Logstash being able to set project wide annotations is also a boon.

@renaudguerin
Copy link
Contributor

Also in need of this to add a GCP Config Connector annotation to each app's namespace.

@KarstenSiemer
Copy link
Contributor

KarstenSiemer commented Sep 21, 2021

@jessesuen what do you think about this feature? Would it make sense?
If so, lets think about how we could implement it and I'll try to create a pr for it.

I guess that, since the creating of the namespace is controlled in the sync options, the namespace labels should be controlled there, too. This would be somewhat convenient but would force us to change other features too, since syncOptions is an array.
but anything else would make the CRD too complicated imo. Also adding the ability to add annotations as well would make sense.

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: foo
  namespace: argocd
spec:
  destination:
    namespace: bar
  syncPolicy:
    syncOptions:
      createNamespace:
        enabled: true
        labels:
          io.foo.bar/this: labelValue
          io.foo.bar/that: labelValue
      validate:
        enabled: false

But changing the crd this way is a breaking change and a lot of work, which is crazy for such a small feature.
We could however add a new field like:

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: foo
  namespace: argocd
spec:
  destination:
    namespace: bar
    createNamespace:
      labels:
        io.foo.bar/this: labelValue
        io.foo.bar/that: labelValue
  syncPolicy:
    syncOptions:
    - Validate=false

This would also make sense to place it directly next to the namespace name it would create. But it still looks a bit confusing.
Just using "labels" and ommiting the createNamespace would be a bad choice imo, since it could create the impression that argocd will always add the labels to the destination namespace and not only to namespaces it creates for this app.

@whyman
Copy link

whyman commented Jan 24, 2022

👍 From us too, for External Secrets key name restrictions

@mcorbin
Copy link

mcorbin commented Apr 4, 2022

I'm also interested by this feature (I can also probably find some time to work on it).
A lot of tools rely on labels or annotations on namespaces and implementing this would I think help a lot of people.

@rishabh625
Copy link
Contributor

Instead of adding it as feature, just committing those namespaces in git with required annotations and creating application to manage it wouldn't be enough?

@insanemal
Copy link
Author

If that were enough would we be here?

@rishabh625
Copy link
Contributor

rishabh625 commented Apr 6, 2022

I would like to volunteer for contributing to this feature if no one else is working

@crenshaw-dev
Copy link
Member

crenshaw-dev commented Aug 2, 2022

@insanemal how would you feel about this?

- CreateNamespaceLabels=io.foo.bar/this=labelValue,io.foo.bar/that=labelValue

I like @KarstenSiemer's suggestion to add new fields, but those are big (breaking?) changes. Might be a good idea for v3.0.

@ashutosh16 want to see if you have time to work on this?

@insanemal
Copy link
Author

I need annotations not Labels. But the syntax is good

@ashutosh16
Copy link
Contributor

I'm thinking to add the annotation to the createnamespace sync option itself.

CreateNamespace=true,io.foo.bar/this=x

I made the code change and tested it in local. I'm able to create a namespace with/without annotation. Do we still need a separate sync option?

@crenshaw-dev
Copy link
Member

crenshaw-dev commented Aug 4, 2022

@ashutosh16 I think we'll probably want to provide the option to accept the labels/annotations JSON-encoded so people are free to include and = or , in their config. Like this:

- 'CreateNamespaceAnnotations={"io.foo.bar/this": "x"}'

@leoluz
Copy link
Collaborator

leoluz commented Aug 5, 2022

Instead of adding it as feature, just committing those namespaces in git with required annotations and creating application to manage it wouldn't be enough?

@insanemal can you please elaborate on why @rishabh625 suggestion above doesn't satisfy your use-case? I don't use Rancher UI but would like to understand the implications a bit better.

@crenshaw-dev
Copy link
Member

Reasons for CreateNamespace, per the original issue thread:

Not sure if either of these applies to @insanemal.

@crenshaw-dev
Copy link
Member

blakepettersson added a commit to blakepettersson/argo-cd that referenced this issue Nov 4, 2022
This builds upon the work that @pasha-codefresh did in argoproj#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.

Closes argoproj#4628
Closes argoproj#6215
Closes argoproj#7799

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
leoluz added a commit that referenced this issue Nov 4, 2022
* namespace labels

Signed-off-by: pashavictorovich <pavel@codefresh.io>

* create namespace should support annotations

Signed-off-by: pashavictorovich <pavel@codefresh.io>

* handle also modification hook

Signed-off-by: pashavictorovich <pavel@codefresh.io>

* regenerate entity on modify hook

Signed-off-by: pashavictorovich <pavel@codefresh.io>

* manifests

Signed-off-by: pashavictorovich <pavel@codefresh.io>

* feat: enable metadata to be set on namespaces

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.

Closes #4628
Closes #6215
Closes #7799

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* fix: don't always track namespaces

For now, only allow namespaces managed with `managedNamespaceMetadata`
to have tracking set by Argo. Ideally we'd like new namespaces to also
be tracked by Argo, but there's currently an issue with a failing
integration test.

Also wrap error message if setting the app instance errors on the
namespace.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* fix: always return true with `hasManagedMetadata`

If `hasManagedMetadata` is set, `true` should always be returned.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* docs: add clarifying docs on resource tracking

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* style: pr tweaks

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* fix: re-add label unsetting

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* Update gitops-engine to current master

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

Signed-off-by: pashavictorovich <pavel@codefresh.io>
Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
Co-authored-by: pashavictorovich <pavel@codefresh.io>
Co-authored-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
ashutosh16 pushed a commit to ashutosh16/argo-cd that referenced this issue Nov 23, 2022
* namespace labels

Signed-off-by: pashavictorovich <pavel@codefresh.io>

* create namespace should support annotations

Signed-off-by: pashavictorovich <pavel@codefresh.io>

* handle also modification hook

Signed-off-by: pashavictorovich <pavel@codefresh.io>

* regenerate entity on modify hook

Signed-off-by: pashavictorovich <pavel@codefresh.io>

* manifests

Signed-off-by: pashavictorovich <pavel@codefresh.io>

* feat: enable metadata to be set on namespaces

This builds upon the work that @pasha-codefresh did in argoproj#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.

Closes argoproj#4628
Closes argoproj#6215
Closes argoproj#7799

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* fix: don't always track namespaces

For now, only allow namespaces managed with `managedNamespaceMetadata`
to have tracking set by Argo. Ideally we'd like new namespaces to also
be tracked by Argo, but there's currently an issue with a failing
integration test.

Also wrap error message if setting the app instance errors on the
namespace.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* fix: always return true with `hasManagedMetadata`

If `hasManagedMetadata` is set, `true` should always be returned.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* docs: add clarifying docs on resource tracking

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* style: pr tweaks

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* fix: re-add label unsetting

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* Update gitops-engine to current master

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

Signed-off-by: pashavictorovich <pavel@codefresh.io>
Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
Co-authored-by: pashavictorovich <pavel@codefresh.io>
Co-authored-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
emirot pushed a commit to emirot/argo-cd that referenced this issue Jan 27, 2023
* namespace labels

Signed-off-by: pashavictorovich <pavel@codefresh.io>

* create namespace should support annotations

Signed-off-by: pashavictorovich <pavel@codefresh.io>

* handle also modification hook

Signed-off-by: pashavictorovich <pavel@codefresh.io>

* regenerate entity on modify hook

Signed-off-by: pashavictorovich <pavel@codefresh.io>

* manifests

Signed-off-by: pashavictorovich <pavel@codefresh.io>

* feat: enable metadata to be set on namespaces

This builds upon the work that @pasha-codefresh did in argoproj#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.

Closes argoproj#4628
Closes argoproj#6215
Closes argoproj#7799

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* fix: don't always track namespaces

For now, only allow namespaces managed with `managedNamespaceMetadata`
to have tracking set by Argo. Ideally we'd like new namespaces to also
be tracked by Argo, but there's currently an issue with a failing
integration test.

Also wrap error message if setting the app instance errors on the
namespace.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* fix: always return true with `hasManagedMetadata`

If `hasManagedMetadata` is set, `true` should always be returned.

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* docs: add clarifying docs on resource tracking

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* style: pr tweaks

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* fix: re-add label unsetting

Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

* Update gitops-engine to current master

Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>

Signed-off-by: pashavictorovich <pavel@codefresh.io>
Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
Co-authored-by: pashavictorovich <pavel@codefresh.io>
Co-authored-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
Signed-off-by: emirot <emirot.nolan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment