New ServiceContext per user REST request #2705

spena · 2019-04-17T20:25:02Z

Description

A new ServiceContext is created per user request so it can be used to initialize Kafka and SR services using user credentials. This affects only KsqlResource and StreamedQueryResources classes.

This is a partial refactoring to allow KSQL impersonate users for every new request. A follow-up PR will include refactoring the KsqlEngine to remove the internal ServiceContext.

Testing done

Update all tests to use the new KsqlResource and StreamedQueryResources classes.
Add a few tests to validate a ServiceContext is used and closed per request.

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 #")

A new ServiceContext is created per user request so it can be used to initialize Kafka and SR services using user credentials.

agavra

Thanks @spena! This is definitely a step in the right direction, but I think we need to address the issue of matching the sandbox engine with correctly sandboxed service contexts (see my comment below #2705 (comment))

agavra · 2019-04-18T00:25:18Z

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

+      return new UserServiceContext(DefaultServiceContext.create(ksqlConfig));
+    }
+
+    // TODO: Create a UserServiceContext using user credentials


Since this class isn't used in this PR let's introduce it in a future one.

Oh, mmm, I left this code from another patch I have. I was splitting the other patch with only the refactor, and I forgot to remove it. Thanks for catching this.

agavra · 2019-04-18T00:29:26Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/resources/KsqlResourceTest.java

-import java.util.Map;
-import java.util.Optional;
-import java.util.Properties;
+import java.util.*;


nit: I think our guidelines prefer explicit imports

Thanks Intellij optimizations hehe.

You can disable that in your intellij settings: https://stackoverflow.com/a/3589885/2258040

agavra · 2019-04-18T00:30:39Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/resources/KsqlResourceTest.java

@@ -252,8 +241,12 @@

  private String streamName;

+  private List<ServiceContext> serviceContexts = new ArrayList<>();


It looks like all usages are contains, why not use a Set?

Right. It's better to use Set

agavra · 2019-04-18T00:34:48Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/resources/KsqlResourceTest.java

@@ -288,8 +281,14 @@ public void setUp() throws IOException, RestClientException {

    streamName = KsqlIdentifierTestUtil.uniqueIdentifierName();

-    when(schemaInjectorFactory.apply(any())).thenReturn(sandboxSchemaInjector);
-    when(schemaInjectorFactory.apply(serviceContext)).thenReturn(schemaInjector);
+    when(schemaInjectorFactory.apply(any())).thenAnswer(inv -> {


I think it would be clearer to use mockito hamcrest here:

when(schemaInjectorFactory.apply(argThat(isIn(serviceContexts)))).thenReturn(schemaInjector); when(schemaInjectorFactory.apply(any())).thenReturn(sandboxSchemaInjector);

Nice. I like that proposal better.

agavra · 2019-04-18T00:40:05Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/resources/KsqlResourceTest.java

+  @Test
+  public void shouldUseOneServiceContextPerKsqlRequest() {
+    // Given:
+    final ServiceContext sc = spy(TestServiceContext.create());


let's avoid spies if possible! this leads me to two questions:

why do we need a spy instead of a mock? can you mock whatever is necessary or use a simpler KSQL statement list?

I'm not sure I see the connection between the test title (use one per request) and what we're actually asserting (was closed once). What if I created 10 different service contexts during that time and just closed one of them?

Aa, you got the same question I had after submitting this patch. I was thinking on mocking the ServiceContext supplier and the ServiceContext itself, and keep track of the # of times a get() and close get called. Would that be a good approach?

I like that approach :D

I found a way to do it.
done.

agavra · 2019-04-18T00:43:57Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/resources/KsqlResourceTest.java

  private void setUpKsqlResource() {
    ksqlResource = new KsqlResource(
        ksqlConfig,
        ksqlEngine,
-        serviceContext,
+        () -> createTestServiceContext(),


Suggested change

() -> createTestServiceContext(),

this::createTestServiceContext,

agavra · 2019-04-18T00:44:53Z

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/resources/KsqlResourceTest.java

+    doAnswer(inv -> {
+      Object m = inv.getMock();
+      if (serviceContexts.contains(m)) {
+        serviceContexts.remove(m);


what are we using this for? I don't see any tests that require us to properly maintain the list of service contexts?

It's not testing anything. I used just to remove objects from the list of serviceContexts. Do you think this is not necessary? I suspect now that it might not, as this is just a test class, and list is cleared on every new test case.

I will remove this code instead of fixing it. Thes are just tests. The serviceContexts list is cleared on every test, so it is harmless to leave them there.

agavra · 2019-04-18T00:45:35Z

...est-app/src/test/java/io/confluent/ksql/rest/server/resources/StreamedQueryResourceTest.java

+        activenessRegistrar);
+  }
+
+  private StreamedQueryResource givenStreamedQueryResource(final ServiceContext serviceContext) {


same comment as above, this should take in a Supplier<ServiceContext> and we should call it from above

agavra · 2019-04-18T00:46:10Z

...est-app/src/test/java/io/confluent/ksql/rest/server/resources/StreamedQueryResourceTest.java

+    queryResource.streamQuery(VALID_STREAM_REQUEST);
+
+    // Then:
+    Mockito.verify(sc, Mockito.times(1)).close();


same comment, this assert doesn't really match the title of the test

agavra · 2019-04-18T00:48:59Z

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

      final List<ParsedStatement> statements,
      final Map<String, Object> propertyOverrides,
      final String sql
  ) {
+    requireSandbox(serviceContext);
+
    validateOverriddenConfigProperties(propertyOverrides);
    final KsqlExecutionContext ctx = requireSandbox(snapshotSupplier.get());
    final Injector injector = injectorFactory.apply(ctx, serviceContext);


as part of this refactor, I think we also need to make sure that KsqlEngine#getServiceContext is removed - otherwise it circumvents the refactor. There is some complexity here, though, which is that when we create a sandbox we need to make sure that operations on the sandbox always use the same sandbox service context (e.g. there is a 1:1 mapping between sandbox engines and sandboxed ServiceContexts). Let me know if you want to get on a call to discuss this.

See discussion here for context: #2436 (comment)

Is that because some of the sandbox clients (e.g. the topic client) accumulate state when you have multiple statements in 1 request?

I'll throw an idea into this discussion and then run away: one option here would be to pass an interface into KsqlEngine.createSandbox that takes the current service context and returns a sandbox, that createSandbox could use internally to create the sandbox service context. This way we keep the relationship between the execution context and service context in tact.

There is a plan to refactor the KsqlEngine to remove the internal ServiceContext. I'm not sure how that would be done yet, but I wanted to submit this partial PR due to several files I had to change. Or do you think there is a relationship on this PR?

Based on a manual scan I think this might be safe independent of removing it from the KsqlEngine. For now, anywhere that is necessary that they are paired it is being accessed through KsqlEngine#getServiceContext.

rodesai · 2019-04-18T06:11:49Z

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



  public KsqlResource(
      final KsqlConfig ksqlConfig,
      final KsqlEngine ksqlEngine,
-      final ServiceContext serviceContext,
+      final Supplier<ServiceContext> serviceContextFactory,


Will this be a Supplier longer term? Seems like we want to pass in some info about the principal associated with the request.

I initially did not add the supplier. There was a new DefaultServiceContext created on every request. However, I did not know how to verify it on the unit tests. So, I ended up using this supplier for that.

The plan for the next patch is to use a Function instead, where I can pass user information, like serviceContextFactory.apply(principal).

rodesai

Took a pass through. The change mostly makes sense to me. Left some comments inline.

It isn't necessary to make the engine refactor for this PR.

agavra

LGTM - the new unit tests are nice :)

agavra · 2019-04-19T18:40:30Z

...est-app/src/test/java/io/confluent/ksql/rest/server/resources/StreamedQueryResourceTest.java

-import java.util.LinkedList;
-import java.util.Map;
-import java.util.Scanner;
+import java.util.*;


agavra

LGTM - one question inline

agavra · 2019-04-24T23:25:25Z

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

@@ -244,6 +245,7 @@ public void configureBaseApplication(
        new JacksonMessageBodyProvider(JsonMapper.INSTANCE.mapper);
    config.register(jsonProvider);
    config.register(JsonParseExceptionMapper.class);
+    config.register(new KsqlRestServiceContextBinder(ksqlConfig));


How will this work when we need to configure it differently for each request?

The binder will bind the factory using the class type instead of the instance of the class. Like:

bindFactory(KsqlRestServiceContextFactory.class).to(ServiceContext.class);

This way, the binder will make sure to create a new KsqlRestServiceContextFactory for every new REST request. I then can get the user token and other security credentials using dependency injection in the factory.

One thing, though, is that I cannot add the KsqlConfig with this code. The only way I will do it is by calling a static method of the factory to set this during the Binder initialization. After that, every new instance of the Factory will have the ksqlConfig set + the user credentials.

rodesai

LGTM!

Create one ServiceContext per REST request

15908b7

A new ServiceContext is created per user request so it can be used to initialize Kafka and SR services using user credentials.

spena added this to the 5.3 milestone Apr 17, 2019

spena requested a review from a team April 17, 2019 20:25

spena self-assigned this Apr 17, 2019

agavra previously requested changes Apr 18, 2019

View reviewed changes

agavra requested a review from a team April 18, 2019 00:54

rodesai reviewed Apr 18, 2019

View reviewed changes

Address first round of feedback

39961d5

agavra self-requested a review April 18, 2019 22:07

agavra approved these changes Apr 19, 2019

View reviewed changes

spena added 4 commits April 23, 2019 11:55

Replace 'import java.util.*' with individual imports

a7f9679

Merge branch 'master' into rbac_service_refactor

01df30d

Merge branch 'master' into rbac_service_refactor

f49807e

Inject a ServiceContext on each REST call using @context annotations

a4e3900

agavra self-requested a review April 24, 2019 23:08

agavra approved these changes Apr 24, 2019

View reviewed changes

agavra requested a review from a team April 24, 2019 23:30

rodesai approved these changes Apr 25, 2019

View reviewed changes

Merge branch 'master' into rbac_service_refactor

93b8b32

spena merged commit 6915220 into confluentinc:master Apr 25, 2019

		@@ -252,8 +241,12 @@

		private String streamName;

		private List<ServiceContext> serviceContexts = new ArrayList<>();

	() -> createTestServiceContext(),
	this::createTestServiceContext,

New ServiceContext per user REST request #2705

New ServiceContext per user REST request #2705

Conversation

spena commented Apr 17, 2019

Description

Testing done

Reviewer checklist

agavra 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

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

rodesai left a comment

Choose a reason for hiding this comment

agavra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agavra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodesai left a comment

Choose a reason for hiding this comment

agavra left a comment •

edited

Loading