Only update cluster-ca-key-generation from CaReconciler #10749

katheris · 2024-10-22T10:39:43Z

Type of change

Select the type of your PR

Bugfix

Description

When rolling pods to trust new CA cert signed by new CA key only update the cluster-ca-key-generation
annotation. Do not update cluster-ca-cert-generation or clients-ca-cert-generation.

In PR #10711 I added logic to update the cluster-ca-cert-gen , clients-ca-cert-gen and cluster-ca-key-gen annotations before rolling the pods. However, the pods are being rolled to trust a new CA cert that is signed by a new CA key. They are still presenting certificates that were issued by the old CA cert.

This means that if the reconcile fails after rolling the Kafka pods as part of the CaReconciler rolling update, but before it runs the KafkaReconciler logic, when it comes back in it would incorrectly think that the new CA cert was used everywhere and remove the old one from the ca-cert Secret.

We should only update the cluster-ca-key-gen annotation before doing the rolling update

Checklist

Please go through this checklist and make sure all applicable tasks have been done

Write tests
Make sure all tests pass
Update documentation
Check RBAC rights for Kubernetes / OpenShift roles
Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
Reference relevant issue(s) and close them after merging
Update CHANGELOG.md
Supply screenshots for visual changes, such as Grafana dashboards

When rolling pods to trust new CA cert signed by new CA key only update the cluster-ca-key-generation annotation. Do not update cluster-ca-cert-generation or clients-ca-cert-generation. Signed-off-by: Katherine Stanley <[email protected]>

scholzj

LGTM. Regression tests should be run once the CI queues are empty.

Skip restarting pods that have an up-to-date CA key annotation if no other restart reasons present. Signed-off-by: Katherine Stanley <[email protected]>

scholzj · 2024-10-22T18:10:35Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/CaReconciler.java

            LOGGER.debugCr(reconciliation, "Rolling Pod {} due to {}", pod.getMetadata().getName(), podRollReasons.getReasons());
            return podRollReasons;


It is better to wrap this into else branch to have return validation.

scholzj · 2024-10-22T18:14:36Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/CaReconciler.java

@@ -575,6 +575,15 @@ Future<Void> rollingUpdateForNewCaKey() {
                false,
                eventPublisher
        ).rollingRestart(pod -> {
+            if (podRollReasons.getReasons().size() == 1 && podRollReasons.contains(RestartReason.CLUSTER_CA_CERT_KEY_REPLACED)) {


Why is this needed? Why do we even need the rolling update here for Clients CA key replacement?

It is not used internally, so we do not need to establish trust

It should be enough to roll this in the regular rolling update where it would anyway roll to change the annotation, or?

katheris · 2024-10-23T09:03:37Z

I'm investigating the failure, I've worked out what's causing it, just trying to determine how best to address it

* Add explicit else clauses. * Update KubernetesRestartEvent tests to use the force-replace annotation to trigger CA key replacement. Signed-off-by: Katherine Stanley <[email protected]>

scholzj · 2024-10-23T21:50:38Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/CaReconciler.java

+            if (podRollReasons.getReasons().size() == 1 && podRollReasons.contains(RestartReason.CLUSTER_CA_CERT_KEY_REPLACED)) {
+                // Check if this pod has been rolled previously
+                int clusterCaKeyGeneration = clusterCa.keyGeneration();
+                int podClusterCaKeyGeneration = Annotations.intAnnotation(pod, Ca.ANNO_STRIMZI_IO_CLUSTER_CA_KEY_GENERATION, clusterCaKeyGeneration);
+                if (clusterCaKeyGeneration == podClusterCaKeyGeneration) {
+                    LOGGER.debugCr(reconciliation, "Not rolling Pod {} since the CA cert key generation is correct.", pod.getMetadata().getName());
+                    return RestartReasons.empty();
+                } else {
+                    LOGGER.debugCr(reconciliation, "Rolling Pod {} due to {}", pod.getMetadata().getName(), podRollReasons.getReasons());
+                    return podRollReasons;
+                }
+            } else {
+                LOGGER.debugCr(reconciliation, "Rolling Pod {} due to {}", pod.getMetadata().getName(), podRollReasons.getReasons());
+                return podRollReasons;
+            }


So, does this need to be updated to handle both reasons = both annotations? Otherwise, why are we patching the clients CA generation in the PodSet?

Signed-off-by: Katherine Stanley <[email protected]>

Update the CaReconciler to only roll pods when the cluster CA key is replaced, not when the clients CA key is replaced Signed-off-by: Katherine Stanley <[email protected]>

katheris · 2024-10-24T16:07:06Z

@scholzj @ppatierno I've pushed some updates to only roll when the cluster CA key is replaced, not the clients CA key (as we can leave that to the KafkaRoller) and only use the annotations, not the keyReplaced() method. I haven't done any manual testing yet, so take a look if you want but if you'd prefer to wait til I've done some manual testing for the clients CA key I'll update here when I've done that.

scholzj · 2024-10-24T23:00:52Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/RestartReason.java

-    /**
-     * Clients CA private key was replaced
-     */
-    CLIENT_CA_CERT_KEY_REPLACED("Trust new clients CA certificate signed by new key"),


I guess this should be removed from docs as well (search for ClientCaCertKeyReplaced).

scholzj · 2024-10-24T23:09:08Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/CaReconciler.java

-
-        // cluster CA needs to be fully trusted
-        // it is coming from a cluster CA key replacement which didn't end successfully (i.e. CO stopped) and we need to continue from there
-        if (clusterCa.keyReplaced() || isClusterCaNeedFullTrust) {


I hope we are 100% sure the clusterCa.keyReplaced() is not needed :-o

scholzj · 2024-10-24T23:09:19Z

/azp run regression

azure-pipelines · 2024-10-24T23:09:31Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2024-10-24T23:09:36Z

/azp run zookeeper-regression

azure-pipelines · 2024-10-24T23:09:47Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2024-10-25T07:48:39Z

@katheris Looks like the tests related to CA renewal failed: SecurityST.testAutoReplaceClientsCaKeysTriggeredByAnno

katheris added this to the 0.44.0 milestone Oct 22, 2024

scholzj approved these changes Oct 22, 2024

View reviewed changes

scholzj requested a review from ppatierno October 22, 2024 12:15

ppatierno approved these changes Oct 22, 2024

View reviewed changes

Skip updated pods during CA key replacement

5264ee0

Skip restarting pods that have an up-to-date CA key annotation if no other restart reasons present. Signed-off-by: Katherine Stanley <[email protected]>

scholzj reviewed Oct 22, 2024

View reviewed changes

Address scholzj review comments

3a0b308

* Add explicit else clauses. * Update KubernetesRestartEvent tests to use the force-replace annotation to trigger CA key replacement. Signed-off-by: Katherine Stanley <[email protected]>

katheris force-pushed the fixCaCertAnnotationPatching branch from 43d8730 to b7c43ba Compare October 23, 2024 15:59

scholzj reviewed Oct 23, 2024

View reviewed changes

Add test for KafkaRoller restart function

54aa4d9

Signed-off-by: Katherine Stanley <[email protected]>

katheris force-pushed the fixCaCertAnnotationPatching branch from 10b951e to ee009e1 Compare October 24, 2024 15:45

CaReconciler only rolls for pods for cluster CA

65debe5

Update the CaReconciler to only roll pods when the cluster CA key is replaced, not when the clients CA key is replaced Signed-off-by: Katherine Stanley <[email protected]>

katheris force-pushed the fixCaCertAnnotationPatching branch from ee009e1 to 65debe5 Compare October 24, 2024 16:04

scholzj reviewed Oct 24, 2024

View reviewed changes

scholzj mentioned this pull request Oct 25, 2024

Revert "Annotate StrimziPodSets before rolling during CA renewal" #10762

Merged

scholzj modified the milestones: 0.44.0, 0.45.0 Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only update cluster-ca-key-generation from CaReconciler #10749

Only update cluster-ca-key-generation from CaReconciler #10749

katheris commented Oct 22, 2024

scholzj left a comment

scholzj Oct 22, 2024

scholzj Oct 22, 2024

katheris commented Oct 23, 2024

scholzj Oct 23, 2024

katheris commented Oct 24, 2024

scholzj Oct 24, 2024

scholzj Oct 24, 2024

scholzj commented Oct 24, 2024

azure-pipelines bot commented Oct 24, 2024

scholzj commented Oct 24, 2024

azure-pipelines bot commented Oct 24, 2024

scholzj commented Oct 25, 2024

		LOGGER.debugCr(reconciliation, "Rolling Pod {} due to {}", pod.getMetadata().getName(), podRollReasons.getReasons());
		return podRollReasons;

Only update cluster-ca-key-generation from CaReconciler #10749

Are you sure you want to change the base?

Only update cluster-ca-key-generation from CaReconciler #10749

Conversation

katheris commented Oct 22, 2024

Type of change

Description

Checklist

scholzj left a comment

Choose a reason for hiding this comment

scholzj Oct 22, 2024

Choose a reason for hiding this comment

scholzj Oct 22, 2024

Choose a reason for hiding this comment

katheris commented Oct 23, 2024

scholzj Oct 23, 2024

Choose a reason for hiding this comment

katheris commented Oct 24, 2024

scholzj Oct 24, 2024

Choose a reason for hiding this comment

scholzj Oct 24, 2024

Choose a reason for hiding this comment

scholzj commented Oct 24, 2024

azure-pipelines bot commented Oct 24, 2024

scholzj commented Oct 24, 2024

azure-pipelines bot commented Oct 24, 2024

scholzj commented Oct 25, 2024