OCPBUGS-49441: Use different certificates for gRPC interoperability test routes #29757
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci-robot
added
jira/severity-critical
|
@alebedev87: This pull request references Jira Issue OCPBUGS-49441, which is valid. 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. DetailsIn 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
bot
added
the
approved
alebedev87
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
alebedev87
force-pushed
the
ocpbugs-49441-grpc-1-67-certs
branch
from
1b8896b to
8525a88
Compare
Contributor
Author
|
/retest |
|
Job Failure Risk Analysis for sha: 8525a88
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 8525a88
New tests seen in this PR at sha: 8525a88
|
alebedev87
force-pushed
the
ocpbugs-49441-grpc-1-67-certs
branch
from
8525a88 to
72b4ed0
Compare
Contributor
Author
|
/retest |
alebedev87
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
/retest |
|
/assign |
|
Job Failure Risk Analysis for sha: 72b4ed0
|
Contributor
Author
|
/retest-required |
Thealisyed
reviewed
May 26, 2025
| pemCrt, err := certgen.MarshalCertToPEMString(tlsCrtData) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| _, tlsCrt2Data, tlsPrivateKey2, err := certgen.GenerateKeyPair("Root CA", notBefore, notAfter) |
There was a problem hiding this comment.
Why is a new Root CA being generated here? Is tlsCrt2Data intended to be a different root CA or a certificate signed by the first root CA?
Contributor
Author
There was a problem hiding this comment.
Why is a new Root CA being generated here?
That's how GenerateKeyPair works. It generates a self signed root CA certificate and then uses it for the test certificate (which we get as second return value).
Is tlsCrt2Data intended to be a different root CA or a certificate signed by the first root CA?
It's intended to be a different certificate. I think that it may still be possible to sign it with the same root CA though. But I stick to what GenerateKeyPair does.
Having different certificates for secure routes instructs router to enable alpn option in crt-list command on HAProxy frontends (e.g. fe_sni). This results into the usage of ALPN TLS extension for the protocol negotiation. Without ALPN, grpc client will refuse to establish a connection to OpenShift router.
test/extended/router/grpc-interop.go
Outdated
Comment on lines
265
to
266
| Key: derKey2, | ||
| Certificate: pemCrt2, |
There was a problem hiding this comment.
Why use those formats (pem and der) for the cert and key?
Contributor
Author
There was a problem hiding this comment.
Those are the formats returned by crypto/x509's CreateCertificate function. Both can be used to encode certificates or keys. DER is binary format while PEM is base64. I didn't see a lot the usage of DER for route's keys but it seems to be accepted by the API and router validation. Also, it is not that important when we set the key programmatically (not from a YAML manifest). So, I prefer to be consistent across the router's tests and keep using DER (http2 test does use DER format too).
Contributor
Author
There was a problem hiding this comment.
I followed up on this one. It turned out that MarshalCertToDERFormat was a misnomer.
alebedev87
commented
May 26, 2025
Contributor
Author
alebedev87
left a comment
alebedev87
left a comment
There was a problem hiding this comment.
@Thealisyed : Thanks for the review! Here are the answers, feel free to follow up in the comments.
| pemCrt, err := certgen.MarshalCertToPEMString(tlsCrtData) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| _, tlsCrt2Data, tlsPrivateKey2, err := certgen.GenerateKeyPair("Root CA", notBefore, notAfter) |
Contributor
Author
There was a problem hiding this comment.
Why is a new Root CA being generated here?
That's how GenerateKeyPair works. It generates a self signed root CA certificate and then uses it for the test certificate (which we get as second return value).
Is tlsCrt2Data intended to be a different root CA or a certificate signed by the first root CA?
It's intended to be a different certificate. I think that it may still be possible to sign it with the same root CA though. But I stick to what GenerateKeyPair does.
Having different certificates for secure routes instructs router to enable alpn option in crt-list command on HAProxy frontends (e.g. fe_sni). This results into the usage of ALPN TLS extension for the protocol negotiation. Without ALPN, grpc client will refuse to establish a connection to OpenShift router.
test/extended/router/grpc-interop.go
Outdated
Comment on lines
265
to
266
| Key: derKey2, | ||
| Certificate: pemCrt2, |
Contributor
Author
There was a problem hiding this comment.
Those are the formats returned by crypto/x509's CreateCertificate function. Both can be used to encode certificates or keys. DER is binary format while PEM is base64. I didn't see a lot the usage of DER for route's keys but it seems to be accepted by the API and router validation. Also, it is not that important when we set the key programmatically (not from a YAML manifest). So, I prefer to be consistent across the router's tests and keep using DER (http2 test does use DER format too).
alebedev87
force-pushed
the
ocpbugs-49441-grpc-1-67-certs
branch
from
72b4ed0 to
44a9087
Compare
Contributor
Author
|
/retest-required |
|
Thanks for the answers and for the follow up PR for /lgtm |
|
Job Failure Risk Analysis for sha: 44a9087
|
|
Job Failure Risk Analysis for sha: 44a9087
|
|
Job Failure Risk Analysis for sha: 44a9087
|
|
Job Failure Risk Analysis for sha: 44a9087
|
|
Job Failure Risk Analysis for sha: 44a9087
|
1 similar comment
|
Job Failure Risk Analysis for sha: 44a9087
|
…ability test routes Before this change the gRPC routes were using the same certificate which prevented the usage of ALPN to avoid the connection coalescing. This commit introduces different certificates for edge and reencrypt routes.
alebedev87
force-pushed
the
ocpbugs-49441-grpc-1-67-certs
branch
from
44a9087 to
cf83316
Compare
Contributor
Author
|
Rebased from |
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alebedev87, Thealisyed The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
Contributor
|
@alebedev87: The following tests failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
|
Job Failure Risk Analysis for sha: cf83316
|
|
Job Failure Risk Analysis for sha: cf83316
|
Member
|
/retest-required |
|
Job Failure Risk Analysis for sha: cf83316
|
openshift-merge-bot
bot
merged commit 9c672e0
into
openshift:main
40 of 59 checks passed
|
@alebedev87: Jira Issue OCPBUGS-49441: 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-49441 has not been moved to the MODIFIED state. DetailsIn 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. |
Contributor
|
[ART PR BUILD NOTIFIER] Distgit: openshift-enterprise-tests |
Contributor
|
/jira refresh |
|
@melvinjoseph86: Jira Issue OCPBUGS-49441: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-49441 has been moved to the MODIFIED state. DetailsIn 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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this change the gRPC routes were using the same certificate which prevented the usage of ALPN to avoid the connection coalescing. This PR introduces different certificates for edge and reencrypt routes.
PR to test the fix in combination with grpc-go
v1.67.0: #29780.