Skip to content

Adds server TLS policy property to target HTTPS proxy resource#8857

Merged
SarahFrench merged 1 commit into
GoogleCloudPlatform:mainfrom
FiskilBen:add-server-tls-policy-for-https-proxy
Sep 14, 2023
Merged

Adds server TLS policy property to target HTTPS proxy resource#8857
SarahFrench merged 1 commit into
GoogleCloudPlatform:mainfrom
FiskilBen:add-server-tls-policy-for-https-proxy

Conversation

@FiskilBen

Copy link
Copy Markdown
Contributor

Fixes hashicorp/terraform-provider-google#14980

Adds the server_tls_policy property to the TargetHTTPSProxy resource so that it may be configured with mTLS.

I was unsure whether this field should be in the beta or ga provider. The field is available in the v1 API, but the networksecurity.ServerTlsPolicy resource is only available with the beta API. Any guidance here would be appreciated.

This is the last change required for end-to-end deployment of mTLS load balancers with terraform.

Release Note Template for Downstream PRs (will be copied)

compute: Added server_tls_policy argument to target_https_proxy resource

@google-cla

google-cla Bot commented Sep 5, 2023

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@modular-magician

Copy link
Copy Markdown
Collaborator

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

@SarahFrench, 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 modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Sep 5, 2023
allow_open = "false"
mtls_policy {
client_validation_mode = "ALLOW_INVALID_OR_MISSING_CLIENT_CERT"
client_validation_trust_config = "projects/${data.google_project.project.number}/locations/global/trustConfigs/${google_certificate_manager_trust_config.default.name}"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I tried using the id of the google_certificate_manager_trust_config resource in this parameter. However, when the tests executed the same configuration resulted in a plan that wanted to update the tls_policy from client_validation_trust_config = projects/${data.google_project.project.id}/locations/global/trustConfigs/${google_certificate_manager_trust_config.default.name} to client_validation_trust_config = projects/${data.google_project.project.number}/locations/global/trustConfigs/${google_certificate_manager_trust_config.default.name} (i.e. the trust config reference link used the project number in the API but the project ID in the terraform state). I'm not sure if this is a problem with my example, a bug with the trust_config resource, or a bug with the server_tls_policy resource.

@FiskilBen

Copy link
Copy Markdown
Contributor Author

Not sure why the CLA check is failing. I have signed the CLA on behalf of Fiskil PTY LTD but I don't see it in the "manage CLAs" page.

@SarahFrench

Copy link
Copy Markdown
Contributor

Not sure why the CLA check is failing. I have signed the CLA on behalf of Fiskil PTY LTD but I don't see it in the "manage CLAs" page.

Is that as as Individual signer (versus Corporate signer)? For individual signers you need the email address used to sign the CLA to match the email present on your commits in this PR. Is the email used on this commit that same as you used for the CLA?

It's different for corporate signers - see here for details about both scenarios

@FiskilBen

Copy link
Copy Markdown
Contributor Author

Yeah I got an email that the CLA was voided (as a corporate signer), they've told me what I need to do to fix it. I'll get that sorted out.

@FiskilBen

Copy link
Copy Markdown
Contributor Author

CLA issues resolved! :)

@modular-magician modular-magician removed the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Sep 11, 2023
@modular-magician

Copy link
Copy Markdown
Collaborator

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 ( 4 files changed, 169 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 4 files changed, 290 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 10 insertions(+))
TF OiCS: Diff ( 4 files changed, 192 insertions(+))

@modular-magician

Copy link
Copy Markdown
Collaborator

Tests analytics

Total tests: 3026
Passed tests 2726
Skipped tests: 297
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_targetHttpsProxyMtlsExample|TestAccDataSourceGoogleServiceAccountAccessToken_basic|TestAccProjectIamPolicy_invalidMembers

Get to know how VCR tests work

@modular-magician

Copy link
Copy Markdown
Collaborator

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

- !ruby/object:Provider::Terraform::Examples
name: 'target_https_proxy_mtls'
primary_resource_id: 'default'
min_version: 'beta'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was unsure whether this field should be in the beta or ga provider. The field is available in the v1 API, but the networksecurity.ServerTlsPolicy resource is only available with the beta API. Any guidance here would be appreciated.

Generally we make sure that the GA/Beta versions of the provider match v1/beta versions of Google APIs.

Having said that, this situation is more complex and I wasn't sure what we'd do in a situation where a test is focused on a GA resource but needs a beta resource present in the test. From a quick look, I can't see any tests in the GA provider that include provider = google-beta so I think you've made the correct choice of how to handle the test. Thanks!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to run the test manually to make sure it's working as expected, but assuming it passes I'll approve this PR

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test ran ok, but currently there's a freeze on merging PRs. Once that's lifted I'll approve and merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

implement serverTlsPolicy for https_proxy resources

3 participants