You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nifi.apache.org by adamonduty <gi...@git.apache.org> on 2016/02/23 06:32:18 UTC
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
GitHub user adamonduty opened a pull request:
https://github.com/apache/nifi/pull/246
NIFI-1180: Modify PutS3Object to enable encryption
This adds a property to set server side encryption on PutS3Object. In addition, it adds `s3.sseAlgorithm` to `FetchS3Object` so users of that processor will know if object encryption is turned on.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/adamonduty/nifi NIFI-1180
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/246.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #246
----
commit 522787dd5f6a11a0e6b8e97266106dfc66d9c09e
Author: Adam Lamar <ad...@gmail.com>
Date: 2016-02-21T06:12:56Z
NIFI-1180: Modify PutS3Object to enable encryption
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
Posted by adamonduty <gi...@git.apache.org>.
Github user adamonduty commented on the pull request:
https://github.com/apache/nifi/pull/246#issuecomment-190465236
For what its worth, I already made the change to `server-side-encryption` in 33805e2e7d849ec3d1cd48b4e0d63c5c7bfc0792. Sorry if that wasn't clear.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
Posted by adamonduty <gi...@git.apache.org>.
Github user adamonduty commented on the pull request:
https://github.com/apache/nifi/pull/246#issuecomment-189572998
@alopresto Thanks for the comments! You're right that I should write some integration tests. It worked correctly when I tested it manually, but let me get back to you.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the pull request:
https://github.com/apache/nifi/pull/246#issuecomment-190467028
@adamonduty that looks good. I understand the confusion around this undocumented convention.
My only outstanding request would be to combine the two commits into a single commit (via `rebase --interactive` and `push --force`) to condense the PR and then one of the committers should be able to merge it. Feel free to include my linked simple validation tests as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/246
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the pull request:
https://github.com/apache/nifi/pull/246#issuecomment-206705777
@adamonduty Will do in the morning. Don't want to miss merging into both branches now that `1.0` and `0.x` have split. (Provided docs on that on [the wiki](https://cwiki.apache.org/confluence/display/NIFI/Contributor+Guide#ContributorGuide-Stepstomerge/closepullrequestswithtwomainbranches). ) Those commits will also be **GPG-signed** now that [GitHub supports it](https://help.github.com/articles/signing-commits-using-gpg/).
Thanks for reminding me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
Posted by adamonduty <gi...@git.apache.org>.
Github user adamonduty commented on the pull request:
https://github.com/apache/nifi/pull/246#issuecomment-189581189
@alopresto I looked up the inheritance chain and there aren't any other processors that follow the `name`/`displayName` convention. They all set `name` only. Is this convention documented somewhere? I couldn't find it in the developer guide, contributor guide, or mailing list...
Also, doesn't changing the name attribute break backwards compatibility for existing attributes? The name field is used in `flow.xml.gz`, so changing it would cause a processor to "forget" the setting, unless I'm mistaken...
I pushed another commit with integration tests for the server side encryption attribute.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
Posted by adamonduty <gi...@git.apache.org>.
Github user adamonduty commented on the pull request:
https://github.com/apache/nifi/pull/246#issuecomment-190560346
@alopresto Done! Let me know if anything else needs addressed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
Posted by adamonduty <gi...@git.apache.org>.
Github user adamonduty commented on the pull request:
https://github.com/apache/nifi/pull/246#issuecomment-207135460
Awesome, thanks @alopresto!!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/246#discussion_r54322948
--- Diff: nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/main/java/org/apache/nifi/processors/aws/s3/PutS3Object.java ---
@@ -177,10 +179,19 @@
.addValidator(StandardValidators.TIME_PERIOD_VALIDATOR)
.build();
+ public static final PropertyDescriptor SERVER_SIDE_ENCRYPTION = new PropertyDescriptor.Builder()
+ .name("Server Side Encryption")
--- End diff --
`.name()` should be used to set a unique name (in the form `"server-side-encryption"`). `.displayName()` can be used for a human-readable form as above.
```java
public static final PropertyDescriptor SERVER_SIDE_ENCRYPTION = new PropertyDescriptor.Builder()
.name("server-side-encryption")
.displayName("Server Side Encryption")
.description("Specifies the algorithm used for server side encryption.")
.required(true)
.allowableValues(NO_SERVER_SIDE_ENCRYPTION, ObjectMetadata.AES_256_SERVER_SIDE_ENCRYPTION)
.defaultValue(NO_SERVER_SIDE_ENCRYPTION)
.build();
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/246#discussion_r54505449
--- Diff: nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/main/java/org/apache/nifi/processors/aws/s3/PutS3Object.java ---
@@ -407,6 +420,12 @@ public void process(final InputStream rawIn) throws IOException {
}
}
+ final String Sse = context.getProperty(SERVER_SIDE_ENCRYPTION).getValue();
--- End diff --
Minor note -- naming convention would be `sse`, or preferably, `serverSideEncryption`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the pull request:
https://github.com/apache/nifi/pull/246#issuecomment-190332181
@adamonduty You are correct that the `.name`/`.displayName` convention is not documented well. I pinged @joewitt to look for a canonical source for the convention and we need to improve the documentation.
In this case, I am ok with not changing the existing `PropertyDescriptors`, as it would break backwards compatibility as you noted. Unfortunately, this is a side-effect of the attributes not being set "properly" in the first place -- the `displayName` value is visible to the end user through the UI and may need to be changed, while the `name` value is used for object resolution in the application and so should not need to change if the messaging changes (imagine future internationalization where the `displayName` value is language-dependent). However, I do think in this case, the new `PropertyDescriptor` should follow the correct convention and a comment added noting the reason for the difference with other `PropertyDescriptor`s in the class.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the pull request:
https://github.com/apache/nifi/pull/246#issuecomment-190851300
I do not have AWS S3 credentials to run the integration tests myself but the code looks good to me. +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the pull request:
https://github.com/apache/nifi/pull/246#issuecomment-189543249
I did not see any tests for this new functionality. I wrote a very simple validation test for this new option here: [`PutS3ObjectTest.groovy`](https://github.com/alopresto/nifi/blob/8abd3b8f7087fee1f6f7f97fe5867853cc346249/nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/test/groovy/org.apache.nifi.processors.aws.s3/PutS3ObjectTest.groovy)
How much integration testing did you do for this feature?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
Posted by mosermw <gi...@git.apache.org>.
Github user mosermw commented on the pull request:
https://github.com/apache/nifi/pull/246#issuecomment-190377532
When a PropertyDescriptor does not have a .displayName() set, then the .name() is used in the UI. There was a time when .displayName() did not exist and .name() displayed in the UI, so that's why you see many standard processors that do not use displayName. displayName was added so that you could change the name used in the UI without breaking backward compatibility in the flow.xml.
So the original comment from @alopresto represents the latest best practice for NiFi processor development.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
Posted by adamonduty <gi...@git.apache.org>.
Github user adamonduty commented on the pull request:
https://github.com/apache/nifi/pull/246#issuecomment-206685104
@alopresto What do you think about merging this one with your new committing superpowers? :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
Posted by adamonduty <gi...@git.apache.org>.
Github user adamonduty commented on a diff in the pull request:
https://github.com/apache/nifi/pull/246#discussion_r54327468
--- Diff: nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/main/java/org/apache/nifi/processors/aws/s3/PutS3Object.java ---
@@ -177,10 +179,19 @@
.addValidator(StandardValidators.TIME_PERIOD_VALIDATOR)
.build();
+ public static final PropertyDescriptor SERVER_SIDE_ENCRYPTION = new PropertyDescriptor.Builder()
+ .name("Server Side Encryption")
--- End diff --
That makes sense, but there are also 6 instances above this code that doesn't follow the same rule. Which is best: match the existing style, or match the rule?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1180: Modify PutS3Object to enable encrypt...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/246#discussion_r54328675
--- Diff: nifi-nar-bundles/nifi-aws-bundle/nifi-aws-processors/src/main/java/org/apache/nifi/processors/aws/s3/PutS3Object.java ---
@@ -177,10 +179,19 @@
.addValidator(StandardValidators.TIME_PERIOD_VALIDATOR)
.build();
+ public static final PropertyDescriptor SERVER_SIDE_ENCRYPTION = new PropertyDescriptor.Builder()
+ .name("Server Side Encryption")
--- End diff --
I totally understand consistency over convention. If I'm being selfish, I'd say it is better to do it correctly *and* correct the other existing settings. The preferred and prescribed form is to use `.name()` for the unique value. See `EncryptContent` processor for a more complete example.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---