Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable running Graylog REST API on different context path #2634

Merged
merged 17 commits into from Aug 10, 2016
Merged

Conversation

@joschi
Copy link
Contributor

@joschi joschi commented Aug 8, 2016

Description

Refs #2603

Motivation and Context

Users might (rightfully) want to run the Graylog REST API and the web interface on the same network listener with the web interface being served on the root context path (/) and the Graylog REST API being served from a path below that (/api).

This PR merges RestApiService and WebInterfaceService into a single service which handles setting up the required handlers accordingly.

How Has This Been Tested?

Painfully.

Things that might still be broken and might only be working on my machine™:

  • Swagger UI in general
  • The generic 404-handler so that the web interface can have an arbitrary entry-point in the web browser
  • Generated links in the Graylog REST API

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • 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
    • Well, the upgrade notes at least.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@@ -20,6 +20,6 @@
import retrofit2.http.POST;

public interface RemoteDeflectorResource {
@POST("/system/deflector/cycle")
@POST("system/deflector/cycle")

This comment has been minimized.

@kroepke

kroepke Aug 9, 2016
Member

Are these changes strictly necessary, or rather can we catch this in a central place?
I'm worried that plugins that implement a ProxiedResource will forget to make the path relative and thus break at runtime.

This comment has been minimized.

@joschi

joschi Aug 9, 2016
Author Contributor

Yes, these are necessary.

Otherwise the custom context path would be overridden (because the paths in these Retrofit resource classes are absolute and not relative as they should be). I haven't looked into it too deeply but I haven't found a suitable mechanism in Retrofit's documentation yet.

This hasn't been a problem before because the Graylog REST API couldn't run on a different context path.

Also see Retrofit 2 — Url Handling, Resolution and Parsing.

This comment has been minimized.

@kroepke

kroepke Aug 9, 2016
Member

Ok bummer. I was asking because at least the audit log plugin needs the corresponding change in this case.

This comment has been minimized.

@kroepke

kroepke Aug 9, 2016
Member

Could we potentially add a runtime check for the affected resources to log error messages?
Not sure where in the code this would have to go, the closest I could find is

public <T> T get(Node node, final String authorizationToken, Class<T> interfaceClass) {
final OkHttpClient okHttpClient = this.okHttpClient.newBuilder()
.addInterceptor(chain -> {
final Request original = chain.request();
Request.Builder builder = original.newBuilder()
.header(HttpHeaders.ACCEPT, MediaType.JSON_UTF_8.toString())
.method(original.method(), original.body());
if (authorizationToken != null) {
builder = builder.header(HttpHeaders.AUTHORIZATION, authorizationToken);
}
return chain.proceed(builder.build());
})
.build();
final Retrofit retrofit = new Retrofit.Builder()
.baseUrl(node.getTransportAddress())
.addConverterFactory(JacksonConverterFactory.create(objectMapper))
.client(okHttpClient)
.build();
return retrofit.create(interfaceClass);

This comment has been minimized.

@kroepke

kroepke Aug 10, 2016
Member

We agreed to simply fix the affected resources in the relevant plugins.
Since the default is to run the API on a sub path, this will fail early on which is good enough.

onComplete: function(swaggerApi, swaggerUi){

// We are taking the base path of the system API here. This will break if you should ever rename it lol.
if(swaggerApi.System && !apiTarget.startsWith(swaggerApi.System.basePath)) {

This comment has been minimized.

@kroepke

kroepke Aug 9, 2016
Member

I think startsWith is not supported in older browsers. Probably better to use
apiTarget.lastIndexOf(swaggerApi.System.basePath, 0) === 0

This comment has been minimized.

@joschi

joschi Aug 10, 2016
Author Contributor

(╯°□°)╯︵ ┻━┻

This comment has been minimized.

@kroepke
Copy link
Member

@kroepke kroepke commented Aug 9, 2016

I need to test a couple of more scenarios tomorrow, but overall this looks good!

@kroepke
Copy link
Member

@kroepke kroepke commented Aug 10, 2016

I've tested the following combinations with 2 nodes:

  • out of the box defaults (only adjusting ports for running on the same box)
  • API on /api (node 1) and /schmapi (node 2), web default
  • same as above, web on /web
  • using all current plugins

The only thing that currently does not work is to put the web listener on a separate IP/Port. The assets then all end up in a 404.

@kroepke
Copy link
Member

@kroepke kroepke commented Aug 10, 2016

Works now on the separate listener.

@kroepke
Copy link
Member

@kroepke kroepke commented Aug 10, 2016

lgtm

@kroepke kroepke merged commit 5645a31 into master Aug 10, 2016
4 checks passed
4 checks passed
@garybot2
ci-server-integration Jenkins build graylog2-server-integration-pr 1238 has succeeded
Details
@garybot2
ci-web-linter Jenkins build graylog-pr-linter-check 721 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@kroepke kroepke deleted the issue-2603 branch Aug 10, 2016
kroepke added a commit that referenced this pull request Aug 10, 2016
since merging #2634 the default config.js isn't working with npm start anymore
@kroepke kroepke mentioned this pull request Aug 10, 2016
2 of 9 tasks complete
joschi pushed a commit that referenced this pull request Aug 10, 2016
edmundoa added a commit that referenced this pull request Aug 10, 2016
* Update dev config.js to new default

since merging #2634 the default config.js isn't working with npm start anymore

* Serve assets from "/" when running in development mode

- Introduce `AppConfig.gl2DevMode()`
- Use the function in Navigation.jsx instead of checking for the global
  variable directly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants