You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by baank <gi...@git.apache.org> on 2017/11/02 22:36:47 UTC
[GitHub] nifi pull request #2248: NIFI-4256 - Add support for all AWS S3 Encryption O...
GitHub user baank opened a pull request:
https://github.com/apache/nifi/pull/2248
NIFI-4256 - Add support for all AWS S3 Encryption Options
Rebased and updated to include feedback discussed in the JIRA.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/baank/nifi master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2248.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 #2248
----
commit 0e9f62710e2ec08df9ca749d0c2b98f745b26314
Author: Naden Franciscus <na...@nbnco.com.au>
Date: 2017-11-02T22:28:27Z
NIFI-4256 - Add support for all AWS S3 Encryption Options
----
---
[GitHub] nifi issue #2248: NIFI-4256 - Add support for all AWS S3 Encryption Options
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/2248
Reviewing
---
[GitHub] nifi issue #2248: NIFI-4256 - Add support for all AWS S3 Encryption Options
Posted by jvwing <gi...@git.apache.org>.
Github user jvwing commented on the issue:
https://github.com/apache/nifi/pull/2248
@baank, thanks for putting in all the work on this PR so far, it is looking pretty good. The full build with contrib-check passed. I have been able to run through the encryption functionality without problems so far. I have a few comments and questions:
* I see warnings like "The service APIs should not be bundled with the implementations" when starting NiFi:
```
2017-11-19 03:01:29,148 WARN [main] org.apache.nifi.nar.ExtensionManager Controller Service org.apache.nifi.processors.aws.s3.encryption.EncryptedS3ClientService is bundled with its supporting APIs org.apache.nifi.processors.aws.s3.S3ClientService. The service APIs should not be bundled with the implementations.
2017-11-19 03:01:29,158 WARN [main] org.apache.nifi.nar.ExtensionManager Controller Service org.apache.nifi.processors.aws.s3.encryption.EncryptedS3PutEnrichmentService is bundled with its supporting APIs org.apache.nifi.processors.aws.s3.service.S3PutEnrichmentService. The service APIs should not be bundled with the implementations.
```
As part of the recent restructuring of the nifi-aws-bundle, service interfaces are now defined in the nifi-aws-service-api project, with implementations in one of the other projects. I think we should move the interface definitions for S3ClientService and S3PutEnrichmentService into the nifi-aws-service-api project.
* In PutS3Object, I recommend that calling the S3PutEnrichmentService be the very last thing before making the request to S3, to provide the maximum range of modification options. At the moment, there are some ACL settings that follow enrichment. Does that make sense?
* Why does the EncryptedS3PutEnrichmentService offer to capture and use the MD5 hash of the key? I vaguely understood that the AWS SDK would handle the MD5 hash when necessary, which I also understood to be for get requests, and I'm not sure when I would fill it in. The documentation is not very clear, but [ObjectMetadata.setSSECustomerKeyMd5](http://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/services/s3/model/ObjectMetadata.html#setSSECustomerKeyMd5-java.lang.String-) says "for internal use only". The service seemed to work just fine for me without providing it, and it is not described as required. From line 163:
```
if (StringUtils.isNotBlank(customerKeyMD5)) {
putObjectRequest.getMetadata().setSSECustomerKeyMd5(customerKeyMD5);
}
```
When would we need this?
---
[GitHub] nifi issue #2248: NIFI-4256 - Add support for all AWS S3 Encryption Options
Posted by baank <gi...@git.apache.org>.
Github user baank commented on the issue:
https://github.com/apache/nifi/pull/2248
Have submitted a new pull request with feedback incorporated.
---
[GitHub] nifi pull request #2248: NIFI-4256 - Add support for all AWS S3 Encryption O...
Posted by baank <gi...@git.apache.org>.
Github user baank closed the pull request at:
https://github.com/apache/nifi/pull/2248
---