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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: latest
Choose a base branch
from
Open

feat: allow overriding the advertised address #40

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions src/CiaoService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,15 @@ export interface ServiceOptions {
* configured to ignore this interface, the service won't be advertised on the interface.
*/
restrictedAddresses?: (InterfaceName | IPAddress)[];

/**
* 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;
Comment on lines +112 to +118
Copy link
Member

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).

Copy link
Author

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.


/**
* The service won't advertise ipv6 address records.
* This can be used to simulate binding on 0.0.0.0.
Expand Down Expand Up @@ -295,6 +304,7 @@ export class CiaoService extends EventEmitter {
private port?: number;

private readonly restrictedAddresses?: Map<InterfaceName, IPAddress[]>;
private readonly overrideAdvertised?: IPAddress;
private readonly disableIpv6?: boolean;

private txt: Buffer[];
Expand Down Expand Up @@ -391,6 +401,15 @@ export class CiaoService extends EventEmitter {
}
}
}

if (options && options.overrideAdvertised) {
if (typeof options.overrideAdvertised === "string") {
this.overrideAdvertised = options.overrideAdvertised;
} else {
throw new Error("Found invalid type for 'overrideAdvertised' NetworkManager option!");
}
}

this.disableIpv6 = options.disabledIpv6;

this.txt = options.txt? CiaoService.txtBuffersFromRecord(options.txt): [];
Expand Down Expand Up @@ -882,6 +901,16 @@ export class CiaoService extends EventEmitter {
aaaaUniqueLocalRecordMap[name] = new AAAARecord(this.hostname, networkInterface.uniqueLocalIpv6, true);
reverseAddressMap[networkInterface.uniqueLocalIpv6] = new PTRRecord(formatReverseAddressPTRName(networkInterface.uniqueLocalIpv6), this.hostname);
}

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);
}
}
Comment on lines +905 to +913
Copy link
Member

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.

Copy link
Author

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!

Copy link
Member

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 👍

}

if (this.subTypePTRs) {
Expand Down