feat: Support regional Compute Target TCP Proxy #2915
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| proxyBind: | ||
| description: |- | ||
| Immutable. This field only applies when the forwarding rule that references | ||
| this target proxy has a loadBalancingScheme set to INTERNAL_SELF_MANAGED. | ||
| type: boolean | ||
| proxyHeader: | ||
| description: |- | ||
| Specifies the type of proxy header to append before sending data to | ||
| Immutable. Specifies the type of proxy header to append before sending data to |
There was a problem hiding this comment.
This is generated by make ready-pr. The field is immutable in regional TCP proxy, but can be updated by setProxyHeader method in global TCP Proxy.
@maqiuyujoyce currently, like other locational resources, we support regional and global in one ComputeTargetTCPProxy kind, is there a way to handle the case? Can we update the field description by ourselves anywhere?
Or, maybe we should consider starting support ComputeTargetTCPProxy and RegionalComputeTargetTCPProxy kind separately?
There was a problem hiding this comment.
IMO it depends on the use case and the priority:
- If it is a high priority to support this resource, and the common use cases include that the regional ComputeTargetTCPProxy referenced by other resources, then I suggest we do some hacks in the CRD generation to remove the
Immutable.marking to unblock it asap. - If it is not super high priority (it's unclear from the ticket) or the resource won't be commonly referenced by other resources (it's referenced in ComputeForwardingRule), then we can do some comparison between the two resources of different locationalities, and decide as a team whether we want to support the regional one as a separate resource.
I actually don't recall much feedback about our locationality design, so it'll mainly be up to ourselves to make a decision based on how its pros and cons will impact direct controllers. We could reach out to the service team to gather some information though.
No matter which option we choose, I don't see it makes a big difference in direct controller migration later.
| @@ -106,6 +111,7 @@ spec: | |||
| type: string | |||
| required: | |||
| - backendServiceRef | |||
| - location | |||
There was a problem hiding this comment.
It looks like you also added a required field to an existing CRD. This is a breaking change. If we plan to support regional Compute Target TCP Proxy into the existing ComputeTargetTCPProxy CRD, then we need to make this field optional and default it with value global.
|
Closing this PR. Discussed offline, we plan to support regional TCP proxy through direct controller. |
Change description
Supporting regional Compute Target TCP Proxy is a prerequisite for FR b/246759060.
Since it's already supported in Terraform long times ago(4.x version), update servicemappings will be faster&straight-forward, we don't need to update TF library. So I implement that way in this PR.
An alternative will be migrating Compute Target TCP Proxy resource to direct controller, but I feel it's not necessary for now as it takes longer time to address the FR.
Assigned to @yuwenma @maqiuyujoyce Any thoughts on the implementation of regional Compute Target TCP Proxy?
Tests you have done
make ready-prto ensure this PR is ready for review.