Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NIFI-4256 - Add support for all AWS S3 Encryption Options #2248

Closed
wants to merge 3 commits into from

Conversation

nadenf
Copy link

@nadenf nadenf commented Nov 2, 2017

Rebased and updated to include feedback discussed in the JIRA.

@jvwing
Copy link
Contributor

jvwing commented Nov 15, 2017

Reviewing

@jvwing
Copy link
Contributor

jvwing commented Nov 20, 2017

@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 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?

@nadenf nadenf closed this Nov 24, 2017
@nadenf
Copy link
Author

nadenf commented Nov 24, 2017

Have submitted a new pull request with feedback incorporated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants