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

refactor: move tls block to spec.client #1690

Merged
merged 5 commits into from
Oct 28, 2024
Merged

Conversation

theSuess
Copy link
Member

This change allows for tls configuration on operator-managed grafana instances.
Fixes #1675

@theSuess
Copy link
Member Author

theSuess commented Sep 24, 2024

@diranged can you try this against your setup?

You'll have to use the same insecureSkipVerify configuration as in the e2e test unless your certificate also includes the service host name

@diranged
Copy link

diranged commented Oct 7, 2024

@theSuess I can test this if you want to push an image up...

@theSuess
Copy link
Member Author

@diranged sorry for the late reply, was out for a conference the last few days.

You can use ghcr.io/grafana/grafana-operator:pr-1690 to test this change. Be sure to also update the CRDs to use the version from this branch

@diranged
Copy link

@theSuess,
Sorry for my delay too! So its easy for me to test a custom image ... however, it's trickier to test the CRD because the CRDs are actively managed and pushed by ArgoCD and kept in sync that way. If your code passes tests, can we just get it released and try it out?

@theSuess theSuess force-pushed the refactor/tls-settings-global branch from 098e184 to 9601bf7 Compare October 25, 2024 08:31
@theSuess theSuess marked this pull request as ready for review October 28, 2024 08:20
@theSuess theSuess force-pushed the refactor/tls-settings-global branch from 9601bf7 to 379fa73 Compare October 28, 2024 08:20
@theSuess theSuess enabled auto-merge October 28, 2024 08:36
@weisdd
Copy link
Collaborator

weisdd commented Oct 28, 2024

@theSuess the code seems to be fine as confirmed by e2e. My only suggestion would be to, probably, update the docs with TLS block proposal to say that it's now advised to use the top-level block.

@theSuess theSuess force-pushed the refactor/tls-settings-global branch from 60b8fd6 to fb86b8b Compare October 28, 2024 09:31
@theSuess theSuess force-pushed the refactor/tls-settings-global branch from fb86b8b to 2bc49a6 Compare October 28, 2024 09:43
@theSuess theSuess added this pull request to the merge queue Oct 28, 2024
Merged via the queue into master with commit b07526a Oct 28, 2024
14 checks passed
@theSuess theSuess deleted the refactor/tls-settings-global branch October 28, 2024 10:15
@diranged
Copy link

Closing the loop here - this works!

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.

[Bug] Since 5.13.0 - Grafana Operator cannot manage TLS protected internal Grafanas
3 participants