Skip to content

Conversation

@kimxogus
Copy link
Contributor

@kimxogus kimxogus commented Nov 5, 2025

In mimir helm chart with helm release name mimir, this should be mimir-rollout-operator.NAMESPACE.svc, but it's always rollout-operator.NAMESPACE.svc unless .Values.fullnameOverride is set.
This broke mimir's rollout operator and upgrading any statefulsets in the namespace.

@kimxogus kimxogus requested a review from a team as a code owner November 5, 2025 09:19
@kimxogus
Copy link
Contributor Author

kimxogus commented Nov 5, 2025

I've also removed default cert name defaults to chart fullname. Current default certification is too general name for this.

@dimitarvdimitrov
Copy link
Contributor

I've also removed default cert name defaults to chart fullname. Current default certification is too general name for this.

won't this break the existing installations? From what I saw the rollout-operator creates the certificate with that name itself. But this also means we need some secret name. I'm not sure how this will work if you change the secret name during an upgrade.

At the very least, let's do this in a separate PR. Right now I think the priority should be to get the rollout-operator working correctly in mimir-distributed 6.0.0 and the secret name is only a cosmetic change

@dimitarvdimitrov
Copy link
Contributor

i'm happy to merge this if you undo the secret name change. i think it's unnecessary for a bugfix PR

@dimitarvdimitrov dimitarvdimitrov changed the title fix(rollout-operator): Fix rollout-operator cert's dns name [rollout-operator] Always set server-tls.self-signed-cert.dns-name Nov 5, 2025
@dimitarvdimitrov
Copy link
Contributor

i'm happy to merge this if you undo the secret name change. i think it's unnecessary for a bugfix PR

CI is failing because of the same change. The generated role is now invalid

@kimxogus
Copy link
Contributor Author

kimxogus commented Nov 5, 2025

@dimitarvdimitrov That change will recreate rollout-operator pod and then the pod will create the cert secret too. This won't break existing installations.

@kimxogus
Copy link
Contributor Author

kimxogus commented Nov 5, 2025

@dimitarvdimitrov Is there a script to generate helm doc automatically?

@kimxogus kimxogus changed the title [rollout-operator] Always set server-tls.self-signed-cert.dns-name [rollout-operator] Always set server-tls.self-signed-cert.dns-name and unset default cert secret name Nov 5, 2025
Signed-off-by: tanner <[email protected]>
@kimxogus kimxogus force-pushed the fix/rollout-operator-cert-dna-name branch from f1718ba to a9fa603 Compare November 5, 2025 10:29
@kimxogus kimxogus changed the title [rollout-operator] Always set server-tls.self-signed-cert.dns-name and unset default cert secret name [rollout-operator] Always set server-tls.self-signed-cert.dns-name Nov 5, 2025
@dimitarvdimitrov
Copy link
Contributor

the CI uses this

docker run --rm --volume "$(pwd):/helm-docs" jnorwood/helm-docs:v1.8.1

i'll find a second reviewer and merge this

@dimitarvdimitrov dimitarvdimitrov merged commit f1d9e74 into grafana:main Nov 5, 2025
11 checks passed
dimitarvdimitrov added a commit to grafana/mimir that referenced this pull request Nov 5, 2025
This includes a fix for server-tls.self-signed-cert.dns-name which was
always set to `rollout-operator.NAMESPACE.svc` instead of using the
full release name. This broke rollout operator functionality when
upgrading statefulsets.

See: grafana/helm-charts#3989
@kimxogus kimxogus deleted the fix/rollout-operator-cert-dna-name branch November 5, 2025 11:46
@kimxogus
Copy link
Contributor Author

kimxogus commented Nov 5, 2025

@dimitarvdimitrov Thank you for quick review and merge. I opened a separate PR #3990

dimitarvdimitrov added a commit to grafana/mimir that referenced this pull request Nov 5, 2025
Upgrades the rollout-operator chart dependency to 0.37.1.

This includes a fix from
grafana/helm-charts#3989 where
server-tls.self-signed-cert.dns-name was always set to
`rollout-operator.NAMESPACE.svc` instead of using the full release name
(e.g., `mimir-rollout-operator.NAMESPACE.svc`). This broke the rollout
operator and prevented upgrading statefulsets.

related to #13338
dimitarvdimitrov added a commit to grafana/mimir that referenced this pull request Nov 5, 2025
Upgrades the rollout-operator chart dependency to 0.37.1.

This includes a fix from
grafana/helm-charts#3989 where
server-tls.self-signed-cert.dns-name was always set to
`rollout-operator.NAMESPACE.svc` instead of using the full release name
(e.g., `mimir-rollout-operator.NAMESPACE.svc`). This broke the rollout
operator and prevented upgrading statefulsets.

related to #13338

(cherry picked from commit 16aa2be)
dimitarvdimitrov added a commit to grafana/mimir that referenced this pull request Nov 5, 2025
…7.1 (#13361)

Upgrades the rollout-operator chart dependency to 0.37.1.

This includes a fix from
grafana/helm-charts#3989 where
server-tls.self-signed-cert.dns-name was always set to
`rollout-operator.NAMESPACE.svc` instead of using the full release name
(e.g., `mimir-rollout-operator.NAMESPACE.svc`). This broke the rollout
operator and prevented upgrading statefulsets.

closes #13338

(cherry picked from commit 16aa2be)
msgSend pushed a commit to msgSend/grafana-helm-charts that referenced this pull request Nov 7, 2025
…rafana#3989)

* fix(rollout-operator): Fix rollout-operator cert's dns name

Signed-off-by: tanner <[email protected]>

* bump patch version

Signed-off-by: tanner <[email protected]>

---------

Signed-off-by: tanner <[email protected]>
msgSend pushed a commit to msgSend/grafana-helm-charts that referenced this pull request Nov 7, 2025
…rafana#3989)

* fix(rollout-operator): Fix rollout-operator cert's dns name

Signed-off-by: tanner <[email protected]>

* bump patch version

Signed-off-by: tanner <[email protected]>

---------

Signed-off-by: tanner <[email protected]>
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.

2 participants