Skip to content

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 requested review from batazor and cpanato May 25, 2020 09:05
@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
@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.

6 participants