Fix value template for global ComputeTargetHTTPSProxy

[gemmahou]

Change description

Fixes b/345274481

Fix value template for global ComputeTargetHTTPSProxy.
Resource created successfully:

Events:
  Type     Reason              Age              From                                Message
  ----     ------              ----             ----                                -------
  Warning  DependencyNotFound  49s              computetargethttpsproxy-controller  reference ComputeURLMap default/computetargethttpsproxy-dep-certmgr is not found
  Normal   Updating            16s              computetargethttpsproxy-controller  Update in progress
  Normal   UpToDate            4s (x2 over 5s)  computetargethttpsproxy-controller  The resource is up to date

Tests you have done

• Run make ready-pr to ensure this PR is ready for review.
• Perform necessary E2E testing for changed resources.

[justinsb]

Looks good; is this covered by a test?

[maqiuyujoyce]

/lgtm

Agreed with @justinsb that we should add a test case for it.

[gemmahou]

Added dynamic test to cover certificateMapRef field

--- PASS: TestCreateNoChangeUpdateDelete (0.12s)
    --- PASS: TestCreateNoChangeUpdateDelete/compute (0.00s)
        --- PASS: TestCreateNoChangeUpdateDelete/compute/basic-globaltargethttpsproxy (301.47s)
PASS
{"severity":"info","timestamp":"2024-06-24T18:48:56.177Z","msg":"Stopping and waiting for non leader election runnables"}
{"severity":"info","timestamp":"2024-06-24T18:48:56.177Z","msg":"Stopping and waiting for leader election runnables"}
{"severity":"info","timestamp":"2024-06-24T18:48:56.177Z","msg":"Stopping and waiting for caches"}
{"severity":"info","timestamp":"2024-06-24T18:48:56.177Z","msg":"Stopping and waiting for webhooks"}
{"severity":"info","timestamp":"2024-06-24T18:48:56.177Z","logger":"controller-runtime.webhook","msg":"Shutting down webhook server with timeout of 1 minute"}
{"severity":"info","timestamp":"2024-06-24T18:48:56.177Z","msg":"Wait completed, proceeding to shutdown the manager"}
ok      github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/dynamic      311.376s

[maqiuyujoyce]

According to the field description:

k8s-config-connector/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_computetargethttpsproxies.compute.cnrm.cloud.google.com.yaml

Line 64 in 8d78e49

URLs to certificate manager certificate resources that are used to authenticate connections between users and the load balancer.

sslCertificates and certificateManagerCertificates fields can not be defined together.

And according to the latest TF doc: https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/compute_target_https_proxy#certificate_manager_certificates

Certificate manager certificates only apply when the load balancing scheme is set to INTERNAL_MANAGED. For EXTERNAL and EXTERNAL_MANAGED, use certificate_map instead.

Feels like certificateManagerCertificates and certificateMap are mutually exclusive fields, and certificateManagerCertificates/ certificateMap and sslCertificates are mutually exclusive fields as well. I.e. only one out of the three fields can be configured.

Could you verify that certificate map is indeed configured correctly in the underlying GCP resource when ssl certificates are configured?

Once the behavior is confirmed, could you update the field descriptions of those fields to reflect the correct usage of these fields?

[gemmahou]

The default value for load balancing schemes is EXTERNAL, so I'm able to created a target https proxy with certificateMap: https://screenshot.googleplex.com/7Lm35rNZ6hkmXMZ.png

However, certificateManagerCertificates/certificateMap and sslCertificates are mutually exclusive fields and I noticed that sslCertificates is discarded when both are set. I updated the test and field description.

[yuwenma]

/lgtm

Defer to @maqiuyujoyce comment on verifying the field and updating the doc.

[yuwenma]

/lgtm
/approve

Thank you for the change, looks good on my side

/hold In case @maqiuyujoyce want to take another pass. Feel free to unhold

[maqiuyujoyce]

Looks like there are fixes for three fields. Wondering if you can add test cases for the other two fixes.

[maqiuyujoyce]

Similarly, is there a test case covering this fix?

[gemmahou]

I noticed another issue with serverTlsPolicyRef. There's error when resolving project: "error": "Update call failed: error fetching live state: error converting resource config: unable to resolve missing value: project", and I already specified projectRef in the yaml for NetworkSecurityServerTLSPolicy resource.

This field is introduced in #941, and this is a DCL based reference field. I don't think it's necessary to address it here, so I reverted my change back and created a ticket(b/349429138) for this. Lmk if that sounds reasonable.

[maqiuyujoyce]

/lgtm
/approve

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maqiuyujoyce, yuwenma

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [maqiuyujoyce,yuwenma]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[yuwenma]

/hold cancel