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

[sled agent] Add default routes, using GZ addresses as gateways #1147

Merged
merged 8 commits into from
Jun 5, 2022

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Jun 2, 2022

Follow-up to #1066

This ensures that L3 routing from off-device can reach the external Nexus interface.

@smklein smklein requested review from bnaecker and rcgoodfellow June 3, 2022 10:53
sled-agent/src/services.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@bnaecker bnaecker left a comment

Choose a reason for hiding this comment

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

Looks good!

Just to make sure I understand, the issue here was that by hanging the IP addresses of an etherstub, they're acting like a separate L2 segment, even though they're in the same L3 subnet of the sled. Thus we need forwarding, to get the OS to deliver packets from the GZ's L2 segment to the etherstub's, and routing for the OS to automatically update routing tables to describe the interfaces used to do that forwarding. Is that a fair summary?

@rmustacc
Copy link

rmustacc commented Jun 3, 2022

So a non-global zone shouldn't need anything special here whether with maghemite or special services enabled here. What led us to this conclusion and were we setting a default gateway?

@smklein
Copy link
Collaborator Author

smklein commented Jun 3, 2022

Just to make sure I understand, the issue here was that by hanging the IP addresses of an etherstub, they're acting like a separate L2 segment, even though they're in the same L3 subnet of the sled. Thus we need forwarding, to get the OS to deliver packets from the GZ's L2 segment to the etherstub's, and routing for the OS to automatically update routing tables to describe the interfaces used to do that forwarding. Is that a fair summary?

I believe so? The concrete issue I faced was:

  • Try to ping sled agent address (in Helios GZ) from a different machine (my Linux laptop) -> Observe success
  • Try to ping nexus address (in Helios non-GZ) from a different machine (my Linux laptop) -> Observe failure
  • experimentally, updating the routeadm arguments fixed this issue.

So a non-global zone shouldn't need anything special here whether with maghemite or special services enabled here.

Just to make sure we're on the same page - Maghemite is not currently integrated with Omicron, so this code is not assuming it's present.

What led us to this conclusion and were we setting a default gateway?

We set routes in non-global zones here:

let gz_route_subnet = if !service.gz_addresses.is_empty() {
// If this service supplies its own GZ address, add a route.
//
// This is currently being used for the DNS service.
//
// TODO: consider limitng the number of GZ addresses which
// can be supplied - now that we're actively using it, we
// aren't really handling the "many GZ addresses" case, and it
// doesn't seem necessary now.
Ipv6Network::new(service.gz_addresses[0], AZ_PREFIX).unwrap()
} else {
// Otherwise, add a route to the global Zone's sled address for
// everything within the AZ.
Ipv6Network::new(self.underlay_address, AZ_PREFIX).unwrap()
};
running_zone.add_route(gz_route_subnet).await.map_err(|err| {
Error::ZoneCommand { intent: "Adding Route".to_string(), err }
})?;

Which is implemented by this:

pub async fn add_route(
&self,
destination: ipnetwork::Ipv6Network,
) -> Result<(), RunCommandError> {
self.run_cmd(&[
"/usr/sbin/route",
"add",
"-inet6",
&format!("{}/{}", destination.network(), destination.prefix()),
"-inet6",
&destination.ip().to_string(),
])?;
Ok(())
}

To be clear, I'm totally open to alternate solutions in this space.

@rmustacc
Copy link

rmustacc commented Jun 3, 2022

I believe so? The concrete issue I faced was:

* Try to ping sled agent address (in Helios GZ) from a different machine (my Linux laptop) -> Observe success

* Try to ping nexus address (in Helios non-GZ) from a different machine (my Linux laptop) -> Observe failure

* experimentally, updating the `routeadm` arguments fixed this issue.

I'd like to work with you to root cause this a bit more. In particular, the thing that's weird here is that in a normal context, we're not actually supposed to be forwarding in the non-global zone. Forwarding would suggest that we have multiple interfaces and want to take traffic that came in on one that wasn't destined for us and that it should be sent out another (or some other internal rewriting is going on with ipf which shouldn't be on the scene here). So in this context to me, the requirement to have forwarding doesn't make sense and seems like something else may be going on here.

So a non-global zone shouldn't need anything special here whether with maghemite or special services enabled here.

Just to make sure we're on the same page - Maghemite is not currently integrated with Omicron, so this code is not assuming it's present.

My point was that you said in the start of this:

Presumably we'll have a more sophisticated solution here once maghemite is integrated, but this works in the meantime.

And that the integration of maghemite should not impact the non-global zone as it will not run there or the set of configuration that it requires.

What led us to this conclusion and were we setting a default gateway?

We set routes in non-global zones here:

let gz_route_subnet = if !service.gz_addresses.is_empty() {
// If this service supplies its own GZ address, add a route.
//
// This is currently being used for the DNS service.
//
// TODO: consider limitng the number of GZ addresses which
// can be supplied - now that we're actively using it, we
// aren't really handling the "many GZ addresses" case, and it
// doesn't seem necessary now.
Ipv6Network::new(service.gz_addresses[0], AZ_PREFIX).unwrap()
} else {
// Otherwise, add a route to the global Zone's sled address for
// everything within the AZ.
Ipv6Network::new(self.underlay_address, AZ_PREFIX).unwrap()
};
running_zone.add_route(gz_route_subnet).await.map_err(|err| {
Error::ZoneCommand { intent: "Adding Route".to_string(), err }
})?;

Which is implemented like this:

OK. I would have expected just a simple default route for everything to go back to the gateway. So I think we can simplify what's going on here.

@smklein smklein changed the title [sled agent] Enable IPv6 routing for non-global zones [sled agent] Add default routes, using GZ addresses as gateways Jun 3, 2022
@smklein
Copy link
Collaborator Author

smklein commented Jun 3, 2022

Robert, reading your feedback, I agree with you - routing / forwarding really shouldn't be necessary for the non-global zones.

I went ahead and re-did this PR - it accomplishes the same goal, but through a different mechanism. Rather than enabling this routing in non-global zones, I changed the way routes are getting set up.

Rather than routing to addresses within the AZ of the system, I implemented your suggestion, and added a default route which points to a corresponding gateway address from the global zone.

This works - I can now access Nexus from off-machine. This is kinda interesting - I basically only changed the routing tables of the non-global zones, which... shouldn't have been necessary to actually reach those zones, but perhaps may have been necessary for ping to respond to addresses coming from outside the AZ subnet.

@rcgoodfellow
Copy link
Contributor

Try to ping sled agent address (in Helios GZ) from a different machine (my Linux laptop) -> Observe success
Try to ping nexus address (in Helios non-GZ) from a different machine (my Linux laptop) -> Observe failure

Do I understand correctly that in the above, your laptop was pinging from a source address outside the AZ /48, the non-global zones were set up with a route onto the AZ /48, and changing that route to be a more general default route allowed communications to succeed from your laptop?

@smklein
Copy link
Collaborator Author

smklein commented Jun 3, 2022

Try to ping sled agent address (in Helios GZ) from a different machine (my Linux laptop) -> Observe success
Try to ping nexus address (in Helios non-GZ) from a different machine (my Linux laptop) -> Observe failure

Do I understand correctly that in the above, your laptop was pinging from a source address outside the AZ /48, the non-global zones were set up with a route onto the AZ /48, and changing that route to be a more general default route allowed communications to succeed from your laptop?

This is accurate, as far as I understand it.

Copy link
Contributor

@rcgoodfellow rcgoodfellow left a comment

Choose a reason for hiding this comment

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

LGTM

@smklein smklein enabled auto-merge (squash) June 5, 2022 15:47
@smklein smklein disabled auto-merge June 5, 2022 15:48
@smklein smklein merged commit a23b0d5 into main Jun 5, 2022
@smklein smklein deleted the ipv6-forwarding branch June 5, 2022 15:48
leftwo pushed a commit that referenced this pull request Feb 9, 2024
Crucible changes:
Remove unused fields in IOop (#1149)
New downstairs clone subcommand. (#1129)
Simplify the do_work_task loop (#1150)
Move `Guest` stuff into a module (#1125)
Bump nix to 0.27.1 and use new safer Fd APIs (#1110)
Move `FramedWrite` work to a separate task (#1145)
Use fewer borrows in ExtentInner API (#1147)
Update Rust crate reedline to 0.28.0 (#1141)
Update Rust crate tokio to 1.36 (#1143)
Update Rust crate slog-bunyan to 2.5.0 (#1139)
Update Rust crate rayon to 1.8.1 (#1138)
Update Rust crate itertools to 0.12.1 (#1137)
Update Rust crate byte-unit to 5.1.4 (#1136)
Update Rust crate base64 to 0.21.7 (#1135)
Update Rust crate async-trait to 0.1.77 (#1134)
Discard deferred msgs (#1131)
Minor Downstairs cleanup (#1127)
Update test_fail_live_repair to support pstop (#1128)
Ignore client messages after stopping the IO task (#1126)
Move client IO task into a struct (#1124)
Bump Rust to 1.75 and fix new Clippy lints (#1123)

Propolis changes:
PHD: convert to async (#633)
PHD: assume specialized Windows images (#636)
propolis-standalone-config needn't be a crate
standalone: Use tar for snapshot/restore
phd: use latest "lab-2.0-opte" target, not a specific version (#637)
PHD: add tests for migration of running processes (#623)
PHD: fix `cargo xtask phd` tidy not doing anything (#630)
PHD: add documentation for `cargo xtask phd` (#629)
standalone: improve virtual device creation errors (#632)
phd: add Windows Server 2019 guest adapter (#627)
PHD: add `cargo xtask phd` to make using PHD nicer (#619)
leftwo added a commit that referenced this pull request Feb 9, 2024
Crucible changes:
Remove unused fields in IOop (#1149)
New downstairs clone subcommand. (#1129)
Simplify the do_work_task loop (#1150)
Move `Guest` stuff into a module (#1125)
Bump nix to 0.27.1 and use new safer Fd APIs (#1110) Move `FramedWrite`
work to a separate task (#1145) Use fewer borrows in ExtentInner API
(#1147)
Update Rust crate reedline to 0.28.0 (#1141)
Update Rust crate tokio to 1.36 (#1143)
Update Rust crate slog-bunyan to 2.5.0 (#1139)
Update Rust crate rayon to 1.8.1 (#1138)
Update Rust crate itertools to 0.12.1 (#1137)
Update Rust crate byte-unit to 5.1.4 (#1136)
Update Rust crate base64 to 0.21.7 (#1135)
Update Rust crate async-trait to 0.1.77 (#1134)
Discard deferred msgs (#1131)
Minor Downstairs cleanup (#1127)
Update test_fail_live_repair to support pstop (#1128) Ignore client
messages after stopping the IO task (#1126) Move client IO task into a
struct (#1124)
Bump Rust to 1.75 and fix new Clippy lints (#1123)

Propolis changes:
PHD: convert to async (#633)
PHD: assume specialized Windows images (#636)
propolis-standalone-config needn't be a crate
standalone: Use tar for snapshot/restore
phd: use latest "lab-2.0-opte" target, not a specific version (#637)
PHD: add tests for migration of running processes (#623) PHD: fix `cargo
xtask phd` tidy not doing anything (#630) PHD: add documentation for
`cargo xtask phd` (#629) standalone: improve virtual device creation
errors (#632) phd: add Windows Server 2019 guest adapter (#627)
PHD: add `cargo xtask phd` to make using PHD nicer (#619)

Co-authored-by: Alan Hanson <[email protected]>
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.

4 participants