OCPBUGS-44842: certrotation: set not-before/not-after annotations #1889
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vrutkovs: This pull request references Jira Issue OCPBUGS-44842, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
vrutkovs
force-pushed
the
cert-annotations-not-before-not-after
branch
3 times, most recently
from
ff84afa to
91216c7
Compare
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@vrutkovs: This pull request references Jira Issue OCPBUGS-44842, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
1 similar comment
|
/retest |
|
@vrutkovs: This pull request references Jira Issue OCPBUGS-44842, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/cherrypick release-4.18 |
|
@vrutkovs: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions 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-sigs/prow repository. |
ibihim
left a comment
ibihim
left a comment
There was a problem hiding this comment.
Technically, the wiring is fine, it makes sense to add NotBefore and NotAfter to EnsureTLSMetadataUpdate.
But the consequence is,
- that we need to invoke it twice and
- usually set the
AdditionalAnnotationshand it down and execute it, while we could easily just set it in the given Secret.
To leverage EnsureTLSMetadataUpdate, we would need to split cert creation from injecting it into the Secret or create a more general EnsureSecretUpdate function that expects a TLS cert and then updates the metadata at once.
Yes, its unfortunate but see below
It would require us to copy paste the code which does:
|
|
/lgtm |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
openshift-ci
bot
removed
the
lifecycle/stale
| // Update not-before/not-after annotations | ||
| c.AdditionalAnnotations.NotBefore = leaf.NotBefore.Format(time.RFC3339) | ||
| c.AdditionalAnnotations.NotAfter = leaf.NotAfter.Format(time.RFC3339) | ||
| _ = c.AdditionalAnnotations.EnsureTLSMetadataUpdate(&secret.ObjectMeta) |
There was a problem hiding this comment.
Should there be an unit test for this?
This ensures every secret managed by this controller has valid annotation set. Instead of analyzing potentially sensitive secret on customer cluster we should be able to tell if the certificate rotation didn't happen and certificate expired by looking into annotations
vrutkovs
force-pushed
the
cert-annotations-not-before-not-after
branch
from
91216c7 to
f42dc03
Compare
|
/approve |
1 similar comment
|
/approve |
openshift-ci
bot
added
the
approved
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Elbehery, gangwgr, jsafrane, sjenning, vrutkovs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@vrutkovs: all tests passed! Full PR test history. Your PR dashboard. Instructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
@vrutkovs: Jira Issue OCPBUGS-44842: Some pull requests linked via external trackers have merged: The following pull requests linked via external trackers have not merged: These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-44842 has not been moved to the MODIFIED state. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@vrutkovs: new pull request created: #1969 In response to this:
Instructions 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-sigs/prow repository. |
|
/cherrypick release-4.19 release-4.18 |
|
@vrutkovs: new pull request created: #1970 In response to this:
Instructions 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-sigs/prow repository. |
9 participants
This ensures every secret managed by this controller has valid
not-before/not-after annotation set.
Instead of analyzing potentially sensitive secret
on customer cluster we should be able to tell if the certificate
rotation didn't happen and certificate expired by looking into
annotations.
Tested in openshift/cluster-kube-apiserver-operator#1768 and openshift/cluster-authentication-operator#742