Merge pull request GoogleCloudPlatform#1 from gfxcc/rebase_hayden_for…

…_url_param_only

* Use url_param_only instead of virtual field
* Rename new field includeMaxPathLength to includeMaxIssuerPathLength for local consistence

mmv1/api/type.rb

@@ -97,10 +97,6 @@ module Fields
 
       # A pattern that maps expected user input to expected API input.
       attr_reader :pattern
-
-      # Used in the schema to indicate if a change in a field requires
-      # the resource to be destroyed and recreated.
-      attr_reader :force_new
     end
 
     include Fields
@@ -136,7 +132,6 @@ def validate
       check :update_id, type: ::String
       check :fingerprint_name, type: ::String
       check :pattern, type: ::String
-      check :force_new, type: :boolean
 
       check_default_value_property
       check_conflicts

mmv1/products/privateca/api.yaml

@@ -171,6 +171,12 @@ objects:
                     description: |
                       Refers to the "CA" X.509 extension, which is a boolean value. When this value is missing,
                       the extension will be omitted from the CA certificate.
+                  - !ruby/object:Api::Type::Boolean
+                    name: 'includeMaxIssuerPathLength'
+                    url_param_only: true
+                    description: |
+                      Must be set to use the value of max_issuer_path_length, either 0 or greater than 0.
+                      If not set, max_issuer_path_length will be unset.
                   - !ruby/object:Api::Type::Integer
                     name: 'maxIssuerPathLength'
                     description: |
@@ -981,11 +987,23 @@ objects:
               description: |
                 Describes values that are relevant in a CA certificate.
               properties:
+                - !ruby/object:Api::Type::Boolean
+                  name: 'includeIsCa'
+                  url_param_only: true
+                  description: |
+                    Must be set to use the value of is_ca, either true or false. If not set, is_ca will
+                    be unset.
                 - !ruby/object:Api::Type::Boolean
                   name: 'isCa'
                   description: |
                     Refers to the "CA" X.509 extension, which is a boolean value. When this value is missing,
                     the extension will be omitted from the CA certificate.
+                - !ruby/object:Api::Type::Boolean
+                  name: 'includeMaxIssuerPathLength'
+                  url_param_only: true
+                  description: |
+                    Must be set to use the value of max_issuer_path_length, either 0 or greater than 0.
+                    If not set, max_issuer_path_length will be unset.
                 - !ruby/object:Api::Type::Integer
                   name: 'maxIssuerPathLength'
                   description: |
@@ -1408,11 +1426,23 @@ objects:
                 description: |
                   Describes values that are relevant in a CA certificate.
                 properties:
+                  - !ruby/object:Api::Type::Boolean
+                    name: 'includeIsCa'
+                    url_param_only: true
+                    description: |
+                      Must be set to use the value of is_ca, either true or false. If not set, is_ca will
+                      be unset.
                   - !ruby/object:Api::Type::Boolean
                     name: 'isCa'
                     description: |
                       Refers to the "CA" X.509 extension, which is a boolean value. When this value is missing,
                       the extension will be omitted from the CA certificate.
+                  - !ruby/object:Api::Type::Boolean
+                    name: 'includeMaxIssuerPathLength'
+                    url_param_only: true
+                    description: |
+                      Must be set to use the value of max_issuer_path_length, either 0 or greater than 0.
+                      If not set, max_issuer_path_length will be unset.
                   - !ruby/object:Api::Type::Integer
                     name: 'maxIssuerPathLength'
                     description: |

mmv1/products/privateca/terraform.yaml

@@ -54,14 +54,6 @@ overrides: !ruby/object:Overrides::ResourceOverrides
           kms_key_name: 'BootstrapKMSKeyWithPurposeInLocation(t, "ASYMMETRIC_SIGN", "us-central1").CryptoKey.Name'
           pool_name: 'BootstrapSharedCaPoolInLocation(t, "us-central1")'
           pool_location: "\"us-central1\""
-    virtual_fields:
-      - !ruby/object:Api::Type::Boolean
-        name: 'include_max_path_length'
-        default_value: false
-        description: |
-          Must be set to use the value of max_issuer_path_length, either 0 or greater than 0.
-          If not set, max_issuer_path_length will be unset.
-        force_new: true
     properties:
       type: !ruby/object:Overrides::Terraform::PropertyOverride
         description: |
@@ -88,21 +80,6 @@ overrides: !ruby/object:Overrides::ResourceOverrides
 
       ~> **Note:** The Certificate Authority that is referenced by this resource **must** be 
       `tier = "ENTERPRISE"`
-    virtual_fields:
-      - !ruby/object:Api::Type::Boolean
-        name: 'include_is_ca'
-        default_value: false
-        description: |
-          Must be set to use the value of is_ca, either true or false. If not set, is_ca will
-          be unset.
-        force_new: true
-      - !ruby/object:Api::Type::Boolean
-        name: 'include_max_path_length'
-        default_value: false
-        description: |
-          Must be set to use the value of max_issuer_path_length, either 0 or greater than 0.
-          If not set, max_issuer_path_length will be unset.
-        force_new: true
     properties:
       config.x509Config: !ruby/object:Overrides::Terraform::PropertyOverride
         custom_flatten: 'templates/terraform/custom_flatten/privateca_certificate_509_config.go.erb'
@@ -161,19 +138,6 @@ overrides: !ruby/object:Overrides::ResourceOverrides
         custom_expand: 'templates/terraform/custom_expand/privateca_certificate_509_config.go.erb'
       publishingOptions: !ruby/object:Overrides::Terraform::PropertyOverride
         diff_suppress_func: 'emptyOrUnsetBlockDiffSuppress'
-    virtual_fields:
-      - !ruby/object:Api::Type::Boolean
-        name: 'include_is_ca'
-        default_value: false
-        description: |
-          Must be set to use the value of is_ca, either true or false. If not set, is_ca will
-          be unset.
-      - !ruby/object:Api::Type::Boolean
-        name: 'include_max_path_length'
-        default_value: false
-        description: |
-          Must be set to use the value of max_issuer_path_length, either 0 or greater than 0.
-          If not set, max_issuer_path_length will be unset.
     iam_policy: !ruby/object:Api::Resource::IamPolicy
       allowed_iam_role: 'roles/privateca.certificateManager'
       method_name_separator: ':'

mmv1/provider/terraform/virtual_fields.rb

@@ -49,17 +49,12 @@ class VirtualFields < Api::Object
       # The default value for the field (defaults to false)
       attr_reader :default_value
 
-      # Whether or not a change to this field requires the resource to be
-      # destroyed and recreated (defaults to false)
-      attr_reader :force_new
-
       def validate
         super
         check :name, type: String, required: true
         check :description, type: String, required: true
         check :type, type: Class, default: Api::Type::Boolean
         check :default_value, default: false
-        check :force_new, type: :boolean, default: false
       end
     end
   end

mmv1/templates/terraform/examples/privateca_capool_all_fields.tf.erb

@@ -9,8 +9,6 @@ resource "google_privateca_ca_pool" "<%= ctx[:primary_resource_id] %>" {
   labels = {
     foo = "bar"
   }
-  include_is_ca = true
-  include_max_path_length = true
   issuance_policy {
     allowed_key_types {
       elliptic_curve {
@@ -52,7 +50,9 @@ resource "google_privateca_ca_pool" "<%= ctx[:primary_resource_id] %>" {
         object_id_path = [1, 5, 7]
       }
       ca_options {
+        include_is_ca = true
         is_ca = true
+        include_max_issuer_path_length = true
         max_issuer_path_length = 10
       }
       key_usage {

mmv1/templates/terraform/examples/privateca_certificate_authority_subordinate.tf.erb

@@ -4,8 +4,6 @@ resource "google_privateca_certificate_authority" "<%= ctx[:primary_resource_id]
   pool = "<%= ctx[:vars]["pool_name"] %>"
   certificate_authority_id = "<%= ctx[:vars]["certificate_authority_id"] %>"
   location = "<%= ctx[:vars]["pool_location"] %>"
-  # Required to set max_issuer_path_length to 0
-  include_max_path_length = true
   config {
     subject_config {
       subject {
@@ -20,6 +18,7 @@ resource "google_privateca_certificate_authority" "<%= ctx[:primary_resource_id]
       ca_options {
         is_ca = true
         # Force the sub CA to only issue leaf certs
+        include_max_issuer_path_length = true
         max_issuer_path_length = 0
       }
       key_usage {

mmv1/templates/terraform/resource.erb

@@ -114,9 +114,6 @@ func resource<%= resource_name -%>() *schema.Resource {
               Type: <%= tf_type(field) -%>,
               Optional: true,
               Default:  <%= go_literal(field.default_value) -%>,
-              <% if field.force_new -%>
-              ForceNew: true,
-              <% end -%>
             },
 <%        end -%>
 <%      end -%>

mmv1/third_party/terraform/tests/resource_privateca_ca_pool_test.go

@@ -55,8 +55,6 @@ resource "google_privateca_ca_pool" "default" {
   name = "tf-test-my-capool%{random_suffix}"
   location = "us-central1"
   tier = "ENTERPRISE"
-  include_is_ca = true
-  include_max_path_length = true
   publishing_options {
     publish_ca_cert = false
     publish_crl = true
@@ -105,7 +103,9 @@ resource "google_privateca_ca_pool" "default" {
         object_id_path = [1,5,7]
       }
       ca_options {
+	include_is_ca = true
         is_ca = true
+	include_max_issuer_path_length = true
         max_issuer_path_length = 10
       }
       key_usage {
@@ -139,8 +139,6 @@ resource "google_privateca_ca_pool" "default" {
   name = "tf-test-my-capool%{random_suffix}"
   location = "us-central1"
   tier = "ENTERPRISE"
-  include_is_ca = true
-  include_max_path_length = true
   publishing_options {
     publish_ca_cert = true
     publish_crl = true
@@ -189,7 +187,9 @@ resource "google_privateca_ca_pool" "default" {
         object_id_path = [1, 7]
       }
       ca_options {
+	include_is_ca = true
         is_ca = true
+	include_max_issuer_path_length = true
         max_issuer_path_length = 10
       }
       key_usage {

mmv1/third_party/terraform/utils/privateca_utils.go

@@ -1,7 +1,6 @@
 package google
 
 import (
-	"fmt"
 	"strconv"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
@@ -22,52 +21,39 @@ import (
 // Expander utilities
 
 func expandPrivatecaCertificateConfigX509ConfigCaOptions(v interface{}, d TerraformResourceData, config *Config) (interface{}, error) {
-	// Virtual fields to distinguish between unset booleans and booleans set with a default value.
+	// Fields include_is_ca, include_max_issuer_path_length are used to distinguish between 
+	// unset booleans and booleans set with a default value.
 	// Unset is_ca or unset max_issuer_path_length either allow any values for these fields when
 	// used in an issuance policy, or allow the API to use default values when used in a
 	// certificate config. A default value of is_ca=false means that issued certificates cannot
 	// be CA certificates. A default value of max_issuer_path_length=0 means that the CA cannot
 	// issue CA certificates.
-
-	// includeIsCa can be nil for Certificate Authority resources
-	includeIsCa := d.Get("include_is_ca")
-	includePathLength := d.Get("include_max_path_length")
-
 	if v == nil {
-		if includeIsCa != nil && includeIsCa.(bool) {
-			return nil, fmt.Errorf("include_is_ca cannot be set without setting ca_options.is_ca")
-		}
-		if includePathLength.(bool) {
-			return nil, fmt.Errorf("include_max_path_length cannot be set without setting ca_options.max_issuer_path_length")
-		}
-		return v, nil
+		return nil, nil
 	}
 	l := v.([]interface{})
 	if len(l) == 0 || l[0] == nil {
-		if includeIsCa != nil && includeIsCa.(bool) {
-			return nil, fmt.Errorf("include_is_ca cannot be set without setting ca_options.is_ca")
-		}
-		if includePathLength.(bool) {
-			return nil, fmt.Errorf("include_max_path_length cannot be set without setting ca_options.max_issuer_path_length")
-		}
 		return nil, nil
 	}
-
 	raw := l[0]
 	original := raw.(map[string]interface{})
-	transformed := make(map[string]interface{})
 
-	//if original["is_ca"].(bool) && !includeIsCa.(bool) {
-	//	return nil, fmt.Errorf("You must set include_is_ca=true with ca_options.is_ca=true")
-	//}
-	//if original["max_issuer_path_length"].(int) > 0 && !includePathLength.(bool) {
-	//	return nil, fmt.Errorf("You must set include_max_path_length=true with ca_options.max_issuer_path_length>0")
-	//}
+	includeIsCa := false
+	// For resource CertificateAuthority, there is not a field named `include_is_ca`.
+	if original["include_is_ca"] != nil && original["include_is_ca"].(bool) {
+		includeIsCa = true
+	}
+	isCa := original["is_ca"].(bool)
+
+	includePathLength := original["include_max_issuer_path_length"].(bool)
+	max_issuer_path_length := original["max_issuer_path_length"].(int)
 
-	if (includeIsCa != nil && includeIsCa.(bool)) || original["is_ca"].(bool) {
+	transformed := make(map[string]interface{})
+
+	if includeIsCa || isCa {
 		transformed["isCa"] = original["is_ca"]
 	}
-	if includePathLength.(bool) || original["max_issuer_path_length"].(int) > 0 {
+	if includePathLength || max_issuer_path_length > 0 {
 		transformed["maxIssuerPathLength"] = original["max_issuer_path_length"]
 	}
 	return transformed, nil
@@ -326,12 +312,18 @@ func flattenPrivatecaCertificateConfigX509ConfigCaOptions(v interface{}, d *sche
 	val, exists := original["isCa"]
 	transformed["is_ca"] =
 		flattenPrivatecaCertificateConfigX509ConfigCaOptionsIsCa(val, d, config)
-	_ = d.Set("include_is_ca", exists)
+	// CertificateAuthority does not fields `include_is_ca`.
+	// Expecting non-CertificateAuthority resource does not have field `certificate_authority_id` at top level.
+	if exists && d.Get("certificate_authority_id") == nil {
+		transformed["include_is_ca"] = true
+	}
 
 	val, exists = original["maxIssuerPathLength"]
 	transformed["max_issuer_path_length"] =
 		flattenPrivatecaCertificateConfigX509ConfigCaOptionsMaxIssuerPathLength(val, d, config)
-	_ = d.Set("include_max_path_length", exists)
+	if exists {
+		transformed["include_max_issuer_path_length"] = true
+	}
 
 	return []interface{}{transformed}
 }