jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] Add Azure KeyVault support -- vault management and certificate/key/secret operations (#416)
Date Mon, 06 Nov 2017 14:29:12 GMT
nacx commented on this pull request.

Thanks @jmspring! Huge effort!

I'm adding some general comments after a first review, but I still have to go in deeper detail
through the entire PR again:

* In the model classes, enforce immutable lists.
* Also try to use non-null collections and enforce presence where possible (`tags` are an
example of field where null is OK, but in most cases I'd say it should be safe to enforce
presence in collections).
* Use the `Date` type for fields that represent dates.
* In the API methods, remove all 404 fallbacks from PUT/POST/PATCH methods.
* Consider removing the TrueIf2xx class if jclouds already provides that behavior by default.

Regarding the KeyVaultFilter. It looks like a clone of the `ClientCredentialsSecretFlow` filter.
Instead of overriding it to cover the MWS use case, couldn't we configure the tests to use
"localhost" for the vaultURL so the default OAuthFilter can be used? What are the limitations
that made you change this?

Thanks for all the effort!

> @@ -411,7 +411,7 @@ private OSProfile createOsProfile(String computerName, Template template)
{
 
    private List<NetworkInterfaceCard> createNetworkInterfaceCards(final String nodeName,
final String location,
          AzureTemplateOptions options) {
-      // Prefer a sorted list of NICs with the ones with public IPs first, to
+      // Prefer a sorted listVaults of NICs with the ones with public IPs first, to

typo?

> +      }
+   }
+
+   @AutoValue
+   public abstract static class SubjectAlternativeNames {
+      public abstract List<String> dnsNames();
+
+      public abstract List<String> emails();
+
+      public abstract List<String> upns();
+
+      @SerializedNames({"dns_names", "emails", "upns"})
+      public static SubjectAlternativeNames create(final List<String> dnsNames,
+                                                   final List<String> emails,
+                                                   final List<String> upns) {
+         return new AutoValue_Certificate_SubjectAlternativeNames(dnsNames, emails, upns);

Enforce immutability

> +
+      @Nullable
+      public abstract SubjectAlternativeNames subjectAltNames();
+
+      @Nullable
+      public abstract String subject();
+
+      public abstract int validityMonths();
+
+      @SerializedNames({"ekus", "key_usage", "sans", "subject", "validity_months"})
+      public static X509CertificateProperties create(final List<String> enhancedKeyUsage,
+                                                     final List<String> keyUsage,
+                                                     final SubjectAlternativeNames subjectAltNames,
+                                                     final String subject,
+                                                     final int validityMonths) {
+         return new AutoValue_Certificate_X509CertificateProperties(enhancedKeyUsage, keyUsage,
subjectAltNames, subject, validityMonths);

Enforce immutability in all lists when present. Is there a reason to keep these lists null,
or could we better default to empty lists, to avoid the null-collection anti-pattern?

> +      @SerializedNames({"id", "provider"})
+      public static CertificateIssuer create(final String id,
+                                             final String provider) {
+         return new AutoValue_Certificate_CertificateIssuer(id, provider);
+      }
+   }
+
+   @AutoValue
+   public abstract static class IssuerAttributes {
+      @Nullable
+      public abstract String created();
+
+      public abstract boolean enabled();
+
+      @Nullable
+      public abstract String updated();

Can we use a `Date` type for the created and updated fields?

> +      public abstract String id();
+
+      @SerializedNames({"contacts", "id"})
+      public static Contacts create(final List<Contact> contacts,
+                                    final String id) {
+         return new AutoValue_Certificate_Contacts(contacts, id);
+      }
+   }
+
+   @AutoValue
+   public abstract static class DeletedCertificateBundle {
+      @Nullable
+      public abstract CertificateAttributes attributes();
+
+      @Nullable
+      public abstract String bytes();

[minor] The method name suggests a return type of `byte[]`, but let's keep it as-is if a string
really makes more sense given the content of the field.

> +      public static Contacts create(final List<Contact> contacts,
+                                    final String id) {
+         return new AutoValue_Certificate_Contacts(contacts, id);
+      }
+   }
+
+   @AutoValue
+   public abstract static class DeletedCertificateBundle {
+      @Nullable
+      public abstract CertificateAttributes attributes();
+
+      @Nullable
+      public abstract String bytes();
+
+      @Nullable
+      public abstract String deletedDate();

Change type to `Date`?

> +                 thumbprint
+         );
+      }
+   }
+
+   @Nullable
+   public abstract CertificateAttributes attributes();
+
+   @Nullable
+   public abstract String id();
+
+   @Nullable
+   public abstract Map<String, String> tags();
+
+   @Nullable
+   public abstract String x5t();

Why not "thumbprint" as in other classes?

> +   }
+
+   @AutoValue
+   public abstract static class KeyAttributes {
+      @Nullable
+      public abstract Boolean enabled();
+      @Nullable
+      public abstract String created();
+      @Nullable
+      public abstract String expires();
+      @Nullable
+      public abstract String notBefore();
+      @Nullable
+      public abstract String recoveryLevel();
+      @Nullable
+      public abstract String updated();

Use `Date` types where possible.

> +public interface VaultApi {
+   // Vault operations
+   @Named("vault:list")
+   @SelectJson("value")
+   @GET
+   @Path("/resourcegroups/{resourcegroup}/providers/Microsoft.KeyVault/vaults")
+   @Fallback(EmptyListOnNotFoundOr404.class)
+   List<Vault> listVaults();
+
+   @Named("vault:create_or_update")
+   @PUT
+   @MapBinder(BindToJsonPayload.class)
+   @Fallback(NullOnNotFoundOr404.class)
+   @Path("/resourcegroups/{resourcegroup}/providers/Microsoft.KeyVault/vaults/{vaultName}")
+   Vault createOrUpdateVault(@PathParam("vaultName") String vaultName, @PayloadParam("location")
String location,
+         @PayloadParam("properties") VaultProperties properties);

Add a `tags` parameter after the "location" one, for consistency with other APIs.

> +
+@RequestFilters({ OAuthFilter.class, ApiVersionFilter.class })
+@Consumes(MediaType.APPLICATION_JSON)
+public interface VaultApi {
+   // Vault operations
+   @Named("vault:list")
+   @SelectJson("value")
+   @GET
+   @Path("/resourcegroups/{resourcegroup}/providers/Microsoft.KeyVault/vaults")
+   @Fallback(EmptyListOnNotFoundOr404.class)
+   List<Vault> listVaults();
+
+   @Named("vault:create_or_update")
+   @PUT
+   @MapBinder(BindToJsonPayload.class)
+   @Fallback(NullOnNotFoundOr404.class)

Do not use 404 fallbacks in write (PUT/POST) operations. An unexisting path should be very
unexpected in this case and throw an exception (as opposed to get/delete, where you might
expect the resource to not exist and null is fine as a return value).

> +   @Path("/providers/Microsoft.KeyVault/deletedVaults")
+   @GET
+   @SelectJson("value")
+   @Fallback(EmptyListOnNotFoundOr404.class)
+   List<DeletedVault> listDeletedVaults();
+
+   @Named("vault:get_deleted")
+   @GET
+   @Path("/providers/Microsoft.KeyVault/locations/{location}/deletedVaults/{vaultName}")
+   @Fallback(NullOnNotFoundOr404.class)
+   DeletedVault getDeletedVault(@PathParam("location") String location, @PathParam("vaultName")
String vaultName);
+
+   @Named("vault:purge")
+   @POST
+   @ResponseParser(TrueOn20X.class)
+   @Fallback(FalseOnNotFoundOr404.class)

Remove fallback

> +import com.google.common.base.Supplier;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.jclouds.Constants.PROPERTY_SESSION_INTERVAL;
+import static org.jclouds.oauth.v2.config.OAuthProperties.RESOURCE;
+
+/**
+ * Allow users to customize the api versions for each method call.
+ * <p>
+ * In Azure ARM, each method may have its own api version. This filter allows to
+ * configure the versions of each method, so there is no need to change the code
+ * when Azure deprecates old versions.
+ */

Wrong javadoc

> +   @MapBinder(BindToJsonPayload.class)
+   @Fallback(NullOnNotFoundOr404.class)
+   @Path("/resourcegroups/{resourcegroup}/providers/Microsoft.KeyVault/vaults/{vaultName}")
+   Vault createOrUpdateVault(@PathParam("vaultName") String vaultName, @PayloadParam("location")
String location,
+         @PayloadParam("properties") VaultProperties properties);
+
+   @Named("vault:get")
+   @Path("/resourcegroups/{resourcegroup}/providers/Microsoft.KeyVault/vaults/{vaultName}")
+   @GET
+   @Fallback(NullOnNotFoundOr404.class)
+   Vault getVault(@PathParam("vaultName") String vaultName);
+
+   @Named("vault:delete")
+   @Path("/resourcegroups/{resourcegroup}/providers/Microsoft.KeyVault/vaults/{vaultName}")
+   @DELETE
+   @ResponseParser(TrueOn20X.class)

Is this needed? Doesn't jclouds return true for 2xx by default when the return type is a boolean?
If so, let's remove tis parser 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-74423778
Mime
View raw message