You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2020/10/29 00:08:16 UTC
[GitHub] [incubator-gobblin] Will-Lo opened a new pull request #3142: [GOBBLIN-1304] Adds group ownership service
Will-Lo opened a new pull request #3142:
URL: https://github.com/apache/incubator-gobblin/pull/3142
Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
### JIRA
- [x] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-1304
### Description
- [x] Here are some details about my PR, including screenshots (if applicable):
Utilizes the request option `owning_group` in GaaS so that users within the same group can modify and delete each others' flows.
### Tests
- [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
### Commits
- [x] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] arjun4084346 commented on a change in pull request #3142: [GOBBLIN-1304] Adds group ownership service
Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3142:
URL: https://github.com/apache/incubator-gobblin/pull/3142#discussion_r517643826
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-api/src/main/snapshot/org.apache.gobblin.service.flowconfigsV2.snapshot.json
##########
@@ -67,6 +67,11 @@
"type" : "boolean",
"doc" : "Return the compiled flow as a string. If enabled, the flow is not added.",
"default" : false
+ }, {
+ "name" : "owningGroup",
Review comment:
We may need to handle this in FlowSpecDeserializer/FlowSpecSerializer
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] Will-Lo commented on a change in pull request #3142: [GOBBLIN-1304] Adds group ownership service
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on a change in pull request #3142:
URL: https://github.com/apache/incubator-gobblin/pull/3142#discussion_r517705772
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-api/src/main/snapshot/org.apache.gobblin.service.flowconfigsV2.snapshot.json
##########
@@ -67,6 +67,11 @@
"type" : "boolean",
"doc" : "Return the compiled flow as a string. If enabled, the flow is not added.",
"default" : false
+ }, {
+ "name" : "owningGroup",
Review comment:
I think the FlowSpecDeserializer/Serializer should be fine, since it reads the fields from `getConfigAsProperties` which I modified on `FlowSpec`. I'll take a look into seeing what needs to be changed in MySqlSpecStore though, good catch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] codecov-io edited a comment on pull request #3142: [GOBBLIN-1304] Adds group ownership service
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #3142:
URL: https://github.com/apache/incubator-gobblin/pull/3142#issuecomment-721985908
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3142?src=pr&el=h1) Report
> Merging [#3142](https://codecov.io/gh/apache/incubator-gobblin/pull/3142?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/c35bfaf5298f1a97902367c28ecbfb13612da8cd?el=desc) will **decrease** coverage by `4.28%`.
> The diff coverage is `10.52%`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3142?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3142 +/- ##
============================================
- Coverage 46.05% 41.76% -4.29%
+ Complexity 9604 8764 -840
============================================
Files 1989 1993 +4
Lines 75896 75986 +90
Branches 8452 8462 +10
============================================
- Hits 34951 31735 -3216
- Misses 37662 41230 +3568
+ Partials 3283 3021 -262
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3142?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...pache/gobblin/configuration/ConfigurationKeys.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29uZmlndXJhdGlvbi9Db25maWd1cmF0aW9uS2V5cy5qYXZh) | `0.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
| [.../org/apache/gobblin/service/ServiceConfigKeys.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9TZXJ2aWNlQ29uZmlnS2V5cy5qYXZh) | `0.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
| [...obblin/service/FlowConfigResourceLocalHandler.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93Q29uZmlnUmVzb3VyY2VMb2NhbEhhbmRsZXIuamF2YQ==) | `18.39% <0.00%> (-0.44%)` | `2.00 <0.00> (ø)` | |
| [.../apache/gobblin/service/FlowConfigsV2Resource.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93Q29uZmlnc1YyUmVzb3VyY2UuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [.../apache/gobblin/service/GroupOwnershipService.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9Hcm91cE93bmVyc2hpcFNlcnZpY2UuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [...ice/LocalGroupOwnershipPathAlterationListener.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9Mb2NhbEdyb3VwT3duZXJzaGlwUGF0aEFsdGVyYXRpb25MaXN0ZW5lci5qYXZh) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [...he/gobblin/service/LocalGroupOwnershipService.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9Mb2NhbEdyb3VwT3duZXJzaGlwU2VydmljZS5qYXZh) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [...che/gobblin/service/NoopGroupOwnershipService.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9Ob29wR3JvdXBPd25lcnNoaXBTZXJ2aWNlLmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [.../java/org/apache/gobblin/runtime/api/FlowSpec.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvYXBpL0Zsb3dTcGVjLmphdmE=) | `43.01% <0.00%> (-0.49%)` | `13.00 <0.00> (ø)` | |
| [...in/service/modules/core/GobblinServiceManager.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9jb3JlL0dvYmJsaW5TZXJ2aWNlTWFuYWdlci5qYXZh) | `63.34% <66.66%> (+0.13%)` | `34.00 <0.00> (ø)` | |
| ... and [167 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3142?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3142?src=pr&el=footer). Last update [c35bfaf...4b41716](https://codecov.io/gh/apache/incubator-gobblin/pull/3142?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] Will-Lo commented on a change in pull request #3142: [GOBBLIN-1304] Adds group ownership service
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on a change in pull request #3142:
URL: https://github.com/apache/incubator-gobblin/pull/3142#discussion_r518926214
##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/spec_store/MysqlSpecStore.java
##########
@@ -83,8 +83,8 @@
+ "spec LONGBLOB, " + NEW_COLUMN + " JSON, PRIMARY KEY (spec_uri))";
private static final String EXISTS_STATEMENT = "SELECT EXISTS(SELECT * FROM %s WHERE spec_uri = ?)";
protected static final String INSERT_STATEMENT = "INSERT INTO %s (spec_uri, flow_group, flow_name, template_uri, "
- + "user_to_proxy, source_identifier, destination_identifier, schedule, tag, isRunImmediately, spec, " + NEW_COLUMN + ") "
- + "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON DUPLICATE KEY UPDATE spec = VALUES(spec), " + NEW_COLUMN + " = VALUES(" + NEW_COLUMN + ")";
+ + "user_to_proxy, source_identifier, destination_identifier, schedule, tag, isRunImmediately, owning_group, spec, " + NEW_COLUMN + ") "
Review comment:
Would we introduce backwards incompatible changes with that? The field was already initialized in sql tables as `owning_group` a few months back, so I stuck with that. Otherwise we would need to possibly rename the column names of existing flowspec stores stored in mysql.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] Will-Lo commented on a change in pull request #3142: [GOBBLIN-1304] Adds group ownership service
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on a change in pull request #3142:
URL: https://github.com/apache/incubator-gobblin/pull/3142#discussion_r518924533
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -185,20 +203,102 @@ public void testBadPartialUpdate() throws Exception {
_client.partialUpdateFlowConfig(flowId, flowConfigPatch);
}
- @Test (expectedExceptions = RestLiResponseException.class)
+ @Test
public void testDisallowedRequester() throws Exception {
- ServiceRequester testRequester = new ServiceRequester("testName", "testType", "testFrom");
- _requesterService.setRequester(testRequester);
+ try {
+ ServiceRequester testRequester = new ServiceRequester("testName", "testType", "testFrom");
+ _requesterService.setRequester(testRequester);
+
+ Map<String, String> flowProperties = Maps.newHashMap();
+ flowProperties.put("param1", "value1");
+
+ FlowConfig flowConfig = new FlowConfig().setId(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_4))
+ .setTemplateUris(TEST_TEMPLATE_URI)
+ .setProperties(new StringMap(flowProperties));
+ _client.createFlowConfig(flowConfig);
+
+ testRequester.setName("testName2");
+ _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_4));
+ } catch (RestLiResponseException e) {
+ Assert.assertEquals(e.getStatus(), HttpStatus.ORDINAL_401_Unauthorized);
+ }
+ }
+ @Test
+ public void testGroupRequesterAllowed() throws Exception {
+ ServiceRequester testRequester = new ServiceRequester("testName", "USER_PRINCIPAL", "testFrom");
+ _requesterService.setRequester(testRequester);
Map<String, String> flowProperties = Maps.newHashMap();
- flowProperties.put("param1", "value1");
- FlowConfig flowConfig = new FlowConfig().setId(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME))
- .setTemplateUris(TEST_TEMPLATE_URI).setProperties(new StringMap(flowProperties));
- _client.createFlowConfig(flowConfig);
+ FlowConfig flowConfig = new FlowConfig().setId(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_5))
+ .setTemplateUris(TEST_TEMPLATE_URI)
+ .setProperties(new StringMap(flowProperties))
+ .setOwningGroup("testGroup");
+
+ _client.createFlowConfig(flowConfig);
testRequester.setName("testName2");
- _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME));
+ _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_5));
+ }
+
+ @Test
+ public void testGroupRequesterRejected() throws Exception {
Review comment:
testDisallowedRequester doesn't test using owning groups, it was originally used to test the requester service. This tests for a group member that isn't part of a group that exists. They are pretty similar though.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] codecov-io commented on pull request #3142: [GOBBLIN-1304] Adds group ownership service
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #3142:
URL: https://github.com/apache/incubator-gobblin/pull/3142#issuecomment-721985908
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3142?src=pr&el=h1) Report
> Merging [#3142](https://codecov.io/gh/apache/incubator-gobblin/pull/3142?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/c35bfaf5298f1a97902367c28ecbfb13612da8cd?el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `8.60%`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3142?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3142 +/- ##
============================================
- Coverage 46.05% 46.01% -0.04%
+ Complexity 9604 9603 -1
============================================
Files 1989 1993 +4
Lines 75896 75984 +88
Branches 8452 8462 +10
============================================
+ Hits 34951 34962 +11
- Misses 37662 37739 +77
Partials 3283 3283
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3142?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...pache/gobblin/configuration/ConfigurationKeys.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vY29uZmlndXJhdGlvbi9Db25maWd1cmF0aW9uS2V5cy5qYXZh) | `0.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
| [.../org/apache/gobblin/service/ServiceConfigKeys.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9TZXJ2aWNlQ29uZmlnS2V5cy5qYXZh) | `0.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
| [...obblin/service/FlowConfigResourceLocalHandler.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93Q29uZmlnUmVzb3VyY2VMb2NhbEhhbmRsZXIuamF2YQ==) | `18.39% <0.00%> (-0.44%)` | `2.00 <0.00> (ø)` | |
| [.../apache/gobblin/service/FlowConfigsV2Resource.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93Q29uZmlnc1YyUmVzb3VyY2UuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [.../apache/gobblin/service/GroupOwnershipService.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9Hcm91cE93bmVyc2hpcFNlcnZpY2UuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [...ice/LocalGroupOwnershipPathAlterationListener.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9Mb2NhbEdyb3VwT3duZXJzaGlwUGF0aEFsdGVyYXRpb25MaXN0ZW5lci5qYXZh) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [...he/gobblin/service/LocalGroupOwnershipService.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9Mb2NhbEdyb3VwT3duZXJzaGlwU2VydmljZS5qYXZh) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [...che/gobblin/service/NoopGroupOwnershipService.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9Ob29wR3JvdXBPd25lcnNoaXBTZXJ2aWNlLmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [.../java/org/apache/gobblin/runtime/api/FlowSpec.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvYXBpL0Zsb3dTcGVjLmphdmE=) | `43.01% <0.00%> (-0.49%)` | `13.00 <0.00> (ø)` | |
| [...in/service/modules/core/GobblinServiceManager.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9jb3JlL0dvYmJsaW5TZXJ2aWNlTWFuYWdlci5qYXZh) | `63.34% <66.66%> (+0.13%)` | `33.00 <0.00> (-1.00)` | :arrow_up: |
| ... and [14 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3142/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3142?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3142?src=pr&el=footer). Last update [c35bfaf...2d76614](https://codecov.io/gh/apache/incubator-gobblin/pull/3142?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] Will-Lo commented on a change in pull request #3142: [GOBBLIN-1304] Adds group ownership service
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on a change in pull request #3142:
URL: https://github.com/apache/incubator-gobblin/pull/3142#discussion_r514471093
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/FlowConfigsV2Resource.java
##########
@@ -200,4 +210,29 @@ private Properties getHeaders() {
}
return headerProperties;
}
+
+ public void checkRequester(
+ RequesterService requesterService, FlowConfig originalFlowConfig, List<ServiceRequester> requesterList) {
+ if (requesterList == null) {
+ return;
+ }
+
+ try {
+ String serializedOriginalRequesterList = originalFlowConfig.getProperties().get(RequesterService.REQUESTER_LIST);
+ if (serializedOriginalRequesterList != null) {
+ List<ServiceRequester> originalRequesterList = RequesterService.deserialize(serializedOriginalRequesterList);
+ String owningGroup = originalFlowConfig.getProperties().getOrDefault("owning_group", "");
+ // check that the requester is part of the owning group
+ if (!owningGroup.isEmpty()) {
Review comment:
I like the idea of an admin mode! Would the admin mode just be another flow configuration parameter? How would we authenticate that?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] arjun4084346 commented on a change in pull request #3142: [GOBBLIN-1304] Adds group ownership service
Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on a change in pull request #3142:
URL: https://github.com/apache/incubator-gobblin/pull/3142#discussion_r517644933
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-api/src/main/snapshot/org.apache.gobblin.service.flowconfigsV2.snapshot.json
##########
@@ -67,6 +67,11 @@
"type" : "boolean",
"doc" : "Return the compiled flow as a string. If enabled, the flow is not added.",
"default" : false
+ }, {
+ "name" : "owningGroup",
Review comment:
and also update mysql table schema with this field, and handle this field in MysqlSpecStore ?
Feel free to do this in other PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] Will-Lo commented on a change in pull request #3142: [GOBBLIN-1304] Adds group ownership service
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on a change in pull request #3142:
URL: https://github.com/apache/incubator-gobblin/pull/3142#discussion_r518924923
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/GroupOwnershipService.java
##########
@@ -0,0 +1,45 @@
+/*
+ * 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.gobblin.service;
+
+import java.util.List;
+import java.util.stream.Collectors;
+
+
+/**
+ * Service for handling group ownership of flows
+ */
+public abstract class GroupOwnershipService {
+
+ public static String GROUP_OWNERSHIP_PARAMETER = "owning_group";
Review comment:
Whoops this variable isn't being used after owningGroup got moved to be top level, I'll just delete 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #3142: [GOBBLIN-1304] Adds group ownership service
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3142:
URL: https://github.com/apache/incubator-gobblin/pull/3142#discussion_r514363955
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/GroupOwnershipService.java
##########
@@ -0,0 +1,27 @@
+/*
+ * 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.gobblin.service;
+
+import java.util.List;
+
+
+public abstract class GroupOwnershipService {
Review comment:
Add javadoc for this abstract class as well as its public methods.
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/LocalGroupOwnershipService.java
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.gobblin.service;
+
+import com.google.gson.JsonObject;
+import com.typesafe.config.Config;
+import java.io.IOException;
+import java.nio.charset.Charset;
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.io.IOUtils;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import com.google.gson.JsonParser;
+
+
+public class LocalGroupOwnershipService extends GroupOwnershipService {
+ public static final String GROUP_MEMBER_LIST = ServiceConfigKeys.GOBBLIN_SERVICE_PREFIX + "groupMembers.path";
+
+ JsonObject groupOwnerships;
+
+ public LocalGroupOwnershipService(Config config) {
+ Path groupOwnershipFilePath = new Path(config.getString(GROUP_MEMBER_LIST));
Review comment:
I like the idea of having a separate data file to maintain the group ownerships and keeping its deployment separate from the service. It would be good to add the logic to reload the config on detecting a change in the file. Take a look at the PathAlterationObserver class that can help with this. But I am ok to not hold up this PR if substantial changes are needed.
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/LocalGroupOwnershipService.java
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.gobblin.service;
+
+import com.google.gson.JsonObject;
+import com.typesafe.config.Config;
+import java.io.IOException;
+import java.nio.charset.Charset;
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.io.IOUtils;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import com.google.gson.JsonParser;
+
+
+public class LocalGroupOwnershipService extends GroupOwnershipService {
+ public static final String GROUP_MEMBER_LIST = ServiceConfigKeys.GOBBLIN_SERVICE_PREFIX + "groupMembers.path";
+
+ JsonObject groupOwnerships;
+
+ public LocalGroupOwnershipService(Config config) {
+ Path groupOwnershipFilePath = new Path(config.getString(GROUP_MEMBER_LIST));
+ FSDataInputStream in = null;
+ try {
+ FileSystem fs = FileSystem.get(new Configuration());
+ in = fs.open(groupOwnershipFilePath);
+ String jsonString = IOUtils.toString(in, Charset.defaultCharset());
+ JsonParser parser = new JsonParser();
+ this.groupOwnerships = parser.parse(jsonString).getAsJsonObject();
+ in.close();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @Override
+ public boolean isMemberOfGroup(List<ServiceRequester> serviceRequesters, String group) {
+ if (this.groupOwnerships.has(group)) {
+ String member = extractRequesterUser(serviceRequesters);
+ List<String> groupMembers = Arrays.asList(groupOwnerships.get(group).getAsString().split(","));
+ return groupMembers.contains(member);
+ }
+ return false;
+ }
+
+ private static String extractRequesterUser(List<ServiceRequester> requesterList) {
+ return requesterList.stream()
+ .filter(serviceRequester -> serviceRequester.getType().equals(USER_TYPE))
+ .collect(Collectors.toList())
+ .get(0)
Review comment:
Potential NPE in case of requests made by non USER principals.
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/FlowConfigsV2Resource.java
##########
@@ -200,4 +210,29 @@ private Properties getHeaders() {
}
return headerProperties;
}
+
+ public void checkRequester(
Review comment:
Add javadoc explaining the behavior of this method.
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/LocalGroupOwnershipService.java
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.gobblin.service;
+
+import com.google.gson.JsonObject;
+import com.typesafe.config.Config;
+import java.io.IOException;
+import java.nio.charset.Charset;
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.io.IOUtils;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import com.google.gson.JsonParser;
+
+
+public class LocalGroupOwnershipService extends GroupOwnershipService {
+ public static final String GROUP_MEMBER_LIST = ServiceConfigKeys.GOBBLIN_SERVICE_PREFIX + "groupMembers.path";
+
+ JsonObject groupOwnerships;
+
+ public LocalGroupOwnershipService(Config config) {
+ Path groupOwnershipFilePath = new Path(config.getString(GROUP_MEMBER_LIST));
+ FSDataInputStream in = null;
+ try {
+ FileSystem fs = FileSystem.get(new Configuration());
+ in = fs.open(groupOwnershipFilePath);
+ String jsonString = IOUtils.toString(in, Charset.defaultCharset());
+ JsonParser parser = new JsonParser();
+ this.groupOwnerships = parser.parse(jsonString).getAsJsonObject();
+ in.close();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @Override
+ public boolean isMemberOfGroup(List<ServiceRequester> serviceRequesters, String group) {
+ if (this.groupOwnerships.has(group)) {
+ String member = extractRequesterUser(serviceRequesters);
+ List<String> groupMembers = Arrays.asList(groupOwnerships.get(group).getAsString().split(","));
+ return groupMembers.contains(member);
+ }
+ return false;
+ }
+
+ private static String extractRequesterUser(List<ServiceRequester> requesterList) {
+ return requesterList.stream()
+ .filter(serviceRequester -> serviceRequester.getType().equals(USER_TYPE))
Review comment:
We shouldn't assume every request is made by a USER principal. There could be use cases where a SERVICE principal can make a request too.
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GobblinServiceManager.java
##########
@@ -285,6 +287,13 @@ public GobblinServiceManager(String serviceName, String serviceId, Config config
this.isRestLIServerEnabled = ConfigUtils.getBoolean(config,
ServiceConfigKeys.GOBBLIN_SERVICE_RESTLI_SERVER_ENABLED_KEY, true);
+ GroupOwnershipService groupOwnershipService = GobblinConstructorUtils.invokeConstructor(
Review comment:
Should we introduce aliases for the different group ownership service implementations and use an alias resolver to instantiate the configured group ownership service?
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/LocalGroupOwnershipService.java
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.gobblin.service;
+
+import com.google.gson.JsonObject;
+import com.typesafe.config.Config;
+import java.io.IOException;
+import java.nio.charset.Charset;
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.io.IOUtils;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import com.google.gson.JsonParser;
+
+
+public class LocalGroupOwnershipService extends GroupOwnershipService {
+ public static final String GROUP_MEMBER_LIST = ServiceConfigKeys.GOBBLIN_SERVICE_PREFIX + "groupMembers.path";
+
+ JsonObject groupOwnerships;
+
+ public LocalGroupOwnershipService(Config config) {
+ Path groupOwnershipFilePath = new Path(config.getString(GROUP_MEMBER_LIST));
+ FSDataInputStream in = null;
+ try {
+ FileSystem fs = FileSystem.get(new Configuration());
+ in = fs.open(groupOwnershipFilePath);
+ String jsonString = IOUtils.toString(in, Charset.defaultCharset());
+ JsonParser parser = new JsonParser();
+ this.groupOwnerships = parser.parse(jsonString).getAsJsonObject();
+ in.close();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @Override
+ public boolean isMemberOfGroup(List<ServiceRequester> serviceRequesters, String group) {
+ if (this.groupOwnerships.has(group)) {
+ String member = extractRequesterUser(serviceRequesters);
+ List<String> groupMembers = Arrays.asList(groupOwnerships.get(group).getAsString().split(","));
Review comment:
You can use Guava's Splitter:
Splitter.on(',').trimResults().omitEmptyStrings().splitToList(groupOwnerships.get(group).getAsString())
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/LocalGroupOwnershipService.java
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.gobblin.service;
+
+import com.google.gson.JsonObject;
+import com.typesafe.config.Config;
+import java.io.IOException;
+import java.nio.charset.Charset;
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.io.IOUtils;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import com.google.gson.JsonParser;
+
+
+public class LocalGroupOwnershipService extends GroupOwnershipService {
+ public static final String GROUP_MEMBER_LIST = ServiceConfigKeys.GOBBLIN_SERVICE_PREFIX + "groupMembers.path";
+
+ JsonObject groupOwnerships;
+
+ public LocalGroupOwnershipService(Config config) {
+ Path groupOwnershipFilePath = new Path(config.getString(GROUP_MEMBER_LIST));
+ FSDataInputStream in = null;
+ try {
+ FileSystem fs = FileSystem.get(new Configuration());
+ in = fs.open(groupOwnershipFilePath);
+ String jsonString = IOUtils.toString(in, Charset.defaultCharset());
+ JsonParser parser = new JsonParser();
+ this.groupOwnerships = parser.parse(jsonString).getAsJsonObject();
+ in.close();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @Override
+ public boolean isMemberOfGroup(List<ServiceRequester> serviceRequesters, String group) {
+ if (this.groupOwnerships.has(group)) {
+ String member = extractRequesterUser(serviceRequesters);
+ List<String> groupMembers = Arrays.asList(groupOwnerships.get(group).getAsString().split(","));
+ return groupMembers.contains(member);
+ }
+ return false;
+ }
+
+ private static String extractRequesterUser(List<ServiceRequester> requesterList) {
Review comment:
Probably this method can go into the abstract class and presumably, will be needed for other implementations too.
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/FlowConfigsV2Resource.java
##########
@@ -200,4 +210,29 @@ private Properties getHeaders() {
}
return headerProperties;
}
+
+ public void checkRequester(
+ RequesterService requesterService, FlowConfig originalFlowConfig, List<ServiceRequester> requesterList) {
+ if (requesterList == null) {
+ return;
+ }
+
+ try {
+ String serializedOriginalRequesterList = originalFlowConfig.getProperties().get(RequesterService.REQUESTER_LIST);
+ if (serializedOriginalRequesterList != null) {
+ List<ServiceRequester> originalRequesterList = RequesterService.deserialize(serializedOriginalRequesterList);
+ String owningGroup = originalFlowConfig.getProperties().getOrDefault("owning_group", "");
+ // check that the requester is part of the owning group
+ if (!owningGroup.isEmpty()) {
Review comment:
So is the expected behavior that if owningGroup is specified, only members of that group are allowed to perform the request? What if we have a group with a single member that created a flow and that member is no longer in the organization?
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/LocalGroupOwnershipService.java
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.gobblin.service;
+
+import com.google.gson.JsonObject;
+import com.typesafe.config.Config;
+import java.io.IOException;
+import java.nio.charset.Charset;
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.io.IOUtils;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import com.google.gson.JsonParser;
+
+
+public class LocalGroupOwnershipService extends GroupOwnershipService {
+ public static final String GROUP_MEMBER_LIST = ServiceConfigKeys.GOBBLIN_SERVICE_PREFIX + "groupMembers.path";
+
+ JsonObject groupOwnerships;
+
+ public LocalGroupOwnershipService(Config config) {
+ Path groupOwnershipFilePath = new Path(config.getString(GROUP_MEMBER_LIST));
+ FSDataInputStream in = null;
+ try {
+ FileSystem fs = FileSystem.get(new Configuration());
+ in = fs.open(groupOwnershipFilePath);
+ String jsonString = IOUtils.toString(in, Charset.defaultCharset());
+ JsonParser parser = new JsonParser();
+ this.groupOwnerships = parser.parse(jsonString).getAsJsonObject();
+ in.close();
Review comment:
try with resources to avoid having to call in.close()?
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/FlowConfigsV2Resource.java
##########
@@ -200,4 +210,29 @@ private Properties getHeaders() {
}
return headerProperties;
}
+
+ public void checkRequester(
+ RequesterService requesterService, FlowConfig originalFlowConfig, List<ServiceRequester> requesterList) {
+ if (requesterList == null) {
+ return;
+ }
+
+ try {
+ String serializedOriginalRequesterList = originalFlowConfig.getProperties().get(RequesterService.REQUESTER_LIST);
+ if (serializedOriginalRequesterList != null) {
+ List<ServiceRequester> originalRequesterList = RequesterService.deserialize(serializedOriginalRequesterList);
+ String owningGroup = originalFlowConfig.getProperties().getOrDefault("owning_group", "");
+ // check that the requester is part of the owning group
+ if (!owningGroup.isEmpty()) {
Review comment:
We should accommodate an "admin" mode where a user with admin privileges can also make a request.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] Will-Lo commented on a change in pull request #3142: [GOBBLIN-1304] Adds group ownership service
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on a change in pull request #3142:
URL: https://github.com/apache/incubator-gobblin/pull/3142#discussion_r516156242
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/GroupOwnershipService.java
##########
@@ -0,0 +1,27 @@
+/*
+ * 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.gobblin.service;
+
+import java.util.List;
+
+
+public abstract class GroupOwnershipService {
+ public static String USER_TYPE = "USER_PRINCIPAL";
+
+ abstract boolean isMemberOfGroup(List<ServiceRequester> serviceRequesters, String group);
Review comment:
By default it should be protected but I'll explicitly define it as such too
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/GroupOwnershipService.java
##########
@@ -0,0 +1,27 @@
+/*
+ * 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.gobblin.service;
+
+import java.util.List;
+
+
+public abstract class GroupOwnershipService {
+ public static String USER_TYPE = "USER_PRINCIPAL";
+
+ abstract boolean isMemberOfGroup(List<ServiceRequester> serviceRequesters, String group);
Review comment:
Since the class is public abstract by default it should be protected but I'll explicitly define it as such too
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] asfgit closed pull request #3142: [GOBBLIN-1304] Adds group ownership service
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3142:
URL: https://github.com/apache/incubator-gobblin/pull/3142
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #3142: [GOBBLIN-1304] Adds group ownership service
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3142:
URL: https://github.com/apache/incubator-gobblin/pull/3142#discussion_r518477697
##########
File path: gobblin-api/src/main/java/org/apache/gobblin/configuration/ConfigurationKeys.java
##########
@@ -140,6 +140,7 @@
public static final String FLOW_ALLOW_CONCURRENT_EXECUTION = "flow.allowConcurrentExecution";
public static final String FLOW_EXPLAIN_KEY = "flow.explain";
public static final String FLOW_UNSCHEDULE_KEY = "flow.unschedule";
+ public static final String FLOW_OWNING_GROUP_KEY = "flow.owning_group";
Review comment:
Camel case owning_group to owningGroup.
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/LocalGroupOwnershipPathAlterationListener.java
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.gobblin.service;
+
+import com.google.gson.JsonObject;
+import com.google.gson.JsonParser;
+import java.io.IOException;
+import java.nio.charset.Charset;
+import java.nio.file.Files;
+import org.apache.commons.io.IOUtils;
+import org.apache.gobblin.util.filesystem.PathAlterationListenerAdaptor;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class LocalGroupOwnershipPathAlterationListener extends PathAlterationListenerAdaptor {
Review comment:
+1. Well done.
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-client/src/test/java/org/apache/gobblin/service/FlowConfigV2Test.java
##########
@@ -185,20 +203,102 @@ public void testBadPartialUpdate() throws Exception {
_client.partialUpdateFlowConfig(flowId, flowConfigPatch);
}
- @Test (expectedExceptions = RestLiResponseException.class)
+ @Test
public void testDisallowedRequester() throws Exception {
- ServiceRequester testRequester = new ServiceRequester("testName", "testType", "testFrom");
- _requesterService.setRequester(testRequester);
+ try {
+ ServiceRequester testRequester = new ServiceRequester("testName", "testType", "testFrom");
+ _requesterService.setRequester(testRequester);
+
+ Map<String, String> flowProperties = Maps.newHashMap();
+ flowProperties.put("param1", "value1");
+
+ FlowConfig flowConfig = new FlowConfig().setId(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_4))
+ .setTemplateUris(TEST_TEMPLATE_URI)
+ .setProperties(new StringMap(flowProperties));
+ _client.createFlowConfig(flowConfig);
+
+ testRequester.setName("testName2");
+ _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_4));
+ } catch (RestLiResponseException e) {
+ Assert.assertEquals(e.getStatus(), HttpStatus.ORDINAL_401_Unauthorized);
+ }
+ }
+ @Test
+ public void testGroupRequesterAllowed() throws Exception {
+ ServiceRequester testRequester = new ServiceRequester("testName", "USER_PRINCIPAL", "testFrom");
+ _requesterService.setRequester(testRequester);
Map<String, String> flowProperties = Maps.newHashMap();
- flowProperties.put("param1", "value1");
- FlowConfig flowConfig = new FlowConfig().setId(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME))
- .setTemplateUris(TEST_TEMPLATE_URI).setProperties(new StringMap(flowProperties));
- _client.createFlowConfig(flowConfig);
+ FlowConfig flowConfig = new FlowConfig().setId(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_5))
+ .setTemplateUris(TEST_TEMPLATE_URI)
+ .setProperties(new StringMap(flowProperties))
+ .setOwningGroup("testGroup");
+
+ _client.createFlowConfig(flowConfig);
testRequester.setName("testName2");
- _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME));
+ _client.deleteFlowConfig(new FlowId().setFlowGroup(TEST_GROUP_NAME).setFlowName(TEST_FLOW_NAME_5));
+ }
+
+ @Test
+ public void testGroupRequesterRejected() throws Exception {
Review comment:
Curious: how is this test different from testDisallowedRequester?
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/NoopGroupOwnershipService.java
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.gobblin.service;
+
+import com.typesafe.config.Config;
+import java.util.List;
+
+
+public class NoopGroupOwnershipService extends GroupOwnershipService{
Review comment:
Can we use @Alias notations for different Group ownership service implementations?
##########
File path: gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/GroupOwnershipService.java
##########
@@ -0,0 +1,45 @@
+/*
+ * 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.gobblin.service;
+
+import java.util.List;
+import java.util.stream.Collectors;
+
+
+/**
+ * Service for handling group ownership of flows
+ */
+public abstract class GroupOwnershipService {
+
+ public static String GROUP_OWNERSHIP_PARAMETER = "owning_group";
Review comment:
owning_group -> owningGroup?
##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/spec_store/MysqlSpecStore.java
##########
@@ -83,8 +83,8 @@
+ "spec LONGBLOB, " + NEW_COLUMN + " JSON, PRIMARY KEY (spec_uri))";
private static final String EXISTS_STATEMENT = "SELECT EXISTS(SELECT * FROM %s WHERE spec_uri = ?)";
protected static final String INSERT_STATEMENT = "INSERT INTO %s (spec_uri, flow_group, flow_name, template_uri, "
- + "user_to_proxy, source_identifier, destination_identifier, schedule, tag, isRunImmediately, spec, " + NEW_COLUMN + ") "
- + "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON DUPLICATE KEY UPDATE spec = VALUES(spec), " + NEW_COLUMN + " = VALUES(" + NEW_COLUMN + ")";
+ + "user_to_proxy, source_identifier, destination_identifier, schedule, tag, isRunImmediately, owning_group, spec, " + NEW_COLUMN + ") "
Review comment:
owning_group -> owningGroup?
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GobblinServiceManager.java
##########
@@ -304,6 +306,19 @@ public GobblinServiceManager(String serviceName, String serviceId, Config config
this.isRestLIServerEnabled = ConfigUtils.getBoolean(config,
ServiceConfigKeys.GOBBLIN_SERVICE_RESTLI_SERVER_ENABLED_KEY, true);
+ ClassAliasResolver<GroupOwnershipService> groupOwnershipAliasResolver = new ClassAliasResolver<>(GroupOwnershipService.class);
+ String groupOwnershipServiceClass = ServiceConfigKeys.DEFAULT_GROUP_OWNERSHIP_SERVICE;
+ if (config.hasPath(ServiceConfigKeys.GROUP_OWNERSHIP_SERVICE_CLASS)) {
+ groupOwnershipServiceClass = config.getString(ServiceConfigKeys.GROUP_OWNERSHIP_SERVICE_CLASS);
+ }
+ GroupOwnershipService groupOwnershipService;
+ try {
+ groupOwnershipService = (GroupOwnershipService) ConstructorUtils.invokeConstructor(
Review comment:
Use aliases for different group ownership service implementations and use ClassAliasResolver here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org