Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add NON_GCP_PRIVATE_IP_PORT as networkEndpointType for NetworkEndpointGroup #5684

Merged
merged 4 commits into from
Feb 22, 2022

Conversation

AlexanderEllis
Copy link
Contributor

@AlexanderEllis AlexanderEllis commented Feb 7, 2022

This change adds NON_GCP_PRIVATE_IP_PORT as a network_endpoint_type option for the google_compute_network_endpoint_group resource. This is in support for Hybrid Load Balancing.

I added two examples: one with a basic google_compute_network_endpoint_group with the new type, and one with a global external managed load balancer with hybrid connectivity (roughly following the configuration at https://cloud.google.com/load-balancing/docs/https/setting-up-ext-https-hybrid). I also added documentation about the valid combination of this new type and backend service load balancing scheme/balancing modes.

Part of hashicorp/terraform-provider-google#10040

For posterity, this change also sets the instance attribute on the google_compute_network_endpoint resource to be optional, as Hybrid connectivity NEGs use endpoints with just IP and PORT (without an instance). See https://cloud.google.com/load-balancing/docs/negs#hybrid-neg and https://cloud.google.com/load-balancing/docs/https/setting-up-ext-global-https-hybrid#set_up_the_hybrid_connectivity_neg for more details.

If this PR is for Terraform, I acknowledge that I have:

  • Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
  • Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
  • Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
  • Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
  • Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

compute: add `NON_GCP_PRIVATE_IP_PORT` value for `network_endpoint_type` in the `google_compute_network_endpoint_group` resource
compute: Update `instance` attribute for `google_compute_network_endpoint` to be optional, as Hybrid connectivity NEGs use network endpoints with just IP and Port.

@modular-magician
Copy link
Collaborator

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@melinath, please review this PR or find an appropriate assignee.

@modular-magician

This comment was marked as outdated.

@AlexanderEllis AlexanderEllis changed the title Add NON_GCP_PRIVATE_IP_PORT as networkEndpointType for NetworkEndpointGroup Add NON_GCP_PRIVATE_IP_PORT as networkEndpointType for NetworkEndpointGroup Feb 9, 2022
@melinath

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@melinath
Copy link
Member

melinath commented Feb 14, 2022

b/202539913, b/202525233, b/181008964

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

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

This looks good overall - it looks like one of the new tests is failing. I've noted a potential fix below. If you can test it locally prior to pushing, that would be great!

mmv1/products/compute/terraform.yaml Show resolved Hide resolved
mmv1/products/compute/api.yaml Outdated Show resolved Hide resolved
@melinath

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

1 similar comment
@modular-magician

This comment was marked as outdated.

@melinath

This comment was marked as outdated.

@melinath

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

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

LGTM - I'll merge in the morning assuming the gcbrun looks good.

@ct-dh
Copy link
Contributor

ct-dh commented Feb 15, 2022

I was working on my own version of this but hadn't got round to tidying it up to raise a PR. Does this PR support importing the hybrid NEGs as is? I found I had to make more changes than in this PR to get imports to work correctly (this is the diff I had, but as I say I haven't had a chance to tidy it up, please feel free to grab anything if needed): https://github.com/GoogleCloudPlatform/magic-modules/compare/master...ct-dh:feature/hybrid-neg-support?expand=1

@AlexanderEllis
Copy link
Contributor Author

@ct-dh Thanks for flagging that, and thanks for sending over your changes! I'll double check that on my end with the instances built out as well.

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

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

Per discussion above, sounds like there are some changes to be made.

@AlexanderEllis
Copy link
Contributor Author

Updated NetworkEndpoint instance param to be optional and added an example/tests exercising it. Followed the patterns in resource_compute_global_network_endpoint examples for the optional params (custom import and handling). Hat tip to @ct-dh again for the suggestion!

image

@modular-magician

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@melinath
Copy link
Member

@melinath

This comment was marked as outdated.

@modular-magician

This comment was marked as outdated.

@melinath
Copy link
Member

Trying /gcbrun again

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 7 files changed, 364 insertions(+), 25 deletions(-))
Terraform Beta: Diff ( 7 files changed, 364 insertions(+), 25 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))
TF OiCS: Diff ( 8 files changed, 324 insertions(+))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccApigeeEnvironmentIamBindingGenerated|TestAccApigeeEnvironmentIamMemberGenerated|TestAccApigeeEnvironmentIamPolicyGenerated|TestAccCloudbuildWorkerPool_basic|TestAccComputeGlobalForwardingRule_globalForwardingRuleHybridExample|TestAccComputeNetworkEndpointGroup_networkEndpointGroupNonGcpExample|TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccServiceNetworkingPeeredDNSDomain_basic You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=257868

@melinath
Copy link
Member

@AlexanderEllis quick question - could you add a note explaining why the instance field is being changed to optional?

@AlexanderEllis
Copy link
Contributor Author

@AlexanderEllis quick question - could you add a note explaining why the instance field is being changed to optional?

Done: added separate release note and additional links in description for posterity. Thanks!

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

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

LGTM - remaining test failures are unrelated.

@melinath melinath merged commit 3788bd2 into GoogleCloudPlatform:master Feb 22, 2022
lcaggio pushed a commit to lcaggio/magic-modules that referenced this pull request Mar 16, 2022
…intGroup (GoogleCloudPlatform#5684)

* Add NON_GCP_PRIVATE_IP_PORT as networkEndpointType for NetworkEndpointGroup

* Fix the Network Endpoint Group non-GCP example

* Make networkEndpoint instance optional, handle, and add example

* Put the hybrid endpoint in the hybrid NEG
lcaggio pushed a commit to lcaggio/magic-modules that referenced this pull request Mar 17, 2022
…intGroup (GoogleCloudPlatform#5684)

* Add NON_GCP_PRIVATE_IP_PORT as networkEndpointType for NetworkEndpointGroup

* Fix the Network Endpoint Group non-GCP example

* Make networkEndpoint instance optional, handle, and add example

* Put the hybrid endpoint in the hybrid NEG
lcaggio pushed a commit to lcaggio/magic-modules that referenced this pull request Mar 18, 2022
…intGroup (GoogleCloudPlatform#5684)

* Add NON_GCP_PRIVATE_IP_PORT as networkEndpointType for NetworkEndpointGroup

* Fix the Network Endpoint Group non-GCP example

* Make networkEndpoint instance optional, handle, and add example

* Put the hybrid endpoint in the hybrid NEG
betsy-lichtenberg pushed a commit to betsy-lichtenberg/magic-modules that referenced this pull request Apr 25, 2022
…intGroup (GoogleCloudPlatform#5684)

* Add NON_GCP_PRIVATE_IP_PORT as networkEndpointType for NetworkEndpointGroup

* Fix the Network Endpoint Group non-GCP example

* Make networkEndpoint instance optional, handle, and add example

* Put the hybrid endpoint in the hybrid NEG
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.

4 participants