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

Report selected configuration variables via REST API #1484

Merged
merged 14 commits into from Nov 20, 2015

Conversation

Projects
None yet
4 participants
@lennartkoopmann
Member

lennartkoopmann commented Oct 13, 2015

(Adopted to be ready to be merged into the 1.3 branch as requested in #1478)

This pull request adds a REST resource that is reporting a selected list of configuration variables. This is important for Apollo but can on the longer term also be integrated into the web interface.

The ConfigurationVariable class might look a bit convoluted but I tried to make sure that only numbers, strings and booleans can be added to avoid weird toString behavior if somebody is not careful when adding a new configuration variable that is reported.

Please take a close look at the JSON structure in the response. I struggled with that one and could not really find one that satisfied me.

@dennisoelkers dennisoelkers changed the title from Report selected configuration variables via REST AP to Report selected configuration variables via REST API Oct 13, 2015

@GET
@Timed
@ApiOperation(value = "Get all reported configuration variables and their values")
public ConfigurationList getAll() {

This comment has been minimized.

@joschi

joschi Oct 13, 2015

Contributor

This looks awfully manual and error-prone if we introduce or change configuration settings.

JadConfig supports dumping the provided configuration as Map<String, String>, so maybe we should use this instead of listing the configuration settings manually.

This comment has been minimized.

@lennartkoopmann

lennartkoopmann Oct 13, 2015

Member

IMO it is more error prone to dump the whole configuration and blacklisting parts of it because somebody may add a sensitive parameter that can easily be forgotten to be excluded here. Also user defined/plugin beans would be always reported no matter what is in there. This is why I made it so explicit.

The goal is not to dump all configuration but selected values that are useful.

This comment has been minimized.

@dennisoelkers

dennisoelkers Oct 14, 2015

Member

What about adding jackson annotations (JsonProperty/JsonIgnore/JsonView, whichever way around we prefer) to the Configuration class and return it in the REST resource instead of using these convoluted value objects and the very verbose way of creating them?

This comment has been minimized.

@lennartkoopmann

lennartkoopmann Oct 14, 2015

Member

That could work if it is a whitelist approach but I'd use an own annotation like @ApiReported or something to keep it cleaner.

I don't know if having to scan the class for annotations is less convoluted though.

This comment has been minimized.

@joschi

joschi Nov 10, 2015

Contributor

The method and the description of the HTTP resource should at least not say that it dumped all configuration variables. Maybe "selected" or "relevant" would be better?

@bernd

This comment has been minimized.

Member

bernd commented Nov 6, 2015

Any progress on this? 1.3 is due soon! 😃

@JsonProperty
public abstract Object value();
@JsonCreator

This comment has been minimized.

@joschi

joschi Nov 10, 2015

Contributor

The @JsonCreator annotation cannot be used on multiple factory methods or constructors. I've added a failing unit test to demonstrate this and to add some test coverage…

This comment has been minimized.

@lennartkoopmann
@Timed
@ApiOperation(value = "Get all reported configuration variables and their values")
public ConfigurationList getAll() {
List<ConfigurationVariable> config = new ArrayList<>();

This comment has been minimized.

@joschi

joschi Nov 10, 2015

Contributor

Minor nitpick: I'd use ImmutableList.Builder<ConfigurationVariable> which at least supports a chained builder interface.

This comment has been minimized.

@lennartkoopmann

@joschi joschi added this to the 1.3.0 milestone Nov 10, 2015

@joschi joschi self-assigned this Nov 10, 2015

@joschi joschi force-pushed the rest-resource-config branch from 1627f9f to 2e9117d Nov 10, 2015

lennartkoopmann added some commits Nov 12, 2015

Improve REST reporting of selected configuration variables
After talking to Jochen we decided to move logic out of the JAX-RS methods. I also got rid of the multipe implementations of the @JsonCreator methods. #1484
private final String gcWarningThreshold;
public ExposedConfiguration(Configuration configuration, ElasticsearchConfiguration esConfiguration) {

This comment has been minimized.

@joschi

joschi Nov 12, 2015

Contributor

What do we need ConfigurationVariable and ConfigurationList for if we could simply expose this class as JSON?

This comment has been minimized.

@lennartkoopmann

lennartkoopmann Nov 12, 2015

Member

I thought that we might want to dump this into other places in the future, too so I kept it not bound to JSON.

This comment has been minimized.

@joschi

joschi Nov 16, 2015

Contributor

I'd rather get rid of this (currently) unnecessary layer of indirection.

This comment has been minimized.

@lennartkoopmann

lennartkoopmann Nov 19, 2015

Member

org.graylog2.rest cannot access the classes in org.graylog2.configuration so the ExposedConfiguration class is required to map configuration values to a List. We'd have to do this in the JAX-RS logic and that smells.

Given how close the code freeze is and how much this small change is improving our support process I'd ask you to merge it the way it is if you don't have strong opinions against it except a preference for not using this class and moving the logic.

You can of course also perform the change yourself and then merge if you see an easier way that makes sense.

Thank you! :)

joschi added a commit that referenced this pull request Nov 20, 2015

Merge pull request #1484 from Graylog2/rest-resource-config
Report selected configuration settings via REST API

@joschi joschi merged commit 482107d into 1.3 Nov 20, 2015

3 checks passed

ci Jenkins build graylog2-server-integration-pr 373 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

@joschi joschi deleted the rest-resource-config branch Nov 20, 2015

joschi added a commit that referenced this pull request Nov 20, 2015

Merge pull request #1484 from Graylog2/rest-resource-config
Report selected configuration settings via REST API
(cherry picked from commit 482107d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment