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

Don't treat host:port as an IPv6 address #666

Closed
wants to merge 1 commit into from
Closed

Don't treat host:port as an IPv6 address #666

wants to merge 1 commit into from

Conversation

nvwls
Copy link

@nvwls nvwls commented Dec 11, 2015

Addresses chef-server/665

… an IPv6 address

Change-Id: I1d0ddc309e3e115e458b05bea775edc1252814ab
@stevendanna
Copy link
Contributor

Thanks for the contribution. However, I think this would exclude a lot of valid IPv6 addresses since a single : is used to separate groups in an ipv6 address.

@stevendanna
Copy link
Contributor

D'oh I misread this. I think this will work apologies for any confusion.

@stevendanna
Copy link
Contributor

👍

@markan
Copy link
Contributor

markan commented Dec 12, 2015

👍 Makes sense.

@marcparadise
Copy link
Member

This change has the potential to break other configuration - OmnibusHelper.vip_for_url uses this function, and everywhere that we use vip_for_uri, we're appending or otherwise specifying port,. here's one example:

https://github.com/chef/chef-server/blob/master/omnibus/files/private-chef-cookbooks/private-chef/templates/default/oc_erchef.config.erb#L160

In that case, if someone specified a vip that included a port for bifrost, erchef would not be able to communicate with bifrost. It's the same for other usages.

We have an existing issue #50, was the underlying issue you're trying to resolve with this?

@stevendanna
Copy link
Contributor

@marcparadise In thinking about this, while I agree with you at a high level, I don't see this change opening up new issues. Nothing /prevented/ users from specifying a port. In the past, such specifications would fail with whatever weird error the relevant component would raise when configured with an invalid IPv6 address.

As far as I know, there isn't a valid IPv6 address that contains a single :, this does allow for an easy work around for the issue with bookshelf when nginx is listening on non-443, and it doesn't open the user up to particularly more errors that they are open to now.

The only downside to merging this would the cost of continuing to support hostname:port specifications in these config items in the future even once we have a more complete fix.

@nvwls
Copy link
Author

nvwls commented Jan 19, 2016

Based on the comment:

    The only downside to merging this would the cost of continuing to support hostname:port specifications

Is this being deprecated? Or will this be addressed in one form or another?

@marcparadise
Copy link
Member

marcparadise commented May 20, 2016

I believe the underlying need for this has been addressed by #833, which adds a bookshelf_vip attribute that accepts port. @nvwls could you confirm that the reason for this change was the inability to set a custom port for bookshelf?

@marcparadise
Copy link
Member

marcparadise commented Jun 6, 2016

Closing this as resolved by #833. Please re-open this PR if the addition of bookshelf[vip_port] doesn't address your concern.

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.

5 participants