Skip to content

Added enableSoftDelete, DeletedVault, and Purge to method group #432

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
anuchandy merged 16 commits into from
May 11, 2018
Merged

Added enableSoftDelete, DeletedVault, and Purge to method group #432

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
8b01f9b
Added enableSoftDelete, DeletedVault, and Purge to method group
tiffanyachen Apr 25, 2018
840bc67
re-recorded tests
tiffanyachen Apr 25, 2018
6b777b9
Updates from code review
tiffanyachen Apr 26, 2018
e3cedf9
Updated access policy to include storage
tiffanyachen May 1, 2018
ffb3d8c
Moved converter for DeletedVaultInner
tiffanyachen May 1, 2018
8ee4d5b
Added async methods
tiffanyachen May 7, 2018
665b61d
Removed unecessary async methods and SupportsListing<Vault> from Vaul…
tiffanyachen May 9, 2018
5f55bb3
Simplified async access and elevated properties for Vault and Deleted…
tiffanyachen May 10, 2018
531c670
Updated checkstyle
tiffanyachen May 10, 2018
a4dca55
Elevated access policies
tiffanyachen May 11, 2018
929fb8b
Moved recovery of softDeletedVault to independent method
tiffanyachen May 11, 2018
dcd32d0
Merge remote-tracking branch 'upstream/kv2018' into kv2018
tiffanyachen May 11, 2018
b4cdc48
Changed purgeDeletedAsync to return Completable
tiffanyachen May 11, 2018
bec4326
Removed unused interfaces and added beta annotations
tiffanyachen May 11, 2018
e547233
Unused imports in vaultImpl
tiffanyachen May 11, 2018
b739852
Updated javadocs for accessPolicy
tiffanyachen May 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 131 additions & 1 deletion azure-mgmt-keyvault/src/main/java/com/microsoft/azure/management/keyvault/AccessPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,31 @@ interface WithIdentity<ParentT> {
* @return the next stage of access policy definition
*/
WithAttach<ParentT> forUser(ActiveDirectoryUser user);

/**
* Specifies the Active Directory user this access policy is for.
*
* @param userPrincipalName the user principal name of the AD user
* @return the next stage of access policy definition
*/
WithAttach<ParentT> forUser(String userPrincipalName);

/**
* Application ID of the client making request on behalf of a principal.
*
* @param applicationId the application ID
* @return the next stage of access policy definition
*/
WithAttach<ParentT> forApplicationId(String applicationId);

/**
* Specifies the Azure Active Directory tenant ID that should be used for
* authenticating requests to the key vault.
*
* @param tenantId the tenant ID for the key vault.
* @return the next stage of access policy definition
*/
WithAttach<ParentT> forTenantId(String tenantId);

/**
* Specifies the Active Directory group this access policy is for.
Expand Down Expand Up @@ -202,6 +219,30 @@ interface WithPermissions<ParentT> {
* @return the next stage of access policy definition
*/
WithAttach<ParentT> allowCertificatePermissions(List<CertificatePermissions> permissions);

/**
* Allow all permissions for the Ad identity to access storage.
*
* @param the next stage of access policy definition
*/
@Method
WithAttach<ParentT> allowStorageAllPermissions();

/**
* Allow a list of permissions for the AD identity to access storage.
*
* @param permissions the list of permissions allowed
* @return the next stage of access policy definition
*/
WithAttach<ParentT> allowStoragePermissions(StoragePermissions...permissions);

/**
* Allow a list of permissions for the AD identity to access storage.
*
* @param permissions the list of permissions allowed
* @return the next stage of access policy definition
*/
WithAttach<ParentT> allowStoragePermissions(List<StoragePermissions> permissions);
}

/** The final stage of the access policy definition.
Expand Down Expand Up @@ -266,6 +307,23 @@ interface WithIdentity<ParentT> {
* @return the next stage of access policy definition
*/
WithAttach<ParentT> forUser(String userPrincipalName);

/**
* Application ID of the client making request on behalf of a principal.
*
* @param applicationId the application ID
* @return the next stage of access policy definition
*/
WithAttach<ParentT> forApplicationId(String applicationId);

/**
* Specifies the Azure Active Directory tenant ID that should be used for
* authenticating requests to the key vault.
*
* @param tenantId the tenant ID for the key vault.
* @return the next stage of access policy definition
*/
WithAttach<ParentT> forTenantId(String tenantId);

/**
* Specifies the Active Directory group this access policy is for.
Expand Down Expand Up @@ -344,6 +402,30 @@ interface WithPermissions<ParentT> {
* @return the next stage of access policy definition
*/
WithAttach<ParentT> allowSecretPermissions(List<SecretPermissions> permissions);

/**
* Allow all permissions for the Ad identity to access storage.
*
* @param the next stage of access policy definition
*/
@Method
WithAttach<ParentT> allowStorageAllPermissions();

/**
* Allow a list of permissions for the AD identity to access storage.
*
* @param permissions the list of permissions allowed
* @return the next stage of access policy definition
*/
WithAttach<ParentT> allowStoragePermissions(StoragePermissions...permissions);

/**
* Allow a list of permissions for the AD identity to access storage.
*
* @param permissions the list of permissions allowed
* @return the next stage of access policy definition
*/
WithAttach<ParentT> allowStoragePermissions(List<StoragePermissions> permissions);
}

/** The final stage of the access policy definition.
Expand Down Expand Up @@ -510,6 +592,54 @@ interface WithPermissions {
* @return the next stage of access policy update
*/
Update disallowCertificatePermissions(List<CertificatePermissions> permissions);

/**
* Allow all permissions for the Ad identity to access storage.
*
* @param the next stage of access policy definition
*/
@Method
Update allowStorageAllPermissions();

/**
* Allow a list of permissions for the AD identity to access storage.
*
* @param permissions the list of permissions allowed
* @return the next stage of access policy definition
*/
Update allowStoragePermissions(StoragePermissions...permissions);

/**
* Allow a list of permissions for the AD identity to access storage.
*
* @param permissions the list of permissions allowed
* @return the next stage of access policy definition
*/
Update allowStoragePermissions(List<StoragePermissions> permissions);

/**
* Revoke all permissions for the Ad identity to access storage.
*
* @param the next stage of access policy definition
*/
@Method
Update disallowStorageAllPermissions();

/**
* Revoke a list of permissions for the AD identity to access storage.
*
* @param permissions the list of permissions allowed
* @return the next stage of access policy definition
*/
Update disallowStoragePermissions(StoragePermissions...permissions);

/**
* Revoke a list of permissions for the AD identity to access storage.
*
* @param permissions the list of permissions allowed
* @return the next stage of access policy definition
*/
Update disallowStoragePermissions(List<StoragePermissions> permissions);
}
}

Expand Down
36 changes: 36 additions & 0 deletions ...lt/src/main/java/com/microsoft/azure/management/keyvault/CheckNameAvailabilityResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package com.microsoft.azure.management.keyvault;

import com.microsoft.azure.management.apigeneration.Fluent;
import com.microsoft.azure.management.keyvault.implementation.CheckNameAvailabilityResultInner;
import com.microsoft.azure.management.resources.fluentcore.model.HasInner;

/**
* The CheckNameAvailability operation response wrapper.
*
*/
@Fluent(ContainerName = "/Microsoft.Azure.Management.Fluent.KeyVault")
public interface CheckNameAvailabilityResult extends
HasInner<CheckNameAvailabilityResultInner>{

/**
* Get the nameAvailable value.
*
* @return the nameAvailable value
*/
public Boolean nameAvailable();

/**
* Get the reason value.
*
* @return the reason value
*/
public Reason reason();

/**
* Get the message value.
*
* @return the message value
*/
public String message();

@milismsft milismsft May 8, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect to see here some methods such as isAvailable(), unavailabilityReason() and unavailabilityMessage()...

@tiffanyachen tiffanyachen May 9, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in latest commit

}
18 changes: 18 additions & 0 deletions azure-mgmt-keyvault/src/main/java/com/microsoft/azure/management/keyvault/DeletedVault.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package com.microsoft.azure.management.keyvault;

import com.microsoft.azure.management.apigeneration.Fluent;
import com.microsoft.azure.management.keyvault.implementation.DeletedVaultInner;
import com.microsoft.azure.management.resources.fluentcore.arm.models.HasId;
import com.microsoft.azure.management.resources.fluentcore.arm.models.HasName;
import com.microsoft.azure.management.resources.fluentcore.model.HasInner;

/**
* An immutable client-side representation of an Azure Key Vault.
*/
@Fluent(ContainerName = "/Microsoft.Azure.Management.Fluent.KeyVault")
public interface DeletedVault extends
HasInner<DeletedVaultInner>,
HasName,
HasId {

@milismsft milismsft May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tiffanyachen In Fluent we try to avoid exposing the users to the *Inner classes (or any other class that is part of the "implementation" folder except for the *Manager). In this particular case we can "promote" the DeletedVaultProperties properties as methods of this interface rather than having the user navigate through two levels of indirection in order to reach them (one being the .inner()). Would you be able to add the corresponding methods please? The exact mapping is:
location() -> inner().properties().location()
deleteDate() ->inner().properties().deleteDate()
scheduledPurgeDate() -> inner().properties().scheduledPurgeDate()
tags() -> inner().properties().tags()

}
53 changes: 53 additions & 0 deletions azure-mgmt-keyvault/src/main/java/com/microsoft/azure/management/keyvault/Vault.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,24 @@ public interface Vault extends
* retrieve secrets from the key vault.
*/
boolean enabledForTemplateDeployment();

/**
* @return whether soft delete is enabled for this key vault.
*/
boolean softDeleteEnabled();

@anuchandy anuchandy Apr 26, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be consistent with naming pattern of other existing properties returning bool (e.g. enabledForDeployment, enabledForDiskEncryption, enabledForTemplateDeployment), should we rename these new getters, like purgeProtectionEnabled -> enabledForPurgeProtection, softDeleteEnabled -> enabledForSoftDelete

@tiffanyachen tiffanyachen Apr 26, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schaabs and I discussed this a bit and decided to go with this pattern to make it consistent with our other languages.

@milismsft milismsft May 8, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tiffanyachen in this case you need to be consistent with the rest of the Java SDK naming patterns for this service and not other languages. There's a good reason and lots of review discussions before we chose the naming we use today. So unless you want to change all the other method names and introduce your own new naming pattern, then work around the backward compatibility breaks resulted from these change, then my recommendation is to stay with @anuchandy feedback.

@schaabs schaabs May 8, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anuchandy @milismsft In the case of the first three fields, enableForDeployment, enabledForDiskEncryption, and enabledForTemplateDeployment, they are named this way because the user is enabling the vault to be accesed FOR arm deployments, disk encryption, and template deployment. In the case of softDeleteEnabled and purgeProtectionEnabled the user is enabling the features soft delete and purge protection on the vault.

@milismsft milismsft May 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schaabs I don't understand how is soft delete any different then the others... It looks like it is required for instance when setting up SQL TDE (see customer report Azure/azure-libraries-for-net#315).
If "soft delete" and "purge protection" are simple vault properties it might be worth removing the whole "enabled" thing from the name, i.e "bool softDelete()" and "bool purgeProtection()".

@milismsft milismsft May 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the "with" methods could be renamed as "withSoftDelete()" and "withPurgeProtection()" IF these properties are indeed vault only properties.


/**
* @return whether purge protection is enabled for this key vault.
* Purge protection can only be enabled if soft delete is enabled.
*/
boolean purgeProtectionEnabled();

/**
* Get the createMode value.
*
* @return the createMode value
*/
public CreateMode createMode();

/**************************************************************
* Fluent interfaces to provision a Vault
Expand Down Expand Up @@ -183,6 +201,20 @@ interface WithConfigurations {
* @return the next stage of key vault definition
*/
WithCreate withTemplateDeploymentEnabled();

/**
* Enable soft delete for the key vault.
*
* @return the next stage of key vault definition
*/
WithCreate withSoftDeleteEnabled();

/**
* Enable purge protection for the key vault; valid only if soft delete is also enabled.
*
* @return the next stage of key vault definition.
*/
WithCreate withPurgeProtectionEnabled();

/**
* Disable Azure Virtual Machines to retrieve certificates stored as secrets from the key vault.
Expand All @@ -204,6 +236,13 @@ interface WithConfigurations {
* @return the next stage of key vault definition
*/
WithCreate withTemplateDeploymentDisabled();

/**
* Set the createMode value.
*
* @return the next stage of key vault definition
*/
WithCreate withCreateMode(CreateMode createMode);

@milismsft milismsft May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tiffanyachen Could CreateMode.RECOVER be used with an existing Key Vault resource that has not been deleted? What impact other settings prior to reaching this method during the create flow will have when this value is set?

@tiffanyachen tiffanyachen May 10, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can only be used for recovering a deleted vault with soft delete enabled; in any other case it'll result in an error.

@milismsft milismsft May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tiffanyachen If this is the case then we will have to rework a bit the "Create()" flow. What's the maximum properties in the JSON body that can be passed to the create op when CreateMode.RECOVER is specified?
Basically we will need to think of a way to shortcut the current create flow for the user to be able to use this setting. I'm thinking it might be worth having a separate set of methods as part of Vaults.java something like recoverDeletedVault(resourceGroupName, vaultName) or recoverSoftDeletedVault() (plus the Async one), instead of expecting the user to specify the CreateMode via the define()/create() flow... Because I don't see the later how it will ever succeed when we ask the user for SKU and other stuff before it can reach to withCreateMode().

}

/**
Expand Down Expand Up @@ -285,6 +324,20 @@ interface WithConfigurations {
* @return the key vault update stage
*/
Update withTemplateDeploymentEnabled();

/**
* Enable soft delete for the key vault.
*
* @return the next stage of key vault definition
*/
Update withSoftDeleteEnabled();

@anuchandy anuchandy Apr 26, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to disable soft delete once enabled? if so we may need withSoftDeleteDisabled, withPurgeProtectionDisabled

@tiffanyachen tiffanyachen Apr 26, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SoftDelete and PurgeProtection can't be disabled.


/**
* Enable purge protection for the key vault; valid only if soft delete is also enabled.
*
* @return the next stage of key vault definition.
*/
Update withPurgeProtectionEnabled();

/**
* Disable Azure Virtual Machines to retrieve certificates stored as secrets from the key vault.
Expand Down
13 changes: 13 additions & 0 deletions ...lt/src/main/java/com/microsoft/azure/management/keyvault/VaultAccessPolicyParameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package com.microsoft.azure.management.keyvault;
import com.microsoft.azure.management.apigeneration.Fluent;
import com.microsoft.azure.management.keyvault.implementation.VaultAccessPolicyParametersInner;
import com.microsoft.azure.management.resources.fluentcore.model.HasInner;

/**
* Parameters for updating the access policy in a vault.
*/
@Fluent(ContainerName = "/Microsoft.Azure.Management.Fluent.KeyVault")
public interface VaultAccessPolicyParameters extends
HasInner<VaultAccessPolicyParametersInner>{

@milismsft milismsft May 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tiffanyachen similar comments I made about the DeletedVault interface; we should "promote" the properties from VaultAccessPolicyPropertiesInner class as methods of this interface. Also the interface should extend HasId(), HasName() and expose type() and location().

}
2 changes: 1 addition & 1 deletion ...in/java/com/microsoft/azure/management/keyvault/VaultCheckNameAvailabilityParameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import com.fasterxml.jackson.annotation.JsonProperty;

/**
* The parameters used to check the availabity of the vault name.
* The parameters used to check the availability of the vault name.
*/
public class VaultCheckNameAvailabilityParameters {
/**
Expand Down
Loading