Cross-regional Internal HTTPS Load Balancer Certificate Manager validation

[DanielRieske]

The resource google_compute_target_https_proxy can reference from the sslCertificates attribute from two different sources.

Primarly you reference compute SSL certifications but in certain edge cases the API also accepts a reference to a certificate in a certificate manager.

In it's current implementation we only accept a resource link from sslCertificate which should be removed to allow references to a certificate in a certificate manager.

See: hashicorp/terraform-provider-google#15805 (comment)

Closes: hashicorp/terraform-provider-google#15805

Release Note Template for Downstream PRs (will be copied)

compute: removed validation on `sslCertificates` in `google_compute_https_target_proxy`

[modular-magician]

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

@rileykarson, 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.

[DanielRieske]

I do want to tag @shuyama1 here as the ticket got assigned to you and I am unsure if work has already been done on this.
If it has feel free to close this PR and disregard this.

[shuyama1]

@DanielRieske Thanks for looking into the issue and making a fix for it. It does look like we have this field implemented as a ResourceRef of the google_compute_ssl_certificate resource which is the only type allowed for the field when it's initially implemented and the API changed to accept "Certificate Manager certificate URLs" after.

@rileykarson Is this change considered as a change of field type (breaking change) and we need to get this into major release?

[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, 205 insertions(+), 20 deletions(-))
Terraform Beta: Diff ( 5 files changed, 205 insertions(+), 20 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+), 13 deletions(-))
TF OiCS: Diff ( 4 files changed, 156 insertions(+))

[rileykarson]

This is probably a case where we want to maintain full compatibility with the current possible inputs (mainly shortname) & allow specifying the new target type through the resource name (projects/..., the id in Terraform) and the self link since those will be unambiguous with the original target.

Rather than validation we're dropping diff suppression and value transformation with this change.

[modular-magician]

Tests analytics

Total tests: 3038
Passed tests 2732
Skipped tests: 297
Affected tests: 9

Action taken

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

TestAccComputeTargetHttpsProxy_update|TestAccComputeManagedSslCertificate_managedSslCertificateBasicExample|TestAccComputeTargetHttpsProxy_targetHttpsProxyHttpKeepAliveTimeoutExample|TestAccComputeTargetHttpsProxy_targetHttpsProxyGlobalCertificateManagerExample|TestAccComputeTargetHttpsProxy_targetHttpsProxyBasicExample|TestAccGKEHub2Scope_gkehubScopeBasicExample|TestAccGKEHub2ScopeIamBindingGenerated|TestAccGKEHub2ScopeIamMemberGenerated|TestAccGKEHub2ScopeIamPolicyGenerated

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccComputeTargetHttpsProxy_targetHttpsProxyGlobalCertificateManagerExample[Debug log]
TestAccGKEHub2Scope_gkehubScopeBasicExample[Debug log]
TestAccGKEHub2ScopeIamBindingGenerated[Debug log]
TestAccGKEHub2ScopeIamMemberGenerated[Debug log]
TestAccGKEHub2ScopeIamPolicyGenerated[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:}}$
TestAccComputeTargetHttpsProxy_update[Error message] [Debug log]
TestAccComputeManagedSslCertificate_managedSslCertificateBasicExample[Error message] [Debug log]
TestAccComputeTargetHttpsProxy_targetHttpsProxyHttpKeepAliveTimeoutExample[Error message] [Debug log]
TestAccComputeTargetHttpsProxy_targetHttpsProxyBasicExample[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

[DanielRieske]

With the given feedback, I removed the custom validator. With this change users can use this field as they have been but it won't force it to the resourceRef for the sslCertificate resource either.

This should allow for full-compatibility and not introduce a breaking change, I believe to have covered all bases here if I missed something let me know.

[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, 198 insertions(+), 5 deletions(-))
Terraform Beta: Diff ( 5 files changed, 198 insertions(+), 5 deletions(-))
TF Conversion: Diff ( 1 file changed, 1 insertion(+), 5 deletions(-))
TF OiCS: Diff ( 4 files changed, 156 insertions(+))

[modular-magician]

Tests analytics

Total tests: 3045
Passed tests 2745
Skipped tests: 298
Affected tests: 2

Action taken

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

TestAccComputeTargetHttpsProxy_update|TestAccDataSourceGoogleServiceAccountJwt

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccComputeTargetHttpsProxy_update[Debug log]
TestAccDataSourceGoogleServiceAccountJwt[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

[DanielRieske]

I don't think the integration tests failing is related to this PR, could you share the logs if it is?

[rileykarson]

ResourceRef currently means that there's a single target type within the same service- we should revert this to a string. That will probably change the DiffSuppressFunc attached to the schema. We'll want to re-add that DSF by referencing it as a diff_suppress_func.

[DanielRieske]

I am wondering whether this is neccesary, if I revert it to a string that will mean that our custom expander that depends on it being a ResourceRef to pull the base_url to create the full resource reference will have to change too.

resource_type = property.resource_ref.base_url.split('/').last
tpgresource.ParseGlobalFieldValue("<%= resource_type -%>", <%= var_name -%>, "project", d, config, true)

[Hamzawy63]

Hey @DanielRieske

sslCertificate field can accept either certificate manager certificates or ssl certificates but not both. For that reason, it was suggested by certificate manager team to create another field called certificate_manager_certificates which the customer can use whenever they want to use certificate manager certifiacates.

That behaviour is consistent with the gcloud behaviour and should provide a better UX experience. I'm currently working on implementing that using ecoders/decoders in TF (hopefully, a PR will be sent for review later today)

I would suggest we don't merge this change currently, until we check with compute team which behaviour is desired.

Maybe also @rileykarson can suggest which solution is preferred in that case

[rileykarson]

Sorry, I've been pretty delayed in reviews. Checking in with that team seems reasonable. I did not put two and two together that the internal question I answered and this PR were about the same field, good catch @Hamzawy63!

[Hamzawy63]

Hey @rileykarson @DanielRieske
compute team confirmed that they don't have any particular preferences and both solutions are valid, so we lean towards defining a new field certificate_manager_certificates that is used when customers decides to use certificates manager certificates

I'm aware that the resource definition should be consistent with API rather than any other tools (e.g. gcloud).
The difference in our case is that:

• the field sslCertificates can either accept a list of certificate manager certificates or a list of compute certificates but not both. That encourages adding a new field because it makes more sense for customers.
• I will modify PR 9144, such that customers can provide only the resource name instead of the full URL. I think that this is not doable with this solution. the value raw.(string) in custom_expand will return the name of the resource without telling us whether it's a certificate manager resource or a compute resource, feel free to correct me if I'm wrong though.

That being said, If the other proposed solution might introduce long term issues or it's better (from TF perspective) to keep the API consistency, then we go with this solution.

[rileykarson]

the field sslCertificates can either accept a list of certificate manager certificates or a list of compute certificates but not both. That encourages adding a new field because it makes more sense for customers.

Oh, interesting, I'd assumed they could be mixed!

I think the separate fields make enough sense to deviate from the API. That gets rid of the problem of identifying resource kinds when we've already allowed the short form (name) here, since the resource kind will be implied by the list you're adding the resource to.

I will modify #9144, such that customers can provide only the resource name instead of the full URL

I'd lean against doing that unless the API allows it (which is unlikely given that multiple kinds can be sent in the field). We generally support doing so because the GCE API did/does, and have maintained backwards compatibility. Newer resources generally require full ids. We made those values trivially available in Terraform through the id field a while ago, so the transformation isn't as big a deal as it used to be.

[rileykarson]

I think that means we can close this PR to supersede it with #9144. That OK with you @DanielRieske?

[DanielRieske]

Absolutely, I also assumed they could be mixed. In this case it makes more sense to go with @Hamzawy63 solution.