-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: allow overriding the advertised address #40
base: latest
Are you sure you want to change the base?
feat: allow overriding the advertised address #40
Conversation
Pull Request Test Coverage Report for Build 9047408735Details
💛 - Coveralls |
Context how I propose to pass it through in HAP-nodejs: kristof-mattei/HAP-NodeJS#2 and eventually in homebridge: kristof-mattei/homebridge#1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks you for the PR and your contribution 🚀 I think that would be a great option to better support some of the docker deployments.
I had a few comments here and there, happy to discuss some of the points 👍
/** | ||
* If specified, the mDNS server will advertise this address only. | ||
* This is to help in situations like Docker where the interface's IP address does not equal the external one. | ||
* These value are merely here for the purpose of advertising, and does not carry any semantic | ||
* value. It is up to the administrator to verify that the service is reachable on this IP address. | ||
*/ | ||
overrideAdvertised?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the option needs a small note/documentation that the overrideAdvertised
address will be used for every interface the service is advertised on (which might be the case).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll address this one later as I think your other comment / changes will impact this.
if (this.overrideAdvertised) { | ||
if (net.isIPv4(this.overrideAdvertised)) { | ||
aRecordMap[name] = new ARecord(this.hostname, this.overrideAdvertised, true); | ||
reverseAddressMap[this.overrideAdvertised] = new PTRRecord(formatReverseAddressPTRName(this.overrideAdvertised), this.hostname); | ||
} else if (net.isIPv6(this.overrideAdvertised)){ | ||
aaaaUniqueLocalRecordMap[name] = new AAAARecord(this.hostname, this.overrideAdvertised, true); | ||
reverseAddressMap[this.overrideAdvertised] = new PTRRecord(formatReverseAddressPTRName(this.overrideAdvertised), this.hostname); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You documented the option as "If specified, the mDNS server will advertise this address only.". This is not the case.
It currently just adds the address alongside the other addresses of the interface (or addresses filter via restrictedAddresses
option). So it doesn't "override" or "replace" anything. Am I missing something here?
Also, are we handling the loopback interface somewhere? E.g., how should a localhost querier see the service? We definitely should keep the localhost addresses (e.g., 127.0.0.1 or ::1). But exposing the "external" docker address would also be unexpected within the container itself?
Would it make sense then to restructure the "overrideAdvertised" address a bit, to explicitly specify the interface for which the address should be used (e.g., ["en0": "192.168.1.27"]
)? This would also allow to specify different override addresses for each an every interface?
What is you opinion here? Also don't want to make it to complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you specify an IPv4 it'll set that for all interfaces. If the interface has an IPv6 address that one is left alone.
Equally if you put in an IPv6 address it'll set it but leave the IPv4 address alone.
And you're right, there is a mismatch here as we don't override all addresses, only the type (IPv4 or IPv6) of the one specified.
After reading your comments my first thought was to update the setting to something like this: { [if: string]: IPAddress } | IPAddress
In both cases we would ignore lo
as seemingly there is no point to override the advertised address for it.
Now this only allows you to override either IPv4 or IPv6. We could expand it to something like:
type IfIPAddress = IPv4Address | IPv6Address | [IPv4Address, IPv6Address] | [IPv6Address, IPv4Address]
/**
* Service options supplied when creating a new ciao service.
*/
export interface ServiceOptions {
// ...
overrideAdvertised?: { [if: string]: IfIPAddress } | IfIPAddress
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the updated type as explained in #40 (comment) make sense.
If you specify an IPv4 it'll set that for all interfaces. If the interface has an IPv6 address that one is left alone.
Equally if you put in an IPv6 address it'll set it but leave the IPv4 address alone.
And you're right, there is a mismatch here as we don't override all addresses, only the type (IPv4 or IPv6) of the one specified.
In both cases we would ignore
lo
as seemingly there is no point to override the advertised address for it.
Sounds good 👍
@Supereg I started looking at this again. We can override an IPv4 and / or IPv6, either for all, or for a specific interface. For that I propose the following structure:
WDYT? |
@Supereg @donavanbecker thoughts on ^? |
I think the type makes sense. We have great flexibility to override the ip addresses per interface, but also an easy configuration to set a single string as the desired ip address. 👍 |
Cool, let me build that out with your override comments in mind. |
♻️ Current situation
When using HomeBridge with Docker's
bridge
network mode the wrong address is advertised outside of Docker. Many people (including me) don't like to use thehost
mode, as it reduces control.The problem with mDNS is that it broadcasts an address that must be reachable from outside. And this is inherently incompatible with publishing the internal (NATed) address of the container (i.e. in the
172.16.0.0/12
range.💡 Proposed solution
I've added a variable to override the published address. It doesn't change the control given to the library's consumer to define on which network interfaces to bind. It merely overrides the address that is set in the A / AAAA record.
⚙️ Release Notes
Introduce
overrideAdvertise
to set a custom IP address on which the service is reachable.➕ Additional Information
To use this we need to expose this in
hap-nodejs
(already done, need to create PR) andhomebridge
itself. I'm not creating them until this PR has passed scrutiny. I expect changes / feedback which impact the API surface and will merely increase the noise in the consumer's PRs.Testing
None, I merely tested it with Avahi discovery to see that the right addresses were published.
Reviewer Nudging
Where should the reviewer start? what is a good entry point?