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

WIP: Allow split of watch namespaces #1131

Closed
wants to merge 3 commits into from

Conversation

smuda
Copy link
Contributor

@smuda smuda commented Jul 1, 2023

Allows split of namespaces, where Grafana CRDs are watched in namespace specified in $WATCH_NAMESPACE or from all namespaces with label $WATCH_LABEL and GrafanaDashboards, GrafanaFolders and GrafanaDatasources are watched in all namespaces with label $WATCH_LABEL_CRD.

Closes #1007

NOTE 1: I'm not really satisfied with how this PR turned out because there is now two ready-ness probe endpoints. Look at it as a start of a discussion.
NOTE 2: As I'm expecting a discussion with appropriate changes, I have not updated the helm chart to reflect these changes. The documentation needs to be updated as well.
NOTE 3: I've used the PR#1053 as base, which will be removed once that PR has been merged.

@smuda smuda force-pushed the granular-security branch 3 times, most recently from 2fae78a to 534d249 Compare July 12, 2023 06:59
@NissesSenap
Copy link
Collaborator

@smuda sorry for the delay with feedback on this PR.
I asked the other maintainers to take a look as well, but here are my initial thoughts.

Do we need to add some logic if a user tries to use both WATCH_NAMESPACE, WATCH_LABEL and WATCH_LABEL_CRD?

As you say, the secondary healthz endpoint isn't ideal and will be a bit painful to manage, especially in kustomize.
But nothing that is impossible.

I like the thinking of using labels on the namespace instead of providing a specific list.

@smuda
Copy link
Contributor Author

smuda commented Jul 12, 2023

I was thinking of wrapping both healthz endpoints in a singular outer endpoint. What do you think?

Yes, I can add some logic if both WATCH_NAMESPACE and the label config is specified. Or I could remove WATCH_NAMESPACE?

@NissesSenap
Copy link
Collaborator

Yeah I think that sounds like a good idea.

Something something breaking change, I'm not too keen on cutting a 6.0 release, at the same time deprecating env variables is something that I think should be okay in many cases.

But to be honest I would prefer to remove WATCH_NAMESPACE all together, it will be a bit strange to have 3 options to solve the same thing.
At the same time, WATCH_NAMESPACE seems to be the standard within operator sdk.

What do you think @pb82 ?

@pb82
Copy link
Collaborator

pb82 commented Jul 24, 2023

@NissesSenap yes, WATCH_NAMESPACE is the standard variable, at least for all Operators created with the operator-sdk.
Something like WATCH_LABEL makes sense as a more flexible way of getting the list of watched namespaces (it's still not dynamic though, the Operator needs to be restarted to discover new namespaces with the label).

I don't quite understand the difference between the proposed WATCH_LABEL and WATCH_LABEL_CRD?

@smuda
Copy link
Contributor Author

smuda commented Jul 25, 2023

WATCH_LABEL is the label where Grafana is watched while WATCH_LABEL_CRD is the label where GrafanaDashboards, GrafanaFolders and GrafanaDatasources is watched.

That way a more granular security model can be created where the grafana operator is able to view/update for example secrets where WATCH_LABEL is set on the namespace, while still managing dashboards/folders and datasources on other namespaces (with WATCH_LABEL_CRD) where secrets may be more sensitive.

@pb82
Copy link
Collaborator

pb82 commented Aug 8, 2023

@smuda I think we can merge this without adding a breaking change (removing the existing watch_namespace) once we have some documentation of the various flags. Would you mind adding a section about this here: https://github.com/grafana-operator/grafana-operator/blob/master/docs/docs/grafana.md

Thanks!

@smuda
Copy link
Contributor Author

smuda commented Aug 14, 2023

Let me just wrap healthz endpoints in a singular outer endpoint, add some to helm and run a bit more QA.

@github-actions
Copy link

This PR hasn't been updated for a while, marking as stale

@github-actions github-actions bot added the stale label Sep 14, 2023
@smuda
Copy link
Contributor Author

smuda commented Sep 14, 2023

Not really stale, just a lot at work right now.

@HVBE HVBE removed the stale label Sep 14, 2023
@github-actions
Copy link

This PR hasn't been updated for a while, marking as stale

@github-actions github-actions bot added the stale label Oct 15, 2023
@github-actions github-actions bot closed this Oct 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow split of watch namespaces
4 participants