Simplify HTTP configuration #3840

joschi · 2017-05-17T15:18:26Z

Motivation and Context

Users regularly have problems finding the correct settings for the Graylog REST API and Graylog web interface listeners in their environments.

This change set tries to simplify the network settings for the Graylog HTTP interfaces (Graylog REST API and web interface).

The two separate network listeners for the Graylog REST API and the Graylog web interface have been merged, vastly reducing the necessary configuration settings.

While the the Graylog REST API and the Graylog web interface could be configured separately via the rest_listen_uri and web_listen_uri settings in the past, they now run on the same network listener which can be configured via http_bind_address.

The context path for the Graylog REST API is now hard-coded to /api.

Types of changes

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

philicious · 2017-05-18T15:24:50Z

while I never had bigger problems with it, these "duplicate" settings were indeed a little confusing and I like this change 👍

kroepke

Overall this looks much better than the situation before!

I have a few minor nitpicks, regarding methods and constants and only one issue regarding IPv6 wildcard address handling.
That's probably more of an edge case, but at least we should return a valid IP address in that case.

kroepke · 2017-10-02T13:18:10Z

UPGRADING.rst

+---------------------------------+----------------------------------+--------------------+
+| ``web_max_initial_line_length`` | ``http_max_initial_line_length`` | ``4096``           |
+---------------------------------+----------------------------------+--------------------+
+| ``web_thread_pool_size``        | ``http_thread_pool_size``        | ``false``          |


Should be 16 I guess

kroepke · 2017-10-02T13:55:23Z

graylog2-server/src/main/java/org/graylog2/configuration/HttpConfiguration.java

+    public HostAndPort getHttpBindAddress() {
+        return httpBindAddress
+                .requireBracketsForIPv6()
+                .withDefaultPort(9000);


This should also use the constant GRAYLOG_DEFAULT_PORT

kroepke · 2017-10-02T13:56:50Z

graylog2-server/src/main/java/org/graylog2/configuration/HttpConfiguration.java

+        return getUriScheme(isHttpEnableTls());
+    }
+
+    public String getUriScheme(boolean enableTls) {


I think this is only used once in the method above and could be inlined.

kroepke · 2017-10-02T13:58:22Z

graylog2-server/src/main/java/org/graylog2/configuration/HttpConfiguration.java

+    }
+
+    @VisibleForTesting
+    URI getDefaultHttpUri(String path) {


This can be private as it is not directly used in tests (only via its two private usages)

kroepke · 2017-10-02T14:26:54Z

graylog2-server/src/main/java/org/graylog2/shared/initializers/JerseyService.java

                null,
+                bindAddress.getHost(),
+                bindAddress.getPort(),
+                "/",
                null,
                null
        );

        apiHttpServer = setUp("rest",


The prefix is now unused and always "rest" so I think we can rid of the parameter.

kroepke · 2017-10-03T13:04:41Z

graylog2-server/src/main/java/org/graylog2/configuration/HttpConfiguration.java

+        final HostAndPort bindAddress = getHttpBindAddress();
+
+        final URI publishUri;
+        if (WILDCARD_IP_ADDRESS.equals(bindAddress.getHost())) {


This currently does not take the IPv6 wildcard address into account and will publish [::] as the URI, which is invalid.
I think that adding a check against [::] and extending Tools.guessPrimaryNetworkAddress() to also look for IPv6 addresses, would be the best option here.

kroepke · 2017-10-03T13:05:12Z

graylog2-server/src/main/java/org/graylog2/configuration/HttpConfiguration.java

+            final URI defaultHttpUri = getDefaultHttpUri();
+            LOG.debug("No \"http_publish_uri\" set. Using default <{}>.", defaultHttpUri);
+            return defaultHttpUri;
+        } else if (WILDCARD_IP_ADDRESS.equals(httpPublishUri.getHost())) {


This does not take the IPv6 wildcard address into account: [::]. See below for another instance of that.

[ci skip]

kroepke · 2017-10-04T14:01:13Z

With the latest changes I get the error message:

We are experiencing problems connecting to the Graylog server running on http://[fe80:0:0:0:b8dd:21ff:fe2f:35c0%veth3415fad]:9000/api/. Please verify that the server is healthy and working correctly.

When using http_bind_address = [::].
Investigating.

joschi · 2017-10-04T14:20:31Z

@kroepke I guess fe80:0:0:0:b8dd:21ff:fe2f:35c0%veth3415fad is simply the first IPv6 address of the first network interface in the system (for random values of "first").

I'm not convinced that this approach (that worked reasonably well for IPv4) is suitable for IPv6. 🤔

kroepke · 2017-10-05T07:55:04Z

Nevermind, this was due to automatically binding to a different interface. Works as expected.

kroepke · 2017-10-05T07:55:32Z

Looks good, thanks for the rebase and implementing this!

The old options got removed and the user has to change her config to the new one. Refs #3840

Some URLs require some search query parameters to be set, and this stopped working since #3840. Fixes #4290

* Include query and hash params in qualified URLs Some URLs require some search query parameters to be set, and this stopped working since #3840. Fixes #4290 * Use string templates instead of concatenating * Add Routes tests - Test routes with prefix - Test routes with no prefix - Ensure search query parameters are not removed from routes

joschi added the in progress label May 17, 2017

joschi force-pushed the simplify-http-configuration branch from 44c6978 to 28aa7f3 Compare June 13, 2017 12:35

bernd added this to the 3.0.0 milestone Sep 28, 2017

kroepke self-requested a review September 28, 2017 09:41

kroepke self-assigned this Sep 28, 2017

joschi force-pushed the simplify-http-configuration branch from 28aa7f3 to 1c6a4ae Compare October 2, 2017 12:09

kroepke requested changes Oct 3, 2017

View reviewed changes

Jochen Schalanda and others added 14 commits October 4, 2017 11:34

Extract HTTP-related settings into HttpConfiguration

5948bec

Remove web_* configuration settings

2c68f02

WIP - Replace rest_* configuration settings

3f344f4

Make slash at the end of a web interface route optional.

069c955

Properly rewrite location when synchronizing range with browser query.

0388ee8

Use normalized URIs in Routes.qualifyUrls

2cac8e3

Improve documentation for http_* settings

862215d

[ci skip]

Add section to upgrade notes

1db07e0

[ci skip]

Fix link in upgrade notes

1301fa5

[ci skip]

Use correct default for web_thread_pool_size in UPGRADING.rst

a55139d

Use GRAYLOG_DEFAULT_PORT constant instead of magic literal

ec47540

Inline single line HttpConfiguration#getUriScheme(boolean)

b8654b1

Make HttpConfiguration#getDefaultHttpUri(String) private

cd9b9fb

Remove unnecessary "namePrefix" parameter from JerseyService#setUp()

becbce7

joschi force-pushed the simplify-http-configuration branch from d4bb107 to 80f50c3 Compare October 4, 2017 09:35

Add support for IPv6 wildcard addresses in HttpConfiguration

9500a81

joschi force-pushed the simplify-http-configuration branch from 80f50c3 to 9500a81 Compare October 4, 2017 14:19

kroepke approved these changes Oct 5, 2017

View reviewed changes

kroepke merged commit ac21a44 into master Oct 5, 2017

kroepke deleted the simplify-http-configuration branch October 5, 2017 07:55

joschi removed the in progress label Oct 6, 2017

joschi pushed a commit that referenced this pull request Oct 6, 2017

Change wording from "deprecated" to "removed" (#4212)

ee2d790

The old options got removed and the user has to change her config to the new one. Refs #3840

joschi mentioned this pull request Oct 6, 2017

Adapt documentation to new HTTP settings in Graylog 3.0.0 Graylog2/documentation#369

Closed

edmundoa mentioned this pull request Oct 25, 2017

Fix routes including search query parameters #4290

Closed

edmundoa pushed a commit that referenced this pull request Oct 25, 2017

Include query and hash params in qualified URLs

9a1e46d

Some URLs require some search query parameters to be set, and this stopped working since #3840. Fixes #4290

bernd mentioned this pull request Nov 3, 2017

Issues with new HTTP config handling #4315

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify HTTP configuration #3840

Simplify HTTP configuration #3840

joschi commented May 17, 2017

philicious commented May 18, 2017

kroepke left a comment

kroepke Oct 2, 2017

kroepke Oct 2, 2017

kroepke Oct 2, 2017

kroepke Oct 2, 2017

kroepke Oct 2, 2017

kroepke Oct 3, 2017

kroepke Oct 3, 2017

kroepke commented Oct 4, 2017

joschi commented Oct 4, 2017 •

edited

Loading

kroepke commented Oct 5, 2017

kroepke commented Oct 5, 2017

Simplify HTTP configuration #3840

Simplify HTTP configuration #3840

Conversation

joschi commented May 17, 2017

Motivation and Context

Types of changes

Checklist:

philicious commented May 18, 2017

kroepke left a comment

Choose a reason for hiding this comment

kroepke Oct 2, 2017

Choose a reason for hiding this comment

kroepke Oct 2, 2017

Choose a reason for hiding this comment

kroepke Oct 2, 2017

Choose a reason for hiding this comment

kroepke Oct 2, 2017

Choose a reason for hiding this comment

kroepke Oct 2, 2017

Choose a reason for hiding this comment

kroepke Oct 3, 2017

Choose a reason for hiding this comment

kroepke Oct 3, 2017

Choose a reason for hiding this comment

kroepke commented Oct 4, 2017

joschi commented Oct 4, 2017 • edited Loading

kroepke commented Oct 5, 2017

kroepke commented Oct 5, 2017

joschi commented Oct 4, 2017 •

edited

Loading