Skip to content

Conversation

@jianglai
Copy link
Contributor

@jianglai jianglai commented Dec 19, 2024

k8s does not have a way to expose a global load balancer with TCP
endpoints, and setting up node port-based routing is a chore, even with
Terraform (which is what we did with the standalone proxy).

We will use Cloud DNS's geolocation routing policy to ensure that
clients connect to the endpoint closest to them.


This change is Reviewable

@jianglai jianglai changed the title tcp service Expose EPP and WHOIS endpoints on reginal load balancers Dec 19, 2024
@jianglai jianglai added the WIP Work in progress. Don't review yet. label Dec 19, 2024
Copy link
Collaborator

@ptkach ptkach left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @jianglai)


jetty/get-endpoints.py line 55 at r1 (raw file):

    res = []
    lines = run_command(f'kubectl get {resource}/{service}')
    for line in lines.split('\n'):

Just a suggestion I found a hard way myself. Instead of parsing the output as a lines, which is ok, but not great. You can pretty much always use --format=json instead . This allows for safer and cleaner approach.

Code quote:

for line in lines.split('\n'):

@jianglai jianglai force-pushed the tcp-service branch 2 times, most recently from 6d5c936 to 30137b5 Compare December 19, 2024 21:18
@jianglai jianglai requested a review from ptkach December 19, 2024 21:18
Copy link
Contributor Author

@jianglai jianglai left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ptkach)


jetty/get-endpoints.py line 55 at r1 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

Just a suggestion I found a hard way myself. Instead of parsing the output as a lines, which is ok, but not great. You can pretty much always use --format=json instead . This allows for safer and cleaner approach.

Agreed.

@jianglai jianglai removed the WIP Work in progress. Don't review yet. label Dec 19, 2024
Copy link
Contributor Author

@jianglai jianglai left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @github-advanced-security[bot] and @ptkach)

@jianglai jianglai force-pushed the tcp-service branch 2 times, most recently from 7bea165 to 7f5b913 Compare December 20, 2024 15:00
Copy link
Collaborator

@ptkach ptkach left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on @github-advanced-security[bot] and @jianglai)


jetty/get-endpoints.py line 137 at r4 (raw file):

            for service in ['whois', 'whois-canary', 'epp', 'epp-canary']:
                map_key = service.replace('-', '_')
                for ip in get_endpoints('services', service,

It looks like it can be simplified. kubectl accepts jsonpath param, which I think would make it easier by doing something like this jsonpath='{.status.loadBalancer.ingress[0}.ip}' . I've not checked it though, I just realized I'd be really surprised if they didn't and stumbled upon this doc https://kubernetes.io/docs/reference/kubectl/jsonpath/

Copy link
Contributor Author

@jianglai jianglai left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @github-advanced-security[bot] and @ptkach)


jetty/get-endpoints.py line 137 at r4 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

It looks like it can be simplified. kubectl accepts jsonpath param, which I think would make it easier by doing something like this jsonpath='{.status.loadBalancer.ingress[0}.ip}' . I've not checked it though, I just realized I'd be really surprised if they didn't and stumbled upon this doc https://kubernetes.io/docs/reference/kubectl/jsonpath/

It does look much nicer!

Copy link
Collaborator

@ptkach ptkach left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 4 files at r1, 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @github-advanced-security[bot])

k8s does not have a way to expose a global load balancer with TCP
endpoints, and setting up node port-based routing is a chore, even with
Terraform (which is what we did with the standalone proxy).

We will use Cloud DNS's geolocation routing policy to ensure that
clients connect to the endpoint closest to them.
Copy link
Contributor Author

@jianglai jianglai left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @github-advanced-security[bot] and @ptkach)

@jianglai jianglai added this pull request to the merge queue Dec 26, 2024
Merged via the queue into google:master with commit 7641b05 Dec 26, 2024
8 of 9 checks passed
@jianglai jianglai deleted the tcp-service branch December 26, 2024 16:17
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.

2 participants