Add ceritificateManagerCertificates field to ComputeTargetHttpsProxy resource

[Hamzawy63]

Add new field certificateManagerCertificates in the resource compute_target_https_proxy.

Context:
The resource ComputeTargetHttpsProxy has a field called sslCertificates, this field used to reference only ssl certificates. Recently, certificates of type CertificateManagerCertificates has been allowed. However, either all the items of the sslCertificates array will be sslCertificates or certificate manager certificates.

Furthermore, the field in TF couldn't accept certificate manager certificates because of a custom_expand function that only validates compute certificates (sslcertificates). A solution by @DanielRieske (#8941) that should enable using the field with both types of certificates.

The solution suggested here is to use a new field certificate_manager_certificates, that should be used with the customer wants to reference a certificate manager certificate resources. Since this field doesn't exist in the API, I used encoder/decoder to change the the API request/response as needed.

[+] This is consistent with gcloud behaviour, as the customer enters the certificate manager certificates in a new field.
[+] Since it's not allowed to use mixed type of certificates in the array, it might make more sense to use different fields that are mutual exclusive.
[-] Adding a new field isn't consistent with the API definition of computeTargetHttpsProxy

We will confirm with compute team which solution is desired.

Fixes: hashicorp/terraform-provider-google#15805

Release Note Template for Downstream PRs (will be copied)

Add new field `certificateManagerCertificates` in the resource `ComputeTargetHttpsProxy`

[modular-magician]

Hello! I am a robot. It looks like you are a: Community Contributor Googler Core Contributor. Tests will require approval to run.

@roaks3, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 5 files changed, 292 insertions(+), 3 deletions(-))
Terraform Beta: Diff ( 5 files changed, 292 insertions(+), 3 deletions(-))
TF Conversion: Diff ( 1 file changed, 44 insertions(+))
TF OiCS: Diff ( 4 files changed, 147 insertions(+))

[roaks3]

Overall looks good, just a few questions

[roaks3]

Is this necessary? I'm not sure I understand how it is being used

[Hamzawy63]

Do you mean the diff_suppress_func or something else?

The API expects each item to be in full URL format (e.g. //certificatemanager.googleapis.com/projects/../locations/....
However, I allowed the users to only provide the Id, so customers can just provide each reference on the format of projects/../locations/../certficates/.., and I construct the full URL before sending the API request using the custom_expand function.
So, we end up having the following:

• user input -> projects/../locations/../certificates/..
• API response -> //certificatemanager.googleapis.com/projects/../locations/.../..certificates/..

When I run the test, TF triggers an update request(because the field value has changed) and that causes the test to fail.

Is there a better way to handle this situation?

[Hamzawy63]

One more question, Are there any issues with allowing the user to provide the id and then convert it to a full URL before sending the request?

According to rileykarson's comment here #8941 (comment), it's not encouraged to let the user enter the name of the resource unless the API allows doing this, I wonder if it's also the same with letting the user provide an id if the API is expecting a full URL format.

[roaks3]

Yes, I'm talking about the diff_suppress_func. I understand that you are allowing the user to provide an id (which I believe is fine here), and that needs to be converted before being sent to the API, but isn't this all being handled by the expand/encoder/decoder?

I may just be missing something, but I'm wondering what caused the test failure. And for context, I'm partly asking about the diff suppress because I believe the implementation doesn't consider all parts of the id (like location), and that may cause problems.

[Hamzawy63]

1- The encoder and the decoder parts are responsible for merging or splitting the fields sslCertificates and certificate_manager_certificates before constructing the API request and after receiving the response respectively. They have nothing to do with the format of the input provided by the user.

2- The custom expand is responsible for making sure that any certificate manager certificate resource is sent in the API request in the format of //certificatemanager.googleapis.com/projects/../locations/.../..certificates/... So, if the user uses the id to reference the certificate, the custom expand will convert it before sending the API request.

The response that we receive from the API will contain the certificates in the full URL format (//certificatemanager.googleapis.com/projects/../locations/.../..certificates/..). Hence if the user provides only the id, there will be a difference between the input provided by the user and the response from the API.

Error Message:

2023-10-06T18:46:25.436Z [ERROR] sdk.helper_resource: Unexpected error:
  error=
  | After applying this test step, the plan was not empty.
  | stdout:
  | 
  | 
  | Terraform used the selected providers to generate the following execution
  | plan. Resource actions are indicated with the following symbols:
  |   ~ update in-place
  | 
  | Terraform will perform the following actions:
  | 
  |   # google_compute_target_https_proxy.default will be updated in-place
  |   ~ resource "google_compute_target_https_proxy" "default" {
  |       ~ certificate_manager_certificates = [
  |           - "//certificatemanager.googleapis.com/projects/hamzahassan/locations/global/certificates/tf-test-my-certificate4di3je7hnx",
  |           + "projects/hamzahassan/locations/global/certificates/tf-test-my-certificate4di3je7hnx",
  |         ]
  |         id                               = "projects/hamzahassan/global/targetHttpsProxies/tf-test-target-http-proxy4di3je7hnx"
  |         name                             = "tf-test-target-http-proxy4di3je7hnx"
  |         # (8 unchanged attributes hidden)
  |     }
  | 
  | Plan: 0 to add, 1 to change, 0 to destroy.

• This error only happens if the user provides id to reference a certificate resource

[roaks3]

Got it, that all makes sense, thanks for clarifying. Despite my concern with the other parts of the id diffing (like location and project), it appears this same function is used in many other fields with similar structure, so seems good to me.

[modular-magician]

Tests analytics

Total tests: 3120
Passed tests 2799
Skipped tests: 318
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccComputeTargetHttpsProxy_targetHttpsProxyCertificateManagerCertificateExample|TestAccDataprocClusterIamPolicy|TestAccWorkstationsWorkstationConfig_update

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccComputeTargetHttpsProxy_targetHttpsProxyCertificateManagerCertificateExample[Debug log]
TestAccDataprocClusterIamPolicy[Debug log]
TestAccWorkstationsWorkstationConfig_update[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 5 files changed, 293 insertions(+), 3 deletions(-))
Terraform Beta: Diff ( 5 files changed, 293 insertions(+), 3 deletions(-))
TF Conversion: Diff ( 1 file changed, 47 insertions(+))
TF OiCS: Diff ( 4 files changed, 146 insertions(+))

[modular-magician]

Tests analytics

Total tests: 3126
Passed tests 2801
Skipped tests: 318
Affected tests: 7

Action taken

Found 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccAlloydbUser_updateRoles_IAM|TestAccAlloydbUser_updatePassword_BuiltIn|TestAccAlloydbUser_updateRoles_BuiltIn|TestAccAlloydbUser_alloydbUserIamExample|TestAccAlloydbUser_alloydbUserBuiltinExample|TestAccDataSourceGoogleServiceAccountJwt|TestAccDataSourceGoogleServiceAccountAccessToken_basic

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataSourceGoogleServiceAccountJwt[Debug log]
TestAccDataSourceGoogleServiceAccountAccessToken_basic[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccAlloydbUser_updateRoles_IAM[Error message] [Debug log]
TestAccAlloydbUser_updatePassword_BuiltIn[Error message] [Debug log]
TestAccAlloydbUser_updateRoles_BuiltIn[Error message] [Debug log]
TestAccAlloydbUser_alloydbUserIamExample[Error message] [Debug log]
TestAccAlloydbUser_alloydbUserBuiltinExample[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test