You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/08/05 02:45:19 UTC
[GitHub] [nifi] jayaaditya opened a new pull request #4450: NIFI-7681 - Add update-bucket-policy command, add option to specify timeout and fix documentation to include previously implemented commands
jayaaditya opened a new pull request #4450:
URL: https://github.com/apache/nifi/pull/4450
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
https://issues.apache.org/jira/projects/NIFI/issues/NIFI-7681
Apart from the description present in the JIRA ticket, a few other miscellaneous changes have also been made:-
- Both `NiFiClient` and `NiFiRegistryClient` have configs to specify `readTimeout` and `connectionTimeout` parameters, however there was no way to specify those parameters in toolkit. I have added a couple of flags, `-rto` and `-cto` to specify the read and connection timeouts respectively. They can also be specified in the properties file by their longnames `readTimeout` and `connectionTimeout`.
- Toolkit documentation had a few commands missing, so I updated it to reflect all previously implemented commands
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [x] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [x] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [x] Have you verified that the full build is successful on JDK 8?
- [ ] Have you verified that the full build is successful on JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [x] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
----------------------------------------------------------------
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] [nifi] bbende merged pull request #4450: NIFI-7681 - Add update-bucket-policy command, add option to specify timeout and fix documentation to include previously implemented commands
Posted by GitBox <gi...@apache.org>.
bbende merged pull request #4450:
URL: https://github.com/apache/nifi/pull/4450
----------------------------------------------------------------
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] [nifi] bbende commented on pull request #4450: NIFI-7681 - Add update-bucket-policy command, add option to specify timeout and fix documentation to include previously implemented commands
Posted by GitBox <gi...@apache.org>.
bbende commented on pull request #4450:
URL: https://github.com/apache/nifi/pull/4450#issuecomment-670506705
Looks good, thanks! Will merge
----------------------------------------------------------------
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] [nifi] jayaaditya edited a comment on pull request #4450: NIFI-7681 - Add update-bucket-policy command, add option to specify timeout and fix documentation to include previously implemented commands
Posted by GitBox <gi...@apache.org>.
jayaaditya edited a comment on pull request #4450:
URL: https://github.com/apache/nifi/pull/4450#issuecomment-669899024
@bbende Thanks for your quick review! I have incorporated the changes you had suggested. Could you please review again? Thanks once again!
----------------------------------------------------------------
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] [nifi] jayaaditya edited a comment on pull request #4450: NIFI-7681 - Add update-bucket-policy command, add option to specify timeout and fix documentation to include previously implemented commands
Posted by GitBox <gi...@apache.org>.
jayaaditya edited a comment on pull request #4450:
URL: https://github.com/apache/nifi/pull/4450#issuecomment-669899024
@bbende Thanks for your quick review! I have incorporated the changes you have suggested. Could you please review again? Thanks once again!
----------------------------------------------------------------
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] [nifi] jayaaditya commented on pull request #4450: NIFI-7681 - Add update-bucket-policy command, add option to specify timeout and fix documentation to include previously implemented commands
Posted by GitBox <gi...@apache.org>.
jayaaditya commented on pull request #4450:
URL: https://github.com/apache/nifi/pull/4450#issuecomment-669899024
@bbende Thanks for your quick review! I have incorporated the changes you have suggested. Could you please review gain? Thanks once again!
----------------------------------------------------------------
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] [nifi] bbende commented on a change in pull request #4450: NIFI-7681 - Add update-bucket-policy command, add option to specify timeout and fix documentation to include previously implemented commands
Posted by GitBox <gi...@apache.org>.
bbende commented on a change in pull request #4450:
URL: https://github.com/apache/nifi/pull/4450#discussion_r465727136
##########
File path: nifi-docs/src/main/asciidoc/toolkit-guide.adoc
##########
@@ -94,10 +94,42 @@ The following are available commands:
nifi pg-get-services
nifi pg-enable-services
nifi pg-disable-services
+ nifi pg-create-service
+ nifi create-user
+ nifi list-users
+ nifi create-user-group
+ nifi list-user-groups
+ nifi update-user-group
+ nifi get-policy
+ nifi update-policy
+ nifi create-service
+ nifi get-services
+ nifi get-service
+ nifi disable-services
+ nifi enable-services
+ nifi get-reporting-task
+ nifi get-reporting-tasks
+ nifi create-reporting-task
+ nifi set-param
+ nifi delete-param
+ nifi list-param-contexts
+ nifi get-param-context
+ nifi create-param-context
+ nifi delete-param-context
+ nifi merge-param-context
+ nifi import-param-context
+ nifi pg-get-param-context
+ nifi pg-set-param-context
+ nifi list-templates
+ nifi download-template
+ nifi upload-template
+ nifi start-reporting-tasks
+ nifi stop-reporting-tasks
registry current-user
registry list-buckets
registry create-bucket
registry delete-bucket
+ registry update-bucket-policy
Review comment:
SInce we are updating this, there are other registry commands that got added for users/groups/policies that never made it into this list. Can we add those?
##########
File path: nifi-toolkit/nifi-toolkit-cli/src/main/java/org/apache/nifi/toolkit/cli/impl/command/CommandOption.java
##########
@@ -24,6 +24,8 @@
public enum CommandOption {
// General
+ CONNECTION_TIMEOUT("cto", "connectionTimeout", "Timeout parameter for creating a connection to NiFi/Registry", true),
+ READ_TIMEOUT("rto", "readTimeout", "Timeout parameter for reading from NiFi/Registry", true),
Review comment:
Can we add to the description of these to say that the value is in milliseconds?
##########
File path: nifi-toolkit/nifi-toolkit-cli/src/main/java/org/apache/nifi/toolkit/cli/impl/command/registry/bucket/UpdateBucketPolicy.java
##########
@@ -0,0 +1,129 @@
+/*
+ * 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.nifi.toolkit.cli.impl.command.registry.bucket;
+
+import org.apache.commons.cli.ParseException;
+import org.apache.nifi.registry.authorization.AccessPolicy;
+import org.apache.nifi.registry.authorization.Tenant;
+import org.apache.nifi.registry.bucket.Bucket;
+import org.apache.nifi.registry.client.NiFiRegistryClient;
+import org.apache.nifi.registry.client.NiFiRegistryException;
+import org.apache.nifi.toolkit.cli.api.Context;
+import org.apache.nifi.toolkit.cli.impl.client.ExtendedNiFiRegistryClient;
+import org.apache.nifi.toolkit.cli.impl.client.registry.PoliciesClient;
+import org.apache.nifi.toolkit.cli.impl.command.CommandOption;
+import org.apache.nifi.toolkit.cli.impl.command.registry.AbstractNiFiRegistryCommand;
+import org.apache.nifi.toolkit.cli.impl.command.registry.tenant.TenantHelper;
+import org.apache.nifi.toolkit.cli.impl.result.VoidResult;
+import org.apache.nifi.util.StringUtils;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.Set;
+
+public class UpdateBucketPolicy extends AbstractNiFiRegistryCommand<VoidResult> {
+
+
+ public UpdateBucketPolicy() {
+ super("update-bucket-policy", VoidResult.class);
+ }
+
+ @Override
+ public String getDescription() {
+ return "Updates access policy of bucket";
+ }
+
+ @Override
+ public void doInitialize(final Context context) {
+ addOption(CommandOption.BUCKET_NAME.createOption());
+ addOption(CommandOption.BUCKET_ID.createOption());
+ addOption(CommandOption.USER_NAME_LIST.createOption());
+ addOption(CommandOption.USER_ID_LIST.createOption());
+ addOption(CommandOption.GROUP_NAME_LIST.createOption());
+ addOption(CommandOption.GROUP_ID_LIST.createOption());
+ addOption(CommandOption.POLICY_ACTION.createOption());
+ }
+
+
+ @Override
+ public VoidResult doExecute(NiFiRegistryClient client, Properties properties) throws IOException, NiFiRegistryException, ParseException {
+ if (!(client instanceof ExtendedNiFiRegistryClient)) {
+ throw new IllegalArgumentException("This command needs extended registry client!");
+ }
+ final ExtendedNiFiRegistryClient extendedClient = (ExtendedNiFiRegistryClient) client;
+ final PoliciesClient policiesClient = extendedClient.getPoliciesClient();
+
+ final String bucketName = getArg(properties, CommandOption.BUCKET_NAME);
+ String bucketId = getArg(properties, CommandOption.BUCKET_ID);
+
+ final String userNames = getArg(properties, CommandOption.USER_NAME_LIST);
+ final String userIds = getArg(properties, CommandOption.USER_ID_LIST);
+ final String groupNames = getArg(properties, CommandOption.GROUP_NAME_LIST);
+ final String groupIds = getArg(properties, CommandOption.GROUP_ID_LIST);
+
+ final String policyAction = getRequiredArg(properties, CommandOption.POLICY_ACTION);
+ final HashSet<String> permittedActions = new HashSet<>(Arrays.asList("read", "write", "delete"));
+ if (!permittedActions.contains(policyAction)) {
+ throw new IllegalArgumentException("Only read, write, delete actions permitted");
+ }
+ if (StringUtils.isBlank(bucketName) == StringUtils.isBlank(bucketId)) {
+ throw new IllegalArgumentException("Specify either bucket name or bucket id");
+ }
+ if (StringUtils.isBlank(bucketId)) {
+ final Optional<Bucket> optionalBucket = client.getBucketClient().getAll()
+ .stream().filter(b -> bucketName.equals(b.getName())).findAny();
+ if (!optionalBucket.isPresent()) {
+ throw new IllegalArgumentException("Specified bucket does not exist");
+ }
+ bucketId = optionalBucket.get().getIdentifier();
+ } else {
+ try {
+ extendedClient.getBucketClient().get(bucketId);
+ } catch (NiFiRegistryException e) {
+ throw new IllegalArgumentException("Specified bucket does not exist");
+ }
+ }
+ AccessPolicy accessPolicy;
+ String resource = "/buckets/" + bucketId;
+ try {
+ accessPolicy = policiesClient.getAccessPolicy(policyAction, resource);
+ } catch (NiFiRegistryException e) {
+ accessPolicy = new AccessPolicy();
+ accessPolicy.setResource(resource);
+ accessPolicy.setAction(policyAction);
+ }
+ if (!StringUtils.isBlank(userNames) || !StringUtils.isBlank(userIds)) {
+ Set<Tenant> users = TenantHelper.selectExistingTenants(userNames,
+ userIds, extendedClient.getTenantsClient().getUsers());
+ accessPolicy.setUsers(users);
Review comment:
We may want to document that when updating the users it will overwrite any existing users, so if your goal is to add one user to an existing set of users, you need to send in all of them.
We have a little bit of an inconsistency between the NiFi command for UpdatePolicy and NiFi Registry command for CreateOrUpdateAccessPolicy.
The NiFi command has an argument for "overwrite" which determines whether the set of users/groups is overwritten or added to, where as the NiFi Registry command works the same as your command where it is always an overwrite.
##########
File path: nifi-toolkit/nifi-toolkit-cli/src/main/java/org/apache/nifi/toolkit/cli/impl/command/registry/bucket/UpdateBucketPolicy.java
##########
@@ -0,0 +1,129 @@
+/*
+ * 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.nifi.toolkit.cli.impl.command.registry.bucket;
+
+import org.apache.commons.cli.ParseException;
+import org.apache.nifi.registry.authorization.AccessPolicy;
+import org.apache.nifi.registry.authorization.Tenant;
+import org.apache.nifi.registry.bucket.Bucket;
+import org.apache.nifi.registry.client.NiFiRegistryClient;
+import org.apache.nifi.registry.client.NiFiRegistryException;
+import org.apache.nifi.toolkit.cli.api.Context;
+import org.apache.nifi.toolkit.cli.impl.client.ExtendedNiFiRegistryClient;
+import org.apache.nifi.toolkit.cli.impl.client.registry.PoliciesClient;
+import org.apache.nifi.toolkit.cli.impl.command.CommandOption;
+import org.apache.nifi.toolkit.cli.impl.command.registry.AbstractNiFiRegistryCommand;
+import org.apache.nifi.toolkit.cli.impl.command.registry.tenant.TenantHelper;
+import org.apache.nifi.toolkit.cli.impl.result.VoidResult;
+import org.apache.nifi.util.StringUtils;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.Properties;
+import java.util.Set;
+
+public class UpdateBucketPolicy extends AbstractNiFiRegistryCommand<VoidResult> {
+
+
+ public UpdateBucketPolicy() {
+ super("update-bucket-policy", VoidResult.class);
+ }
+
+ @Override
+ public String getDescription() {
+ return "Updates access policy of bucket";
+ }
+
+ @Override
+ public void doInitialize(final Context context) {
+ addOption(CommandOption.BUCKET_NAME.createOption());
+ addOption(CommandOption.BUCKET_ID.createOption());
+ addOption(CommandOption.USER_NAME_LIST.createOption());
+ addOption(CommandOption.USER_ID_LIST.createOption());
+ addOption(CommandOption.GROUP_NAME_LIST.createOption());
+ addOption(CommandOption.GROUP_ID_LIST.createOption());
+ addOption(CommandOption.POLICY_ACTION.createOption());
+ }
+
+
+ @Override
+ public VoidResult doExecute(NiFiRegistryClient client, Properties properties) throws IOException, NiFiRegistryException, ParseException {
+ if (!(client instanceof ExtendedNiFiRegistryClient)) {
+ throw new IllegalArgumentException("This command needs extended registry client!");
+ }
+ final ExtendedNiFiRegistryClient extendedClient = (ExtendedNiFiRegistryClient) client;
+ final PoliciesClient policiesClient = extendedClient.getPoliciesClient();
+
+ final String bucketName = getArg(properties, CommandOption.BUCKET_NAME);
+ String bucketId = getArg(properties, CommandOption.BUCKET_ID);
+
+ final String userNames = getArg(properties, CommandOption.USER_NAME_LIST);
+ final String userIds = getArg(properties, CommandOption.USER_ID_LIST);
+ final String groupNames = getArg(properties, CommandOption.GROUP_NAME_LIST);
+ final String groupIds = getArg(properties, CommandOption.GROUP_ID_LIST);
+
+ final String policyAction = getRequiredArg(properties, CommandOption.POLICY_ACTION);
+ final HashSet<String> permittedActions = new HashSet<>(Arrays.asList("read", "write", "delete"));
+ if (!permittedActions.contains(policyAction)) {
+ throw new IllegalArgumentException("Only read, write, delete actions permitted");
+ }
+ if (StringUtils.isBlank(bucketName) == StringUtils.isBlank(bucketId)) {
+ throw new IllegalArgumentException("Specify either bucket name or bucket id");
+ }
+ if (StringUtils.isBlank(bucketId)) {
+ final Optional<Bucket> optionalBucket = client.getBucketClient().getAll()
+ .stream().filter(b -> bucketName.equals(b.getName())).findAny();
+ if (!optionalBucket.isPresent()) {
+ throw new IllegalArgumentException("Specified bucket does not exist");
+ }
+ bucketId = optionalBucket.get().getIdentifier();
+ } else {
+ try {
+ extendedClient.getBucketClient().get(bucketId);
+ } catch (NiFiRegistryException e) {
+ throw new IllegalArgumentException("Specified bucket does not exist");
+ }
+ }
+ AccessPolicy accessPolicy;
+ String resource = "/buckets/" + bucketId;
+ try {
+ accessPolicy = policiesClient.getAccessPolicy(policyAction, resource);
+ } catch (NiFiRegistryException e) {
+ accessPolicy = new AccessPolicy();
+ accessPolicy.setResource(resource);
+ accessPolicy.setAction(policyAction);
+ }
+ if (!StringUtils.isBlank(userNames) || !StringUtils.isBlank(userIds)) {
+ Set<Tenant> users = TenantHelper.selectExistingTenants(userNames,
+ userIds, extendedClient.getTenantsClient().getUsers());
+ accessPolicy.setUsers(users);
+ }
+ if (!StringUtils.isBlank(groupNames) || !StringUtils.isBlank(groupIds)) {
+ Set<Tenant> groups = TenantHelper.selectExistingTenants(groupNames,
+ groupIds, extendedClient.getTenantsClient().getUserGroups());
+ accessPolicy.setUserGroups(groups);
+ }
+ AccessPolicy updatedPolicy = StringUtils.isBlank(accessPolicy.getIdentifier())
+ ? policiesClient.createAccessPolicy(accessPolicy)
+ : policiesClient.updateAccessPolicy(accessPolicy);
+ println(updatedPolicy.getIdentifier());
+ return VoidResult.getInstance();
Review comment:
If we want the result to be void, then we should wrap the println with `if (shouldPrint(properties)) { ... }`, this would make it only print when in interactive mode of the CLI, but not during non-interactive mode.
If you want the command to always return the policy id, then instead of println with Void, you would instead return a StringResult like:
`return new StringResult(identifier, getContext().isInteractive());`
Usually a create command returns the id as a string result and an update returns void, so since this command is a combo of either create or update, you could return different results depending whether you called create or update.
----------------------------------------------------------------
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