Add option to configure max origin bound header size (issue #598)

[dvlato]

Fixes issue #598

[mikkokar]

Great PR @dvlato

Just would simplify a bit by eliminating the USE_DEFAULT_MAX_HEADER_SIZE code, and hopefully you could migrate that one Scala test to Kotlin.

[mikkokar]

Why do we set the default max header size to zero? Should it be something like 16KB etc?

[dvlato]

The default max header size is 8K in HttpConfig. 0 means 'use the default value'.

[mikkokar]

I don't understand this. Builder already defaults to DEFAULT_MAX_HEADER_SIZE. Therefore testing for zero value shouldn't be necessary?

I would then add a check to Builder.setMaxSize to ensure that consumers don't attempt to set it to zero:

    public Builder setMaxHeadersSize(int maxHeaderSize) {
         // Something like this
        if (maxHeaderSize <= 0) {
            throw new IllegalArgumentException("...");
        }

        this.maxHeadersSize = maxHeaderSize;
        return this;
    }

Alternatively we could do this in the build() method:

    public HttpConfig build() {
        this.maxHeaderSize = this.maxHeaderSize == 0 : DEFAULT_MAX_HEADER_SIZE: this.maxHeaderSize;
        return new HttpConfig(this);
    }

[dvlato]

As mentioned, 0 is the default value passed from the BackendService class so it can use an integer to represent the value. It is documented in the setter for BackendService but not in the HttpConfig one, let me add it.
Probably we could find a better alternative...

I don't see much of a difference between reading 0 in the builder or in the constructor, but I don't know if I like modifying the contents of the builder to get the values I want in the final object. Maybe we can remove the constant for 0 though.

[mikkokar]

This is from the BackendService class:

public static final int DEFAULT_RESPONSE_TIMEOUT_MILLIS = 1000;
public static final int USE_DEFAULT_MAX_HEADER_SIZE = 0;

This looks bit odd because the default value is declared for the response timeout, but not for the header size. I would make the DEFAULT_HEADER_SIZE a public constant field in the HttpConfig like so:

public final class HttpConfig {

    public static final int DEFAULT_MAX_HEADER_SIZE = 8192;

And then refer to it from BackendService Builder like so:

private Builder(BackendService backendService) {
    ..
    this.maxHeaderSize = HttpConfig.DEFAULT_MAX_HEADER_SIZE;

Then you could eliminate that USE_DEFAULT_MAX_HEADER_SIZE = 0 everywhere. This would be somewhat simpler and make the default value more obvious for the maintainers.

[mikkokar]

Okay. @a-dlatorre this doesn't work because the BackendService class is in the api-module, and HttpConfig is in the client module. Therefore BackendService doesn't have access to the HttpConfig definitions.

[mikkokar]

Is this test supposed to fail due to too long headers? I have completely forgotten.

We should emphasise its intention by explicitly configuring the maxHeaderSize in the test fixture settings. Otherwise all future maintainers will be always guessing the test intention.

Like so:

  override protected def beforeAll() = {
    super.beforeAll()
    styxServer.setBackends(
      "/badResponseFromOriginSpec/" -> HttpBackend(
        "app-1", Origins(originOne),
        connectionPoolConfig = ConnectionPoolSettings(maxConnectionsPerHost = 1)

        // HERE: you'd have to add this:
        maxHeaderSize = 999
      )
    )
  }

But as we are migrating away from Scala tests, I would take this opportunity to move it over to the Kotlin FT suite.

[dvlato]

It was indeed failing due to long headers... We have another test for the code path in which we are using HostProxy instead of BackendServicesRouter , and we are also migrating away from the latter so I don't think migration the test to Kotlin is too useful in this case.

[mikkokar]

Looks like setting the default value is less straightforward than it first looks.
Will get back to this once the Application Router code is removed.
Therefore approving now.