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

Ignores IPv6 addresses containing a zone index #5981 #5984

Merged
merged 1 commit into from
Dec 10, 2022

Conversation

louismrose
Copy link
Contributor

A possible fix for #5981. Ignores IP addresses that contain a zone index when trying to determine which IP address to use for port forwarding.

@nicks nicks self-requested a review December 7, 2022 17:13
@louismrose louismrose force-pushed the issue-5981 branch 4 times, most recently from 1b1552d to 23d3ac3 Compare December 7, 2022 18:44
Fixes tilt-dev#5981

Signed-off-by: Louis Rose <louis.rose@anaplan.com>
Copy link
Member

@nicksieger nicksieger left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for your submission!

@louismrose
Copy link
Contributor Author

I don't have write perms here, so will wait for somebody else to merge. If there's more to do here, do let me know. Thanks!

@nicks
Copy link
Member

nicks commented Dec 9, 2022

feels like this needs a unit test, to ensure someone doesn't break it in the future.

i'm really confused why you're hitting this codepath at all, since you're really only supposed to hit this path when you manually specify a host. the default behavior should be to use kubectl's built-in resolution logic. i'm not opposed to this pr, i just worry we're missing a deeper bug here.

@nicksieger
Copy link
Member

There's some context in the linked issue. Apparently some mac OS machines will return an ipv6 address with a zone index when looking up localhost.

Copy link
Member

@nicks nicks left a comment

Choose a reason for hiding this comment

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

OK, i think this is harmless, but posted a separate fix for the broader defaulting issue.

@nicks nicks merged commit 3fea136 into tilt-dev:master Dec 10, 2022
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.

3 participants