You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/07/28 04:41:53 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
michaeljmarshall opened a new pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468
…rConfig
### Motivation
Currently, it is not possible to pass custom configuration to a function worker for use in an Authentication or Authorization plugin. In order to support plugins created by end users, we need to make sure that configurations passed in at function worker initialization are passed through to the `ServiceConfiguration`. This PR seeks to make it possible to pass in those arbitrary configurations.
### Modifications
Update the `FunctionWorkerStarter` class to generate the `ServiceConfiguration` by reading it in from a file instead of building the class by converting the `WorkerConfig` to the `ServiceConfiguration`.
### Alternative Solution
We could also change how we generate the `WorkerConfig`. It is currently created by reading the config file into the class using an ObjectMapper. This does not really follow the general paradigm that Pulsar uses to generate config classes (at least the paradigm that I've noticed) where we use the `PulsarConfigurationLoader` class to load a config from a file. However, I am not sure that we want to remove the `WorkerConfig.load` method, given that it appears well tested.
I'm open to this solution as well. Please let me know what you think.
### Verifying this change
There are not currently tests validating this configuration logic. I'm happy to add a test, but I'm not sure where to add it. Please let me know how to proceed here.
Note that the only change in behavior here is that the `ServiceConfiguration` class will now have fields that were not present in the `WorkerConfig`. Previously, these fields would have been dropped when we created the `WorkerConfig`. As such, while this change will technically introduce changes to the configuration in the `ServiceConfiguration` class, it is only changing configuration for fields not present in the `WorkerConfig`.
Additionally, one potential difference could also come from differences in default values between the `WorkerConfig` and the `ServiceConfiguration`. There could be conflicts now, or in the future, that could make it confusing to figure out which configuration is _actually_ applied. This might be worth adding test coverage.
Note, also, that the `ServiceConfigruation` added in this PR is only used to configure the `AuthenticationService` and the `AuthorizationService`.
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): no
- The public API: no
- The schema: no
- The default values of configurations: **possibly**
- The wire protocol: no
- The rest endpoints: no
- The admin cli options: no
- Anything that affects deployment: **yes (custom configuration)**
### Documentation
#### For contributor
For this PR, do we need to update docs?
I don't think additional documentation is needed because I think this change will make pulsar's configuration work as expected.
#### For committer
For this PR, do we need to update docs?
- If yes,
- if you update docs in this PR, label this PR with the `doc` label.
- if you plan to update docs later, label this PR with the `doc-required` label.
- if you need help on updating docs, create a follow-up issue with the `doc-required` label.
- If no, label this PR with the `no-need-doc` label and explain why.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468#issuecomment-899816400
@jerrypeng - thank you for taking a look. It seems to me that solution 2 is more consistent with the way the broker and proxy load their configurations. Let me know what you think.
> Simply have the custom AuthenticationProvider / Authorization provider read from another config file during initialization. You control the code for these custom plugins so you can do whatever you want in them. This is custom code so it would also make sense that they have custom configs load from custom config files.
I don't think this solution is consistent with the way that the `AuthenticationProvider` and the `AuthorizationProvider` interfaces are written and documented. If an implementation is expected to read its own config from a file, why does the framework pass in a `ServiceConfiguration` when calling the `initialize` method?
https://github.com/apache/pulsar/blob/b8dce10a88cc186897fa600f3595b10c4af61b8c/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProvider.java#L39-L47
https://github.com/apache/pulsar/blob/b8dce10a88cc186897fa600f3595b10c4af61b8c/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java#L87-L97
Further, the broker and proxy components already load their configurations in such a way that they pass arbitrary configuration to the authn/authz providers. It seems appropriate to me that the function worker should load configuration in the same way as the broker and proxy components. (In this PR, I only modified the way that the function worker loads the config for the authn/authz providers to align with the broker and proxy. I mention other approaches above.)
> Add a map in WorkerConfig for configs that are not explicitly declared and they can get passed into the plugins.
This map already exists. It is the `properties` map, as defined in the `PulsarConfiguration` interface. We can populate this map by initializing the `WorkerConfig` class using the `PulsarConfigurationLoader`. I describe why I didn't initially use this solution above, but I am happy to modify this PR to use this solution, if that solution is more appropriate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468#issuecomment-903537157
makes sense
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468#issuecomment-888085743
@freeznet @nlu90 please review it when you have time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468#issuecomment-894387962
> How about add the missing fields into the `WorkerConfig`?
@nlu90 - this isn't an option because the fields are for extensions created by users. Note that the `ServiceConfigruation` already has a map for arbitrary config. The `WorkerConfig` does not have this map. An alternate solution could instead add the map to the `WorkerConfig`, but I'm not sure how that would impact a function worker's runtime.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468#issuecomment-895773456
@sijie
I agree that this patch must be marked with `release/note-required` as it is an important change to be noted.
IIUC this patch here are are not reading the broker.conf file, but we are reading the worker configuration file using ServiceConfiguration.
The previous approach was to reconstruct the ServiceConfiguration from the workerConfig
```
private ServiceConfiguration getServiceConfiguration() {
ServiceConfiguration serviceConfiguration = PulsarConfigurationLoader.convertFrom(workerConfig);
serviceConfiguration.setClusterName(workerConfig.getPulsarFunctionsCluster());
return serviceConfiguration;
}
```
So the behaviour should be the same and in the future we won't be able to maintain the `PulsarConfigurationLoader.convertFrom(workerConfig)` conversion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468#issuecomment-894537539
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468#issuecomment-897018623
@sijie and @nlu90 - I've answered your questions below.
> First of all, this ticket should be marked as doc-required as it introduces a different behavior to load customized settings.
This change makes it _possible_ to load customized settings for the `AuthenticationService` and the `AuthorizationService`. It only affects these classes and the classes they load (like custom plugins). It does not introduce a different behavior, but rather fixes the missing ability to add custom settings to the `properties` map in the `ServiceConfiguration` object. In all other pulsar components, the `ServiceConfiguration` is loaded so that the `properties` map contains all custom fields from the loaded yaml config file.
> Secondly, the worker is designed to load settings from the worker file config, not from the broker file config. I don't think we should change the constructor to pass in another broker service configuration object.
This PR does not change which file is loaded. It only changes how the `ServiceConfiguration` object is constructed.
> Why can't we access the underlying key/value map from the worker config object?
We cannot access the key/value map from the worker config because of how the `WorkerConfig` is initialized. The `FunctionWorkerStarter` reads in the yaml file and converts it to a `WorkerConfig` object by calling `WorkerConfig.load(workerArguments.configFile);`. The load method is defined here:
```java
public static WorkerConfig load(String yamlFile) throws IOException {
if (isBlank(yamlFile)) {
return new WorkerConfig();
}
ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
return mapper.readValue(new File(yamlFile), WorkerConfig.class);
}
```
This is the only implementation of the `PulsarConfiguration` interface that uses an `ObjectMapper` to convert the `yaml` into the object. The only fields that are deserialized and populated into the `WorkerConfig` are fields that are explicitly named in the `WorkerConfig` class. The `properties` map in the `WorkerConfig` is empty. The other `PulsarConfiguration` implementations are initially loaded by calling a method from the `PulsarConfigurationLoader` class. That class's underlying logic is to read all fields from the configuration yaml into the `Properties` map and then it uses reflection to properly populate any fields of the config class that share the same name as keys in the properties object.
As @eolivelli pointed out, the current workflow for constructing the `ServiceConfiguration` was to use the `PulsarConfigurationLoader` method named `convertFrom`. This method maps declared fields to declared fields as follows:
```java
public static ServiceConfiguration convertFrom(PulsarConfiguration conf, boolean ignoreNonExistMember) throws RuntimeException {
try {
final ServiceConfiguration convertedConf = ServiceConfiguration.class.newInstance();
Field[] confFields = conf.getClass().getDeclaredFields();
Arrays.stream(confFields).forEach(confField -> {
try {
Field convertedConfField = ServiceConfiguration.class.getDeclaredField(confField.getName());
confField.setAccessible(true);
if (!Modifier.isStatic(convertedConfField.getModifiers())) {
convertedConfField.setAccessible(true);
convertedConfField.set(convertedConf, confField.get(conf));
}
} catch (NoSuchFieldException e) {
if (!ignoreNonExistMember) {
throw new IllegalArgumentException("Exception caused while converting configuration: " + e.getMessage());
}
} catch (IllegalAccessException e) {
throw new RuntimeException("Exception caused while converting configuration: " + e.getMessage());
}
});
return convertedConf;
} catch (InstantiationException e) {
throw new RuntimeException("Exception caused while converting configuration: " + e.getMessage());
} catch (IllegalAccessException e) {
throw new RuntimeException("Exception caused while converting configuration: " + e.getMessage());
}
}
```
Given that the properties field in the `WorkerConfig` is empty, the properties field in the newly created `ServiceConfiguration` is empty too, which means custom configs are not available. This is the central problem I am solving.
> If you can point out specifically which field is missing, we can take a close look at the impact.
In the functions worker, the `ServiceConfiguration` is _only_ used to configure the `AuthenticationService` and the `AuthorizationService`. Both of these services can load custom plugins that users create. Custom plugins need custom configuration. It is not an option to add the specific fields to the `WorkerConfig` or even the `ServiceConfiguration` because these custom plugins are arbitrary by definition.
> I'm in favor of adding this map into WorkerConfig instead of introducing a second configuration.
Can you clarify what you mean when you say "second configuration"? There are already two configuration classes in the `Worker` class. I am simply changing how the `ServiceConfiguration` class is constructed.
### Alternative Solution
As I mentioned in the initial description, we could alternatively change how we load the `WorkerConfig`. Using the `PulsarConfigurationLoader` would populate `properties` field in the `WorkerConfig`, which would mean calling the `convertFrom` method would lead to a properly constructed `ServiceConfiguration`.
I am fine with loading the `WorkerConfig` in a new way, but I am not familiar enough with the functions worker to know if this is a safe change. At worst, it could result in new fields being present in the worker config's `properties` map, which would mean some configurations that were previously ignored are suddenly honored. My change carries a similar kind of risk, but its impact is more limited. It will only affect custom configuration of the `AuthenticationService` and the `AuthorizationService`.
Additionally, given that `WorkerConfig.load` is well tested, I assumed that its construction with an `ObjectMapper` was intentional and that we would not want to change it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468#discussion_r692295904
##########
File path: pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/FunctionWorkerStarter.java
##########
@@ -65,13 +67,17 @@ public static void main(String[] args) throws Exception {
}
WorkerConfig workerConfig;
+ ServiceConfiguration serviceConfiguration;
if (isBlank(workerArguments.configFile)) {
workerConfig = new WorkerConfig();
+ serviceConfiguration = new ServiceConfiguration();
} else {
workerConfig = WorkerConfig.load(workerArguments.configFile);
+ serviceConfiguration = PulsarConfigurationLoader.create(workerArguments.configFile, ServiceConfiguration.class);
Review comment:
This doesn't look correct. the `workerArguments.configFile` points to the `function_worker.yml` file. However PulsarConfigurationLoader.create expects a properties file. I guess this would make sense if PulsarConfigurationLoader was modified to support yaml files?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468#issuecomment-888822941
@eolivelli - thanks for your feedback. I should be able to add tests tomorrow or Friday.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468#issuecomment-895570433
First of all, this ticket should be marked as `doc-required` as it introduces a different behavior to load customized settings.
Secondly, the worker is designed to load settings from the worker file config, not from the broker file config. I don't think we should change the constructor to pass in another broker service configuration object. Why can't we access the underlying key/value map from the worker config object?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468#issuecomment-895400420
@jerrypeng @srkukarni @freeznet @nlu90 - PTAL. This change lets users configure function workers with custom configs that are not members of the `WorkerConfig` class. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nlu90 commented on pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
Posted by GitBox <gi...@apache.org>.
nlu90 commented on pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468#issuecomment-896918266
> > How about add the missing fields into the `WorkerConfig`?
>
> @nlu90 - this isn't an option because the fields are for extensions created by users. Note that the `ServiceConfigruation` already has a map for arbitrary config. The `WorkerConfig` does not have this map. An alternate solution could instead add the map to the `WorkerConfig`, but I'm not sure how that would impact a function worker's runtime.
I'm in favor of adding this map into `WorkerConfig` instead of introducing a second configuration. If you can point out specifically which field is missing, we can take a close look at the impact.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468#issuecomment-902067736
I just checked the logic of `PulsarConfigurationLoader.convertFrom`. It is different from `PulsarConfigurationLoader.create`. `convertForm` does a clear job of checking and setting fields. But `create` just load it as a properties bag.
I agreed with what Jerry commented in https://github.com/apache/pulsar/pull/11468#pullrequestreview-731062476
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468#issuecomment-902133922
> I just checked the logic of `PulsarConfigurationLoader.convertFrom`. It is different from `PulsarConfigurationLoader.create`. `convertForm` does a clear job of checking and setting fields. But `create` just load it as a properties bag.
You are right. I hadn't noticed that the file formatting was different between the broker config (`.conf`) and the function worker config (`.yml`). I had been working as if the files were interchangeable.
Sorry for the confusion @jerrypeng and @sijie, and thank you for your feedback.
Closing this PR, as it is already possible to pass custom configuration in to the `ServiceConfiguration` via the `WorkerConfig`. It can be passed in the `properties` map in the `functions_worker.yml` file. Here is a sample:
```yaml
properties:
myCustomConfig: "some-custom-value"
```
The Object Mapper will put those values in the WorkerConfig's `properties` map. When the `WorkerConfig` is converted to the `ServiceConfiguration`, the `properties` map is properly converted, and the arbitrary configuration is available.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall closed pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
Posted by GitBox <gi...@apache.org>.
michaeljmarshall closed pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nlu90 commented on pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
Posted by GitBox <gi...@apache.org>.
nlu90 commented on pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468#issuecomment-889487688
How about add the missing fields into the `WorkerConfig`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #11468: [Function worker] Load ServiceConfiguration from file, not from Worke…
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #11468:
URL: https://github.com/apache/pulsar/pull/11468#discussion_r692367847
##########
File path: pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/FunctionWorkerStarter.java
##########
@@ -65,13 +67,17 @@ public static void main(String[] args) throws Exception {
}
WorkerConfig workerConfig;
+ ServiceConfiguration serviceConfiguration;
if (isBlank(workerArguments.configFile)) {
workerConfig = new WorkerConfig();
+ serviceConfiguration = new ServiceConfiguration();
} else {
workerConfig = WorkerConfig.load(workerArguments.configFile);
+ serviceConfiguration = PulsarConfigurationLoader.create(workerArguments.configFile, ServiceConfiguration.class);
Review comment:
You are correct. In my haste to get a solution, I missed the file extensions that should have warned me that I was misunderstanding how things worked. I'll close this PR. Thank you for this helpful comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org