-
Notifications
You must be signed in to change notification settings - Fork 30
OCPBUGS-63532: UPSTREAM: <carry>: extend loopback certificate validity to three years #74
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: openshift-apiserver-4.18-kubernetes-1.31.1
Are you sure you want to change the base?
Conversation
| CertificateRequestBlockType = "CERTIFICATE REQUEST" | ||
| ) | ||
|
|
||
| // --------------------------------------------------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // --------------------------------------------------------------------------------------- |
| // | ||
| // NOTE: As part of the work to resolve https://issues.redhat.com/browse/OCPBUGS-61760 | ||
| // and https://issues.redhat.com/browse/OCPBUGS-61759 it was decided that copying | ||
| // the loopback certification creation utility function to a local | ||
| // utility function would save significant effort over cherry-picking | ||
| // the fix from https://github.com/kubernetes/kubernetes/pull/130047 to | ||
| // https://github.com/openshift/kubernetes-client-go . | ||
| // We don't expect to backport many changes on top of this and it seemed | ||
| // lower risk than switching impacted modules to our fork of client-go. | ||
| certPem, keyPem, err := GenerateSelfSignedCertKey(server.LoopbackClientServerNameOverride, nil, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe keep the full explanation in the new file and leave an end-of-line comment here (like "use forked GenerateSelfSignedCertKey with extended expiry"), to minimize the diff surface for future rebases?
| // NOTE: As part of the work to resolve https://issues.redhat.com/browse/OCPBUGS-61760 | ||
| // and https://issues.redhat.com/browse/OCPBUGS-61759 it was decided that copying | ||
| // the loopback certification creation utility function to a local | ||
| // utility function would save significant effort over cherry-picking | ||
| // the fix from https://github.com/kubernetes/kubernetes/pull/130047 to | ||
| // https://github.com/openshift/kubernetes-client-go . | ||
| // We don't expect to backport many changes on top of this and it seemed | ||
| // lower risk than switching impacted modules to our fork of client-go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much more significant than the effort of backporting that change separately is that we'd also need to make all aggregated API servers start using that client-go fork. Unlike the fork of the apiserver module, almost nothing depends on a client-go fork (https://github.com/search?q=org%3Aopenshift+openshift%2Fkubernetes-client-go+language%3A%22Go+Module%22&type=code&l=Go+Module).
Let's be clear at the top as well that the only diff we expect in GenerateSelfSignedCertKeyWithFixtures vs. client-go's v1.31.1 source is the maxAge line. I just verified it myself:
-// GenerateSelfSignedCertKeyWithFixtures creates a self-signed certificate and key for the given host.
-// Host may be an IP or a DNS name. You may also specify additional subject alt names (either ip or dns names)
-// for the certificate.
-//
-// If fixtureDirectory is non-empty, it is a directory path which can contain pre-generated certs. The format is:
-// <host>_<ip>-<ip>_<alternateDNS>-<alternateDNS>.crt
-// <host>_<ip>-<ip>_<alternateDNS>-<alternateDNS>.key
-// Certs/keys not existing in that directory are created.
func GenerateSelfSignedCertKeyWithFixtures(host string, alternateIPs []net.IP, alternateDNS []string, fixtureDirectory string) ([]byte, []byte, error) {
validFrom := time.Now().Add(-time.Hour) // valid an hour earlier to avoid flakes due to clock skew
- maxAge := time.Hour * 24 * 365 // one year self-signed certs
+
+ // NOTE: Modified from the original of 1 year to 3 years to fix
+ // - https://issues.redhat.com/browse/OCPBUGS-61760
+ // - https://issues.redhat.com/browse/OCPBUGS-61759
+ // Achieves the same result as the upstream change in https://github.com/kubernetes/kubernetes/pull/130047
+ maxAge := time.Hour * 24 * (3*365 + 1) // three year self-signed certs
baseName := fmt.Sprintf("%s_%s_%s", host, strings.Join(ipsToStrings(alternateIPs), "-"), strings.Join(alternateDNS, "-"))
certFixturePath := filepath.Join(fixtureDirectory, baseName+".crt")
@@ -228,3 +181,12 @@
}
return ss
}5cdf722 to
158711c
Compare
|
@everettraven: This pull request references Jira Issue OCPBUGS-63532, 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. |
|
@everettraven: This pull request references Jira Issue OCPBUGS-63532, 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. |
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
158711c to
c36d827
Compare
Updates the loopback certificate generation logic to extend the certificate validity to three years as a basis to fix both https://issues.redhat.com/browse/OCPBUGS-61760 and https://issues.redhat.com/browse/OCPBUGS-61759.
Verified by:
Manually verified using the steps in the bug against a cluster bot cluster launched with all three PRs.
openshift-apiserver certificate:
oauth-apiserver certificate: