-
Notifications
You must be signed in to change notification settings - Fork 447
OCPBUGS-59968: Cert Controller should live fetch SAN IPs during cert rotation #5245
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
base: main
Are you sure you want to change the base?
Conversation
@djoshy: This pull request references Jira Issue OCPBUGS-59968, 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: 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. |
Skipping CI for Draft Pull Request. |
bc9a268
to
278d78d
Compare
@djoshy: This pull request references Jira Issue OCPBUGS-59968, 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. |
278d78d
to
a3e78f3
Compare
Thanks for the review, @pablintino I've incorporated the changes; let me know what you think! |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djoshy, pablintino 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 |
/hold holding for QE |
@djoshy: The following tests failed, say
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. |
/jira refresh The requirements for Jira bugs have changed (Jira issues linked to PRs on main branch need to target different OCP), recalculating validity. |
@openshift-bot: This pull request references Jira Issue OCPBUGS-59968, which is invalid:
Comment 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. |
/jira refresh |
@djoshy: This pull request references Jira Issue OCPBUGS-59968, 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. |
/unhold |
This takes inspiration from https://github.com/openshift/cluster-kube-apiserver-operator/blob/main/pkg/operator/certrotationcontroller/dynamic_serving.go. I did not want to vendor a whole package for that type, but most of the mechanism is similar to that; with a few adjustments for it to work with the MCO's cert controller. I also added a unit test for this particular case.
I also chose not to listen on the ARO resource, although I can include it if requested. Since we want to eventually move from vendoring non openshift API, I figured it wasn't worth plumbing that through.
How to verify:
Existing cert rotation mechanisms should remain unchanged. The easiest way to verify this is that installs and upgrades to this PR should be able to successfully scale up nodes. Examining the cert controller logs and the MCO cert objects should confirm this. (see #4669 for what to look for)
I'm not sure there is an easy way to test hostnames changes on a live cluster, edits to the
apiServerInternalURI
in infrastructure object is immediately stomped by thecluster-config-operator
. If somehow we could even force an edit, it is likely to break a number of other configurations on the cluster. So I'm open to other ideas here 😄