Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[stable/ambassador] rbac updates for CRDs and single namespace usage #14388

Closed
wants to merge 1 commit into from

Conversation

Flydiverny
Copy link
Collaborator

What this PR does / why we need it:

Adds a cluster role for checking if ambassador crds are defined, even if running in single namespace mode with role instead of cluster role.
Removes namespace permission when rbac is namespaced.

Which issue this PR fixes

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md
  • Title of the PR starts with chart name (e.g. [stable/chart])

Signed-off-by: Markus Maga <markus@maga.se>
@helm-bot helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 1, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Flydiverny

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested a review from kflynn June 1, 2019 02:42
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2019
@Flydiverny
Copy link
Collaborator Author

/hold
Want to make sure this actually solves whatever use case people with single namespace has!

/assign @kflynn

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 1, 2019
@kflynn
Copy link
Collaborator

kflynn commented Jun 1, 2019

PR looks sane; I’ll wait for feedback from users before doing anything else...

@Flydiverny
Copy link
Collaborator Author

Perhaps we need to skip rbac-crds when rbac.namespaced: true assuming we can't create cluster roles at all in this use case. Which in the end would mean the user can't use CRDs in that setup.

You could still do ambassador in single namespace mode with cluster roles ( rbac.namespaced: false and scope.singleNamespace: true), which would allow for CRDs to be used.

@iNoahNothing
Copy link
Collaborator

Why don't we just skip rbac-crds when crds.create: false?

I could see a usecase of wanting namespaced RBAC while still wanting to use the CRD functionality. If there is some restriction against creating or giving access to anything at the cluster level, then CRDs aren't going to be desirable anyway.

@Flydiverny
Copy link
Collaborator Author

Flydiverny commented Jun 3, 2019

@nbkrause

If you install 2 instances or more of ambassador only one of them can create the CRDs, but all of them could watch for them in their respective namespace. Ambassador requires that it can see that the CRD definitions exist, so they would need a cluster role for this, with ambassadors current logic.


Is there any use case where one would do

rbac.namespaced: true
scope.singleNamespace: false

Wondering if rbac.namespaced is superfluous? It makes little sense to me why one would limit ambassador to only see things in its namespace but be configured to look in all of them :)

@Flydiverny
Copy link
Collaborator Author

Split the fix for #13557 to another PR #14604 so its not blocked :)

@stale
Copy link

stale bot commented Jul 7, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 7, 2019
@stale
Copy link

stale bot commented Jul 21, 2019

This issue is being automatically closed due to inactivity.

@stale stale bot closed this Jul 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
5 participants