Skip to content

feat(TPG>=6.11)!: add endpoint_dns #2180

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 5, 2024
Merged

feat(TPG>=6.11)!: add endpoint_dns #2180

merged 2 commits into from
Dec 5, 2024

Conversation

@apeabody
Copy link
Collaborator

@apeabody apeabody commented Nov 13, 2024 •
edited
Loading

Fixes: #2182

@apeabody apeabody mentioned this pull request Nov 13, 2024
Merged
@apeabody apeabody changed the title fix: use private endpoint fix(private_cluster): use private endpoint Nov 13, 2024
@apeabody apeabody force-pushed the ap/endpoint branch 3 times, most recently from 1a78715 to bca46df Compare November 13, 2024 19:38
@apeabody apeabody changed the title fix(private_cluster): use private endpoint fix(private_cluster)!: use private endpoint Nov 13, 2024
@apeabody apeabody force-pushed the ap/endpoint branch 3 times, most recently from 877ea55 to 81fc902 Compare November 13, 2024 23:11
@apeabody
Copy link
Collaborator Author

apeabody commented Nov 14, 2024 •
edited
Loading

Error doesn't appear related to this change, but is a result of TPG v6.11+

Step #51 - "teardown beta-cluster":         	Error:      	Received unexpected error:
Step #51 - "teardown beta-cluster":         	            	FatalError{Underlying: error while running command: exit status 1; 
Step #51 - "teardown beta-cluster":         	            	Error: Error when reading or editing Subnetwork: googleapi: Error 400: The subnetwork resource 'projects/ci-gke-5db7e0f2-bhp6/regions/us-central1/subnetworks/cft-gke-test-ivye' is already being used by 'projects/ci-gke-5db7e0f2-bhp6/regions/us-central1/forwardingRules/aec5bcafcba7344aa801978cb45df853', resourceInUseByAnotherResource
Step #51 - "teardown beta-cluster":         	            	}
Step #51 - "teardown beta-cluster":         	Test:       	TestBetaCluster

@apeabody apeabody mentioned this pull request Nov 14, 2024
Merged
@zcrisler
Copy link

zcrisler commented Nov 17, 2024

Resolves #2182

@NeckBeardPrince
Copy link

NeckBeardPrince commented Nov 19, 2024

Look forward to this going in.

@apeabody
Copy link
Collaborator Author

apeabody commented Nov 19, 2024

Starting with TPG v6.11.0+ this has been a reliable error during beta-cluster teardown:

Error: Error when reading or editing Subnetwork: googleapi: Error 400: The subnetwork resource 'projects/ci-gke-76cf7158-g9i9/regions/us-central1/subnetworks/cft-gke-test-wner' is already being used by 'projects/ci-gke-76cf7158-g9i9/regions/us-central1/forwardingRules/ac9b02a8710734e47a5f8449ff61e2ec', resourceInUseByAnotherResource

gorge511
gorge511 reviewed Nov 20, 2024
View reviewed changes
@apeabody apeabody force-pushed the ap/endpoint branch 2 times, most recently from 14096f1 to 78e9ff5 Compare November 21, 2024 00:24
gorge511
gorge511 reviewed Nov 21, 2024
View reviewed changes
@apeabody apeabody force-pushed the ap/endpoint branch 5 times, most recently from 7efc72b to d6e1fc1 Compare November 27, 2024 00:00
@apeabody apeabody changed the title fix(private_cluster)!: use private endpoint feat(TPG>=6.11)!: add endpoint_dns Nov 27, 2024
@apeabody apeabody force-pushed the ap/endpoint branch 2 times, most recently from c637305 to 4fb55a9 Compare November 27, 2024 17:09
@apeabody apeabody marked this pull request as ready for review November 27, 2024 18:35
@apeabody apeabody requested review from a team, ericyz and gtsorbo as code owners November 27, 2024 18:35
@apeabody
Copy link
Collaborator Author

apeabody commented Nov 27, 2024

All pre-requisite PRs have been merged, this is now ready for review.

@allthatjazzleo
Copy link

allthatjazzleo commented Dec 2, 2024

how's it now?
can't wait to use this feature :)

@apeabody apeabody merged commit 1f85f66 into master Dec 5, 2024
8 checks passed
@apeabody apeabody deleted the ap/endpoint branch December 5, 2024 19:15
@release-please release-please bot mentioned this pull request Dec 5, 2024
Merged
legal90
legal90 reviewed Mar 18, 2025
View reviewed changes
modules/beta-private-cluster/cluster.tf
for_each = var.enable_private_endpoint && var.deploy_using_private_endpoint ? [1] : [0]
content {
dns_endpoint_config {
allow_external_traffic = var.deploy_using_private_endpoint
Copy link
Contributor

@legal90 legal90 Mar 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@apeabody question: Why is this feature gated by a condition var.enable_private_endpoint && var.deploy_using_private_endpoint ?

It should be possible to have "DNS-based endpoint" feature enabled without enforcing the usage of the private IP / private endpoint 🤔

Copy link
Collaborator Author

@apeabody apeabody Mar 18, 2025

Choose a reason for hiding this comment

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

Hi @legal90 - I was planning to update this behavior as part of a larger change when hashicorp/terraform-provider-google#20369 is released, but I'd be happy to review a PR for just this aspect mow.

Copy link
Contributor

@legal90 legal90 Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @apeabody ,
Thank you for your reply! I found your PR, which fixes that behavior and makes it working as it really should (at least, to my view): #2313
Kudos! I don't have anything to add there.

P.s. I'm sorry if my previous message sounded unpleasant. Thank you for your hard work on this module! ❤️

Copy link
Collaborator Author

@apeabody apeabody Mar 24, 2025

Choose a reason for hiding this comment

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

Thanks for the feedback @legal90! Yes, had a few minutes to prototype a change, should have it committed this week.

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

Reviewers

@bharathkkb bharathkkb bharathkkb approved these changes

@ericyz ericyz Awaiting requested review from ericyz

@gtsorbo gtsorbo Awaiting requested review from gtsorbo

+2 more reviewers

@gorge511 gorge511 gorge511 left review comments

@legal90 legal90 legal90 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support for DNS-based endpoints

7 participants

@apeabody @zcrisler @NeckBeardPrince @allthatjazzleo @gorge511 @legal90 @bharathkkb