nacx commented on this pull request.
Many thanks @jmspring!
This looks quite good! I've added some general comments. Apart from those,
could you paste the summary of the live tests results?
> @@ -53,6 +53,33 @@ Verify service principal
az login -u <Application-id> -p <password> --service-principal --tenant
<Tenant-id>
```
+## KeyVault Live Tests
+
+In order to run the KeyVault live tests, an additional parameter is needed.
This is the `objectId` of
+the service principal being used for the tests. One can retrieve the service
principal `objectId` by
+doing the following using the Azure 2.0 CLI:
Is this property actually needed? Or are now the tests using the existing
service principal supplier to automatically get the `objectId`?
> @@ -252,6 +253,15 @@ NetworkSecurityRuleApi
> getNetworkSecurityRuleApi(@PathParam("resourcegroup") Str
@Delegate
GraphRBACApi getGraphRBACApi();
+ /**
+ * Managing your key vaults as well as the keys, secrets, and certificates
within your key vaults can be
+ * accomplished through a REST interface.+
[minor] Remove the trailing `+`.
> +import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+
+@AutoValue
+public abstract class VaultProperties {
+
+ @AutoValue
+ public abstract static class Permissions {
+
+ @Nullable public abstract List<String> certificates();
+ @Nullable public abstract List<String> keys();
+ @Nullable public abstract List<String> secrets();
+ @Nullable public abstract List<String> storage();
These lists are never null since you enforce immutability and presence. Let's
remove the `@Nullable` annotation and apply the same pattern for all other
collections where you enforce presence.
> + @Fallback(NullOnNotFoundOr404.class)
+ @OAuthResource("https://vault.azure.net";)
+ DeletedKeyBundle deleteKey(@EndpointParam URI vaultBaseUrl,
@PathParam("keyName") String keyName);
+
+ @Named("key:get_versions")
+ @GET
+ @SelectJson("value")
+ @Path("/keys/{keyName}/versions")
+ @Fallback(EmptyListOnNotFoundOr404.class)
+ @OAuthResource("https://vault.azure.net";)
+ List<Key> getKeyVersions(@EndpointParam URI vaultBaseUrl,
@PathParam("keyName") String keyName);
+
+ @Named("key:update")
+ @PATCH
+ @MapBinder(BindToJsonPayload.class)
+ @Path("/keys/{keyName}{keyVersion}")
Shouldn't this be `{keyName}/{keyVersion}`?
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.azurecompute.arm.functions;
+import com.google.common.base.Function;
+import org.jclouds.http.HttpResponse;
+
+import javax.inject.Singleton;
+
+import static org.jclouds.http.HttpUtils.releasePayload;
+/**
+ * Parses an http response code from http responser
+ */
+@Singleton
+public class TrueOn20X implements Function<HttpResponse, Boolean> {
Is this class actually used?
> +import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import com.google.common.base.Supplier;
+import com.google.common.collect.ImmutableList;
+
+import javax.inject.Inject;
+
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
+import static org.testng.AssertJUnit.assertNull;
+
+@Test(groups = "live", testName = "VaultApiLiveTest")
+public class VaultApiLiveTest extends BaseAzureComputeApiLiveTest {
+ @Inject private Supplier<ServicePrincipal> svcPrincipal;
Injection does not work in tests. Remove this and get it from the injector in
the base test class if needed.
> + private static String cryptoText = "R29sZCUyNTIxJTJCR29sZCUyNTIxJTJCR2" +
+ "9sZCUyQmZyb20lMkJ0aGUlMkJBbWVyaWNhbiUyQlJpdmVyJTI1MjE";
+ private static String cryptoAlgorithm = "RSA-OAEP";
+ private static String hashToSign =
"FvabKT6qGwpml59iHUJ72DZ4XyJcJ8bgpgFA4_8JFmM";
+ private static String signatureAlgorithm = "RS256";
+ private static String contentEncryptionKey =
"YxzoHR65aFwD2_IOiZ5rD08jMSALA1y7b_yYW0G3hyI";
+
+ @BeforeClass
+ @Override
+ public void setup() {
+ super.setup();
+ createTestResourceGroup();
+ vaultName = String.format("kv%s",
this.getClass().getSimpleName().toLowerCase());
+ }
+
+ @AfterClass
`alwaysRun = true` to cleanup stuff even if tests fail.
> + return;
+ }
+ }
+
+ DeletedVault deletedVault = api().getDeletedVault(LOCATION, vaultName);
+ if (deletedVault != null) {
+ api().purgeVault(LOCATION, vaultName);
+ waitForOperation("vault", "purge", vaultName);
+ }
+ }
+
+ // Note: Operations on a KeyVault that supports soft delete can take time
+ // to settle. Thus for tests where appropriate, a wait operation with a
+ // one minute timeout will be used to make sure things are in the desired
+ // state before proceeding to the next test.
+ private boolean waitForOperation(String itemType, String operation, String
entityName) {
jclouds already provides clean means to actively wait for things, and allows to
configure the timeouts, etc, via properties. Please use the `Predicates2.retry`
helper instead of manually doing this.
This logic might be useful for users too, so I'd strongly recommend to copy how
[we configure active waits for Azure
ARM](https://github.com/jclouds/jclouds-labs/blob/master/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/config/AzureComputeServiceContextModule.java#L145-L385)
and apply the same pattern here:
1. Refactor each conditional in this method into its own `Predicate` class.
2. Put all those predicates in an `AzurePredicatesModule` (or similar), and
consider moving the ones that already exist (linked above) to that class.
3. For each defined predicate class, add a `@Provides` method that exposes the
predicate encapsulated with the `retry` helper. Consider using the standard
jclouds timeout properties or just create Azure-specific properties to
configure them if that makes sense, but follow the same pattern.
4. [Initialize the predicates in the base test
class](https://github.com/jclouds/jclouds-labs/blob/master/azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/internal/BaseAzureComputeApiLiveTest.java#L106-L112)
so you can easily use them in the tests.
> + if (i == 0) {
+ break;
+ }
+ try {
+ Thread.sleep(6000);
+ } catch (java.lang.InterruptedException ie) {
+ break;
+ }
+ }
+ }
+
+ return done;
+ }
+
+ @Test
+ @Inject
Remove all `@Inject` annotations from tests.
> + ),
+ ImmutableList.<String>of()
+ ))))
+ .build(),
+ null);
+ vaultUri = vault.properties().vaultUri();
+ assertTrue(!vault.name().isEmpty());
+ }
+
+ @Test(dependsOnMethods = "testCreate")
+ public void testGet() {
+ Vault vaultFound = api().getVault(vaultName);
+ assertTrue(!vaultFound.name().isEmpty());
+ }
+
+ @Test(dependsOnMethods = "testGet")
Better depend on the `testCreate` to maximize the chances of this test being
run.
> + "Backup",
+ "Restore",
+ "Purge"
+ ),
+ ImmutableList.<String>of()
+ ))))
+ .build(),
+ null);
+ vaultUri = vault.properties().vaultUri();
+ assertTrue(!vault.name().isEmpty());
+ }
+
+ @Test(dependsOnMethods = "testCreate")
+ public void testGet() {
+ Vault vaultFound = api().getVault(vaultName);
+ assertTrue(!vaultFound.name().isEmpty());
Better `assertNotNull(vaultFound)` to avoid an unexpected NPE?
> + vaultUri = vault.properties().vaultUri();
+ assertTrue(!vault.name().isEmpty());
+ }
+
+ @Test(dependsOnMethods = "testCreate")
+ public void testGet() {
+ Vault vaultFound = api().getVault(vaultName);
+ assertTrue(!vaultFound.name().isEmpty());
+ }
+
+ @Test(dependsOnMethods = "testGet")
+ public void testList() {
+ for (Vault vault : api().listVaults()) {
+ assertNotNull(vault.name());
+ VaultProperties props = vault.properties();
+ assertNotNull(props);
If properties are not nullable there is no need to check this. Deserialization
will already fail.
> +
+ Map<String, String> tags = new HashMap<String, String>();
+ tags.put("purpose", "testing again");
+ KeyBundle updatedKey = api().updateKey(vaultUri, KEY_NAME, "/" +
version, null, null, tags);
+ assertNotNull(updatedKey);
+
+ keys = api().getKeyVersions(vaultUri, KEY_NAME);
+ assertNotNull(keys);
+ boolean found = false;
+ for (Key k : keys) {
+ if (k.kid().contains(version)) {
+ key = k;
+ found = true;
+ break;
+ }
+ }
style minor:
```java
assertTrue(Iterables.anyMatch(keys, new Predicate<Key>() {
@Override boolean apply(Key input) {
return input.kid().contains(version);
}
}));
```
> + assertEquals(certBundle.tags().size(), 1);
+ }
+
+ @Test(dependsOnMethods = "testUpdateCertificate")
+ public void testUpdateCertificateVersion() {
+ // create a new version of the certificate
+ /*
+ * XXX -- update using version complains about needing policy (required
input), yet
+ * passing in the same policy results in the error:
+ *
+ * Policy cannot be updated with a specific version of a certificate
+ *
+ * Will uncomment/fix once this issue is resolved.
+ *
+ */
+ assertTrue(true);
Better throw a SkipException to see the reason why this is not executed in the
test output. this will also serve as a reminder for us that this needs to be
implemented.
> +
> "A6RII-HF2BkBcVa9KcAX3_di4KQE70PXgHf-dlz_RgLOJILeG50wzFeBFCLsjEEPp3itmoai" +
+
"E6vfDidCRm5At8Vjka0G-N_afwkIijfQZLT0VaXvL39cIJE2QN3HJPZM8YPUlkFlYnY4GIRy" +
+
"RWSBpK_KYuVufzUGtDi6Sh8pUa67ppa7DHVZlixlmnVqI3Oeg6XUvMqbFFqVSrcNbRQDwVGL" +
+ "3cUtK-KB1PfKg";
+ private static String keySignedData =
"uO0r4P1cB-fKsDZ8cj5ahiNw8Tdsudt5zLCeEKOt29" +
+
"LAlPDpeGx9Q1SOFNaR7JlRYVelxsohdzvydwX8ao6MLnqlpdEj0Xt5Aadp-kN84AXW238gab" +
+
"S1AUyiWILCmdsBFeRU4wTRSxz2qGS_0ztHkaNln32P_9GJC72ZRlgZoVA4C_fowZolUoCWGj" +
+
"4V7fAzcSoiNYipWP0HkFe3xmuz-cSQg3CCAs-MclHHfMeSagLJZZQ9bpl5LIr-Ik89bNtqEq" +
+
"yP7Jb_fCgHajAx2lUFcRZhSIKuCfrLPMl6wzejQ2rQXX-ixEkDa73dYaPIrVW4IL3iC0Ufxn" +
+ "fxYffHJ7QCRw";
+ private static String keyWrappedData =
"1jcTlu3KJNDBYydhaH9POWOo0tAPGkpsZVizCkHpC" +
+
"3g_9Kg91Q3HKK-rfZynn5W5nVPM-SVFHA3JTankcXX8gx8GycwUh4pMoyil_DV35m2QjyuiT" +
+
"ln83OJXw-nMvRXyKdVfF7nyRcs256kW7gthAOsYUVBrfFS7DFFxsXqLNREsA8j85IqIXIm8p" +
+
"AB3C9uvl1I7SQhLvrwZZXXqjeCWMfseVJwWgsQFyyqH2P0f3-xnngV7cvik2k3Elrk3G_2Cu" +
+
"JCozIIrANg9zG9Z8DrwSNNm9YooxWkSu0ZeDLOJ0bMdhcPGGm5OvKz3oZqX-39yv5klNlCRb" +
+ "r0q7gqmI0x25w";
Consider moving all these huge strings and the ones in the live test to files
in `src/test/resources` for better readability.
> +
> "4V7fAzcSoiNYipWP0HkFe3xmuz-cSQg3CCAs-MclHHfMeSagLJZZQ9bpl5LIr-Ik89bNtqEq" +
+
"yP7Jb_fCgHajAx2lUFcRZhSIKuCfrLPMl6wzejQ2rQXX-ixEkDa73dYaPIrVW4IL3iC0Ufxn" +
+ "fxYffHJ7QCRw";
+ private static String keyWrappedData =
"1jcTlu3KJNDBYydhaH9POWOo0tAPGkpsZVizCkHpC" +
+
"3g_9Kg91Q3HKK-rfZynn5W5nVPM-SVFHA3JTankcXX8gx8GycwUh4pMoyil_DV35m2QjyuiT" +
+
"ln83OJXw-nMvRXyKdVfF7nyRcs256kW7gthAOsYUVBrfFS7DFFxsXqLNREsA8j85IqIXIm8p" +
+
"AB3C9uvl1I7SQhLvrwZZXXqjeCWMfseVJwWgsQFyyqH2P0f3-xnngV7cvik2k3Elrk3G_2Cu" +
+
"JCozIIrANg9zG9Z8DrwSNNm9YooxWkSu0ZeDLOJ0bMdhcPGGm5OvKz3oZqX-39yv5klNlCRb" +
+ "r0q7gqmI0x25w";
+
+ @BeforeMethod
+ public void start() throws IOException {
+ super.start();
+ try {
+ vaultUri = server.getUrl("").toURI();
+ } catch (URISyntaxException use) {
Better add the exception to the method signature instead of silently catching
it? This way if it is thrown tests will be skipped instead of run with a `null`
URI.
> + "Delete",
+ "Recover",
+ "Backup",
+ "Restore",
+ "Purge"
+ ),
+ ImmutableList.<String>of()
+ ))))
+ .build(),
+ null);
+
+ String path = String.format(
+
"/subscriptions/%s/resourcegroups/%s/providers/Microsoft.KeyVault/vaults/%s?%s",
+ subscriptionId, resourceGroup, vaultName, apiVersion
+ );
+ assertSent(server, "PUT", path);
I know this can be kind of a pain, but we need to assert the sent payload too
for all requests that have a body. This *really* helps future maintenance of
the different APIs.
> + ))))
+ .build(),
+ null);
+
+ String path = String.format(
+
"/subscriptions/%s/resourcegroups/%s/providers/Microsoft.KeyVault/vaults/%s?%s",
+ subscriptionId, resourceGroup, vaultName, apiVersion
+ );
+ assertSent(server, "PUT", path);
+
+ assertNotNull(vault);
+ assertNotNull(vault.properties().vaultUri());
+ assertTrue(!vault.name().isEmpty());
+ }
+
+ public void createVaultReturns404() throws InterruptedException {
There is no need to test the 404 behavior if there is no custom `@Fallback`
defined, so you can remove all the 404 tests for those methods that do not have
fallbacks. The default behavior (throwing the exception) is already tested in
jclouds-core.
> +
> "/subscriptions/%s/providers/Microsoft.KeyVault/locations/%s/deletedVaults/%s?%s",
+ subscriptionId, location, vaultName, apiVersion
+ );
+ assertSent(server, "GET", path);
+
+ assertNull(vault);
+ }
+
+
+ // Key mock tests
+ public void listKeys() throws InterruptedException {
+ server.enqueue(jsonResponse("/vaultlistkeys.json").setResponseCode(200));
+ final VaultApi vaultApi = api.getVaultApi(resourceGroup);
+ List<Key> keys = vaultApi.listKeys(vaultUri);
+
+ String path = String.format("/keys?%s", apiVersion);
Same comment about skipping the test.
> @@ -99,6 +100,7 @@ public void setup() {
vaultResourceGroup =
System.getProperty("test.azurecompute-arm.vault.resource.group");
vaultName = System.getProperty("test.azurecompute-arm.vault.name");
vaultCertificateUrl =
System.getProperty("test.azurecompute-arm.vault.certificate.url");
+ tenantId = System.getProperty("test.azurecompute-arm.tenantId");
Change to: `tenantId = injector.getInstance(Key.get(String.class,
Tenant.class));`
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/416#pullrequestreview-80448092
