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

feat: Added support for GCP Private Service Connect #115

Merged
merged 76 commits into from
Jul 4, 2024
Merged

Conversation

amanpruthi
Copy link
Collaborator

@amanpruthi amanpruthi commented Apr 2, 2024

This Pr has been tested for a upgrade scenario as well as a fresh setup scenario. before this we need to publish helm chart.
PR: wandb/helm-charts#102

Required provider upgrade

terraform init -upgrade
terraform plan/apply

@amanpruthi amanpruthi requested review from gls4 and a team as code owners April 2, 2024 12:11
@jsbroks jsbroks linked an issue Apr 30, 2024 that may be closed by this pull request
}

locals {
lb_name = data.kubernetes_ingress_v1.ingress.metadata[0].annotations != null ? data.kubernetes_ingress_v1.ingress.metadata[0].annotations["ingress.kubernetes.io/forwarding-rule"] : ""
Copy link
Member

Choose a reason for hiding this comment

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

lb_name but you are looking at the forward rulename?
Is this value set by the ingress controller when provisioning?

Copy link
Collaborator Author

@amanpruthi amanpruthi May 10, 2024

Choose a reason for hiding this comment

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

Forwarding rule name is the actual load balancer name in gcp.

This is not being tested using the published helm version ... It will be tested after helm version upgrade to 0.13

https://github.com/wandb/helm-charts/pull/102/files

Although we have tested this locally by creating the ingress manually on the cluster .So the terraform code works

Copy link
Member

Choose a reason for hiding this comment

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

Okay I approved the helm PR. I would perf not to use data type and pass in the name explicitly

Copy link
Member

Choose a reason for hiding this comment

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

So lets make sure to test it with the new helm chart before we merge it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jsbroks ingress doesn't have any annotation that allows you specify the loadbalancer name that will be created by the gce controller .
And we need the name to link the internal load balancer to the private service .
Checked the code also there is no such supported annotation https://github.com/kubernetes/ingress-gce/blob/master/pkg/annotations/ingress.go

@amanpruthi amanpruthi requested a review from jsbroks May 10, 2024 10:57
@jsbroks
Copy link
Member

jsbroks commented May 10, 2024

We should probably add a comment saying that this requires the min version of that helm chart in order to work. So once you merge the helm could you comment back on the chart version

@amanpruthi amanpruthi force-pushed the aman/issue-95 branch 2 times, most recently from e2c3aab to 9f8c0b0 Compare June 18, 2024 12:18
@amanpruthi amanpruthi changed the title feat: Added support for GCP Private Service Connect #95 feat: Added support for GCP Private Service Connect Jun 20, 2024
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
@amanpruthi amanpruthi requested a review from jsbroks June 21, 2024 05:49
Copy link
Member

@jsbroks jsbroks left a comment

Choose a reason for hiding this comment

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

Approving but I have not tested. I just want to make sure this doesn't have any creating changes and it is forward and backfowards compatibility. Could you also indicuate which version of the helm chart this needs to be on

@amanpruthi
Copy link
Collaborator Author

amanpruthi commented Jul 4, 2024

it required operator wandb helm chart min version 0.13.0

@amanpruthi amanpruthi merged commit 4c47c0a into main Jul 4, 2024
6 checks passed
@amanpruthi amanpruthi deleted the aman/issue-95 branch July 4, 2024 06:05
jsbroks pushed a commit that referenced this pull request Jul 4, 2024
## [3.6.0](v3.5.0...v3.6.0) (2024-07-04)

### Features

* Added support for GCP Private Service Connect ([#115](#115)) ([4c47c0a](4c47c0a))
@jsbroks
Copy link
Member

jsbroks commented Jul 4, 2024

This PR is included in version 3.6.0 🎉

@zacharyblasczyk
Copy link
Contributor

zacharyblasczyk commented Jul 15, 2024

Technically this was a breaking change as you can't downgrade the version of the provider

Screen Shot 2024-07-15 at 12 40 52 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants