-
Notifications
You must be signed in to change notification settings - Fork 460
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
Issue 3138 - Conformance Tests for BackendTLSPolicy - normative #3212
base: main
Are you sure you want to change the base?
Issue 3138 - Conformance Tests for BackendTLSPolicy - normative #3212
Conversation
Skipping CI for Draft Pull Request. |
76c8e10
to
6d9ab9e
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: candita The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test pull-gateway-api-verify |
1 similar comment
/test pull-gateway-api-verify |
0ec34a8
to
75551a0
Compare
/test pull-gateway-api-verify |
75551a0
to
7626aaa
Compare
/test pull-gateway-api-verify |
7626aaa
to
1bc71f0
Compare
/test pull-gateway-api-verify |
1bc71f0
to
99e7eac
Compare
/test pull-gateway-api-verify |
99e7eac
to
b774245
Compare
/test pull-gateway-api-verify |
b774245
to
91488aa
Compare
/test pull-gateway-api-verify |
/test pull-gateway-api-test |
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.
b8ea586
to
c2bb7ec
Compare
// If the request was made to URI backendTLS, then get the server name indication and | ||
// add it to the RequestAssertions. It will be echoed back later. | ||
if strings.Contains(r.RequestURI, "backendTLS") { | ||
sni, err = sniffForSNI(r.RemoteAddr) |
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.
I don't get how this works. Are we supposed to send an HTTP request and then send a TLS request later?
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.
If we are just serving this as TLS already you can just do r.TLS.ServerName
I think
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.
The idea is that you send an HTTP or HTTPS request and the BackendTLSPolicy adds what is needed for the backend request to succeed (certificate and SNI). The echo service finds the SNI and echoes it back to the test to prove the BackendTLSPolicy was used.
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.
I still need to dig into this a bit deeper, but I think #3212 (comment) is relevant to this question.
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.
The logic seems to be: on an HTTP request, on a new TCP listener, accept a new connection, get the TLS handshake data, then close the connection.
Should it not be: accept an HTTPS request and report the SNI?
0ad1310
to
63b3c23
Compare
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.
Looking good! Thanks @candita!!! Mostly minor comments.
Also I was digging into the test details a bit when I got pulled away from a meeting so I still need to come back and do another round of review here, but please check my comments and LMKWYT 👍
return fmt.Errorf("failed creating key: %w", err) | ||
} | ||
return 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.
I know it's sometimes hard to justify the value, but ideally we should add some unit tests for these new functions. If nothing else, because it's a good habit.
Response: http.Response{StatusCode: 200}, | ||
Backend: "infra-backend-v1", | ||
Namespace: "gateway-conformance-infra", | ||
SNI: "abc.example.com", |
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.
We're copying this string literal in several places, and it seems like we're tying it to test cases where TLS is in play. Perhaps we should
- make the hostname something more explicit, like perhaps including "gateway api" and "tls testing" or something?
- use a constant to keep track and use it in the multiple places it's being referred to right now
Namespace: "gateway-conformance-infra", | ||
SNI: "abc.example.com", | ||
}) | ||
}) |
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.
Digging into the MakeRequestAndExpectEventuallyConsistentResponse
code one thing I noticed is that it appears that is always sends the http://
scheme?
Meaning: we're sending requests to the Gateway in the clear, and then proving that the Gateway added the correct backend TLS configuration on the backend and then served the response back out in the clear.
So (if I'm not misunderstanding anything) that's a perfectly valid test but to me it kind of begs the question: just for completeness should we not also test when the frontend connection has a distinct TLS configuration and verify the fingerprint on the clientside, and then verify the different fingerprint on the server side just to to thoroughly prove that the implementations are working properly?
Note that when I dug into this I was in a bit of a rush because I had a meeting about to start, so it's possible I'm misunderstanding, but take a look and LMKWYT and I'll come back and take another look at this myself soon 🤔
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.
Yes, I agree, except we have a separate test for frontend TLS in TLSRouteSimpleSameNamespace This PR adds a test just for backend TLS. Adding a test for both frontend and backend TLS is needed.
There are a couple of subtests that are missing. I'd like to get just one merged before following up with the others. This is my to do list:
- add a test case where the backend tls validation is misconfigured and ensure it returns an error
- add a test case where frontend connection is also TLS
- add a test case where frontend connection is also TLS and misconfigured
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.
Ok cool, thanks for the heads up. Those test cases sound good, if you think it's feasible I might just add the point that we should test the fingerprints at each level as this helps to ensure more definitively everything is working exactly as expected.
I'm very much down with getting one valuable test in for now and follow up with the others. I would just ask if they're not already, get those test cases laid out in a follow-up issue somewhere, and then please feel free to consider this comment resolved at your discretion. 👍
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.
I'm not sure if you saw that the test cases were laid out, debated, and updated in #3138. And based on your feedback I can add:
- verify fingerprints on both frontend connection and backend connection, where each has a distinct but correct TLS configuration. (Use edge-terminated TLS and BackendTLSPolicy together)
- misconfigure frontend tls validation and ensure it returns an error
- misconfigure backend tls validation and ensure it returns an error
Do I understand your point?
BackendTLSPolicy.
conformance/base/manifests.yaml - fix yaml conformance/tests/backendtlspolicy.yaml - fix yaml conformance/tests/tlsroute-simple-same-namespace.go - rename cert for sharing conformance/utils/suite/conformance.go - fix a bug in cleanup-base-resources flag application conformance/utils/suite/suite.go - rename cert for sharing
63b3c23
to
19ff922
Compare
What type of PR is this?
/kind test
/area conformance
What this PR does / why we need it:
Add a normative test of Gateway API BackendTLSPolicy implementations.
Which issue(s) this PR fixes:
Fixes #3138
Does this PR introduce a user-facing change?: