-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Avoid unnecessary rolling updates when replacing custom CA #10377
Avoid unnecessary rolling updates when replacing custom CA #10377
Conversation
…trimzi#10364 Signed-off-by: Jakub Scholz <[email protected]>
/azp run regression |
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Jakub Scholz <[email protected]>
/azp run regression |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -251,6 +251,8 @@ Future<Void> reconcileCas(Clock clock) { | |||
} else if (secretName.equals(KafkaResources.kafkaSecretName(reconciliation.name()))) { | |||
clusterCaSecrets.add(secret); | |||
clientsCaSecrets.add(secret); | |||
} else if (secretName.equals(KafkaResources.clusterOperatorCertsSecretName(reconciliation.name()))) { | |||
// The CO certificate is excluded as it is renewed in a separate cycle | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this behaviour happened due to the change from this refactor? #10023 What secret are we now catching in the else
here? Could we not remove the else
clause instead so we explicitly add the Secrets we care about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this behaviour happened due to the change from this refactor? #10023
No, it is not related to #10023. From my point of view, it originates from #6180 (that said, IIRC it did not worked at all before #6180, so it is not as if #6180 broke this)
What secret are we now catching in the else here? Could we not remove the else clause instead so we explicitly add the Secrets we care about?
Everything not listed above it. KE, CC, ETO, EUO, ZOO. I do not think we want to have a dedicated check for each of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, makes sense
Type of change
Description
This PR updates how we handle renewals / replacing of custom cluster CA. The delayed renewal of the Cluster Operator certificate was causing the replacement event to be detected twice:
This PR changes the behavior and ignores the Cluster Operator certificate generation. That helps to avoid the whole second run that is unnecessary (The cluster operator certificate is still updated but without the whole CA replacement process).
This change should affect only custom CAs.
This should resolve #10364.
Checklist