JCLOUDS-1304: add azure customdata, JCLOUDS-1305: add support for ssh keys in keyvault #398
Conversation
VRanga000
changed the title
There was a problem hiding this comment.
Thanks, @VRanga000 and apologies for the late review!
I see the key vault option being added but it is not used anywhere. Are there plans to add that later? In that case, Does it make more sense to add that option then too?
| @@ -41,7 +41,17 @@ | |||
| private List<IpOptions> ipOptions = ImmutableList.of(); | |||
| private WindowsConfiguration windowsConfiguration; | |||
| private List<Secrets> secrets = ImmutableList.of(); | |||
|
|
|||
| private String customData; | |||
There was a problem hiding this comment.
In mots providers, we declare this as a byte[], so we don't have to deal with charset issues, etc. We just get the bytes to be base64-encoded. Mind changing the type and method signatures to accept a byte array?
| * Sets the KeyVault id and secret separated with ":" | ||
| */ | ||
| public AzureTemplateOptions keyVaultIdAndSecret(String keyVaultIdAndSecret) { | ||
| this.keyVaultIdAndSecret = keyVaultIdAndSecret; |
There was a problem hiding this comment.
Validate that the parameter has the right format.
|
Thanks for the review comments @nacx ! I do need to add (back) the code to actually use the keyvaultidAndSecret, will do that. Will address the other comments as well. |
|
Hello @nacx - need your input on implementation for keyvault support. The azure compute adapter code was refactored to use the VirtualMachineAPI instead of the deployment API. As a result, keyvaults cannot be accessed merely by reference (as we could in the deployment template). So - I think we need to implement a new "feature" for azurecompute-arm - namely the keyvault API (https://docs.microsoft.com/en-us/rest/api/keyvault/). So I think we should restrict this PR to just "custom data" support" and remove the keyvault refs. Then create a new issue for keyvault support. Thoughts? Thanks much! |
Looks good to me! |
|
@VRanga000 what do you think about splitting the PR into |
|
@VRanga000 Is it possible to split off the custom data support sooner than later and issue the PR with that? That way we can merge the PR and get azure-armcompute promoted out of labs. The Keyvault work still relies on the things we talked about last week. Let me know if I can help at all. |
|
thanks @nacx fyi I'm looking at |
|
Andrea, Ignasi -
I guess Vikas never came back on the one PR?
I'm actually working on the KeyVault stuff myself at the moment.
…-j
On Sep 29, 2017 2:41 AM, "Andrea Turli" ***@***.***> wrote:
thanks @nacx <https://github.com/nacx> fyi I'm looking at KeyVaultApi,
focusing on the feature first and then we can together add that at the
level of the ComputeAdapter?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#398 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHljtVIPHy-2cL2rjVhBuPiS0mKfq_Kks5snLtjgaJpZM4N8x9t>
.
|
|
thanks @jmspring! Do you think it would make sense to work on the same branch? my wip is available at https://github.com/andreaturli/jclouds-labs/tree/feature/vault-api in case you want to have a look. I can make |
|
Andrea -
I'll be getting back to this in depth tomorrow. I looked at your work and
some that was done internally. I'm happy to look at your branch and do PRs
to it.
For the next couple of days it's my focus. What auth issues are you having?
…-j
On Sep 29, 2017 11:33 AM, "Andrea Turli" ***@***.***> wrote:
thanks @jmspring <https://github.com/jmspring>!
Do you think it would make sense to work on the same branch? my wip is
available at https://github.com/andreaturli/jclouds-labs/tree/
feature/vault-api in case you want to have a look.
I can make *keys operations work for some authorization issues I'm still
trying to figure out
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#398 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHljtngq8STUPcG6mpxtCYfEnuSbhViks5snTfzgaJpZM4N8x9t>
.
|
|
Excellent @jmspring I can't list/create keys or certificates into a newly created key vault because of an Authorization issue |
|
Ok, I will start with looking into that.
.
|
|
@andreaturli - I hope to have some progress on this this week. I'm going through the live tests now for Vault API. Do you have thoughts on externalizing the tenantId which is hard coded in the live test? |
I don't have the context of the discussion, but isn't it part of the |
|
Closing this. With the recent addition of the keyvault API and the configuration of the custom data in a different PR this should be better done in a new one. |
No description provided.