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

[jaeger-operator] Added config to make operator watch in arbitrary namespaces #109

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

davelosert
Copy link
Contributor

The current helm-chart only allows a somewhat "all or nothing" strategy when it comes to RBAC and watching namespaces: Either the operator only has rights and watches the namespace it was deployed to or it has cluster-wide permissions with a ClusterRoleBinding.

This PR introduces a configuration to watch for arbitrary namespaces.
If set, it also creates a ClusterRole, but only RoleBindings in all defined namespaces.

@davelosert davelosert force-pushed the Operator_Watch_Arbitrary_Namespaces branch 3 times, most recently from 606eacd to 51258ec Compare May 25, 2020 07:19
@davelosert
Copy link
Contributor Author

Setting WATCH_NAMESPACE to a comma-separated list currently does not work with the operator due to a bug I reported in the operator-repository

So I guess this PR should only be merged once that bug got fixed.

@naseemkullah naseemkullah changed the title Added config to make operator watch in arbitrary namespaces [jaeger-operator] Added config to make operator watch in arbitrary namespaces May 25, 2020
@cpanato
Copy link
Member

cpanato commented Aug 22, 2020

@davelosert can you update this PR? so we can finish the review and merge. If this makes no sense anymore feel free to close it

@davelosert davelosert force-pushed the Operator_Watch_Arbitrary_Namespaces branch from 51258ec to 30c2267 Compare August 27, 2020 05:39
Signed-off-by: David Losert <david.losert@gmx.de>
@davelosert davelosert force-pushed the Operator_Watch_Arbitrary_Namespaces branch from 30c2267 to 9b99bc9 Compare August 27, 2020 05:44
@davelosert
Copy link
Contributor Author

@cpanato : Updated the PR and adjusted the version. Should be ready for merge now 🙂

Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Base automatically changed from master to main February 27, 2021 18:28
@rafaribe
Copy link
Contributor

What's keeping this PR from getting merged? I'm currently experiencing an issue with the namespaces and I think this could fix it.

@DodgeCamaro
Copy link

Any updates?

@stevehipwell
Copy link
Contributor

I'm not sure the logic here is correct, nor is it correct currently either, as setting jaeger.namespace should override the release namespace. If this extra logic is actually required at that point it should be layered on top of this making sure that the CR created by the chart can always be observed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants