[
https://issues.apache.org/jira/browse/NIFI-1325?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15093066#comment-15093066
]
James Wing commented on NIFI-1325:
----------------------------------
>From reviewing the patch on 2015-01-11:
Mans, you appear to have faithfully implemented the
AbstractAWSProcessor-subclass design suggested on the dev email list. I have
to admit that I don't really like this design. It feels weird to me to have a
deprecated class which we are still actively using without any future intention
of dropping. It's deprecated because we don't want processor developers to
directly inherit from it, but it's still in active use in the inheritance
chain. The backward compatibility problem is ugly any way I look at it, but I
guess I would prefer a solution involving interfaces or annotations to signal
AWSCredentialProvider readiness over the subclass.
Things I like:
* Your implementation worked in my brief testing
* Individual components can still specify credentials separate from the shared
service
Suggestions:
* Please consider using
[DefaultAWSCredentialsProviderChain|http://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/auth/DefaultAWSCredentialsProviderChain.html]
instead of AnonymousAWSCredentialProvider() as the unconfigured default
credential. This provider covers a number of common AWS credential
configuration strategies, including EC2 instance profiles/roles.
* If we do have anonymous credentials, I believe we could use the same
StaticCredentialProvider used for basic creds, I don't think we need a custom
AnonymousAWSCredentialsProvider.
* "Assume Role Name" could be renamed "Assume Role Session Name" to clarify
that it identifies the session, not the role. [See the RoleSessionName argument
in AssumeRole action in the STS SDK
docs|http://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRole.html].
* You could add some attributes to the AWSCredentialsProviderServiceImpl to
improve the presentation in the Add Controller Services dialog:
{code}
@CapabilityDescription("Defines credentials for Amazon Web Services
processors.")
@Tags({ "aws", "credentials" })
public class AWSCredentialsProviderServiceImpl ...
{code}
* Naming Conventions - Someone more familiar with NiFi code should chime in,
but don't I think the *Impl convention for a concrete implementation of an
interface is widespread in the NiFi codebase. I looked at the
nifi-couchbase-bundle for comparison, and the interface is
CouchbaseClusterControllerService and the concrete class
CouchbaseClusterService, but I do not really know what the standard is. One
interesting fact is that the class name AWSCredentialsProviderServiceImpl
appears in the Controller Services dialog, so it effectively becomes part of
the user experience. That might explain "CouchbaseClusterService". If there
is a display name, I didn't see any other services using it.
Thanks for putting this together!
> Enhance AWS S3 fetch to access bucket across accounts
> -----------------------------------------------------
>
> Key: NIFI-1325
> URL: https://issues.apache.org/jira/browse/NIFI-1325
> Project: Apache NiFi
> Issue Type: Improvement
> Components: Core Framework
> Affects Versions: 0.4.1
> Environment: All
> Reporter: Mans Singh
> Assignee: Tony Kurc
> Priority: Minor
> Labels: easyfix
> Fix For: 0.4.1
>
> Attachments: nifi-1325.patch.zip, nifi-1325.patch.zip
>
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> The AWS S3 Fetch Object component does not allow access to bucket across
> accounts. AWS S3 Fetch Object with can be enhanced to provide this
> functionality by using assume role session/credentials
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
