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-1769: Implemented SSE with KMS. #1126

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@miquillo

miquillo commented Oct 12, 2016

No description provided.

@miquillo miquillo changed the title from Implemented SSE with KMS. Jira #NIFI-1769 to NIFI-1769: Implemented SSE with KMS. Oct 12, 2016

@jvwing

Validating the correct combination of property choices seems awkward and unnecessary in onTrigger, as these property selections are not being evaluated from Expression Language. Instead, how about implementing a customValidate() method to make sure the properties are sensible before the processor may be started? I recommend taking a look at AbstractAWSProcessor::customValidate, both for how it validates and for how it incorporates validation errors from it's super() type.

After validating the properties, I think the onTrigger behavior can be much simpler, maybe just checking to add the key management params.

@@ -39,47 +80,6 @@
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import com.amazonaws.services.s3.model.AmazonS3Exception;

This comment has been minimized.

@jvwing

jvwing Oct 18, 2016

Contributor

Would you please only commit changes to the imports, and not re-sort them? I assume your IDE is doing this to "help" you, but it makes the diffs harder to understand.

@jvwing

jvwing Oct 18, 2016

Contributor

Would you please only commit changes to the imports, and not re-sort them? I assume your IDE is doing this to "help" you, but it makes the diffs harder to understand.

.defaultValue(NO_SERVER_SIDE_ENCRYPTION)
.build();
public static final PropertyDescriptor AWS_KMS_KEY = new PropertyDescriptor.Builder()
.name("aws-kms-key")
.displayName("Server Side Encryption using KMS")

This comment has been minimized.

@jvwing

jvwing Oct 18, 2016

Contributor

I recommend the displayName be a bit more direct, this field is the alias of a KMS key, and it's going to be used for SSE.

@jvwing

jvwing Oct 18, 2016

Contributor

I recommend the displayName be a bit more direct, this field is the alias of a KMS key, and it's going to be used for SSE.

@@ -50,14 +44,19 @@
import org.junit.Ignore;
import org.junit.Test;
import com.amazonaws.AmazonClientException;

This comment has been minimized.

@jvwing

jvwing Oct 18, 2016

Contributor

Same thing again about re-sorting imports.

@jvwing

jvwing Oct 18, 2016

Contributor

Same thing again about re-sorting imports.

@@ -458,6 +471,13 @@ public void process(final InputStream rawIn) throws IOException {
// single part upload
//----------------------------------------
final PutObjectRequest request = new PutObjectRequest(bucket, key, in, objectMetadata);
if (keyId != null) {
if (!context.getProperty(SIGNER_OVERRIDE).getValue().equals("AWSS3V4Signer")) {

This comment has been minimized.

@jvwing

jvwing Oct 18, 2016

Contributor

Would it be enough to check that it's not V2? I don't think we need to make it impossible to get wrong, as long as we make a good faith attempt to help them get it right. I'm thinking of a few things -

  1. The default should now be V4, I would prefer we not force users to nail down their signature version
  2. AWS regions and SDK versions are complicated, for example we don't check if your region supports V4
  3. If or when AWS comes out with signature V5, we would have to update this field
@jvwing

jvwing Oct 18, 2016

Contributor

Would it be enough to check that it's not V2? I don't think we need to make it impossible to get wrong, as long as we make a good faith attempt to help them get it right. I'm thinking of a few things -

  1. The default should now be V4, I would prefer we not force users to nail down their signature version
  2. AWS regions and SDK versions are complicated, for example we don't check if your region supports V4
  3. If or when AWS comes out with signature V5, we would have to update this field
if (keyId != null) {
if (!context.getProperty(SIGNER_OVERRIDE).getValue().equals("AWSS3V4Signer")) {
getLogger().error("Uploading with AWS:KMS requires S3V4 signature, please enable it");
return;

This comment has been minimized.

@jvwing

jvwing Oct 18, 2016

Contributor

The return here routes the flowfile to success without going to S3. Is that intended?

@jvwing

jvwing Oct 18, 2016

Contributor

The return here routes the flowfile to success without going to S3. Is that intended?

if (keyId != null) {
if (!context.getProperty(SIGNER_OVERRIDE).getValue().equals("AWSS3V4Signer")) {
getLogger().error("Uploading with AWS:KMS requires S3V4 signature, please enable it");
return;

This comment has been minimized.

@jvwing

jvwing Oct 18, 2016

Contributor

Same issue with return routing the flowfile to success.

@jvwing

jvwing Oct 18, 2016

Contributor

Same issue with return routing the flowfile to success.

@joewitt

This comment has been minimized.

Show comment
Hide comment
@joewitt

joewitt Feb 14, 2017

Contributor

@miquillo will you have a chance to address james feedback? I'd like to get PRs which have stalled closed out or completed. Thanks

Contributor

joewitt commented Feb 14, 2017

@miquillo will you have a chance to address james feedback? I'd like to get PRs which have stalled closed out or completed. Thanks

@miquillo

This comment has been minimized.

Show comment
Hide comment
@miquillo

miquillo Feb 21, 2017

I'm sorry but I don't see any time in the coming weeks to finish this. We're currently not using this feature anymore, so I propose we close this PR and don't add it to the project.

miquillo commented Feb 21, 2017

I'm sorry but I don't see any time in the coming weeks to finish this. We're currently not using this feature anymore, so I propose we close this PR and don't add it to the project.

@asfgit asfgit closed this in 5df6622 Feb 21, 2017

@jvwing

This comment has been minimized.

Show comment
Hide comment
@jvwing

jvwing Feb 21, 2017

Contributor

Thanks for taking a go at this, @miquillo.

Contributor

jvwing commented Feb 21, 2017

Thanks for taking a go at this, @miquillo.

@baank

This comment has been minimized.

Show comment
Hide comment
@baank

baank Jul 27, 2017

This is currently a blocker for us and I have the resources to get this over the line.

Please re-open and I will work to ensure the pull request is of a sufficient quality to get merged.

baank commented Jul 27, 2017

This is currently a blocker for us and I have the resources to get this over the line.

Please re-open and I will work to ensure the pull request is of a sufficient quality to get merged.

@jvwing

This comment has been minimized.

Show comment
Hide comment
@jvwing

jvwing Jul 27, 2017

Contributor

@baank - Thanks for being willing to work on this. A new pull request might be easier.

Contributor

jvwing commented Jul 27, 2017

@baank - Thanks for being willing to work on this. A new pull request might be easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment