NIFI-1833 - Azure Storage processors by aperepel · Pull Request #1636 · apache/nifi

aperepel · 2017-03-29T23:32:37Z

Thank you for submitting a contribution to Apache NiFi.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
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.
Has your PR been rebased against the latest commit within the target branch (typically master)?
Is your initial contribution a single, squashed commit?

For code changes:

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?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
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:

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 travis-ci for build issues and submit an update to your PR as soon as possible.

aperepel · 2017-03-29T23:33:10Z

Initial PR submission, will inquire about checkstyle rules

jtstorck · 2017-04-04T16:32:15Z

Reviewing...

jtstorck · 2017-04-04T16:59:13Z

...re-processors/src/main/java/org/apache/nifi/processors/azure/AbstractAzureBlobProcessor.java

+    public static final PropertyDescriptor BLOB_TYPE = new PropertyDescriptor.Builder()
+            .name("Blob type")
+            .description("Blobs can be block type of page type")
+            .expressionLanguageSupported(true)


Adding expression language support prevents the UI from creating the dropdown box with "Block" and "Page" values. If expression language is used for the value of the property in the config of the processor, validation fails. Is it necessary to support EL? If so, perhaps remove the allowable values, and use StandardValidators.NON_EMPTY_EL_VALIDATOR? Otherwise, EL should be disabled.

jtstorck · 2017-04-04T17:15:03Z

...re-processors/src/main/java/org/apache/nifi/processors/azure/AbstractAzureBlobProcessor.java

+     * Get a reference to a blob based on the type.
+     *
+     */
+    protected CloudBlob getBlob(CloudBlobContainer container, String blobType, String blobPath) throws URISyntaxException, StorageException {


Should blobType be checked more explicitly here? If blobType doesn't match the case of "Block", won't this end up returning a page blob? If an invalid blobType is passed in, is it appropriate that a page blob is returned, or should a ProcessorException be thrown?

jtstorck · 2017-04-04T18:16:31Z

Reviewing after squash...

jtstorck · 2017-04-04T18:30:10Z

nifi-nar-bundles/nifi-azure-bundle/nifi-azure-nar/pom.xml

+
+        <dependency>
+            <groupId>org.apache.nifi</groupId>
+            <artifactId>nifi-standard-services-api-nar</artifactId>


Since nifi-azure-processors depends on nifi-standard-processors, this dependency should be changed to nifi-standard-nar, which specifies a dependency on nifi-standard-services-api-nar itself. This will enable nifi-azure-nar's classloader to have the classes that are depended upon (in nifi-standard-processors) by nifi-azure-processors, which in turn allows nifi-azure-processors to specify nifi-standard-processors as a provided dependency, which fixes the standard processors being loaded twice.

<dependency> <groupId>org.apache.nifi</groupId> <artifactId>nifi-standard-nar</artifactId> <type>nar</type> </dependency>

Done, verified changes with a live Azure account.

jtstorck · 2017-04-04T18:33:58Z

nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/pom.xml

        </dependency>
+        <dependency>
+            <groupId>org.apache.nifi</groupId>
+            <artifactId>nifi-standard-processors</artifactId>


Please specify this dependency as provided, so that the processors in nifi-standard-processors aren't found in nifi-azure-nar as well as nifi-standard-nar.

<dependency> <groupId>org.apache.nifi</groupId> <artifactId>nifi-standard-processors</artifactId> <version>${project.version}</version> <scope>provided</scope> </dependency>

Done, verified changes with a live Azure account.

jtstorck · 2017-04-04T21:53:12Z

...-azure-processors/src/main/java/org/apache/nifi/processors/azure/AbstractAzureProcessor.java

+public abstract class AbstractAzureProcessor extends AbstractProcessor {
+
+    public static final Relationship REL_SUCCESS = new Relationship.Builder().name("success").description("All FlowFiles that are received are routed to success").build();
+    protected static final Relationship REL_FAILURE = new Relationship.Builder().name("failure").description("Any failed fetches will be transferred to the failure relation.").build();


This relationship is used in PutAzureBlobStorage, but the description of "fetches" doesn't match the function of that processor.

jtstorck · 2017-04-04T22:14:27Z

...-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureBlobStorage.java

+
+    @Override
+    protected boolean isListingResetNecessary(final PropertyDescriptor property) {
+        // TODO - implement


Is it intended that the listing reset is never necessary?

Implemented the method.

jtstorck · 2017-04-04T22:15:52Z

...-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureBlobStorage.java

+
+    @Override
+    protected Scope getStateScope(final ProcessContext context) {
+        return Scope.CLUSTER;


Should this not support local state storage?

IMO, local storage doesn't make sense. With the local scope one has to schedule the list on the primary node only and lose it after a failover event basically. List operation favors a single node feeding the cluster in general to avoid duplicates. BTW, cluster scope works fine in a single node deployment.

OK. I can see how you'd want to force cluster state, since the files are being pulled from a remote location. Makes it easier for the user to not have to worry about the possible confusion of local vs cluster state and primary-only scheduling.

jtstorck · 2017-04-04T22:19:39Z

...-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureBlobStorage.java

+        return listing;
+    }
+
+    protected static CloudStorageAccount createStorageConnection(ProcessContext context) {


This method can be private.

Inlined the method, dropped 'static'

jtstorck · 2017-04-04T22:21:03Z

...-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureBlobStorage.java

+        try {
+            storageAccount = CloudStorageAccount.parse(storageConnectionString);
+        } catch (IllegalArgumentException | URISyntaxException e) {
+            System.out.println("\nConnection string specifies an invalid URI.");


Should these messages be shown as bulletins to the user, or at least logged?

Reworked to log properly, removed sys.out.

… really

… ProcessContext.getName()

jtstorck · 2017-04-05T15:42:43Z

...re-processors/src/main/java/org/apache/nifi/processors/azure/AbstractAzureBlobProcessor.java

+    public static final PropertyDescriptor BLOB = new PropertyDescriptor.Builder().name("Blob").description("The filename of the blob").addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+            .expressionLanguageSupported(true).required(true).defaultValue("${azure.blobname}").build();
+
+    public static final List<PropertyDescriptor> properties = Collections


This should probably be set to private, and by convention for static vars, should be uppercased.

jtstorck · 2017-04-05T15:43:51Z

...-azure-processors/src/main/java/org/apache/nifi/processors/azure/AbstractAzureProcessor.java

+
+    public static final Relationship REL_SUCCESS = new Relationship.Builder().name("success").description("All successfully processed FlowFiles are routed to this relationship").build();
+    public static final Relationship REL_FAILURE = new Relationship.Builder().name("failure").description("Unsuccessful operations will be transferred to the failure relationship.").build();
+    public static final Set<Relationship> RELATIONSHIPS = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(REL_SUCCESS, REL_FAILURE)));


This should be changed to private.

Neither private, protected nor package-default will work, compilation errors. Needs to be public.

I do not get a compilation issue with the below statement:

private static final Set<Relationship> RELATIONSHIPS = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(REL_SUCCESS, REL_FAILURE)));

jtstorck · 2017-04-05T15:44:43Z

...-azure-processors/src/main/java/org/apache/nifi/processors/azure/AbstractAzureProcessor.java

+     * @return The newly created CloudStorageAccount object
+     *
+     */
+    protected static CloudStorageAccount createStorageAccountFromConnectionString(String storageConnectionString) throws IllegalArgumentException, URISyntaxException, InvalidKeyException {


This method can be private.

jtstorck · 2017-04-05T15:45:26Z

...-azure-processors/src/main/java/org/apache/nifi/processors/azure/AbstractAzureProcessor.java

+            storageAccount = CloudStorageAccount.parse(storageConnectionString);
+        } catch (IllegalArgumentException | URISyntaxException e) {
+            throw e;
+        } catch (InvalidKeyException e) {


This could be added to the above catch, since it's doing the same thing, just throwing the exception.

jtstorck · 2017-04-05T15:48:38Z

...processors/src/main/java/org/apache/nifi/processors/azure/storage/FetchAzureBlobStorage.java

+    @WritesAttribute(attribute = "azure.length", description = "The length of the blob fetched")
+})
+public class FetchAzureBlobStorage extends AbstractAzureBlobProcessor {
+    public static final List<PropertyDescriptor> PROPERTIES = Collections


This can be changed to private.

jtstorck · 2017-04-05T16:02:45Z

...-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureBlobStorage.java

+
+    @Override
+    protected Scope getStateScope(final ProcessContext context) {
+        return Scope.CLUSTER;


OK. I can see how you'd want to force cluster state, since the files are being pulled from a remote location. Makes it easier for the user to not have to worry about the possible confusion of local vs cluster state and primary-only scheduling.

jtstorck · 2017-04-05T16:08:07Z

...-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureBlobStorage.java

+            BlobRequestOptions blobRequestOptions = null;
+            OperationContext operationContext = null;
+
+            for (ListBlobItem blob : container.listBlobs(prefix, true, EnumSet.of(BlobListingDetails.METADATA), blobRequestOptions, operationContext)) {


Since blobRequestOptions and operationContent are always null here, might want to remove the declarations of them and pass nulls to the listBlobs method?

jtstorck · 2017-04-05T16:15:55Z

...-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureBlobStorage.java

+        return listing;
+    }
+
+    protected CloudStorageAccount createStorageConnection(ProcessContext context) {


This could be private... I notice that there are similar methods defined in AbstractAzureProcessor. The connection methods could be decoupled into an interface with default methods for reuse within AbstractAzureProcessor and ListAzureBlobStorage?

jtstorck · 2017-04-05T16:18:25Z

...e-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureBlobStorage.java

+
+            final Map<String, String> attributes = new HashMap<>();
+            long length = flowFile.getSize();
+            session.read(flowFile, new InputStreamCallback() {


Can this be replace with a lambda as well?

session.read(flowFile, rawIn -> { final InputStream in = new BufferedInputStream(rawIn); try { blob.upload(in, length); BlobProperties properties = blob.getProperties(); attributes.put("azure.container", containerName); attributes.put("azure.primaryUri", blob.getSnapshotQualifiedUri().toString()); attributes.put("azure.etag", properties.getEtag()); attributes.put("azure.length", String.valueOf(length)); attributes.put("azure.timestamp", String.valueOf(properties.getLastModified())); } catch (StorageException | URISyntaxException e) { throw new IOException(e); } });

Done. Additionally, checking for the input stream type to avoid double-wrapping into a buffered stream.

jtstorck · 2017-04-05T16:24:00Z

...-azure-processors/src/main/java/org/apache/nifi/processors/azure/storage/utils/BlobInfo.java

+        return etag.compareTo(o.etag);
+    }
+
+    protected BlobInfo(final Builder builder) {


With the builder being available for constructing this class, typically this constructor should be private.

jtstorck

Thanks for making these changes!

jtstorck · 2017-04-06T19:12:38Z

@aperepel Can you add usage documentation about the risks of allowing the account name and key to be stored as flowfile attributes? Mentioning that sensitive attributes are not supported, and that anyone with provenance access for the processors will be able to see the attribute values. Also, the flowfile repository, where the attributes are stored, does not support encryption internally, and unless the repository is being written to a TDE partition, the attribute values could possibly be accessed there, as well.

jtstorck · 2017-04-10T15:37:46Z

@aperepel Integration tests are written for the processors, but aren't included in the build. There should probably be some tests that do get run with the build, using the TestRunner. The CloudStorageAccount would have to be mocked, which at a minimum you could do by making an extension of the class under test in which the createStorageAccount method used by the test returns a mock.

jtstorck · 2017-04-11T16:28:24Z

...processors/src/main/java/org/apache/nifi/processors/azure/storage/FetchAzureBlobStorage.java

+import com.microsoft.azure.storage.blob.CloudBlobContainer;
+
+@Tags({ "azure", "microsoft", "cloud", "storage", "blob" })
+@CapabilityDescription("Retrieves contents of an Azure Storage Blob, writing the contents to the content of the FlowFile")


Should this be changed to say "Retrieves contents of a blob from an Azure Storage Container..."?

jtstorck · 2017-04-11T16:29:24Z

...-processors/src/main/java/org/apache/nifi/processors/azure/storage/ListAzureBlobStorage.java

+@TriggerSerially
+@Tags({ "azure", "microsoft", "cloud", "storage", "blob" })
+@SeeAlso({ FetchAzureBlobStorage.class, PutAzureBlobStorage.class })
+@CapabilityDescription("Lists blobs in an Azure Storage container. Listing details are attached to an empty FlowFile for use with FetchAzureBlobStorage")


Container should be capitalized here, flowfile lowercased?

jtstorck · 2017-04-11T16:30:45Z

...e-processors/src/main/java/org/apache/nifi/processors/azure/storage/PutAzureBlobStorage.java

+
+@Tags({ "azure", "microsoft", "cloud", "storage", "blob" })
+@SeeAlso({ ListAzureBlobStorage.class, FetchAzureBlobStorage.class })
+@CapabilityDescription("Puts content into an Azure Storage Blob")


Should this be changed to say "Azure Storage Container"?

jtstorck · 2017-04-11T16:32:36Z

...s/docs/org.apache.nifi.processors.azure.storage.FetchAzureBlobStorage/additionalDetails.html

+
+<h2>Apache NiFi Azure Processors</h2>
+
+<h3>Important Security Note</h3>


Since these are important security notices, should they be presented in the main documentation of the processor, rather than additional details?

Addressed dependency issues from the review. Addressed a checkstyle issue. Review: reworded the descriptions. Review: implemented the reset condition logic. Review: dropped static qualifier from method signatures, not required really Review: removed sys.out, inlined a single method to get access to the ProcessContext.getName() Switched to HTTPS as per MSFT recommendation. Some DRY. Dropped cruft. Addressing review suggestions from 4/5 Review: documentation improvements Review: documentation improvements This closes apache#1636. Signed-off-by: Bryan Rosander <brosander@apache.org>

aperepel force-pushed the NIFI-1833-pr branch 4 times, most recently from f87e0a8 to 41c1247 Compare April 3, 2017 17:54

jtstorck suggested changes Apr 4, 2017

View reviewed changes

NIFI-1833 - Azure Storage processors

dc266d4

aperepel force-pushed the NIFI-1833-pr branch from 3809062 to dc266d4 Compare April 4, 2017 17:43

jtstorck suggested changes Apr 4, 2017

View reviewed changes

aperepel added 2 commits April 4, 2017 14:57

Addressed dependency issues from the review.

7d1818b

Addressed a checkstyle issue.

bc338ff

jtstorck suggested changes Apr 4, 2017

View reviewed changes

aperepel added 5 commits April 4, 2017 18:47

Review: reworded the descriptions.

5848267

Review: implemented the reset condition logic.

fe1e19c

Review: dropped static qualifier from method signatures, not required…

50b9fa1

… really

Review: removed sys.out, inlined a single method to get access to the…

d9ac033

… ProcessContext.getName()

Switched to HTTPS as per MSFT recommendation. Some DRY. Dropped cruft.

72f589d

jtstorck suggested changes Apr 5, 2017

View reviewed changes

Addressing review suggestions from 4/5

1ab7cba

jtstorck approved these changes Apr 6, 2017

View reviewed changes

aperepel added 2 commits April 10, 2017 13:30

Review: documentation improvements

80b79e6

Review: documentation improvements

61d20b7

jtstorck suggested changes Apr 11, 2017

View reviewed changes

jtstorck mentioned this pull request May 1, 2017

NIFI-1833 Updates to Azure Storage Processor PR to fix dependencies and integration tests #1719

Closed

11 tasks

asfgit closed this in f30c816 May 2, 2017


		<h2>Apache NiFi Azure Processors</h2>

		<h3>Important Security Note</h3>

Conversation

aperepel commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

aperepel commented Mar 29, 2017

Uh oh!

jtstorck commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtstorck commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aperepel commented Mar 29, 2017 •

edited

Loading