certrotation: cover corner case when invalid secret type has necessary metadata#1687
certrotation: cover corner case when invalid secret type has necessary metadata#1687vrutkovs wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
This covers a corner case Standa found, follow up for #1681 (comment) |
|
/test unit |
vrutkovs
force-pushed
the
improve-cert-rotation-type
branch
from
5843cc5 to
3880026
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vrutkovs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
vrutkovs
force-pushed
the
improve-cert-rotation-type
branch
from
3880026 to
14409e3
Compare
|
/retest |
| // this is done before content update to prevent unexpected rollouts | ||
| if ensureMetadataUpdate(signingCertKeyPairSecret, c.Owner, c.AdditionalAnnotations) && ensureSecretTLSTypeSet(signingCertKeyPairSecret) { | ||
| needsMetadataUpdate := ensureMetadataUpdate(signingCertKeyPairSecret, c.Owner, c.AdditionalAnnotations) | ||
| needsTypeUpdate := ensureSecretTLSTypeSet(signingCertKeyPairSecret) |
There was a problem hiding this comment.
Why does ensureSecretTLSTypeSet have to mess with the content of the secret?
xref:
There was a problem hiding this comment.
ensureSecretTLSTypeSet may change secret type if its not kubernetes.io/tls
| t.Helper() | ||
| actions := client.Actions() | ||
| if len(actions) != 2 { | ||
| if len(actions) != 4 { |
There was a problem hiding this comment.
why does the create scenario sends additional get and update requests?
There was a problem hiding this comment.
Previously only one create was invoked, because ensureMetadataUpdate returned true (no ownership annotations/refs) and ensureSecretTLSTypeSet returned false (if ResourceVersion is unset no type change necessary). That means the secret was created after needNewSigningCertKeyPair was returning true.
With this change needsMetadataUpdate || needsTypeUpdate and secret is created with empty data and required annotaitons/ownerrefs, later being updated with secret contents when needNewSigningCertKeyPair returns true.
This may seem like extra actions, but it protects us from unexpected changes when secret contents and metadata are fully compliant, but secret type is still invalid. Its not possible in the standard flow for born-in-4.7-clusters, but may be possible in later cycles
| Data: map[string][]byte{"tls.crt": {}, "tls.key": {}}, | ||
| ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "signer", | ||
| ResourceVersion: "10", | ||
| Annotations: map[string]string{ |
There was a problem hiding this comment.
why the change, the scenario says update no annotations
There was a problem hiding this comment.
hmm, I should rename this to "no necessary signer annotations"
…y metadata In this case cert-rotation won't convert secret type as `ensureSecretTLSTypeSet` would never run
vrutkovs
force-pushed
the
improve-cert-rotation-type
branch
from
14409e3 to
67adba7
Compare
|
@vrutkovs: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Included in #1693 |
2 participants
In this case cert-rotation won't convert secret type as
ensureSecretTLSTypeSetwould never run