Thanks Jim! I'll review the PR today/tomorrow. Meanwhile, I've sent you a patch that fixes the build and checkstyle violations. Feel free to add it to the PR! :)
On 30 October 2017 at 22:36, Jim Spring <jmspr...@gmail.com> wrote: > Added PR — > > https://github.com/jclouds/jclouds-labs/pull/416 > > Currently failing with no violations listed — > > https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/1850/org.apache.jclouds.labs$azurecompute-arm/violations/ > > I’ve added notes regarding the status in the PR and a couple of things to > look into. > > -jim > > > On October 30, 2017 at 1:32:21 AM, Ignasi Barrera (n...@apache.org) wrote: > > Thanks Jim. The plan LGTM. > > For the tests in (3), we could do it in two steps. A first impl that > relies on an existing storage account, and then we can see if we > really need to add the functionality to create the storage accounts > dynamically. (Is there any planned ETA for the storage account APIs to > the new ARM version?). > > Don't worry about huge PRs :) Happy to review! > > On 29 October 2017 at 04:01, Jim Spring <jmspr...@gmail.com> wrote: >> An update — >> >> I’ll probably finish mock tests tomorrow and issue the PR outlined in > (1). >> That said, there are two API calls that will be stubbed out in tests: >> >> - Update Certificate Policy based off of a certificate version >> - Azure is giving an error that modifying the policy of a versioned >> certificate is not allowed (even in the case where the policy doesn’t >> change). I need to talk with the product team on this. >> - Merge Certificate >> - I don’t think there can be a live test for this as it’s specific to the >> case where you have a CSR generated with Azure KeyVault and manually have >> the certificate signed externally. I’ll also check with the product team >> to see if there is some hack around this at least to add a test. >> >> Regarding (3), the operations rely upon an external storage account being >> created (this is confirmed by the product team), in essence the KeyVault >> takes over management of keys and shared access signatures for the > storage >> account. The Azure storage account API relies on the old XML based API >> which predates the Azure Resource Manager JSON apis. >> >> My proposal still stands at the three prior steps, with the caveat of > tests >> above. >> >> For (3), I will see if I can either leverage existing JClouds > functionality >> or I will (in Test) implement a minimal set of functionality needed to > set >> up the storage account in order to complete the KeyVault APIs. >> >> So, I should have the PR for (1) tomorrow, I’ll make sure to integrate >> Andrea’s earlier input re: camel case. And it will be directly from the >> branch I am developing on into JClouds proper. >> >> The PR, sadly, won’t be a small one. But, I’ve stuck to inner classes, >> etc. to not cause class file bloat. >> >> -j >> >> >> On October 26, 2017 at 3:10:05 AM, Ignasi Barrera (n...@apache.org) > wrote: >> >> Sounds like a good plan. >> >> Regarding the tests, yes, mock tests are important. The implement "the >> contract" between jclouds and the "provider API docs". They are very >> useful to early detect incorrect json mappings, serialization issues, >> verify pagination, redirections, authentication, and other stuff, so >> please, include them. >> >> WRT (3), agree. Let's first clarify the usage patterns and expected >> workflow when using the API and then let's discuss how can we >> translate that into jclouds APIs. >> >> >> Thanks for taking care of this! >> >> I. >> >> On 26 October 2017 at 05:10, Jim Spring <jmspr...@gmail.com> wrote: >>> @Ignasi, @Andrea - >>> >>> Here is my proposal - the PR should be done in sections. I would propose >>> three: >>> >>> 1. A PR implementing Vault, Key, Secret, and Certificate operations. >>> 2. A PR implementing decrypt/encrypt/wrap/unwrap/sign/verify operations. >>> 3. A PR implementing Storage Account and SAS operations >>> >>> The reasoning is as follows: >>> >>> (1) implements the management of the vault itself and cryptographic >>> primitives. Applications will want to leverage the vault for the > storage, >>> retrival, and versioning of these resources. >>> >>> (2) adds the ability to use keys from (1) to perform operations. With >>> non-HSM keys, these could probably be done externally. But this builds / >>> is additive off of (1) and, in my opinion, will be used by fewer people >>> than those wanting the support of (1). >>> >>> (3) To keep it clean, SA and SAS operations directly tie the KeyVault to >>> needing to also implement Azure Storage Service functionality. Or have > an >>> existing storage account. If I were to think about the live tests, one >>> would need to create a storage account in order to leverage this >>> functionality. Storage accounts use the older ASM/XML based API. Before >>> implementing this, I would prefer to talk with the internal KeyVault > team >>> to understand more about this area. >>> >>> In terms of timing, I will have (1) done (minus feedback) either later >>> tonight or tomorrow including live tests. Are mock tests still required? >>> >>> I could have (2) done towards the end of this week/early next. >>> >>> (3) will depend on meetings with the KeyVault team to understand a bit >> more >>> about documentation gaps and usage patters. >>> >>> Thoughts? >>> >>> Thanks >>> -jim >>> >>> >>> On October 23, 2017 at 1:50:38 AM, Andrea Turli (andrea.tu...@gmail.com) >>> wrote: >>> >>> FYI Jim has already started fixing some issue at >>> https://github.com/andreaturli/jclouds-labs/pull/9/files >>> >>> I'd suggest to get my implementation in my andreaturli:feature/vault-api >>> branch and open a PR against jclouds/jclouds-labs. >>> >>> Andrea >>> >>> On Mon, Oct 23, 2017 at 10:18 AM, Ignasi Barrera <n...@apache.org> > wrote: >>> >>>> > How much of the KeyVault api should we implement? I’ve started >> creating >>>> actual tests for the Key operations and will be prioritizing the list > in >>>> order that I’ll be adding them. >>>> >>>> I don't know how complex the API is, but it is quite straightforward >>>> to implement APIs in jclouds once you have a couple methods right, so >>>> I'd say we'd try to implement all methods, if possible. I'll be happy >>>> to help here. >>>> >>>> > >>>> > I’ll try and do a PR per set (if you don’t mind Andrea). >>>> >>>> PRs always welcome! >>>> >>>> > >>>> > I’ll try and catch you guys on IRC tomorrow morning pacific time. >>>> >>>> I've been quite offline recently but I hope to start having more time, >>>> so feel free to ping me whenever you find me there. >>>> >>>> > >>>> > -j >>>> > >>>> > Sent from my iThingy >>>> > >>>> >> On Oct 17, 2017, at 15:39, Jim Spring <jmspr...@gmail.com> wrote: >>>> >> >>>> >> Andrea - >>>> >> >>>> >> Take a look at the PR I did against your feature/vault-api branch. >> I’m >>>> extending the tests now. >>>> >> >>>> >> I’ll do a PR once I get all the key tests implemented. >>>> >> >>>> >> Would it make sense to find some time to chat? Or are you ok with me >>>> moving forward on extending things? >>>> >> >>>> >> -jim >>>> >> >>>> >> >>>> >>> On October 17, 2017 at 1:52:20 AM, Andrea Turli ( >>>> andrea.tu...@gmail.com) wrote: >>>> >>> >>>> >>> Thanks Jim! >>>> >>> >>>> >>> An additional property makes sense for liveTests as well. >>>> >>> Direct use of Azure AD GraphApi looks a bit hard to me, so let's >> make >>>> >>> jclouds accept an extra parameter that will allow at least to >> specify >>>> an >>>> >>> already existing objectID. >>>> >>> wdyt? >>>> >>> >>>> >>> On Tue, Oct 17, 2017 at 10:33 AM, Ignasi Barrera <n...@apache.org> >>>> wrote: >>>> >>> >>>> >>> > Thanks Jim! >>>> >>> > >>>> >>> > I think it makes sense for the live tests to get the value >>> configured >>>> >>> > as a property. >>>> >>> > >>>> >>> > If someone wants to directly use the KeyVault API, I think it >> makes >>>> >>> > sense to have the objectId as a required parameter (as it is > now). >>> I >>>> >>> > don't see an immediate need to auto-discover it or to have it >>>> >>> > configured per-context in advance, so I'd say a property just for >>> the >>>> >>> > tests is fine. >>>> >>> > >>>> >>> > >>>> >>> > I. >>>> >>> > >>>> >>> > On 16 October 2017 at 20:44, Jim Spring <jmspr...@gmail.com> >> wrote: >>>> >>> > > Andrea - >>>> >>> > > >>>> >>> > > A PR is present plumbing in the ObjectID. I’ll be looking at >>>> fleshing >>>> >>> > out >>>> >>> > > the tests more today. >>>> >>> > > >>>> >>> > > -jim >>>> >>> > > >>>> >>> > > >>>> >>> > > On October 16, 2017 at 9:48:39 AM, Jim Spring ( > jmspr...@gmail.com) >> >>> >>>> >>> > wrote: >>>> >>> > > >>>> >>> > > Andrea, Ignasi - >>>> >>> > > >>>> >>> > > I’ve managed to figure out the authentication issue Andrea was >>>> running >>>> >>> > into >>>> >>> > > with his KeyVault implementation keeping the live tests from >>>> working. I >>>> >>> > > don’t think a PR is needed, but I am cleaning up the code to >> just >>>> double >>>> >>> > > check. >>>> >>> > > >>>> >>> > > Basically, the issue is as follows: >>>> >>> > > >>>> >>> > > 1. KeyVault relies upon Azure AD for access control (this is > the >>>> Object >>>> >>> > ID >>>> >>> > > passed in) >>>> >>> > > 2. A service principal (or other Azure AD object) typically has >>>> two IDs >>>> >>> > > associated with it: >>>> >>> > > - The name or AppID >>>> >>> > > - An ObjectID >>>> >>> > > >>>> >>> > > For Azure tests, currently, one must specify the Service >>> Principal >>>> >>> > Name/App >>>> >>> > > ID as well as the secret. For the creation of the KeyVault in >> the >>>> test, >>>> >>> > > one needs the “ObjectID” of the Service Principal used to > login. >>>> >>> > > >>>> >>> > > There are two ways to fix this: >>>> >>> > > >>>> >>> > > 1. Implement the Azure AD Graph API in order to look the >>>> information up >>>> >>> > > (note, the SP may not have access to do so) >>>> >>> > > 2. Add another parameter to be specified for Vault Live tests, >>> say >>>> >>> > > something like 'test.azurecompute-arm.identity.objectid’ >>>> >>> > > >>>> >>> > > Thoughts? >>>> >>> > > >>>> >>> > > I’m going through my code changes now and will reach out for >>>> anything >>>> >>> > > required there to Andrea. >>>> >>> > > >>>> >>> > > -jim >>>> >>> > >>>>
