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

Fix IPv6 compatibility issue with starter pod #437

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

zzhao2010
Copy link
Contributor

Summary
Bug Fixed: Starter doesn't work in an IPv6 cluster

This pull request addresses the IPv6 compatibility issue in the k6-operator as outlined in the referenced issue. Specifically, the changes ensure that the k6-curl container can correctly handle IPv6 addresses.

Changes Made
Introduced the net package to handle IPv6 addresses.
Hostname Wrapping: Utilized net.JoinHostPort to wrap the hostname in square brackets if it is a literal IPv6 address, ensuring proper formatting.

Testing
Verified that the k6-curl container now supports both IPv6 and IPv4 addresses.
Conducted tests on local k8s cluster via KIND to confirm the functionality of the k6 operator with the updated IPv6 support.

Snapshot of starter pod that deals with IPv6 address correctly:
image
Snapshot of starter pod that still works with IPv4 address:
image

Impact
These changes enhance the compatibility and robustness of the k6-operator, allowing it to function correctly in environments utilizing IPv6 addresses.

@CLAassistant
Copy link

CLAassistant commented Aug 7, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@yorugac yorugac left a comment

Choose a reason for hiding this comment

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

Hi @zzhao2010, thank you for the PR!
I like this solution: it's super-concise 😄

It should be tested a bit more though. I'll be able to look into how to test it internally only in a couple of weeks. Meanwhile, could you please add reproducible examples with kind cluster? It appears they have it here:
https://kind.sigs.k8s.io/docs/user/configuration/#ip-family
I assume you mentioned the same in your description.

There is separately ipv6 and dual configuration: I think we should be checking both separately.
If you could add a folder e2e/ipv6 with relevant files for such a test, that'd be great.

PS Signing CLA is a hard requirement for any PR: please sign it 🙂

@frittentheke
Copy link
Contributor

While I highly appreciate IPv6 to be working ... and this PR is nothing more than a bugix. But there also is #87.

If you, @yorugac, agree to move to a native k6 client / HTTP call by the Operator to kick off them K6 runners that change can also "fix" the IPv6 endpoint issue.

@LCaparelli
Copy link

While I highly appreciate IPv6 to be working ... and this PR is nothing more than a bugix. But there also is #87.

I don't think one solution excludes the other. The fix with brackets is here, and would be greatly appreciated. For now we're using a fork.

I'd suggest attacking both fronts: this fix as a quick win and #87 as a more robust, long-term approach. :-)

@zzhao2010
Copy link
Contributor Author

zzhao2010 commented Aug 31, 2024

Hi @yorugac

could you please add reproducible examples with kind cluster? It appears they have it here:
https://kind.sigs.k8s.io/docs/user/configuration/#ip-family
I assume you mentioned the same in your description.

That is correct. I created the ipv6 local cluster via KIND using the exactly same config file in this url.

If you could add a folder e2e/ipv6 with relevant files for such a test, that'd be great.

You needed me to add the kind config file in the e2e/ipv6 folder? For the test script and crd file, the test.js and test.yaml that are already in e2e folder work. And I did test with dual cluster as well. With dual configuration, the primary ip would be ipv4. And it works with both ipv4 and ipv6 as the snapshots show in the description.

PS Signing CLA is a hard requirement for any PR: please sign it 🙂

Signed.

@zzhao2010 zzhao2010 requested a review from yorugac August 31, 2024 07:04
Copy link
Collaborator

@yorugac yorugac left a comment

Choose a reason for hiding this comment

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

Hi @zzhao2010, thank you for the update! LGTM.

Noting for posterity's sake. Depending on the setup, pure ipFamily: ipv6 might fail on DNS resolution. ipFamily: dual seems more reliable in this context.
Potentially related: kubernetes-sigs/kind#3114

@yorugac yorugac merged commit 64be967 into grafana:main Sep 10, 2024
8 checks passed
@yorugac
Copy link
Collaborator

yorugac commented Sep 10, 2024

I'd suggest attacking both fronts: this fix as a quick win and #87 as a more robust, long-term approach. :-)

Yes, I think the same. Removal of starter won't happen fast, but this fix is small and a quick win 🙌

@zzhao2010
Copy link
Contributor Author

Hi @yorugac Can I ask when are you planning to build a image that includes this fix? The latest image was built 2 months ago.

@zzhao2010 zzhao2010 deleted the ipv6_compatibility branch September 17, 2024 21:40
@yorugac
Copy link
Collaborator

yorugac commented Sep 18, 2024

@zzhao2010, yes, there was a bit of a skew with the releases last time: 2 releases were done with the short window. The next one will be after k6 v0.54 so ~ next week. (k6 will be released on Tuesday)
And yes, in case anyone is wondering, we do intend to improve this workflow so that releases of k6-operator are less dependent on k6 (issue).

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.

Starter doesn’t work in an IPv6 cluster
5 participants