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

Add feature flag to enable discovery and use of public IPaddr for clustering. #2719

Merged
merged 4 commits into from
Nov 10, 2021

Conversation

dtrejod
Copy link
Contributor

@dtrejod dtrejod commented Sep 24, 2021

Before this change, Alertmanager would refuse to startup if using a
advertise address binding to any address (0.0.0.0), and the host only
had an interface with a public IP address. After this change we feature
flag permitting the use of a discovered public address for cluster
gossiping.

…stering.

Before this change, Alertmanager would refuse to startup if using a
advertise address binding to any address (0.0.0.0), and the host only
had an interface with a public IP address. After this change we feature
flag permitting the use of a discovered public address for cluster
gossiping.

Signed-off-by: Devin Trejo <[email protected]>
Signed-off-by: Devin Trejo <[email protected]>
@dtrejod dtrejod force-pushed the dt/public-addr-advertise branch from d414408 to cbea6e7 Compare September 24, 2021 15:47
Copy link
Member

@roidelapluie roidelapluie left a comment

Choose a reason for hiding this comment

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

Thanks!

reconnectInterval = kingpin.Flag("cluster.reconnect-interval", "Interval between attempting to reconnect to lost peers.").Default(cluster.DefaultReconnectInterval.String()).Duration()
peerReconnectTimeout = kingpin.Flag("cluster.reconnect-timeout", "Length of time to attempt to reconnect to a lost peer.").Default(cluster.DefaultReconnectTimeout.String()).Duration()
tlsConfigFile = kingpin.Flag("cluster.tls-config", "[EXPERIMENTAL] Path to config yaml file that can enable mutual TLS within the gossip protocol.").Default("").String()
allowInsecureAdvertise = kingpin.Flag("cluster.allow-insecure-public-advertise-address", "[EXPERIMENTAL] Allow alertmanager to discover and listen on a public address.").Bool()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
allowInsecureAdvertise = kingpin.Flag("cluster.allow-insecure-public-advertise-address", "[EXPERIMENTAL] Allow alertmanager to discover and listen on a public address.").Bool()
allowInsecureAdvertise = kingpin.Flag("cluster.allow-insecure-public-advertise-address-discovery", "[EXPERIMENTAL] Allow alertmanager to discover and listen on a public address.").Bool()

Maybe we should make it clear in the name of the flag that it is only for discovery?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me. Done

@dtrejod dtrejod requested a review from roidelapluie October 20, 2021 21:23
}

// discoverAdvertiseAddress will attempt to get a single IP address to use as
// the advertise address when one is not explictly provided. It defualts to
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// the advertise address when one is not explictly provided. It defualts to
// the advertise address when one is not explicitly provided. It defaults to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Done

if ip == nil {
return nil, errors.Errorf("failed to parse private IP '%s'", privateIP)
if addr == "" {
return nil, errors.New("no public IP found, explicit advertise addr not provided")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return nil, errors.New("no public IP found, explicit advertise addr not provided")
return nil, errors.New("no private/public IP found, explicit advertise addr not provided")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@roidelapluie roidelapluie reopened this Nov 9, 2021
@roidelapluie
Copy link
Member

close/reopen to trigger ci

Copy link
Member

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

lgtm

@roidelapluie roidelapluie merged commit fad7969 into prometheus:main Nov 10, 2021
@roidelapluie
Copy link
Member

Thanks!

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.

3 participants