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

Feature: match allow on namespace annotations #348

Closed
devthejo opened this issue May 16, 2023 · 8 comments · Fixed by #376
Closed

Feature: match allow on namespace annotations #348

devthejo opened this issue May 16, 2023 · 8 comments · Fixed by #376
Labels
carvel-accepted This issue should be considered for future work and that the triage process has been completed carvel-triage This issue has not yet been reviewed for validity enhancement This issue is a feature request

Comments

@devthejo
Copy link
Contributor

Describe the problem/challenge you have
We have multi-tenant clusters where isolation is done by rancher. The mechanism is simple, anyone is scoped in a rancher project, anyone can create new namespace and scope it in it's project scope by adding an annotation that is matching the project id. To summarize this annotation is the guarantee that this namespace will not be accessed by another tenant.
In our dev cluster, we have unpredictable new namespace that will be created for review environments and we want be able to use SecretImport in them, but the constraint in SecretExport based on the destination namespace is not sufficient to address our case.

Describe the solution you'd like
We would like be able to specify ToNamespaceAnnotations as following, instead of ToNamespace on SecretExport

kind: SecretExport
metadata:
  name: user-password
  namespace: user1
spec:
  toNamespaceAnnotations:
    projectId: my-project-id

This could also be

kind: SecretExport
metadata:
  name: user-password
  namespace: user1
spec:
  toNamespaceAnnotationKey: projectId
  toNamespaceAnnotationValue: my-project-id

Anything else you would like to add:
I can try to make a PR if this feature is welcome :-)


Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you would like to work on this issue.

@devthejo devthejo added carvel-triage This issue has not yet been reviewed for validity enhancement This issue is a feature request labels May 16, 2023
@neil-hickey
Copy link
Contributor

hi @devthejo , thanks for submitting!

  • Is this annotation specific to rancher or your use-case? What does the annotation look like?
  • I'm not very familiar with rancher, is this "tenant via annotation" a standard part of rancher?

I'm not opposed to you making the PR, I just want to make sure that this feature would be useful for the project and not just for your particular use-case if it is specific to your environment.

@devthejo
Copy link
Contributor Author

devthejo commented May 16, 2023

hi @neil-hickey, thanks for your reactivity!

  • Is this annotation specific to rancher or your use-case? What does the annotation look like?
  • I'm not very familiar with rancher, is this "tenant via annotation" a standard part of rancher?

This is the way rancher do, yes we can call this "tenant via annotation" and this feature could be useful for any user of rancher that want to use secretgen-controller.

This is how it's look like:

apiVersion: v1
kind: Namespace
metadata:
  annotations:
    field.cattle.io/projectId: "abcde:12345"
  name: my-namespace

@neil-hickey
Copy link
Contributor

Ok great, thanks for that background info!

That seems like a genuine use-case for secretgen controller. It's purpose is to make secrets available (copy/move etc) across namespaces (as one main use-case) - in this case the annotation is providing that tenancy.

Let me know if you need any help with PRs :)

@devthejo
Copy link
Contributor Author

here is the PR: #337
:-)

@vrabbi
Copy link

vrabbi commented May 23, 2023

Just my 2 cents:
It i think would make more sense possibly to generalize this a bit with a standard k8s selector object using something similar to the matchLabels or matchExpression formats. This would allow for other models of tenancy that exist which use labels for example and not annotations.

The format im thinking could like like:

selectorMatchFields:
- key:
operator: <[In|NotIn|Exists|DoesNotExist]>
values: [ ]

This for example is used in cartographer supply chains and offers a lot of flexibility.

The key in the annotation case would be metadata.annotations[<rancher annotation] and the operator would be In and the value would be the tennant id.
This would allow matching on any field of a namespace or even multiple fields as it is an array.

While a bit more verbose, i think the extra flexibility may be worth it.

The code for this exists in cartographer and could im guessing be re used here without making it too difficult.

@renuy renuy moved this to To Triage in Carvel May 30, 2023
@devthejo
Copy link
Contributor Author

OK @vrabbi, thanks for advice, I'll drop a new PR very soon.

@devthejo
Copy link
Contributor Author

devthejo commented May 31, 2023

here is the new PR according to selectorMatchFields spec: #376 !

@renuy renuy moved this from To Triage to In Progress in Carvel Jul 12, 2023
@renuy renuy added carvel-accepted This issue should be considered for future work and that the triage process has been completed and removed carvel-triage This issue has not yet been reviewed for validity labels Jul 12, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Closed in Carvel Jul 13, 2023
@gary-van-woerkens
Copy link

Can't wait for the release !!! 🤤

@github-actions github-actions bot added the carvel-triage This issue has not yet been reviewed for validity label Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carvel-accepted This issue should be considered for future work and that the triage process has been completed carvel-triage This issue has not yet been reviewed for validity enhancement This issue is a feature request
Projects
Archived in project
5 participants