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

mon: ensure IPv6 addresses are wrapped in [] before appending port #14255

Closed
wants to merge 1 commit into from

Conversation

matthewpi
Copy link
Contributor

When using IPv6 addresses as the primary address family on a cluster, the --public-bind-addr flag is set to $(ROOK_POD_IP):$(PORT), this causes a problem when the POD_IP is an IPv6 address as the port gets joined to the actual IPv6 address rather than as a port on the IP address.

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

When using IPv6 addresses as the primary address family on a cluster,
the `--public-bind-addr` flag is set to `$(ROOK_POD_IP):$(PORT)`, this
causes a problem when the POD_IP is an IPv6 address as the port gets
joined to the actual IPv6 address rather than as a port on the IP
address.

Signed-off-by: Matthew Penner <me@matthewp.io>
@BlaineEXE
Copy link
Member

Thanks for contributing @matthewpi !

I actually got a report about this same thing yesterday and worked up a fix. #14248

I came to the same conclusions you did. The fix I have ready also adds unit tests and makes a best-effort attempt to handle the DualStack: true case as much as possible, so I am inclined to prefer the larger fix in place there. However, since we are arriving at similar approaches, I am happy to have your details on the git commit attribution.

The bigger issue for me is that I have been struggling to stand up an IPv6 environment to do testing. Do you have an IPv6 Kube cluster that you could test #14248 on?

And, for posterity, do you have a good guide for setting up such an environment? I think it would be good to follow up after this to add an IPv6 test to our CI so we don't have any regressions like this again.

Let me know what you'd like to do for code attribution. Being a maintainer, I don't particularly care so much about having my name be on commits as long as the code is fixed and it tests okay. It may be easiest for you to cherry-pick and squash together the commit from #1428, and then we can track the fix here in your PR.

@matthewpi
Copy link
Contributor Author

I actually got a report about this same thing yesterday and worked up a fix. #14248

I came to the same conclusions you did. The fix I have ready also adds unit tests and makes a best-effort attempt to handle the DualStack: true case as much as possible, so I am inclined to prefer the larger fix in place there.

I didn't realize that there was already an open PR for this, I was just trying to fix my dev cluster ASAP. Kinda need Ceph online for a lot of applications to work haha.

The bigger issue for me is that I have been struggling to stand up an IPv6 environment to do testing. Do you have an IPv6 Kube cluster that you could test #14248 on?

I do have an environment to test this on, I'll test those changes shortly and let you know.

And, for posterity, do you have a good guide for setting up such an environment? I think it would be good to follow up after this to add an IPv6 test to our CI so we don't have any regressions like this again.

I can write one up. I bootstrapped my own cluster from the ground-up, but preferring primarily IPv6 over IPv4 comes down to a few CLI flags. My cluster is DualStacked, just reverses the IP order so IPv6 is used first for SingleStack services.

Let me know what you'd like to do for code attribution. Being a maintainer, I don't particularly care so much about having my name be on commits as long as the code is fixed and it tests okay. It may be easiest for you to cherry-pick and squash together the commit from #1428, and then we can track the fix here in your PR.

I'm fine either way, if your code works it would just be easier to merge that PR and close this one assuming no other changes need to be made.

@matthewpi
Copy link
Contributor Author

Closed in favor of #14248

@matthewpi matthewpi closed this May 23, 2024
@matthewpi matthewpi deleted the mon-ipv6 branch May 23, 2024 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants