feat: add REST /metadata path to display KSQL server information (replaces /info) #3313

spena · 2019-09-09T17:22:53Z

Description

This new path (/metadata) will replace the /info path in the next KSQL major release. It is added to have a standard API between Confluent components that show information about the current cluster.

For now, this path displays the following:

/metadata
{
  "version": "5.4.0-SNAPSHOT",
  "clusterId": {
    "id": "",
    "scope": {
      "kafka-cluster": "H6tCQq0zSWan9_L-TZHa9A",
      "ksql-cluster": "default_"
    }
  }
}

/metadata/id
{
  "id": "",
  "scope": {
    "kafka-cluster": "H6tCQq0zSWan9_L-TZHa9A",
    "ksql-cluster": "default_"
  }
}

Follow-up PRs:

Call /metadata instead of /info from the KSQL CLI
Add documentation about the new path
Remove the /info path in KSQL 6.x (pending Issue)

Testing done

Added unit tests
Manual verification

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

big-andy-coates

Thanks @spena

LGTM, with a few nits...

It would be good to add something to RestApiTest to test this new end point end-to-end

Also, a SecureIntegerationTest to ensure you don't need to authenticate to access it, maybe?

Finally, can you create a Github issue to remove the old /info URL, mark it for milestone 6.0, i.e. our next breaking change, and then include the PR number in your description please?

big-andy-coates · 2019-09-12T08:36:45Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/KsqlRestApplication.java

@@ -189,10 +196,25 @@ public static String getCommandsStreamName() {
        requireNonNull(rocksDBConfigSetterHandler, "rocksDBConfigSetterHandler");
  }

+  private static KsqlRestConfig injectRestConfigDefaults(final KsqlRestConfig restConfig) {


nit: private to the bottom of the file

big-andy-coates · 2019-09-12T08:37:36Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/KsqlRestApplication.java

+    );
+
+    // REST paths that are public and do not require authentication
+    restConfigs.put(RestConfig.AUTHENTICATION_SKIP_PATHS,


could this not potentially be overwriting a config a user has set?

Should we not be adding to the existing setting, rather than overwriting?

big-andy-coates · 2019-09-12T08:39:28Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/KsqlRestApplication.java

+    restConfigs.put(RestConfig.AUTHENTICATION_SKIP_PATHS,
+        Joiner.on(",").join(authenticationSkipPaths));
+
+    return new KsqlRestConfig(restConfigs);


Rather than adjusting the supplied KsqlConfig, is there some reason you can't add the RestConfig.AUTHENTICATION_SKIP_PATHS when building the original KsqlConfig in KsqlRestApplication.buildApplication?

I could, but the KsqlConfig is built from the below line that does not include the RestConfig properties:
final KsqlConfig ksqlConfig = new KsqlConfig(restConfig.getKsqlConfigProperties());

And the KsqlRestApplication constructor passes the KsqlRestConfig to the parent class, no the KsqlConfig. So I had to overwrite the KsqlRestConfig instead. I'm not sure why we have two different configurations, btw. Do you think we should join KsqlConfig and KsqlRetsConfig into one?

big-andy-coates · 2019-09-12T08:42:01Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/filters/KsqlAuthorizationFilter.java

@@ -52,4 +63,8 @@ public void filter(final ContainerRequestContext requestContext) {
      requestContext.abortWith(Errors.accessDenied(t.getMessage()));
    }
  }
+
+  private boolean requiresAuthorization(final String path) {
+    return !UNAUTHORIZED_ENDPOINTS.contains(path);


Are we case sensitive on paths? Not sure. if not, then this should also not be case sensitive.

Yes, we are case sensitive.

big-andy-coates · 2019-09-12T08:43:00Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/filters/KsqlAuthorizationFilter.java

@@ -32,6 +34,11 @@
 public class KsqlAuthorizationFilter implements ContainerRequestFilter  {
  private static final Logger log = LoggerFactory.getLogger(KsqlAuthorizationFilter.class);

+  private static final Set<String> UNAUTHORIZED_ENDPOINTS = ImmutableSet.of(
+      "/metadata",


Would be good to pick these up from constants in ServerMetadataResource...

big-andy-coates · 2019-09-12T08:51:06Z

ksql-rest-model/src/main/java/io/confluent/ksql/rest/entity/ServerClusterId.java

+  private static final String KAFKA_CLUSTER = "kafka-cluster";
+  private static final String KSQL_CLUSTER = "ksql-cluster";
+
+  @JsonProperty("id")


nit: unnecessary, given there is a getId method

big-andy-coates · 2019-09-12T08:51:32Z

ksql-rest-model/src/main/java/io/confluent/ksql/rest/entity/ServerClusterId.java

+  @JsonProperty("id")
+  private final String id;
+
+  @JsonProperty("scope")


Why is there no getter for this field?

big-andy-coates · 2019-09-12T08:51:54Z

ksql-rest-model/src/main/java/io/confluent/ksql/rest/entity/ServerMetadata.java

+      @JsonProperty("version") final String version,
+      @JsonProperty("clusterId") final ServerClusterId clusterId
+  ) {
+    this.version = version;


nit: null checks.

big-andy-coates · 2019-09-12T08:52:40Z

ksql-rest-model/src/test/java/io/confluent/ksql/rest/entity/ServerMetadataTest.java

+
+import static org.junit.Assert.assertEquals;
+
+public class ServerMetadataTest {


need same test for ServerClusterId.

big-andy-coates · 2019-09-12T08:53:38Z

ksql-rest-model/src/test/java/io/confluent/ksql/rest/entity/ServerMetadataTest.java

+    // Given:
+    final ServerMetadata expected = new ServerMetadata(
+        "1.0.0",
+        ServerClusterId.of("kafka1", "ksql1")


Use a mock for ServerClusterId in this test means this test is not dependant on ServerClusterId impl. This test would then only be testing this class, which is a good thing.

spena · 2019-09-19T20:50:44Z

Thanks @big-andy-coates for the review.

spena added this to the 5.4 milestone Sep 9, 2019

spena requested a review from a team September 9, 2019 17:22

spena self-assigned this Sep 9, 2019

spena force-pushed the metadata_rest_api branch from d221d97 to 4a4a6ac Compare September 9, 2019 21:32

big-andy-coates approved these changes Sep 12, 2019

View reviewed changes

spena added 3 commits September 16, 2019 09:19

feat: add new REST API (/metadata) to return Server info

a7d06cc

feat: exclude new /metadata endpoint from authorization checks

b4c78eb

feat: exclude /metadata path from REST authentication

f8d5748

spena force-pushed the metadata_rest_api branch from 4a4a6ac to 010b122 Compare September 16, 2019 17:19

fix: address Andy's feedback

385e1fe

spena force-pushed the metadata_rest_api branch from 010b122 to 385e1fe Compare September 16, 2019 21:56

spena merged commit 8be29b9 into confluentinc:master Sep 19, 2019

spena deleted the metadata_rest_api branch September 19, 2019 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add REST /metadata path to display KSQL server information (replaces /info) #3313

feat: add REST /metadata path to display KSQL server information (replaces /info) #3313

spena commented Sep 9, 2019

big-andy-coates left a comment •

edited

Loading

big-andy-coates Sep 12, 2019

spena Sep 16, 2019

big-andy-coates Sep 12, 2019

spena Sep 16, 2019

big-andy-coates Sep 12, 2019

spena Sep 16, 2019

big-andy-coates Sep 12, 2019

spena Sep 16, 2019

big-andy-coates Sep 12, 2019

spena Sep 16, 2019

big-andy-coates Sep 12, 2019

spena Sep 16, 2019

big-andy-coates Sep 12, 2019

spena Sep 16, 2019

big-andy-coates Sep 12, 2019

spena Sep 16, 2019

big-andy-coates Sep 12, 2019

spena Sep 16, 2019

big-andy-coates Sep 12, 2019

spena Sep 16, 2019

spena commented Sep 19, 2019


		import static org.junit.Assert.assertEquals;

		public class ServerMetadataTest {

feat: add REST /metadata path to display KSQL server information (replaces /info) #3313

feat: add REST /metadata path to display KSQL server information (replaces /info) #3313

Conversation

spena commented Sep 9, 2019

Description

Testing done

Reviewer checklist

big-andy-coates left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spena commented Sep 19, 2019

big-andy-coates left a comment •

edited

Loading