Mans, Thanks for your continued efforts on this. I'll get this applied and start scoping it out.
On Mon, Jan 11, 2016 at 4:54 PM, M Singh <mans2si...@yahoo.com.invalid> wrote: > Hey Folks: > I've uploaded a patch for nifi-1325 ( > https://issues.apache.org/jira/browse/NIFI-1325). I've added unit test > cases and added integration tests (ignored in the checkin since they > require aws resources arns) - that uses the credentials provider controller > and they passed. > Please let me know if you have any thoughts/comments for me. > Thanks again and looking forward for your feedback. > Mans > > On Saturday, January 9, 2016 1:43 PM, Aldrin Piri < > aldrinp...@gmail.com> wrote: > > > Mans, > > Sounds great. Feel free to let us know if you have any issues and we are > happy to work through it with you. Thanks again for taking this work on! > > On Sat, Jan 9, 2016 at 4:21 PM, M Singh <mans2si...@yahoo.com.invalid> > wrote: > > > Sounds like a plan, Aldrin. Let me explore this path. > > Mans > > > > On Saturday, January 9, 2016 1:16 PM, Aldrin Piri < > > aldrinp...@gmail.com> wrote: > > > > > > Mans, > > > > In the way I specified via the linked snippet, we could potentially just > > have it implement the AWSCredentialsProvider signature, and in the case > > that the prior properties are used instead of the controller service, > > create a CredentialsProvider (something along the lines of a > > BasicAWSCredentials Provider) that just returns a credentials object and > a > > no-op refresh. > > > > Unfortunately due to some ambiguity about the extension points for the > > codebase, we are being very sensitive to those items and are avoiding > such > > breaking changes. I agree there could be some confusion, but changing > the > > particular structure in terms of operation and configuration is one we > > certainly cannot do as it would break flows on upgrade. In the interim, > > the controller service allows us to provide implementations for various > > types of credentials. I do agree, that when we are afforded the luxury > of > > breaking type changes, the currently established set of properties would > > also best be served in that controller service type of role. > > > > On Sat, Jan 9, 2016 at 3:51 PM, M Singh <mans2si...@yahoo.com.invalid> > > wrote: > > > > > Hi Aldrin: > > > Just to clarify that the current abstract aws processor (s3, sns, and > > sqs) > > > would implement both createClient methods as mentioned below: > > > @Deprecatedprotected ClientType createClient(final ProcessContext > > context, > > > final AWSCredentials credentials, final ClientConfiguration config) > > > protected abstract ClientType createClient(final ProcessContext > context, > > > final AWSCredentialsProvider credentials, final ClientConfiguration > > > config);} > > > > > > I had already started working on aws creds provider service controller. > > > In my imp for the nifi aws processors I had removed the createClient > with > > > aws creds, replacing it with creds provider argument, but will put it > > back > > > as you've recommended. > > > If we follow this path - the configuration for the aws processors will > > > still have the original properties (aws secrets/access key, credentials > > > file, etc) for backward compatibility and a aws credentials service > > > controller which have the same properties (aws secrets/access key/creds > > > files/anonymous option) along with the cross account attributes. IMHO > - > > > this will be confusing and my suggestion was to make the breaking > change. > > > But I will work through your recommendation. > > > If there is any other advice/recommendation, please let me know. > > > Thanks again > > > > > > > > > On Saturday, January 9, 2016 11:30 AM, Aldrin Piri < > > > aldrinp...@gmail.com> wrote: > > > > > > > > > Mans, > > > > > > Fair points concerning the duplication. I was thinking that in > > conjunction > > > with marking that method deprecated we would also drop the abstract > > > classifier and require implementers subclassing the original class to > > > provide the override explicitly. It's not ideal, but does alleviate > the > > > issues concerning excess methods in the interface. Sorry for omission > of > > > what is certainly a very valid issue. > > > > > > Outside of that, the items you are establishing sounds like the right > > > path. I hashed this out a little more fully to better express my ideas > > > [1]. > > > > > > [1] https://gist.github.com/apiri/6a17b71e261f457daecc > > > > > > On Sat, Jan 9, 2016 at 1:17 PM, M Singh <mans2si...@yahoo.com.invalid> > > > wrote: > > > > > > > Hi Aldrin: > > > > Even if we subclass AbstractAWSProcessor and overwrite the > onScheduled > > > > method, we still have to add (rather then replace createClient with > aws > > > > creds argument) a createClient method that would take the credential > > > > provider argument rather than the aws credentials argument (the > current > > > > implementation). > > > > current nifi aws createClient (with aws credentials) > > > > protected abstract ClientType createClient(final ProcessContext > > > > context, final AWSCredentials credentials, final ClientConfiguration > > > > config); > > > > new nifi aws createClient (with aws credentials provider) > > > > protected abstract ClientType createClient(final ProcessContext > > > > context, final AWSCredentialsProvider credentialsProvider, final > > > > ClientConfiguration config); > > > > Regarding overwriting onScheduled method. Here is snippet of current > > > > AbstractAWSProcessor.onScheduled method (using aws credentials): > > > > @OnScheduled public void onScheduled(final ProcessContext > > context) > > > > { final ClientType awsClient = createClient(context, > > > > getCredentials(context), createConfiguration(context)); > > > > this.client = awsClient;... > > > > So, in the subclass AbstractAWSProcessor.onScheduled method, we will > > > check > > > > if controller is available and if so, call the create client with the > > > > credentials provider method. In this case each of the nifi aws > > processors > > > > (currently s3, sns, and sqs) will have to provide two implementation > of > > > > create client (one with aws creds - the current one, and one with aws > > > creds > > > > provider). > > > > I might be missing something, but it looks like there will > duplication > > of > > > > amazon client creation (one using creds and one using creds provider > > from > > > > the controller) along with two createClient method in Nifi's > > > > AbstractAWSProcessor which might causing confusion. But that is just > > my > > > > thought. > > > > Let me know what you think. > > > > > > > > Thanks again. > > > > Mans > > > > > > > > > > > > > > > > On Saturday, January 9, 2016 9:20 AM, Aldrin Piri < > > > > aldrinp...@gmail.com> wrote: > > > > > > > > > > > > Mans, > > > > > > > > I think the ControllerService is definitely the right play moving > > > forward. > > > > What I think we can do is subclass the current AWSAbstractProcessor > and > > > > override onScheduled to provide a way to interact with the > > > > ControllerService and, should one not be configured, defer to the > > parent > > > > implementation. > > > > > > > > We can mark AWSAbstractProcessor as deprecated and maintain backward > > > > compatibility while adding some new functionality in for the > > accompanying > > > > processors. > > > > > > > > > > > > On Sat, Jan 9, 2016 at 8:01 AM, M Singh <mans2si...@yahoo.com.invalid > > > > > > wrote: > > > > > > > > > Hi: > > > > > I have started working on implementing a controller for creating > > creds > > > > > provider and changing the createClient method to use the > controller. > > > > > If there is any advice/feedback on this, please let me know. > > > > > Thanks again. > > > > > Mans > > > > > > > > > > > > > > > > > > > > On Friday, January 8, 2016 12:48 PM, M Singh > > > > > <mans2si...@yahoo.com.INVALID> wrote: > > > > > > > > > > > > > > > Hi Aldrin: > > > > > The unfortunate things is that AWSCredentialsProvider does not > > inherit > > > > > from AWSCredentials interface. > > > > > As far as I can see, the provider interface is much more flexible > and > > > > > provides everything with we/anyone can need. As we can see, the > > creds > > > > > based constructors (AmazonS3/SQS/SNSClients) internally create a > > static > > > > > creds provider instance. If we support both the creds and creds > > > provider > > > > > based arguments, it could also confusing and error prone for > > developers > > > > > extending the class. > > > > > Even if we have an adapter how would the subclass of > > > AbstractAWSProcessor > > > > > call createClient allow the two arguments (creds and creds > provider) > > to > > > > > work seemlessly. Let me know if you have any other thoughts/paths I > > can > > > > > investigate. > > > > > Thanks for the feedback and I am learning a lot with this > experience. > > > > > Mans > > > > > > > > > > On Friday, January 8, 2016 11:49 AM, Aldrin Piri < > > > > aldrinp...@gmail.com> > > > > > wrote: > > > > > > > > > > > > > > > Mans, > > > > > > > > > > Thanks for sticking with this and continuing to see things through, > > the > > > > > community certainly appreciates it as these are very popular > > processors > > > > and > > > > > this functionality will help a wide base of users. > > > > > > > > > > I am poking around a bit more and thinking we might be able to work > > > > > something out with a class that adapts an AWSCredentialsProvider to > > > > > AWSCredentials. The AWSProvider interface is just composition of > > > > > AWSCredentials with a refresh method. Need to mull things over a > > bit, > > > > and > > > > > dig through the associated libraries to understand how these are > > > > typically > > > > > used, but this feels like it could be another avenue to consider > and > > > > where > > > > > I am directing my attention at the moment. > > > > > > > > > > --aldrin > > > > > > > > > > On Fri, Jan 8, 2016 at 9:58 AM, M Singh > <mans2si...@yahoo.com.invalid > > > > > > > > wrote: > > > > > > > > > > > Just one more note Joe (as mentioned in the Jira Ticket) > > > > > > From what I can see, we cannot just deprecate createClient method > > in > > > > the > > > > > > AbstractAWSProcessor which uses the AWSCredentials argument, > since > > > the > > > > > > subclasses AbstractS3/SNS/SQSProcessor call that to create the > > > > respective > > > > > > clients. We will have to change the argument to > > > AWSCredentialProvider. > > > > > > If I can assist with any other investigation, please let me know. > > > > > > Thanks again. > > > > > > > > > > > > On Friday, January 8, 2016 5:31 AM, M Singh > > > > > > <mans2si...@yahoo.com.INVALID> wrote: > > > > > > > > > > > > > > > > > > Thanks Joe. > > > > > > If you think that we can accept the change to creds provider, I > > will > > > > work > > > > > > on making all the components in nifi aws processors to be > > consistent. > > > > I > > > > > > think using the creds provider interface is the way to go since > it > > is > > > > > more > > > > > > flexible and at this moment we just have 3 aws processors to > > migrate. > > > > > > Looking forward to hearing from you/anyone else for > > advice/feedback. > > > > > > Mans > > > > > > > > > > > > On Friday, January 8, 2016 5:18 AM, Joe Witt < > > joe.w...@gmail.com> > > > > > > wrote: > > > > > > > > > > > > > > > > > > Mans, > > > > > > > > > > > > I am working to put out a proposed roadmap and then probably > won't > > be > > > > > > very responsive until later tonight. Will try to help then if no > > one > > > > > > else has had a chance to. > > > > > > > > > > > > That said I see what you mean in terms of a breaking change in > the > > > > > > processor implementation as far as anyone else that has extended > > it. > > > > > > There have been some discussions recently about this and I think > > the > > > > > > plan is to start annotating everything with the audience and > > > stability > > > > > > of a given bit of code. Processors are not meant to be locked > down > > > > > > APIs. So, for now, given that it has been ambiguous to the > > community > > > > > > the best course is to probably just deprecate a given method if > it > > > > > > cannot be safely repurposed and then use a new one which does > meet > > > the > > > > > > need in the event the controller service is supplied. This last > > > > > > statement though is not based on me having looked at the code in > > any > > > > > > detail yet. > > > > > > > > > > > > Thanks > > > > > > Joe > > > > > > > > > > > > On Fri, Jan 8, 2016 at 8:08 AM, M Singh > > <mans2si...@yahoo.com.invalid > > > > > > > > > > wrote: > > > > > > > Hi Joe: > > > > > > > I have not worked with the controller interface and aws > > processors > > > so > > > > > > perhaps you can help me understand it . > > > > > > > From what I can see (as mentioned in > > > > > > https://issues.apache.org/jira/browse/NIFI-1325): currently, > the > > > Nifi > > > > > > AbstractAWSProcessor has a method > > > > > > > protected abstract ClientType createClient(final ProcessContext > > > > > context, > > > > > > final AWSCredentials credentials, final ClientConfiguration > > config); > > > > > > > This method is overridden in the AbstractS3/SNS/SQSProcesors to > > > > provide > > > > > > the respective amazon s3/sns/sqs client using AWSCredentials > > > argument. > > > > > > > Here is a snippet from AmazonS3Client: > > > > > > > public AmazonS3Client(AWSCredentials awsCredentials, > > > > > > ClientConfiguration clientConfiguration) { > > > > > > super(clientConfiguration); this.awsCredentialsProvider = > > new > > > > > > StaticCredentialsProvider(awsCredentials); init(); } > > > > > > > So, AmazonS3/SNS/SQSClient created with AWSCredentials use > > > > > > StaticCredentialsProvider in their implementation. > > > > > > > All the AWSCredentials impls are static creds > > (Anonymous/Properties > > > > > > Credentials) except for the STSSessionCredentials which has a > > refresh > > > > > > method but is deprecated in favor of the > > > STSSessionCredentialsProvider > > > > > > interface. AWSCredentials is the interface being used in nifi > aws > > > > > > processors. > > > > > > > The AWSCredentialsProvider interface has a fresh method which > all > > > > it's > > > > > > subclasses implement appropriately - the static ones (like > > > > > > PropertyFileCredentialsProvider/StaticCredentialsProvider have a > no > > > op > > > > > for > > > > > > refresh method) as follows: > > > > > > > public void refresh() {} > > > > > > > From what I can see, there is no common interface available for > > > > > > AWSCredentials and AWSCredentialsProvider that Nifi's > > > > > > AbstractAWSProcessor.createClient can support. So if we need to > > use > > > > the > > > > > > controller interface with creds providers, will will have to > change > > > > > > AbstractAWSProcessor.createClient to the following > > > > > > > > > > > > > > protected abstract ClientType createClient(final ProcessContext > > > > > context, > > > > > > final AWSCredentialsProvider credentialsProvider, > > > > > > > final ClientConfiguration config); > > > > > > > This appears to be a breaking change for the clients who have > > > > extended > > > > > > the AbstractAWSProcessor.createClient with the AWSCredentials > > > argument > > > > > > rather that the AWSCredentialsProvider. > > > > > > > So, can you please elaborate on how the AbstractAWSProcessor > will > > > be > > > > > > able to support both the current impl (ie, invoking aws > components > > > with > > > > > > creds) and the proposed credentials provider interface ? > > > > > > > Thanks > > > > > > > Mans > > > > > > > > > > > > > > On Thursday, January 7, 2016 9:00 PM, Joe Witt < > > > > joe.w...@gmail.com> > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Mans, > > > > > > > > > > > > > > It appears to me that there is a path for this not to be a > > breaking > > > > > > > change for the flow. By creating a controller service to > handle > > > the > > > > > > > credential provider piece you should be able to just update the > > > > > > > processor to support that controller service interface. If the > > > user > > > > > > > sets that controller service then you use that and if they > don't > > > then > > > > > > > you revert to using the older properties. We can mark those > > > > > > > properties as no longer the preferred model and deprecate them > in > > > the > > > > > > > codebase then when we reach a 1.0 milestone we can remove them. > > > > > > > > > > > > > > Thanks > > > > > > > Joe > > > > > > > > > > > > > > On Thu, Jan 7, 2016 at 9:07 PM, M Singh > > > <mans2si...@yahoo.com.invalid > > > > > > > > > > > wrote: > > > > > > >> Hi: > > > > > > >> Just wanted to mention that if we go with the creds provider > > > > interface > > > > > > it will be breaking change for nifi aws components as mentioned > in > > > > > > https://issues.apache.org/jira/browse/NIFI-1325. > > > > > > >> Also, I am considering creating one aws creds provider > > controller > > > > > which > > > > > > will provide creds provider based on property file, basic, > > anonymous > > > or > > > > > > assume role session params. > > > > > > >> Please let me know if there is any additional feedback for me. > > > > > > >> Thanks again. > > > > > > >> Mans > > > > > > >> > > > > > > >> On Thursday, January 7, 2016 2:56 PM, M Singh > > > > > > <mans2si...@yahoo.com.INVALID> wrote: > > > > > > >> > > > > > > >> > > > > > > >> Hi Joe: > > > > > > >> Based on your feedback I will try to explore the controller > > > > interface > > > > > > for aws creds provider. > > > > > > >> Thanks for your advice. > > > > > > >> Mans > > > > > > >> > > > > > > >> On Thursday, January 7, 2016 4:15 AM, Joe Witt < > > > > joe.w...@gmail.com > > > > > > > > > > > > wrote: > > > > > > >> > > > > > > >> > > > > > > >> Mans > > > > > > >> > > > > > > >> Appreciate you pushing this forward. There is a related idea > to > > > > > better > > > > > > >> handle aws credentials for all the aws procs. Will look more > > and > > > > > > respond. > > > > > > >> > > > > > > >> Thanks > > > > > > >> Joe > > > > > > >> On Jan 7, 2016 6:52 AM, "M Singh" > <mans2si...@yahoo.com.invalid > > > > > > > > wrote: > > > > > > >> > > > > > > >>> Hi: > > > > > > >>> Just wanted to follow-up and see if anyone has any feedback > on > > . > > > > > > >>> https://issues.apache.org/jira/browse/NIFI-1325. > > > > > > >>> Thanks > > > > > > >>> Mans > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
