CORE: Allow HTTPClient to parse headers from properties.

[wolflex888]

Currently, a header can be set within the catalog properties with a prefix header.. We are using this property to pass in properties like header.X-Iceberg-Access-Delegation to enable vended-credential. The base headers were defined here back in 1.7.x, but this behavior disappeared when we refactored the code to use AuthManager.

I want to have HTTPClient parse the headers from the configs it got from RestSessionCatalog instead of having RestSessionCatalog set it every time it wants to make a request.

[adutra]

@wolflex888 thank you so much for reporting this.

I was initially surprised because this functionality was supposed to work just as before. After digging a bit, I found the culprit: in this commit I inadvertently removed one line that should not have been removed:

77a25fa#diff-86450612dbe323d6d06cbc3846aa1913f042eaedadc0ca027c36bfbe08d3a46cL174

I think the proper "fix" would be to just put that line back. For example:

  public RESTSessionCatalog() {
    this(
        config -> {
          HTTPClient.Builder builder =
              HTTPClient.builder(config).uri(config.get(CatalogProperties.URI));
          configHeaders(config).forEach(builder::withHeader);
          return builder.build();
        },
        null);
  }

I am sorry for the inconvenience.

[wolflex888]

@wolflex888 thank you so much for reporting this.

I was initially surprised because this functionality was supposed to work just as before. After digging a bit, I found the culprit: in this commit I inadvertently removed one line that should not have been removed:

77a25fa#diff-86450612dbe323d6d06cbc3846aa1913f042eaedadc0ca027c36bfbe08d3a46cL174

I think the proper "fix" would be to just put that line back. For example:

  public RESTSessionCatalog() {
    this(
        config -> {
          HTTPClient.Builder builder =
              HTTPClient.builder(config).uri(config.get(CatalogProperties.URI));
          configHeaders(config).forEach(builder::withHeader);
          return builder.build();
        },
        null);
  }

I am sorry for the inconvenience.

Let me test that a bit. I think i did something similar and it did not work as expected because there's also this

[wolflex888]

I am moving out the function configHeaders to RESTUtil so RESTCatalog can use it here. It's passing the clientBuilder to RESTSessionCatalog in class construction; therefore, only doing this in RESTSessionCatalog would not work.

[adutra]

Thanks @wolflex888 for fixing this!

\cc @nastra

[nastra]

we haven't been setting the headers here previously which might be because it was missed earlier. I believe it should be fine to set them here

[nastra]

can you please add a test to TestRESTCatalog that make sure that headers are properly passed from the properties to the underlying HTTP client when a request is being done?

[danielcweeks]

Thanks @wolflex888 for catching this. I'm +1 pending addition of tests per @nastra

[nastra]

all of this refactoring shouldn't really be necessary

[nastra]

this shouldn't be necessary. Can you please revert the refactorings from 1536b7f and only add the new tests?

[nastra]

this is specific to REST, so shouldn't be in CatalogProperties. RESTUtil would be the right place but I'm currently not sure whether we should be exposing this

[nastra]

I was thinking of something similar to this, where we provide a custom client that we spy on and check what headers are sent:

@Test
  public void testDefaultHeadersPropagated() {
    Map<String, String> catalogHeaders =
        ImmutableMap.of("Authorization", "Bearer client-credentials-token:sub=catalog");

    Map<String, String> properties =
        ImmutableMap.of(
            "header.test-header",
            "test-value",
            CatalogProperties.URI,
            httpServer.getURI().toString());
    Map<String, String> expectedHeaders = ImmutableMap.of("test-header", "test-value");

    HTTPClient client =
        Mockito.spy(
            HTTPClient.builder(properties)
                .withHeaders(RESTUtil.merge(RESTUtil.configHeaders(properties), catalogHeaders))
                .uri(properties.get(CatalogProperties.URI))
                .build());
    RESTCatalog catalog =
        new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> client);
    catalog.initialize("test", properties);

    assertThat(catalog.namespaceExists(Namespace.of("non-existing"))).isFalse();

    Mockito.verify(client)
        .execute(
            reqMatcher(HTTPMethod.GET, "v1/config", expectedHeaders, Map.of()),
            eq(ConfigResponse.class),
            any(),
            any());
    Mockito.verify(client)
        .execute(
            reqMatcher(HTTPMethod.HEAD, "v1/namespaces/non-existing", expectedHeaders, Map.of()),
            any(),
            any(),
            any());
  }

I haven't executed/tried this, so you might need to tune/adjust this test a little bit

[wolflex888]

Still trying to figure out why passing in HTTPClient using a lambda function here will close the client.

[adutra]

It will be closed by this try-with-resources block when initialize is called:

iceberg/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java

Line 201 in 3d95a69

try (RESTClient initClient = clientBuilder.apply(props);

[wolflex888]

Sorry for being slow, but how do we usually deal with this kind of situation? Normally, clientBuilder will build a new HttpClient for us to use, but in this case we are returning a static client. We can't do Mokito.spy unless it's static.

[adutra]

Try this:

Mockito.doNothing().when(client).close();