-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Network segments #3431
Network segments #3431
Conversation
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.
Made an initial pass and noted a few things.
agent/agent.go
Outdated
Leave() error | ||
LANMembers() []serf.Member | ||
LANSegmentMembers(name string) ([]serf.Member, error) |
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.
We should s/name/segment/ for this one so it's clearer what this does.
agent/agent.go
Outdated
|
||
segments = append(segments, consul.NetworkSegment{ | ||
Name: segment.Name, | ||
Bind: segment.Bind, |
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.
The Bind
and Advertise
values here won't be correct if they were empty any we used the default ones. Is that OK?
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.
Good catch, fixed
agent/agent.go
Outdated
@@ -2105,6 +2169,11 @@ func (a *Agent) loadMetadata(conf *Config) error { | |||
a.state.metadata[key] = value | |||
} | |||
|
|||
// The segment isn't reloadable so we only add it once. |
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.
Why did I add this? It seems like we call unloadMetadata
anyway, so this extra check isn't necessary and we can just add it always.
agent/agent_endpoint.go
Outdated
var members []serf.Member | ||
if wan { | ||
members = s.agent.WANMembers() | ||
} else { | ||
} else if segment == "" { |
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.
This could be simplified to always just call LanSegmentMembers(segment)
since that can handle the empty string case internally.
agent/config.go
Outdated
// (Enterprise-only) NetworkSegment is the network segment for this client to join | ||
Segment string `mapstructure:"segment"` | ||
|
||
// Segments |
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.
Needs a doc comment.
agent/consul/config.go
Outdated
@@ -49,6 +49,16 @@ func init() { | |||
} | |||
} | |||
|
|||
// (Enterprise-only) |
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.
Should have a brief doc comment.
// Fire the event | ||
return m.srv.serfLAN.UserEvent(eventName, args.Payload, false) | ||
// Fire the event on all LAN segments | ||
segments := m.srv.LANSegments() |
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.
How come you decided not to include the default segment in LANSegments()
, which would eliminate the special cases? I think you had that way partway through development.
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.
Yeah, I removed it from the segments map on the server but I should have re-added it to the map that LANSegments()
returns, fixed
} else { | ||
mgr = m.srv.KeyManagerLAN() | ||
segments := m.srv.LANSegments() |
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.
Doh this doesn't special case for the default segment, see comment above we should probably just put the default segment in here and avoid the special cases.
command/agent.go
Outdated
} | ||
|
||
if !cfg.Server && len(cfg.Segments) > 0 { | ||
cmd.UI.Error("Cannot define segments on clients") |
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.
Maybe "Segments can only be defined on servers" to be more symmetric with the other message.
c2766bd
to
19fd123
Compare
19fd123
to
7e565d7
Compare
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.
Made a detailed pass through everything. Nice job on the guide - I think that's super easy to follow. Noted some things and then I think we are good to merge this.
@@ -2105,6 +2169,8 @@ func (a *Agent) loadMetadata(conf *Config) error { | |||
a.state.metadata[key] = value | |||
} | |||
|
|||
a.state.metadata[structs.MetaSegmentKey] = conf.Segment |
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.
Now I remember why I did that - since the segment isn't reloadable, this would let them change their metadata about it. I think this is fine as-is vs. adding code complexity (my thing didn't work since the map is empty). We explicitly document what can be reloaded.
agent/config.go
Outdated
return fmt.Errorf("Cannot exceed network segment limit of %d", SegmentLimit) | ||
} | ||
|
||
takenPorts := make(map[int]string, len(conf.Segments)) |
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.
Realized we also need to make sure the names are unique, otherwise we will overlay one on the other when we create them.
agent/config.go
Outdated
return &result, nil | ||
} | ||
|
||
func ValidateSegments(conf *Config) error { |
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.
In OSS we should probably bail with an error if they set the segment on a client, and if they set up any segments for servers. I wonder if it's worth splitting an agent/segment_stub.go here in OSS and creating ValidateSegments()
into there that does those checks, and then moving this into agent/segment.go in Enterprise to represent the two behaviors (you'd have to move the test for this over into Enterprise as well).
agent/consul/segment_stub.go
Outdated
) | ||
|
||
const ( | ||
errSegmentsNotSupported = "network segments are not supported in this version of Consul" |
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 pull out an agent-level segment.go and stub, you could move this error to a common place (maybe structs)?
agent/consul/server.go
Outdated
raftState = "raft/" | ||
snapshotsRetained = 2 | ||
serfLANSnapshot = "serf/local.snapshot" | ||
serfLANSegmentSnapshot = "serf/local-segment-%s.snapshot" |
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.
This could get moved to Enterprise agent/consul/segment.go.
website/source/api/query.html.md
Outdated
@@ -87,6 +87,25 @@ populate the query before it is executed. All of the string fields inside the | |||
doesn't match, or an invalid index is given, then `${match(N)}` will return an | |||
empty string. | |||
|
|||
- `${agent.segment}` has the network segment (Enterprise only) of the agent that |
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.
Enterprise-only
(hyphen for consistency with other places)
# Partial LAN Connectivity | ||
## Configuring Network Segments | ||
|
||
[//]: # ( ~> The network area functionality described here is available only in ) |
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.
Change "network area" to "network segment" and change version to 0.9.3.
By default, all Consul agents in one datacenter are part of a shared gossip pool over the LAN; | ||
this means that the partial connectivity caused by segmented networks would cause health flapping | ||
as nodes failed to communicate. In this guide we will cover the Network Segments feature, added | ||
in [Consul Enterprise](https://www.hashicorp.com/products/consul/) version 0.9.3, which allows users |
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.
Need to link this network segments guide from a new landing page on https://www.consul.io/docs/enterprise/index.html..
|
||
Once the servers have been configured with the correct segment info, the clients only need to specify | ||
their own segment in the [Agent Config](/docs/agent/options.html) and join by connecting to another | ||
agent within the same segment. |
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.
We should add something like "If joining to a Consul server, client will need to provide the server's port for their segment along with the address of the server when performing the join (eg. consul agent -retry-join "consul.domain.internal:1234"
).
client1 4c29819c 192.168.0.5 dc1 | ||
``` | ||
|
||
With this metadata key, we can construct a prepared query that can be used for DNS to return |
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'd link to "/api/query.html" for "prepared query".
command/segment_stub.go
Outdated
return nil, nil | ||
members, err := client.Agent().MembersOpts(consulapi.MembersOpts{}) | ||
if err != nil { | ||
return nil, fmt.Errorf("Error retrieving members: %s", err) |
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.
Just return err
here, since you prepend the same thing above.
@@ -18,6 +18,7 @@ increases both scalability and resilience. Features include: | |||
- [Redundancy Zones](/docs/enterprise/redundancy/index.html) | |||
- [Advanced Federation for Complex Network | |||
Topologies](/docs/enterprise/federation/index.html) | |||
- [Network Segments](/docs/guides/segments.html) |
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.
ping @KFishner this is fine for now but do you want to write a marketing landing thing for later?
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.
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.
One tiny nit then LGTM.
command/segment_stub.go
Outdated
func getSegmentMembers(client *consulapi.Client) ([]*consulapi.AgentMember, error) { | ||
members, err := client.Agent().MembersOpts(consulapi.MembersOpts{}) | ||
if err != nil { | ||
return nil, fmt.Errorf("Error retrieving members: %s", err) |
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.
@kyhavlov just return err
here since we add the same string above
Adds the OSS components for network segments. This feature allows clients to be split up into named network segments, in order to separate their gossip layer from clients in different segments. The client's segment is stored in the
consul-network-segment
key in its node metadata.Servers are always in all segments, and each segment can be configured with its own bind/advertise address and port (along with an option for an rpc listener if the segment's bind address is different from the default rpc addr).