Implementing authentication for Pulsar Functions #3735

jerrypeng · 2019-03-02T22:15:16Z

Motivation

Currently authentication for functions can only work with static credentials which makes every function have the same credentials as any other function which is not practical. Since functions should have the same credentials as the users that submit them.

This is an initial implementation that solves the issue

Currently, users will not be able to specify which function auth provider to use, but I can add that in a subsequent PR once everyone is onboard with this

Master issue: #3763

The workflow of the current API is as such:

User submits function worker with auth data
Worker caches information based on auth data ( implementation specific) that gets stored in the function meta topic which gets distributed to every worker
A worker that is assigned to run an instance of the function gets the auth info from the function metadata topic and setups/configures the function instance to support auth (Details are implementation specific)

Below is a diagram that demonstrates the end-to-end process of function authentication for a possible implementation on function authentication in Kubernetes using tokens issued by Hashicorp's Vault:

The KubernetesFunctionAuthenticationProvider implementation is based on the above diagram

jerrypeng · 2019-03-04T09:04:32Z

rerun java8 tests
rerun integration tests
rerun cpp tests

sijie

@jiazhai since you are working on kerberos authentication, it would be good for you to take a look at the interface here, to make sure if people want to implement kerberos authentication using this interface, is it straightforward to implement one?

sijie · 2019-03-04T16:25:00Z

pulsar-functions/proto/src/main/proto/Function.proto

+}
+
+message FunctionAuthenticationSpec {
+    string data = 1;


I am wondering why are you using string, not bytes. my concern of string is that encoding with locales make things very tricky and hard to debug.

I didn't know what kind of params/data would need to be stored so I just based this off of the authParams String format we already have in Pulsar. We can change this to bytes.

also you can always base64 encode a binary to be stored as a string

@jerrypeng bytes is a more natural representation :-)

@sijie i have changed

sijie · 2019-03-04T16:26:05Z

...r-functions/runtime/src/main/java/org/apache/pulsar/functions/auth/FunctionAuthProvider.java

+import org.apache.pulsar.functions.instance.AuthenticationConfig;
+import org.apache.pulsar.functions.proto.Function;
+
+public interface FunctionAuthProvider {


can you please add javadoc comments in the interface? otherwise other people will have difficulties on understanding how to implement an Auth provider.

sijie · 2019-03-04T16:26:49Z

...r-functions/runtime/src/main/java/org/apache/pulsar/functions/auth/FunctionAuthProvider.java

+
+    void configureAuthenticationConfig(AuthenticationConfig authConfig, Function.FunctionAuthenticationSpec functionAuthenticationSpec);
+
+    Function.FunctionAuthenticationSpec cacheAuthData(String tenant, String namespace, String name, AuthenticationDataSource authenticationDataSource) throws Exception;


Can we avoid exposing protobuf structures in the interface?

Yup we can have a wrapper for this

sijie · 2019-03-04T16:30:23Z

...r-functions/runtime/src/main/java/org/apache/pulsar/functions/auth/FunctionAuthProvider.java

+import org.apache.pulsar.functions.instance.AuthenticationConfig;
+import org.apache.pulsar.functions.proto.Function;
+
+public interface FunctionAuthProvider {


a very high level question here - why do you need a separate AuthProvider? why can you use the AuthenticationProvider interface at the broker? what are the real differences between these two interfaces?

So the AuthenticationProvider is interface to simply do authentication. The interface here for functions is a little bit more involved. It needs to have to ability to:

We need to be able to cache some data in the function meta data topic. This could be the auth data itself or a pointer the auth data.

Based on the auth data of the function, we need to be able to manipulate the function runtime somehow (depends on runtime)

This interface doesn't actually authenticate a user but facilitates the authentication process for functions. Whether the name of the interface is appropriate can be up for discussion

ok. A better name or a javadoc for this class is good to have for this class.

sijie · 2019-03-04T16:32:05Z

...nctions/runtime/src/main/java/org/apache/pulsar/functions/auth/NoOpFunctionAuthProvider.java

+    public Function.FunctionAuthenticationSpec cacheAuthData(String tenant, String namespace, String name,
+                                                             AuthenticationDataSource authenticationDataSource)
+            throws Exception {
+        return null;


do we really need return a null here? or shall returning a spec with a data?

I don't quite follow. We can return whatever we want there as long as it doesn't trigger anything

I mean returning null usually results in NPE if you don't take care of it at the callers.

I use null to gate whether we should do anything or not:

https://github.com/apache/pulsar/pull/3735/files#diff-d906ae3d0366fbf1866c0334f075b32eR394

From an API perspective, it's better to use Optional<X> whenever the returned object might not be applicable.

ok we can change this method to return Optional<Function.FunctionAuthenticationSpe>. Is that what you would recommend?

sijie · 2019-03-04T16:33:10Z

...ions/runtime/src/main/java/org/apache/pulsar/functions/runtime/KubernetesRuntimeFactory.java

+        try {
+            setupClient();
+        } catch (Exception e) {
+            throw new RuntimeException(e);


Can you please add a logging statement before throwing a RuntimeException?
RuntimeException is very hard to debug.

sijie · 2019-03-04T16:35:34Z

pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeSpawner.java

    @Override
    public void close() {
+        // cancel liveness checker before terminating runtime.


I am not sure the change code here is related to Authentication. If there are multiple changes coupled in this pull request, can we try to separate them into multiple different pull requests in future? or at least highlight them at the description?

This was a change that was required in a previous version of the implementation. I don't think its required anymore

sijie · 2019-03-04T16:36:36Z

pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/RuntimeSpawner.java

+            try {
+                runtime.terminate();
+            } catch (Exception e) {
+                log.warn("Failed to terminate function runtime: {}", e, e);


you are logging two e.

sijie · 2019-03-04T16:36:41Z

pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/FunctionActioner.java

        String fqfn = FunctionDetailsUtils.getFullyQualifiedName(details);
+        log.info("{}-{} Terminating function...", fqfn,functionRuntimeInfo.getFunctionInstance().getInstanceId());
+
+        if (functionRuntimeInfo.getRuntimeSpawner() != null) {


it seems that the logic here are not related.

The logic here is relevant since in terminate we have to call cleanUpAuthData before we set the runtimespawner to be null

sijie · 2019-03-04T16:38:45Z

...r-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/KubernetesRuntime.java

+                .run();
+
+        if (!success.get()) {
+            throw new RuntimeException(String.format("Failed to create service account %s for function %s", serviceAccountName, fqfn));


logging before throwing runtime exception

it will be logged else where

it will be logged else where

sorry, at where? if you don't catch RuntimeException, isn't the context lost?

its caught and logged here:

https://github.com/apache/pulsar/pull/3735/files#diff-c2c5e0719db635b36beb4cd6d816abdeL226

jerrypeng · 2019-03-04T22:05:55Z

rerun java8 tests
rerun integration tests

jerrypeng · 2019-03-05T00:20:33Z

rerun java8 tests
rerun cpp tests

…tion_auth

jerrypeng · 2019-03-05T20:41:18Z

@jiazhai can you review this please? Thanks.

jerrypeng · 2019-03-06T01:14:30Z

rerun java8 tests
rerun integration tests

jerrypeng · 2019-03-06T07:55:46Z

rerun java8 tests

jerrypeng · 2019-03-07T05:30:13Z

@merlimat @sijie @srkukarni @jiazhai please review. Thanks!

ivankelly

This needs some unit testing. I know it's hard for k8s, but at least for plaintext, it would be good to seem something exercising the interfaces.

The change seems to assume token auth in a couple of places. This should be explicitly checked, as in, it shouldn't even try to distribute the auth data if anything other than token is used for now.

ivankelly · 2019-03-11T14:06:46Z

...ommon/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java

@@ -35,7 +35,7 @@
 public class AuthenticationProviderToken implements AuthenticationProvider {

    final static String HTTP_HEADER_NAME = "Authorization";
-    final static String HTTP_HEADER_VALUE_PREFIX = "Bearer ";
+    public final static String HTTP_HEADER_VALUE_PREFIX = "Bearer ";


Rather than making this public, you should make getToken(AuthenticationDataSource) static and call it from the other locations.

ivankelly · 2019-03-11T14:09:33Z

pulsar-functions/proto/src/main/proto/Function.proto

+}
+
+message FunctionAuthenticationSpec {
+    bytes data = 1;


This should also include method, which the function runner can use to figure out which function auth provider to use.

The function auth provider should be a cluster wide setting and not on a per function basis right?

Please add comment to clarify the content and usage of "data" field, to summarize the below discussion.

@merlimat i have add comments

ivankelly · 2019-03-11T14:40:39Z

...s/runtime/src/main/java/org/apache/pulsar/functions/auth/KubernetesFunctionAuthProvider.java

+/**
+ * Kubernetes runtime specific functions authentication provider
+ */
+public interface KubernetesFunctionAuthProvider extends FunctionAuthProvider {


It's a bad sign if for the first serious use of an abstraction you need to create another abstraction to work work around shortcomings in the first abstraction. However, I don't think it's even needed in this case.

The functionAuthData returned by cacheAuthData should contain the actual token. The caller of cacheAuthData can then create a secret, and then mount that secret on each of the instance pods. When the instance runs, it can always check for this secret and if it is mounted, the data can be read in and passed to configureAuthenticationConfig.

I don't see why service accounts are needed at all here.

It's a bad sign if for the first serious use of an abstraction you need to create another abstraction to work work around shortcomings in the first abstraction. However, I don't think it's even needed in this case.

Why is that bad? I designed this so that it is cleaner compared to AuthenticationDataSource where you have a mixture of interfaces to support different authentication methods. I imagine in the future, different runtimes will have different requirements / interfaces needed to support authentication. It doesn't make sense to clutter them all together.

The functionAuthData returned by cacheAuthData should contain the actual token.

Why should functionAuthData contain the actual token? This is implementation specific data. Different implementations of FunctionAuthenticationProvider should have the flexibility to use it in a way that makes sense for the implementation.

The caller of cacheAuthData can then create a secret, and then mount that secret on each of the instance pods.

I think there is some misunderstanding here. The architecture of Pulsar Function decouples submitting functions and running functions. The worker that the user actually submits the function do is not necessarily going to be the same worker that is going to run the function. However, the auth data will only be passed to the worker that the user at first submits his or her function to. That is why that worker needs to be able to distribute that auth data or data based on that auth data to the rest of the workers that will potentially need to run a function instance. The interface cacheAuthData returns the data that needs to be distributed to the other workers via the function metadata topic.

I think there is some misunderstanding here.

Ya, I needed to map out the interactions. See my top level comment.

jerrypeng · 2019-03-11T19:06:05Z

@ivankelly I don't think I am assuming a token based authentication. The current implementation of the interfaces only accept token based authentication. Do you have a specific location you can point me to that only will work with token based auth?

merlimat

Just a couple of comments on the new interfaces.

Also, can you add a description of the workflow for the management of credentials when the functions are submitted or scheduled?

merlimat · 2019-03-12T01:18:50Z

pulsar-functions/proto/src/main/proto/Function.proto

+}
+
+message FunctionAuthenticationSpec {
+    bytes data = 1;


Does data includes the credential? Or a pointer to the credentials? Can you add comment to clarify the field meaning?
If it includes credentials, should this be included in the function metadata?

So it can include the actual credentials (not secure) or a pointer to the credentials. This all depends on the underlying implementation of the interface. I left this field open ended so that there can be flexibility of what can be stored in here

I think it should always include the credentials. If you want to make it secure, make it secure using the caller of cacheAuthData, not within cacheAuthData

@ivankelly I don't quite follow what you are proposing. This protobuf message will be stored in the function metadata topic. This message was designed with a generic purpose to store some data for the function authentication provider so that it can distribute it to all the workers. What that data is is up to the function authentication provider implementation. This gives us flexibility in the types of authentication we can support in the future.

pulsar-functions/runtime/pom.xml

merlimat · 2019-03-12T01:24:27Z

...r-functions/runtime/src/main/java/org/apache/pulsar/functions/auth/FunctionAuthProvider.java

+     * @return
+     * @throws Exception
+     */
+    FunctionAuthData cacheAuthData(String tenant, String namespace, String name, AuthenticationDataSource authenticationDataSource) throws Exception;


What is the purpose of caching auth data? If there are credentials being submitted, we should immediately store them in the secrets backend.

This is a mechanism to distribute auth credentials or pointers to auth credentials based on implementtion to workers that need to run an instance of the function. Since in the function architecture, submitting the function and running the function are decoupled and might not happen on the same machine we need a mechanism to distribute some information to workers about how to configure authentication for individual function instances

merlimat · 2019-03-12T01:27:04Z

...nctions/runtime/src/main/java/org/apache/pulsar/functions/auth/NoOpFunctionAuthProvider.java

+    public Function.FunctionAuthenticationSpec cacheAuthData(String tenant, String namespace, String name,
+                                                             AuthenticationDataSource authenticationDataSource)
+            throws Exception {
+        return null;


From an API perspective, it's better to use Optional<X> whenever the returned object might not be applicable.

…tion_auth

jerrypeng · 2019-03-15T01:27:56Z

@ivankelly I have also added some unit tests

sijie

Overall looks good to me.

I have only one comment around functions depending on broker module, this might potentially create some weird recurring dependency when function worker is running as part of broker in future.

I would suggest @jiazhai taking a closer look at the interface since he is working on kerberos authentication to make sure it is easy to add kerberos to functions in future.

sijie · 2019-03-15T03:18:26Z

...ntime/src/main/java/org/apache/pulsar/functions/auth/ClearTextFunctionTokenAuthProvider.java

+
+import java.util.Optional;
+
+import static org.apache.pulsar.broker.authentication.AuthenticationProviderToken.getToken;


I am not sure it is a good idea to reference a method in broker module. this would produce potentially very bad dependency tree. If it is common enough, it should be in pulsar-common, otherwise I would suggest just redoing the logic in functions module.

Ya @merlimat and I discussed this as well. We could just move the get token logic to some utils class in pulsar-common. Originally I actually was just redoing the logic in the functions module, but @ivankelly suggested to reuse the code in the AuthenticationProviderToken class

ya, break it into common

@ivankelly its kind of messy extracting the getToken method from AuthenticationProviderToken since it relies on the on many package local String declared in the class.

I think the cleanest way is just duplicate some of the code

It would also be a big move since AuthenticationDataSource would also need to move the pulsar-common and all the things that AuthenticationDataSource depends on.

@sijie I think pulsar-function-runtime is going to have to depend on pulsar-broker-common for the time being since AuthenticationDataSource interface in part of pulsar-broker-common and pulsar-function-worker already depends on pulsar-broker-common anyways

jerrypeng · 2019-03-15T06:11:38Z

rerun java8 tests

ivankelly · 2019-03-15T14:48:42Z

@jerrypeng So, in the sequence you have, the service account bit is entirely unnecessary. Rather than specifying a service account for the StatefulSet(?), you can just attach the secret as a volumeMount or as a env variable.

ivankelly · 2019-03-15T16:03:34Z

@jerrypeng

So mapping out the interactions here, yields the following sequence diagram.

I think this can be similifed a fair bit.

First of all, service accounts are not needed, you can attached the secret directly to the stateful set. In fact, thats exactly what you are doing. service accounts are for pods to authenticate with k8s services. We're not doing that, and we don't need to do that for this implementation. It may be needed for vault at a later stage, but let's not make assumptions about how that'll be used until we have concretely worked out the flow for that.

Secondly, i don't think we need an interface for attaching the secret to the stateful set. Whatever the auth data is that we are passing in, we should assume it is secret, so if there is auth data, the k8s runtime should attach that as a secret. Then it is up to configureAuthenticationConfig to know how to do something with that authData. So we should move the attachment of the secret volume and the mount into the k8s runtime itself.

so the resulting sequence would look like

ivankelly · 2019-03-15T15:45:10Z

...ions/runtime/src/main/java/org/apache/pulsar/functions/runtime/KubernetesRuntimeFactory.java

+
+        // adjust the auth config to support auth
+        if (instanceConfig.getFunctionAuthenticationSpec() != null) {
+            getAuthProvider().configureAuthenticationConfig(authConfig, getFunctionAuthData(instanceConfig.getFunctionAuthenticationSpec()));


I don't understand what this call is for?

This call is to configure the authConfig appropriately to support auth because it will get passed into KubernetesRuntime and eventually in the the function instance. And the fields in the authConfig is what the function instance or the pulsar client in the instance will use for authentication

ivankelly · 2019-03-15T15:48:30Z

...r-functions/runtime/src/main/java/org/apache/pulsar/functions/auth/FunctionAuthProvider.java

+     * @param authConfig authentication configs passed to the function instance
+     * @param functionAuthData function authentication data that is provider specific
+     */
+    void configureAuthenticationConfig(AuthenticationConfig authConfig, FunctionAuthData functionAuthData);


configureAuthenticationConfig sounds weird, why not just configureAuth?

sure I can do that but the reason I named the method configureAuthenticationConfig is because we are literally configuring AuthenticationConfig based on FunctionAuthData

ivankelly · 2019-03-15T16:12:54Z

pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/auth/FunctionAuthData.java

+ * A wrapper for authentication data for functions
+ */
+public class FunctionAuthData {
+    private byte[] data;


Add a string member here to hold which functions auth provider created the auth data. this can be used by the k8s runtime provider at least validate the auth data is of the type expected. I know this will be configured at a cluster level, but people screw up configuration all the time. Better to be defensive.

jerrypeng · 2019-03-15T18:20:41Z

@ivankelly responding to your comments

First of all, service accounts are not needed, you can attached the secret directly to the stateful set. In fact, thats exactly what you are doing. service accounts are for pods to authenticate with k8s services. We're not doing that, and we don't need to do that for this implementation. It may be needed for vault at a later stage, but let's not make assumptions about how that'll be used until we have concretely worked out the flow for that.

let me experiment with not having a service account and see if that works

Secondly, i don't think we need an interface for attaching the secret to the stateful set. Whatever the auth data is that we are passing in, we should assume it is secret, so if there is auth data, the k8s runtime should attach that as a secret. Then it is up to configureAuthenticationConfig to know how to do something with that authData. So we should move the attachment of the secret volume and the mount into the k8s runtime itself.

I don't believe that is the correct design choice because now the runtime is making assumptions on what the data in FunctionAuthData is. The current implementation KubernetesSecretsTokenAuthProvider stores the name of the secret (pointer to it) in the data field of FunctionAuthData. That is an workflow establish in KubernetesSecretsTokenAuthProvider because we cache the name via cacheAuthData in the function metadata topic. How can we make that assumption for another FunctionAuthProvider impl? I could have implemented another version of a Kubernetes Function Authentication Provider that puts the actual token in the data field of FunctionAuthData and set it explicitly as a env variable in the pod spec. While that approach might not be secure, I would like implementations to have that flexibility and not to pigeon hole implementation to a certain workflow.

ivankelly · 2019-03-15T18:33:48Z

@jerrypeng

I don't believe that is the correct design choice because now the runtime is making assumptions on what the data in FunctionAuthData is.

What you are doing now in the case of KubernetesFunctionAuthProvider is premature generalization. We only have one use case for it, and that's very clear in what methods are in the interface. Something for vault with dynamic tokens (i think i figured out the workload for this btw) would need something very different.

Generalization for this interface doesn't make sense even. The AuthProvider interface is a way for the runtime to allow common rest code code to pass information to the common instance code. But between these two points, everything is k8s specific. So we should treat it as such.

The current implementation KubernetesSecretsTokenAuthProvider stores the name of the secret (pointer to it) in the data field of FunctionAuthData. That is an workflow establish in KubernetesSecretsTokenAuthProvider because we cache the name via cacheAuthData in the function metadata topic. How can we make that assumption for another FunctionAuthProvider impl?

We can make this assumption for all k8s based auth providers, because secrets is the way to pass around sensitive information in k8s.

jerrypeng · 2019-03-15T21:54:32Z

@ivankelly you are correct service accounts are not needed to mount secrets in pods. I have remove the service account related code

jerrypeng · 2019-03-17T05:14:19Z

rerun java8 tests

merlimat

👍

merlimat · 2019-03-18T18:28:37Z

pulsar-functions/proto/src/main/proto/Function.proto

+}
+
+message FunctionAuthenticationSpec {
+    bytes data = 1;


Please add comment to clarify the content and usage of "data" field, to summarize the below discussion.

merlimat · 2019-03-18T18:33:51Z

...r-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/KubernetesRuntime.java

+                        "--path",
+                        bkPath,
+                        "--destination-file",
+                        userCodeFilePath);


(not for this PR) instead of passing all these as CLI options, we should instead place these into client.conf which is already automatically picked up by pulsar-admin (and potentially we can reuse also for functions).

jerrypeng · 2019-03-18T21:33:42Z

rerun java8 tests

jerrypeng · 2019-03-18T22:45:16Z

rerun integration tests

merlimat · 2019-03-19T15:44:44Z

run integration tests

jerrypeng added 7 commits March 1, 2019 17:54

Implementing authentication for Pulsar Functions

594bcc8

delete unnecessary changes

1fabe0b

cleaning up

313bcba

improving implementation

027e9f7

fixing tests

3c7e5a2

cleaning up

3ebaf54

add no op implementation

804d178

sijie requested changes Mar 4, 2019

View reviewed changes

sijie requested a review from jiazhai March 4, 2019 16:39

sijie assigned jerrypeng Mar 4, 2019

sijie added area/function area/security labels Mar 4, 2019

jerrypeng added 2 commits March 4, 2019 10:43

cleaning up unnecessary changes

903aa81

refactoring based on comments

6a1a194

ivankelly self-requested a review March 4, 2019 20:37

Merge branch 'master' of github.com:apache/incubator-pulsar into func…

61414aa

…tion_auth

jerrypeng added 3 commits March 5, 2019 12:43

adding comments

814e50d

change data from string type to bytes

17f14c1

add proto file

59b437c

jerrypeng requested a review from sijie March 7, 2019 05:29

ivankelly requested changes Mar 11, 2019

View reviewed changes

merlimat reviewed Mar 12, 2019

View reviewed changes

merlimat added this to the 2.4.0 milestone Mar 14, 2019

jerrypeng added 7 commits March 14, 2019 14:42

addressing comments

9e71238

Merge branch 'master' of github.com:apache/incubator-pulsar into func…

a163eaa

…tion_auth

up merging

7df641b

refactoring get token code

d9f42fc

cleaning up

5a3a621

fix bugs and add tests

9b390c5

add tests

91aff80

jerrypeng requested review from ivankelly and merlimat March 15, 2019 01:28

sijie approved these changes Mar 15, 2019

View reviewed changes

ivankelly requested changes Mar 15, 2019

View reviewed changes

remove service account creation

510a23f

jerrypeng added 2 commits March 16, 2019 22:22

cleanup unused imports

51f33ff

add field for auth provider

29885af

merlimat approved these changes Mar 18, 2019

View reviewed changes

adding comments

3a4e66f

ivankelly approved these changes Mar 19, 2019

View reviewed changes

merlimat merged commit b50760c into apache:master Mar 19, 2019


		void configureAuthenticationConfig(AuthenticationConfig authConfig, Function.FunctionAuthenticationSpec functionAuthenticationSpec);

		Function.FunctionAuthenticationSpec cacheAuthData(String tenant, String namespace, String name, AuthenticationDataSource authenticationDataSource) throws Exception;


		import java.util.Optional;

		import static org.apache.pulsar.broker.authentication.AuthenticationProviderToken.getToken;

Implementing authentication for Pulsar Functions #3735

Implementing authentication for Pulsar Functions #3735

Conversation

jerrypeng commented Mar 2, 2019 • edited

Motivation

jerrypeng commented Mar 4, 2019

sijie left a 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

jerrypeng Mar 4, 2019 • edited

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

jerrypeng commented Mar 4, 2019

jerrypeng commented Mar 5, 2019

jerrypeng commented Mar 5, 2019

jerrypeng commented Mar 6, 2019

jerrypeng commented Mar 6, 2019

jerrypeng commented Mar 7, 2019

ivankelly left a 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

jerrypeng Mar 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerrypeng commented Mar 11, 2019

merlimat left a 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

jerrypeng commented Mar 15, 2019

sijie left a 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

jerrypeng commented Mar 15, 2019

ivankelly commented Mar 15, 2019

ivankelly commented Mar 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerrypeng commented Mar 2, 2019 •

edited

jerrypeng Mar 4, 2019 •

edited

jerrypeng Mar 14, 2019 •

edited