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.

Sign up for GitHub

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

Jump to bottom

Accidentally removed CA should trigger crt recreation (and rolling update) #2756

Merged
merged 2 commits into from Apr 1, 2020
Merged

Accidentally removed CA should trigger crt recreation (and rolling update) #2756

merged 2 commits into from Apr 1, 2020

Conversation

sknot-rh
Copy link
Member

Signed-off-by: Stanislav Knot sknot@redhat.com

Type of change

  • Bugfix

Description

Fixes #542

Checklist

  • Update/write design documentation in ./design
  • 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

@sknot-rh
Accidentally removed CA should trigger crt recreation (and rolling up…
aa06f03 
…date)

Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh sknot-rh added this to the 0.18.0 milestone Mar 27, 2020
samuel-hawker
samuel-hawker reviewed Mar 27, 2020
View reviewed changes
Copy link
Contributor

@samuel-hawker samuel-hawker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just left some thoughts, hope they are useful. :)

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ModelUtils.java Outdated
@@ -234,14 +234,14 @@ public static Secret buildSecret(ClusterCa clusterCa, Secret secret, String name
reasons.add("certificate doesn't exist yet");
shouldBeRegenerated = true;
} else {
if (clusterCa.certRenewed() || (clusterCa.isExpiring(secret, keyCertName + ".crt") && isMaintenanceTimeWindowsSatisfied)) {
if (clusterCa.keyCreated() || clusterCa.certRenewed() || (clusterCa.isExpiring(secret, keyCertName + ".crt") && isMaintenanceTimeWindowsSatisfied)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am reading the logic correctly, might it make sense to put isMaintenanceTimeWindowsSatisfied first, i.e

if (isMaintenanceTimeWindowsSatisfied && ...

As the prior three checks dont matter if this is false?

Copy link
Member Author

@sknot-rh sknot-rh Mar 30, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isMaintenanceTimeWindowsSatisfied is tied just for this part of condition: (clusterCa.isExpiring(secret, keyCertName + ".crt") && isMaintenanceTimeWindowsSatisfied)
However it makes sense to move is as the first one in this scope. Good catch.

operator-common/src/main/java/io/strimzi/operator/cluster/model/Ca.java Outdated
@@ -737,6 +741,10 @@ public boolean keyReplaced() {
return renewalType == RenewalType.REPLACE_KEY;
}

public boolean keyCreated() {
return renewalType == RenewalType.CREATE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly stylistic but I believe .equals is generally preferred to == as we can choose to override how we compare objects and can avoid object hashes not matching https://stackoverflow.com/questions/7520432/what-is-the-difference-between-and-equals-in-java
99% of the time it doesn't matter but thought it was worth a mention.

systemtest/src/test/java/io/strimzi/systemtest/RollingUpdateST.java Outdated
@@ -777,6 +778,41 @@ void testTriggerRollingUpdateAfterOverrideBootstrap() throws CertificateExceptio
// TODO: send and recv messages via this new bootstrap (after client builder) https://github.com/strimzi/strimzi-kafka-operator/pull/2520
}

@Test
void testAccidentallyRemovedCaTriggersRollingUpdate() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be overly nit-picky, but the cause of why it is removed could be seen as not part of the 'test'
I would opt to have it be called something like

testClusterCaRemovedTriggersRollingUpdate

and then have a documented comment explaining the context of this test is it being accidentally removed.

operator-common/src/main/java/io/strimzi/operator/cluster/model/Ca.java
@@ -737,6 +741,10 @@ public boolean keyReplaced() {
return renewalType == RenewalType.REPLACE_KEY;
}

public boolean keyCreated() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isKeyCreated would be slightly more idiomatic, but given the other similar methods are not using is I can understand why you matched the convention

see-quick
see-quick approved these changes Mar 28, 2020
View reviewed changes
Copy link
Member

@see-quick see-quick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway looks good, and thanks for creating the ST :)))

systemtest/src/test/java/io/strimzi/systemtest/RollingUpdateST.java Outdated Show resolved Hide resolved
systemtest/src/test/java/io/strimzi/systemtest/RollingUpdateST.java Outdated Show resolved Hide resolved
ppatierno
ppatierno approved these changes Mar 30, 2020
View reviewed changes
Copy link
Member

@ppatierno ppatierno left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Nothing to add than the comments already left.

@sknot-rh
comments
59a459e 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh sknot-rh added the ready for merge Label for PRs which are ready for merge label Apr 1, 2020
@scholzj scholzj merged commit 0015795 into strimzi:master Apr 1, 2020
@sknot-rh sknot-rh deleted the delete-ca-triggers-ru branch April 21, 2020 12:11
javiercri pushed a commit to javiercri/strimzi-kafka-operator that referenced this pull request Oct 11, 2020
@sknot-rh
Accidentally removed CA should trigger crt recreation (and rolling up…
41b168c 
…date) (strimzi#2756)

* Accidentally removed CA should trigger crt recreation (and rolling update)

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* comments

Signed-off-by: Stanislav Knot <sknot@redhat.com>
Signed-off-by: Javier Criado <javier.criado@MAC00083.home>
klalafaryan pushed a commit to klalafaryan/strimzi-kafka-operator that referenced this pull request Oct 21, 2020
@sknot-rh @klalafaryan
Accidentally removed CA should trigger crt recreation (and rolling up…
1bbf58c 
…date) (strimzi#2756)

* Accidentally removed CA should trigger crt recreation (and rolling update)

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* comments

Signed-off-by: Stanislav Knot <sknot@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuel-hawker samuel-hawker samuel-hawker left review comments

@scholzj scholzj scholzj approved these changes

@ppatierno ppatierno ppatierno approved these changes

@see-quick see-quick see-quick approved these changes

@tombentley tombentley Awaiting requested review from tombentley

Assignees
No one assigned
Labels
ready for merge Label for PRs which are ready for merge
Projects
None yet
Milestone
0.18.0
Development

Successfully merging this pull request may close these issues.

Regenerate certificates when the internal CA one is deleted by accident
5 participants
@sknot-rh @scholzj @ppatierno @see-quick @samuel-hawker