Repository navigation
[FLINK-40279][connector-http] Add HTTP sink OIDC, proxy and HTTP version options - #58
bharathgunapati wants to merge 1 commit into
Conversation
53c3ecf to
7b6a1ae
Compare
b4865ac to
852b683
Compare
| public static final String SINK_HTTP_TIMEOUT_SECONDS = | ||
| FLINK_CONNECTOR_HTTP_SINK + "request.timeout"; | ||
|
|
||
| public static final String SINK_QUERY_HTTP_VERSION = FLINK_CONNECTOR_HTTP_SINK + "http-version"; |
There was a problem hiding this comment.
we should not have QUERY in the variable name as it is confusing
There was a problem hiding this comment.
Agreed. I renamed it to SINK_HTTP_VERSION. The lookup side still has SOURCE_LOOKUP_QUERY_HTTP_VERSION. I left that one alone to keep this PR focused on the sink, but I can rename it in a follow-up if you want both to match.
| private Duration oidcExpiryReduction = | ||
| OidcAccessTokenManager.DEFAULT_TOKEN_EXPIRATION_REDUCTION; | ||
|
|
||
| /** Created on first use so the cached token survives across requests but is not serialized. */ |
There was a problem hiding this comment.
is the comment true that the cached token survives. It seemed to work before when we did not cache the token manager. I think caching the token manager makes sense.
There was a problem hiding this comment.
Yes, the comment is true, but it was too short, so I rewrote it.
What happens now: the token manager is created once per preprocessor instance and kept. Every lookup that goes through the same instance reuses the cached token until it expires. The field is transient, so after deserialization it is created again and a new token is fetched on the next request. The test shouldWorkAfterSerialization covers that case.
Before this PR it did work, but it fetched a new token for every lookup. The polling client created a new preprocessor for each lookup, and the preprocessor created a new token manager on each call, so nothing was ever reused. Now the polling client creates the preprocessor once and the preprocessor keeps its token manager. That is what makes the cache real.
| | http.security.oidc.token.request | optional | OIDC `Token Request` body in `application/x-www-form-urlencoded` encoding | | ||
| | http.security.oidc.token.endpoint.url | optional | OIDC `Token Endpoint` url, to which the token request will be issued | | ||
| | http.security.oidc.token.expiry.reduction | optional | OIDC tokens will be requested if the current time is later than the cached token expiry time minus this value. | | ||
| | http.security.oidc.token.expiry.reduction | optional | OIDC tokens will be requested if the current time is later than the cached token expiry time minus this value. The default is `30s`. | |
There was a problem hiding this comment.
am I right in saying the documented and coded default was 1 second - we are changing it to 30 seconds. Looks like we need to be consistent with existing docs and use 1 second not 30
There was a problem hiding this comment.
You are right that the old default was 1 second in both the code and the docs. I would like to keep 30 seconds. Here is why.
Before this PR the 1 second default did nothing. The lookup fetched a new token on every request, so no token ever lived long enough to get close to its expiry. Now tokens are cached, so this setting is what protects the last requests before a token expires. With 1 second, a request sent just before the cutoff can reach the server after the token has already expired. One second is not enough to cover the request time, the token endpoint time, and clock differences between Flink and the identity provider. The user then gets a 401 that they cannot explain from their config. 30 seconds is the usual value in OIDC clients.
The cost is small. A token is refreshed 29 seconds earlier than before. Tokens usually live for 5 to 60 minutes, so the extra token calls are negligible. Jobs that set http.security.oidc.token.expiry.reduction themselves are not affected. If a token's expires_in is 30 seconds or less, the full lifetime is used, so short test tokens still work. The change is listed in the compatibility note in the PR description.
If you still prefer 1 second in this PR, I will revert it and open a follow-up Jira for the default. But I think users are better off with 30 seconds from the first release where caching works.
0053554 to
6075be7
Compare
|
@davidradl Thanks for the review. All three comments are addressed in the latest push, see the threads above. One more change that is not from the review: the token request had no timeout, and its HTTP client had no connect timeout. If the identity provider did not answer, the sink writer thread would wait forever. The token request now uses the connector's request timeout: All of this also runs end to end in a Flink mini-cluster in the examples repo, see commit 760b181 and I also added two lines to the Restrictions section: the token request does not use the proxy or TLS trust options, and it is bounded by the request timeout. Making the token request use the proxy and trust settings is a good follow-up for lookup and sink together. |
Cover the sink options added in apache/flink-connector-http#58 end to end against the embedded server, which now also plays an OIDC token endpoint, a bearer-protected endpoint and an HTTP proxy that requires basic auth. - MockHttpServer: responder abstraction, request header recording, token endpoint (token-1, token-2, ... with configurable lifetime and delay), bearer-protected endpoint, 407-challenging proxy. - DataStreamHttpSinkIT: bearer token added and reused, refreshed when expired, replaces a configured Authorization header, token endpoint timeout fails the job without sending a sink request, proxy with basic auth and absolute target URI, h2c upgrade for http.sink.http-version. - TableApiHttpSinkIT: OIDC options from DDL, unknown HTTP version rejected. - DataStreamHttpSinkExample: runnable /oidc-protected demo. - CI and README: build the connector from FLINK-40279-sink-oidc-proxy-http-version, which also carries the FLINK-40277 retry behaviour. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ion options Generated-by: Cursor (auto mode) for the initial change; Claude Code (Claude Fable 5.1) for the review follow-ups
6075be7 to
15e5dac
Compare
Purpose of the change
https://issues.apache.org/jira/browse/FLINK-40279
Bring HTTP sink configuration closer to the lookup source for OIDC authentication, HTTP proxy settings, and HTTP version selection (FLINK-40275 umbrella follow-up after #54).
Brief change log
http.sink.http-version,http.sink.proxy.*, and sharedhttp.security.oidc.*wiring on the sink path.OidcAccessTokenManager. Each HTTP request (per-request mode, batch mode, and retries) gets a currentAuthorization: Bearer <token>header. The token is cached untilexpires_in - http.security.oidc.token.expiry.reductionand then refreshed. The bearer replaces any statichttp.sink.header.Authorization; other headers are unchanged. It doesn't need anAuthorizationheader to be declared.http.security.oidc.token.requestis required whenhttp.security.oidc.token.endpoint.urlis set (validated for both Table and DataStream).OIDCAuthHeaderValuePreprocessorcreated a newOidcAccessTokenManageron every call andJavaNetHttpPollingClientbuilt a new preprocessor for every lookup, so each lookup fetched a new token. Now the polling client builds the preprocessor once, and the preprocessor keeps its token manager (transient, thread-safe) and reuses the token until it expires.HttpClientand requests.Review follow-ups:
SINK_QUERY_HTTP_VERSIONtoSINK_HTTP_VERSION;QUERYhad no meaning on the sink.OIDCAuthHeaderValuePreprocessorto say exactly what survives (the token across requests of one instance) and what does not (the manager across deserialization).OidcAccessTokenManagerpreviously sent the token request without a timeout and itsHttpClienthad no connect timeout, so an unanswered identity provider could block the calling thread indefinitely. The token is fetched on the thread that builds the HTTP request (the sink writer thread or the lookup thread), so this now uses the connector's request timeout:http.sink.request.timeoutfor the sink andhttp.source.lookup.request.timeoutfor lookups, for both connecting and reading. The default when constructed directly is 30s. A timeout fails with a message naming the token endpoint.Compatibility note
max-retriesdefault0, writer thread pool default4, HTTP versionHTTP_1_1).http.security.oidc.token.expiry.reductionnow defaults to30s(was1s) for both lookup and sink. The old default had no effect while lookup fetched a token for every request. Now that tokens are cached, 1s left no room for network latency, request duration, or clock skew. Jobs that set the option explicitly are unaffected. If a token'sexpires_inis not greater than the reduction, the full lifetime is used.expires_in - http.security.oidc.token.expiry.reduction, as the OIDC docs already described ("cached and used for subsequent requests"). If the identity provider revokes a token before it expires, requests keep sending it until it expires, and the endpoint can reject them during that time. The token is still fetched when a request is sent, not at plan time, soEXPLAINmakes no network calls (HTTP-145).Verifying this change
RequestSubmitterOidcTest(WireMock token endpoint, both submitters):Authorizationheader is replaced and other headers are kept.http.sink.request.timeoutfails the submit with the timeout as root cause, and no sink request is sent.OIDCAuthHeaderValuePreprocessorTest: the token is reused while valid, refreshed after it expires, still works after serialization, and fails with a timeout when the token endpoint does not answer in time.HttpHeaderUtilsTest,HttpSinkConfigSerializationTest,HttpDynamicTableSinkFactoryTest,JavaNetHttpClientFactoryTest,PerRequestSubmitterTest,BatchRequestSubmitterTest.mvn -pl flink-connector-http spotless:check checkstyle:check test: all tests pass.Impact and documentation
Authentication->Authorizationheader typo.Was generative AI tooling used to co-author this PR?
Generated-by: Cursor (auto mode) for the initial change; Claude Code (Claude Fable 5.1) for the review follow-ups