feat: support custom request headers from java client and migrations tool #8787

vcrfxia · 2022-02-17T18:47:12Z

Description

This PR adds support for specifying custom request headers for requests issued to the ksqlDB server via the Java client, and also when applying migrations with the ksql migrations tool.

When creating a Java client instance, the new ClientOptions#setRequestHeaders(Map<String,String>) method can be used to set custom request headers:

/**
   * Sets custom request headers to be sent with requests to the ksqlDB server.
   * These headers are in addition to any automatic headers such as the
   * authorization header.
   *
   * <p>If this method is called more than once, only the headers passed on
   * the last invocation will be used. To update existing custom headers,
   * use this method in combination with {@link ClientOptions#getRequestHeaders()}.
   *
   * <p>In case of overlap between these custom headers and automatic headers such
   * as the authorization header, these custom headers take precedence.
   *
   * @param requestHeaders custom request headers
   * @return a reference to this
   */
  ClientOptions setRequestHeaders(Map<String, String> requestHeaders);

This PR also introduces a corresponding ClientOptions#getRequestHeaders() method as mentioned in the javadoc.

When using the ksql-migrations tool, you can now pass an optional --headers flag to the apply command in order to specify custom request headers to be sent with all requests to the ksqlDB server (related to applying the migration(s)):

ksql-migrations ... apply ... --headers /path/to/custom/headers/file.txt

The headers file should be formatted as a properties file. Either of the following is acceptable:

X-My-Custom-Header: abcdefg
X-My-Other-Custom-Header: asdfgh

or

X-My-Custom-Header=abcdefg
X-My-Other-Custom-Header=asdfgh

Testing done

Unit + manual.

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

JimGalasyn · 2022-02-17T20:01:58Z

docs/operate-and-deploy/migrations-tool.md

@@ -307,6 +309,26 @@ containing multiple ksqlDB statements fails during the migration, it's possible
 some of the statements will have been run on the ksqlDB server while later statements 
 have not.

+You can optionally pass custom request headers to be sent with all ksqlDB requests 
+made as part of the `apply` command. In order to do so, pass the location of a


Suggested change

made as part of the `apply` command. In order to do so, pass the location of a

made as part of the `apply` command by passing the location of a

JimGalasyn

LGTM, with one suggestion.

jnh5y · 2022-02-17T22:38:14Z

ksqldb-api-client/src/main/java/io/confluent/ksql/api/client/impl/ClientOptionsImpl.java

@@ -238,6 +249,11 @@ public int getHttp2MultiplexingLimit() {
    return http2MultiplexingLimit;
  }

+  @Override
+  public Map<String, String> getRequestHeaders() {
+    return requestHeaders == null ? null : new HashMap<>(requestHeaders);


Would it be better to return an empty map?

I don't feel strongly, though it is a public API so we should decide now. (The javadoc I added says it returns null if it was never set but that can easily be changed.) Would you expect an empty map? I can make that change in order to be consistent with the String fields returning the empty string if not set, rather than null.

Hmm... I tried to look for other similar API instances briefly.

I did this: https://github.com/confluentinc/ksql/blob/master/ksqldb-api-client/src/main/java/io/confluent/ksql/api/client/impl/HttpRequestImpl.java#L29-L30 where empty maps are used rather than null values. I think that's a slight node to changing to empty maps.

Sure, updated.

jnh5y · 2022-02-17T22:41:06Z

ksqldb-api-client/src/main/java/io/confluent/ksql/api/client/impl/ClientOptionsImpl.java

        + ", http2MultiplexingLimit=" + http2MultiplexingLimit
+        + ", requestHeaders='" + requestHeaders + '\''


Random question, have you seen this output? (Asking in case it'd look goofy or not print things.)

Separately, are any of the headers potentially sensitive / something that should not be printed / logged?

The headers are whatever users set (they're completely custom), so in theory they could be sensitive but there's no way for them to specify that as such. Do you think it's better to not log the headers then (or at least the values)? I'm not sure when/if this would actually ever get printed.

If there's not already some strong guidance or precedent here, we are probably fine.

jnh5y · 2022-02-17T23:31:15Z

ksqldb-tools/src/main/java/io/confluent/ksql/tools/migrations/util/MigrationsUtil.java

+
+  private static Map<String, String> loadRequestHeaders(final String headersFile) {
+    if (headersFile == null || headersFile.trim().isEmpty()) {
+      return null;


Maybe return an empty map here as well?

I don't feel strongly about this one. It's an internal helper method and the rest of the code keeps null checks to be defensive anyway.

jnh5y · 2022-02-17T23:31:38Z

ksqldb-tools/src/test/java/io/confluent/ksql/tools/migrations/util/MigrationsUtilTest.java

@@ -46,14 +49,16 @@ public void shouldCreateNonTlsClientOptions() {
    assertThat(clientOptions.getKeyAlias(), is(""));
    assertThat(clientOptions.isUseAlpn(), is(false));
    assertThat(clientOptions.isVerifyHost(), is(true));
+    assertThat(clientOptions.getRequestHeaders(), nullValue());


Does this need to be an emptyMap?

Yes, good catch.

jnh5y · 2022-02-17T23:32:23Z

ksqldb-api-client/src/main/java/io/confluent/ksql/api/client/impl/ClientOptionsImpl.java

@@ -158,6 +164,12 @@ public ClientOptions setHttp2MultiplexingLimit(final int http2MultiplexingLimit)
    return this;
  }

+  @Override
+  public ClientOptions setRequestHeaders(final Map<String, String> requestHeaders) {
+    this.requestHeaders = ImmutableMap.copyOf(requestHeaders);


Do we want to catch for null input or is the NPE that'd come out of copyOf what we want?

Yup, fixed.

jnh5y

Thanks for changing to empty maps!

The comments are non-blocking, nice-to-haves.

vcrfxia added 2 commits February 17, 2022 09:35

feat: support custom request headers from java client

58b32b2

feat: add custom header support when applying migrations

9176f46

vcrfxia requested a review from a team as a code owner February 17, 2022 18:47

docs: add docs for custom headers in migrations tool

978c28b

vcrfxia requested a review from JimGalasyn as a code owner February 17, 2022 19:53

JimGalasyn reviewed Feb 17, 2022

View reviewed changes

JimGalasyn approved these changes Feb 17, 2022

View reviewed changes

docs: feedback

3628298

jnh5y reviewed Feb 17, 2022

View reviewed changes

chore: update to empty map

d2f9d42

jnh5y reviewed Feb 17, 2022

View reviewed changes

jnh5y approved these changes Feb 17, 2022

View reviewed changes

chore: fixes

81e8549

vcrfxia merged commit ffe57f5 into confluentinc:master Feb 22, 2022

vcrfxia deleted the java-client-headers branch February 22, 2022 16:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support custom request headers from java client and migrations tool #8787

feat: support custom request headers from java client and migrations tool #8787

vcrfxia commented Feb 17, 2022 •

edited

Loading

JimGalasyn Feb 17, 2022

JimGalasyn left a comment

jnh5y Feb 17, 2022

vcrfxia Feb 17, 2022

jnh5y Feb 17, 2022

vcrfxia Feb 17, 2022

jnh5y Feb 17, 2022

vcrfxia Feb 17, 2022

jnh5y Feb 17, 2022

jnh5y Feb 17, 2022

vcrfxia Feb 17, 2022

jnh5y Feb 17, 2022

vcrfxia Feb 17, 2022

jnh5y Feb 17, 2022

vcrfxia Feb 17, 2022

jnh5y left a comment

	made as part of the `apply` command. In order to do so, pass the location of a
	made as part of the `apply` command by passing the location of a

		+ ", http2MultiplexingLimit=" + http2MultiplexingLimit
		+ ", requestHeaders='" + requestHeaders + '\''

feat: support custom request headers from java client and migrations tool #8787

feat: support custom request headers from java client and migrations tool #8787

Conversation

vcrfxia commented Feb 17, 2022 • edited Loading

Description

Testing done

Reviewer checklist

Choose a reason for hiding this comment

JimGalasyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnh5y left a comment

Choose a reason for hiding this comment

vcrfxia commented Feb 17, 2022 •

edited

Loading