MartijnVisser commented on code in PR #28676:
URL: https://github.com/apache/flink/pull/28676#discussion_r3543701732
##########
flink-end-to-end-tests/flink-end-to-end-tests-restclient/src/test/java/org/apache/flink/runtime/rest/RestClientITCase.java:
##########
@@ -51,14 +56,22 @@ void testHttpsConnectionWithDefaultCerts() throws Exception
{
"apache/flink/master/flink-runtime-web/web-dashboard/package.json");
try (final RestClient restClient =
RestClient.forUrl(config, Executors.directExecutor(),
httpsUrl)) {
- restClient
- .sendRequest(
- httpsUrl.getHost(),
- 443,
- testUrlMessageHeaders,
- EmptyMessageParameters.getInstance(),
- EmptyRequestBody.getInstance())
- .get(60, TimeUnit.SECONDS);
+ try {
+ restClient
+ .sendRequest(
+ httpsUrl.getHost(),
+ 443,
+ testUrlMessageHeaders,
+ EmptyMessageParameters.getInstance(),
+ EmptyRequestBody.getInstance())
+ .get(60, TimeUnit.SECONDS);
+ } catch (ExecutionException e) {
+ // A 429/non-JSON body arrives as RestClientException only
after a successful TLS
+ // handshake; a cert/trust failure surfaces as SSLException
instead.
+ assertThat(ExceptionUtils.findThrowable(e,
RestClientException.class))
+ .as("expected an HTTP response proving the TLS
handshake succeeded")
+ .isPresent();
Review Comment:
I'd lean towards keeping the assertion rather than pinning the status to
429, for two reasons:
1. What's under test is the TLS handshake, not the HTTP status. A
`RestClientException` is only produced in `readRawResponse`, i.e. after a full
HTTP response has been received, which already proves the default-truststore
handshake completed. Any status code demonstrates that equally well, so
constraining to 429 narrows the assertion in a dimension that isn't what the
test verifies.
2. 429 is not the only shape this transient takes. GHA runners share egress
IPs, and GitHub's rate limiting also surfaces as 403 (secondary/abuse limits)
and occasionally 503 under load, not just 429. Pinning to `TOO_MANY_REQUESTS`
would very plausibly go red again on the next 403, which is the exact flakiness
class this change is trying to remove. "Extend the test when GHA behaviour
changes" would mean discovering it as a red leg on some unrelated PR first.
The assumption here is that `RestClientException` is produced only after a
full HTTP response (a genuine cert/trust failure surfaces as `SSLException` and
still fails; connect-refused/DNS/reset also still fail). I verified that
empirically against the regular endpoint, a valid-TLS non-JSON body, and
expired.badssl.com.
To address the "we should notice if GHA changes" concern without
re-narrowing, I can log the observed `getHttpResponseStatus()` so an unexpected
status is visible in the CI output without failing the leg. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
