-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Implementing authentication for Pulsar Functions #3735
Changes from all commits
594bcc8
1fabe0b
313bcba
027e9f7
3c7e5a2
3ebaf54
804d178
903aa81
6a1a194
61414aa
814e50d
17f14c1
59b437c
9e71238
a163eaa
7df641b
d9f42fc
5a3a621
9b390c5
91aff80
510a23f
51f33ff
29885af
3a4e66f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -141,6 +141,21 @@ message FunctionMetaData { | |
uint64 version = 3; | ||
uint64 createTime = 4; | ||
map<int32, FunctionState> instanceStates = 5; | ||
FunctionAuthenticationSpec functionAuthSpec = 6; | ||
} | ||
|
||
message FunctionAuthenticationSpec { | ||
/** | ||
* function authentication related data that the function authentication provider | ||
* needs to cache/distribute to all workers support function authentication. | ||
* Depending on the function authentication provider implementation, this can be the actual auth credentials | ||
* or a pointer to the auth credentials that this function should use | ||
*/ | ||
bytes data = 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
/** | ||
* classname of the function auth provicer this data is relevant to | ||
*/ | ||
string provider = 2; | ||
} | ||
|
||
message Instance { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
/** | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
package org.apache.pulsar.functions.auth; | ||
|
||
import org.apache.pulsar.broker.authentication.AuthenticationDataSource; | ||
import org.apache.pulsar.client.impl.auth.AuthenticationToken; | ||
import org.apache.pulsar.functions.instance.AuthenticationConfig; | ||
|
||
import java.util.Optional; | ||
|
||
import static org.apache.pulsar.broker.authentication.AuthenticationProviderToken.getToken; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ya, break it into common There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 |
||
|
||
public class ClearTextFunctionTokenAuthProvider implements FunctionAuthProvider { | ||
@Override | ||
public void configureAuthenticationConfig(AuthenticationConfig authConfig, FunctionAuthData functionAuthData) { | ||
authConfig.setClientAuthenticationPlugin(AuthenticationToken.class.getName()); | ||
authConfig.setClientAuthenticationParameters("token:" + new String(functionAuthData.getData())); | ||
} | ||
|
||
@Override | ||
public Optional<FunctionAuthData> cacheAuthData(String tenant, String namespace, String name, AuthenticationDataSource authenticationDataSource) throws Exception { | ||
String token = null; | ||
try { | ||
token = getToken(authenticationDataSource); | ||
} catch (Exception e) { | ||
throw new RuntimeException(e); | ||
} | ||
|
||
if (token != null) { | ||
return Optional.of(FunctionAuthData.builder().data(token.getBytes()).build()); | ||
} | ||
return null; | ||
} | ||
|
||
@Override | ||
public void cleanUpAuthData(String tenant, String namespace, String name, FunctionAuthData functionAuthData) throws Exception { | ||
//no-op | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
/** | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
package org.apache.pulsar.functions.auth; | ||
|
||
import lombok.Builder; | ||
import lombok.Data; | ||
|
||
@Data | ||
@Builder | ||
/** | ||
* A wrapper for authentication data for functions | ||
*/ | ||
public class FunctionAuthData { | ||
/** | ||
* function authentication related data that the function authentication provider | ||
* needs to cache/distribute to all workers support function authentication. | ||
* Depending on the function authentication provider implementation, this can be the actual auth credentials | ||
* or a pointer to the auth credentials that this function should use | ||
*/ | ||
private byte[] data; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
/** | ||
* classname of the function auth provicer this data is relevant to | ||
*/ | ||
private String provider; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
/** | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
package org.apache.pulsar.functions.auth; | ||
|
||
import org.apache.pulsar.broker.authentication.AuthenticationDataSource; | ||
import org.apache.pulsar.functions.instance.AuthenticationConfig; | ||
|
||
import java.util.Optional; | ||
|
||
/** | ||
* This is a generic interface that functions can use to cache and distribute appropriate authentication | ||
* data that is needed to configure the runtime of functions to support appropriate authentication of function instances | ||
*/ | ||
public interface FunctionAuthProvider { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you please add javadoc comments in the interface? otherwise other people will have difficulties on understanding how to implement an Auth provider. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok. A better name or a javadoc for this class is good to have for this class. |
||
|
||
/** | ||
* Set authentication configs for function instance based on the data in FunctionAuthenticationSpec | ||
* @param authConfig authentication configs passed to the function instance | ||
* @param functionAuthData function authentication data that is provider specific | ||
*/ | ||
void configureAuthenticationConfig(AuthenticationConfig authConfig, FunctionAuthData functionAuthData); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. configureAuthenticationConfig sounds weird, why not just configureAuth? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure I can do that but the reason I named the method |
||
|
||
/** | ||
* Cache auth data in as part of function metadata for function that runtime may need to configure authentication | ||
* @param tenant tenant that the function is running under | ||
* @param namespace namespace that is the function is running under | ||
* @param name name of the function | ||
* @param authenticationDataSource auth data | ||
* @return | ||
* @throws Exception | ||
*/ | ||
Optional<FunctionAuthData> cacheAuthData(String tenant, String namespace, String name, AuthenticationDataSource authenticationDataSource) throws Exception; | ||
|
||
/** | ||
* Clean up operation for auth when function is terminated | ||
* @param tenant tenant that the function is running under | ||
* @param namespace namespace that is the function is running under | ||
* @param name name of the function | ||
* @param functionAuthData function auth data | ||
* @throws Exception | ||
*/ | ||
void cleanUpAuthData(String tenant, String namespace, String name, FunctionAuthData functionAuthData) throws Exception; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
/** | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
package org.apache.pulsar.functions.auth; | ||
|
||
import org.apache.pulsar.functions.proto.Function; | ||
|
||
public final class FunctionAuthUtils { | ||
|
||
public static final FunctionAuthData getFunctionAuthData(Function.FunctionAuthenticationSpec functionAuthenticationSpec) { | ||
return FunctionAuthData.builder().data(functionAuthenticationSpec.getData().toByteArray()).build(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
/** | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
package org.apache.pulsar.functions.auth; | ||
|
||
import io.kubernetes.client.models.V1ServiceAccount; | ||
import io.kubernetes.client.models.V1StatefulSet; | ||
|
||
/** | ||
* Kubernetes runtime specific functions authentication provider | ||
*/ | ||
public interface KubernetesFunctionAuthProvider extends FunctionAuthProvider { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
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.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ya, I needed to map out the interactions. See my top level comment. |
||
|
||
/** | ||
* Configure function statefulset spec based on function auth data | ||
* @param statefulSet statefulset spec for function | ||
* @param functionAuthData function auth data | ||
*/ | ||
void configureAuthDataStatefulSet(V1StatefulSet statefulSet, FunctionAuthData functionAuthData); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also include method, which the function runner can use to figure out which function auth provider to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function auth provider should be a cluster wide setting and not on a per function basis right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add comment to clarify the content and usage of "data" field, to summarize the below discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@merlimat i have add comments