Private ca ga #4919

slevenick · 2021-06-25T00:31:56Z

Fixes: hashicorp/terraform-provider-google#8979

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

google_privateca_certificate

google_privateca_certificate_authority

google_privateca_ca_pool

privateca: existing beta resources will no longer function

modular-magician · 2021-06-25T00:42:05Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 17 files changed, 9137 insertions(+), 234 deletions(-))
Terraform Beta: Diff ( 20 files changed, 6023 insertions(+), 1359 deletions(-))
TF Conversion: Diff ( 7 files changed, 2691 insertions(+))
TF OiCS: Diff ( 11 files changed, 239 insertions(+), 59 deletions(-))

modular-magician · 2021-06-25T21:55:30Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 18 files changed, 9376 insertions(+), 234 deletions(-))
Terraform Beta: Diff ( 20 files changed, 6261 insertions(+), 1358 deletions(-))
TF Conversion: Diff ( 7 files changed, 2691 insertions(+))
TF OiCS: Diff ( 11 files changed, 239 insertions(+), 59 deletions(-))

modular-magician · 2021-06-25T23:34:14Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccPrivatecaCaPoolIamBindingGenerated|TestAccPrivatecaCaPoolIamMemberGenerated|TestAccPrivatecaCaPoolIamPolicyGenerated|TestAccPrivatecaCaPool_privatecaCapoolUpdate|TestAccCloudbuildWorkerPool_withNetwork|TestAccContainerNodePool_withGPU|TestAccEventarcTrigger_basic|TestAccEventarcTrigger_transport You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=193745

modular-magician · 2021-06-25T23:53:34Z

Tests failed during RECORDING mode: TestAccPrivatecaCaPoolIamPolicyGenerated|TestAccPrivatecaCaPoolIamMemberGenerated|TestAccPrivatecaCaPoolIamBindingGenerated|TestAccCloudbuildWorkerPool_withNetwork|TestAccContainerNodePool_withGPU Please fix these to complete your PR

modular-magician · 2021-06-28T16:23:49Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 18 files changed, 9376 insertions(+), 234 deletions(-))
Terraform Beta: Diff ( 21 files changed, 6262 insertions(+), 1359 deletions(-))
TF Conversion: Diff ( 7 files changed, 2691 insertions(+))
TF OiCS: Diff ( 11 files changed, 239 insertions(+), 59 deletions(-))

modular-magician · 2021-06-28T18:01:07Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccPrivatecaCaPoolIamBindingGenerated|TestAccPrivatecaCaPoolIamMemberGenerated|TestAccPrivatecaCaPoolIamPolicyGenerated|TestAccCloudbuildWorkerPool_withNetwork|TestAccContainerNodePool_withGPU|TestAccPrivatecaCaPool_privatecaCapoolAllFieldsExample|TestAccPrivatecaCaPool_privatecaCapoolBasicExample|TestAccTags You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=194075

modular-magician · 2021-06-28T18:23:00Z

Tests failed during RECORDING mode: TestAccContainerNodePool_withGPU Please fix these to complete your PR

rileykarson

I haven't run through the fields to see how well the tests cover them, but I've done a general pass on the resource. Interim comments inline, none of them big.

rileykarson · 2021-06-28T20:42:28Z

mmv1/products/privateca/api.yaml

+                description: |
+                  Describes values that are relevant in a CA certificate.
+                properties:
+                  - !ruby/object:Api::Type::Boolean


We can't AtLeastOneOf these, right? Both expect to be able to be omitted?

We could add Required on is_ca in all cases I think. It would only be settable to false in most cases, except when creating a CertificateAuthority, but that is due to API validation

And setting false explicitly is the same to the API as not sending the value at all

rileykarson · 2021-06-28T20:43:00Z

mmv1/products/privateca/api.yaml

+              - !ruby/object:Api::Type::NestedObject
+                name: 'keyUsage'
+                description: |
+                  Optional. Indicates the intended use for keys that correspond to a certificate.


Suggested change

Optional. Indicates the intended use for keys that correspond to a certificate.

Indicates the intended use for keys that correspond to a certificate.

nit: Implied by the field being optional

rileykarson · 2021-06-28T20:43:34Z

mmv1/products/privateca/api.yaml

+                name: 'keyUsage'
+                description: |
+                  Optional. Indicates the intended use for keys that correspond to a certificate.
+                properties:


Are these subobjects not required when the parent is set? Same w/ their subfields

Not technically, but due to API behavior not returning these when all sub-values are set to false I have added a custom flattener & marked all of these subobjects as required.

I think this is a reasonable way to handle the tricky situation where these fields have slightly different behaviors between the 3 PrivateCA resources although the structure of the objects is almost exactly identical. We could also look into customizediffs, or not allowing objects with all-false values to prevent this in the future without marking these as required

Well, some of them are actually required depending on the resource.

For example, caOptions.isCa must be set to true for a CertificateAuthority resource.

mmv1/products/privateca/api.yaml

mmv1/products/privateca/terraform.yaml

mmv1/third_party/terraform/tests/privateca_capool_test.go

modular-magician · 2021-06-29T19:16:37Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 19 files changed, 9119 insertions(+), 233 deletions(-))
Terraform Beta: Diff ( 22 files changed, 5901 insertions(+), 1254 deletions(-))
TF Conversion: Diff ( 7 files changed, 2691 insertions(+))
TF OiCS: Diff ( 11 files changed, 244 insertions(+), 59 deletions(-))

modular-magician · 2021-06-29T19:50:09Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 19 files changed, 9127 insertions(+), 233 deletions(-))
Terraform Beta: Diff ( 22 files changed, 5909 insertions(+), 1254 deletions(-))
TF Conversion: Diff ( 7 files changed, 2691 insertions(+))
TF OiCS: Diff ( 11 files changed, 244 insertions(+), 59 deletions(-))

modular-magician · 2021-06-29T20:50:19Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccContainerNodePool_withGPU|TestAccPrivatecaCaPool_privatecaCapoolEmptyBaseline|TestAccPrivatecaCertificate_privatecaCertificateNoAuthorityExample|TestAccPrivatecaCertificate_privatecaCertificateConfigExample|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthorityBasicExample|TestAccPrivatecaCertificate_privatecaCertificateCsrExample You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=194301

modular-magician · 2021-06-29T21:09:08Z

Tests failed during RECORDING mode: TestAccPrivatecaCertificate_privatecaCertificateNoAuthorityExample|TestAccContainerNodePool_withGPU Please fix these to complete your PR

modular-magician · 2021-06-29T21:21:46Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 19 files changed, 9207 insertions(+), 233 deletions(-))
Terraform Beta: Diff ( 22 files changed, 5990 insertions(+), 1254 deletions(-))
TF Conversion: Diff ( 7 files changed, 2691 insertions(+))
TF OiCS: Diff ( 11 files changed, 244 insertions(+), 59 deletions(-))

modular-magician · 2021-06-29T21:26:44Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccContainerNodePool_withGPU|TestAccPrivatecaCaPool_privatecaCapoolEmptyBaseline|TestAccPrivatecaCertificate_privatecaCertificateNoAuthorityExample|TestAccPrivatecaCertificate_privatecaCertificateConfigExample|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthorityBasicExample|TestAccPrivatecaCaPool_privatecaCapoolUpdate|TestAccPrivatecaCertificate_privatecaCertificateCsrExample You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=194303

modular-magician · 2021-06-29T21:41:22Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 19 files changed, 9211 insertions(+), 233 deletions(-))
Terraform Beta: Diff ( 23 files changed, 5995 insertions(+), 1255 deletions(-))
TF Conversion: Diff ( 7 files changed, 2691 insertions(+))
TF OiCS: Diff ( 11 files changed, 244 insertions(+), 59 deletions(-))

modular-magician · 2021-06-29T21:50:08Z

Tests failed during RECORDING mode: TestAccPrivatecaCertificate_privatecaCertificateNoAuthorityExample|TestAccContainerNodePool_withGPU Please fix these to complete your PR

rileykarson

Did my best at cross-referencing the fields w/ the tests, but really only did a good job @ the first one. Three resources is a lot and +- 2k is pretty hefty! Couple extra comments.

mmv1/third_party/terraform/utils/privateca_utils.go

mmv1/products/privateca/api.yaml

modular-magician · 2021-06-29T22:59:12Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 19 files changed, 9226 insertions(+), 314 deletions(-))
Terraform Beta: Diff ( 23 files changed, 6123 insertions(+), 1449 deletions(-))
TF Conversion: Diff ( 7 files changed, 2691 insertions(+))
TF OiCS: Diff ( 11 files changed, 244 insertions(+), 59 deletions(-))

modular-magician · 2021-06-29T23:18:22Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccContainerNodePool_withGPU|TestAccPrivatecaCertificate_privatecaCertificateNoAuthorityExample|TestAccPrivatecaCaPool_privatecaCapoolUpdate You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=194306

modular-magician · 2021-06-29T23:42:43Z

Tests failed during RECORDING mode: TestAccContainerNodePool_withGPU Please fix these to complete your PR

modular-magician · 2021-06-30T00:33:16Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccContainerNodePool_withGPU|TestAccPrivatecaCertificate_privatecaCertificateNoAuthorityExample|TestAccPrivatecaCaPool_privatecaCapoolUpdate You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=194399

slevenick · 2021-06-30T17:58:31Z

PrivateCA tests pass in VCR!

pmansour

thanks for putting this together! sorry for the delay, but I left a bunch of comments

mmv1/products/privateca/terraform.yaml

pmansour · 2021-06-30T00:34:32Z

mmv1/products/privateca/terraform.yaml

-      encoder: templates/terraform/encoders/certificate_authority.go.erb
-      pre_delete: templates/terraform/pre_delete/privateca_certificate_authority.go.erb
+      decoder: templates/terraform/decoders/treat_deleted_state_as_gone.go.erb
+      post_create: templates/terraform/post_create/privateca_authority_enable.go.erb


not sure if this is always going to be a good thing, since CAs are (by default) created in the STAGED state to allow for pre-production validation, and it's not possible to get back to this state after enabling a CA.

Can this auto-enablement behavior be gated by a flag, so that customers who want to keep it STAGED can do so?

That is a little complicated as it would require manual steps outside of Terraform to enable the CA, or somewhat complicated state management within Terraform itself. It should be possible in the future to enable this via Terraform, but I believe this is a reasonable default behavior at the moment

pmansour · 2021-06-30T00:36:25Z

mmv1/products/privateca/terraform.yaml

    examples:
      - !ruby/object:Provider::Terraform::Examples
        name: "privateca_certificate_config"
-        min_version: "beta"
        primary_resource_id: "default"
        vars:
          certificate_authority_id: "my-certificate-authority" 


in the new v1 resource model it's uncommon to require a CA ID for a certificate, since certificates are nested directly under CaPool resources (not CAs) and the idea is to decouple certs from their specific issuing CAs.

This field isn't required, and there is a test that tests this behavior. I think this fits with the new model by allowing the option

pmansour · 2021-07-01T00:11:20Z

mmv1/products/privateca/api.yaml

+        description: The name of the CaPool this Certificate Authority belongs to.
+        required: true
+        input: true
+        url_param_only: true
      - !ruby/object:Api::Type::String
        name: 'name'
        description: |


(misplaced comment, should be on line 110)

'tier' is no longer an input field on a CA. it gets set on the CaPool resource and the service copies it to an OUTPUT_ONLY field on each CA within that pool.

pmansour · 2021-07-01T00:16:27Z

mmv1/products/privateca/api.yaml

              description: |
-                Types of public keys that are supported. At a minimum, we support RSA and ECDSA, for the key sizes or curves listed: https://cloud.google.com/kms/docs/algorithms#asymmetric_signing_algorithms
+                Types of public keys that are supported. At a minimum, we support RSA, for the key sizes or curves listed: https://cloud.google.com/kms/docs/algorithms#asymmetric_signing_algorithms


perhaps a more accurate description is "the key formats that are supported".

also, FWIW, we still support ECDSA keys. do you know if the "ECDSA" part was dropped from the API or here?

(and same for publicKey.format below)

It doesn't look present in the REST API docs, which is why I removed it here

mmv1/templates/terraform/pre_delete/privateca_authority_disable.go.erb

pmansour · 2021-07-01T00:29:58Z

mmv1/templates/terraform/post_create/privateca_authority_enable.go.erb

+
+	log.Printf("[DEBUG] Enabling CertificateAuthority: %#v", obj)
+
+	res, err = sendRequest(config, "POST", billingProject, url, userAgent, nil)


this returns a LongRunningOperation object and doesn't always finish immediately. Do you need to await these operations anywhere before doing something else?

mmv1/third_party/terraform/tests/resource_privateca_ca_pool_test.go

google-cla bot added the cla: yes label Jun 25, 2021

slevenick requested review from rileykarson and removed request for rileykarson June 28, 2021 18:32

rileykarson reviewed Jun 28, 2021

View reviewed changes

slevenick added 12 commits June 28, 2021 15:08

first pass

1192103

Adding new fields

794d5d5

Add all fields test

a9303c5

Capool seems to work

106e523

adding example

352610f

Certificate tests

e3a6d98

Enable/disable authority

e048c28

Finish privateca, tests seem to pass

01b77ba

Add update to capool, PATCH and update_mask

e27fda0

Fix IAM tests

ba831ee

Maybe adding send empty

4679435

Add flattener, required on x509 config sub-fields with boolean values

e249f39

slevenick force-pushed the private-ca-ga branch from f83f83e to e249f39 Compare June 29, 2021 19:06

PR feedback

1141896

Add CA to certificate no authority test

0a60321

Fix noca test

f9b6ed7

slevenick requested a review from rileykarson June 29, 2021 21:50

rileykarson reviewed Jun 29, 2021

View reviewed changes

mmv1/third_party/terraform/utils/privateca_utils.go Show resolved Hide resolved

mmv1/products/privateca/api.yaml Show resolved Hide resolved

Add comments, PR feedback mark fields as output-only

7b2c8e9

slevenick requested a review from rileykarson June 30, 2021 17:58

rileykarson approved these changes Jun 30, 2021

View reviewed changes

slevenick merged commit 644f1a3 into GoogleCloudPlatform:master Jun 30, 2021

pmansour reviewed Jul 1, 2021

View reviewed changes

slevenick mentioned this pull request Jul 1, 2021

PrivateCA PR feedback #4939

Merged

5 tasks

	Optional. Indicates the intended use for keys that correspond to a certificate.
	Indicates the intended use for keys that correspond to a certificate.


		log.Printf("[DEBUG] Enabling CertificateAuthority: %#v", obj)

		res, err = sendRequest(config, "POST", billingProject, url, userAgent, nil)

Private ca ga #4919

Private ca ga #4919

Conversation

slevenick commented Jun 25, 2021

modular-magician commented Jun 25, 2021

Diff report:

modular-magician commented Jun 25, 2021

Diff report:

modular-magician commented Jun 25, 2021

modular-magician commented Jun 25, 2021

modular-magician commented Jun 28, 2021

Diff report:

modular-magician commented Jun 28, 2021

modular-magician commented Jun 28, 2021

rileykarson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Jun 29, 2021

Diff report:

modular-magician commented Jun 29, 2021

Diff report:

modular-magician commented Jun 29, 2021

modular-magician commented Jun 29, 2021

modular-magician commented Jun 29, 2021

Diff report:

modular-magician commented Jun 29, 2021

modular-magician commented Jun 29, 2021

Diff report:

modular-magician commented Jun 29, 2021

rileykarson left a comment

Choose a reason for hiding this comment

modular-magician commented Jun 29, 2021

Diff report:

modular-magician commented Jun 29, 2021

modular-magician commented Jun 29, 2021

modular-magician commented Jun 30, 2021

slevenick commented Jun 30, 2021

pmansour left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment