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

nginx_proxy: Listen on IPv6 by default #3475

Closed
wants to merge 2 commits into from

Conversation

mraerino
Copy link

This makes nginx listen on IPv6. That's it.
There is no functionality change to the IPv4 path that has existed so far.

Benefits include:

  • The trusted_networks auth backend in core now works with IPv6 when using nginx
  • Enables using IPv6 to expose the homeassistant install to the world (with some extra config also allows getting a cert via LE)

I've used an extra listening directive instead of [::]:443 ipv6only=off because with that config ipv4 gets ipv6-mapped in access logs and people might not want that.

Related issues

home-assistant/supervisor#2133

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @mraerino

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@agners
Copy link
Member

agners commented Feb 19, 2024

I've used an extra listening directive instead of [::]:443 ipv6only=off because with that config ipv4 gets ipv6-mapped in access logs and people might not want that.

Yeah I agree, this makes sense. 👍

Can you also adjust the CHANGELOG.md and bump version in config.yaml? I think this change probably deserves a major bump, IMHO.

@mraerino
Copy link
Author

I think this change probably deserves a major bump, IMHO.

so you see this as a breaking change? what do you think could cause issues about this change?
according to semver, new feature (like ipv6 support) would be considered minor bumps?

@agners
Copy link
Member

agners commented Feb 19, 2024

so you see this as a breaking change? what do you think could cause issues about this change?
according to semver, new feature (like ipv6 support) would be considered minor bumps?

I guess this is very much debatable. Semver is quite clear for libraries or software with a clear API. However, add-ons pack-up many pieces of software. Does a major bump of anyone of those mandate a major bump of the add-on? 🤔

Now in this case it is really a default configuration change of the add-on. The outward facing "API" if we want to call it like that indeed changes. You definitely can argue this is only adding a feature, so a minor bump should be sufficient. 🤷‍♂️

@mraerino
Copy link
Author

i did the version bump

@mraerino
Copy link
Author

@agners how can i help move this forward?

@agners
Copy link
Member

agners commented Feb 29, 2024

First off, I need to add that currently IPv6 support works even without this PR. But that is because the add-ons have no IPv6 support at all. In that environment, it seems that Docker exposes the ports by default on all address families:

        "NetworkSettings": {
...
            "Ports": {
                "443/tcp": [
                    {
                        "HostIp": "0.0.0.0",
                        "HostPort": "1443"
                    },
                    {
                        "HostIp": "::",
                        "HostPort": "1443"
                    }
                ]
            },

As I've mentioned in home-assistant/supervisor#2133 (comment), I don't 100% understand your environment. But how does the NetworkSettings of NGNIX look in your environment?

In essence, this change will only become relevant once we add IPv6 support. Currently there is no IPv6 at all inside the container (not even a link local address). Have you tested this change on a installation with the current setup? I am not against merging that already today, but we should be careful to not break current installations 😅

@agners agners marked this pull request as draft February 29, 2024 10:17
@github-actions github-actions bot added the stale label Apr 29, 2024
@github-actions github-actions bot closed this May 6, 2024
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.

2 participants