-
Notifications
You must be signed in to change notification settings - Fork 141
Add mesh VPN support to the CDH #763
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
base: main
Are you sure you want to change the base?
Add mesh VPN support to the CDH #763
Conversation
00e1f81 to
6fb1d22
Compare
60d552e to
8cdf322
Compare
8062c00 to
2163ae5
Compare
fitzthum
left a comment
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.
A few comments on the first half of the PR
confidential-data-hub/overlay-network/src/overlay_network/nebula.rs
Outdated
Show resolved
Hide resolved
confidential-data-hub/overlay-network/src/overlay_network/nebula.rs
Outdated
Show resolved
Hide resolved
confidential-data-hub/overlay-network/src/overlay_network/nebula.rs
Outdated
Show resolved
Hide resolved
confidential-data-hub/overlay-network/src/overlay_network/nebula.rs
Outdated
Show resolved
Hide resolved
confidential-data-hub/overlay-network/src/overlay_network/nebula.rs
Outdated
Show resolved
Hide resolved
confidential-data-hub/overlay-network/src/overlay_network/config_templates.rs
Outdated
Show resolved
Hide resolved
0f1fdb4 to
f425b00
Compare
f425b00 to
39ee471
Compare
|
Do we want a compile-time feature flag for the overlay-network AND a CdhConfig setting to enable it? We should definitely have the config setting. I'm wondering if the compile-time feature confuses the matter (though it has its merits). |
We probably should have a feature so the code can be completely removed for trypophobes (or people who just want a really small guest tcb). |
39ee471 to
15daed7
Compare
eae3ed2 to
10013de
Compare
10013de to
b576ee3
Compare
b576ee3 to
6c9dca2
Compare
6c9dca2 to
53844ac
Compare
29ff8bd to
8ed80ac
Compare
3b925a1 to
dc3ce4e
Compare
dc3ce4e to
f9f33de
Compare
| pub image: ImageConfig, | ||
|
|
||
| #[serde(default)] | ||
| pub overlay_network: OverlayNetworkConfig, |
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 have this as pub overlay_network: Some(OverlayNetworkConfig) and the OverlayNetworkConfig struct as below, I think you would not need the enable flag as rust would require nebula to be defined at compilation time. Later, if someone adds support for another overlay network we could have them under cargo feature flags.
pub struct OverlayNetworkConfig {
pub nebula: NebulaConfig,
}With this I think you should be able to clean up multiple other places in this PR. E.g. you may not need the validate function and the overlay_network cargo feature flag.
|
|
||
| const NEBULA_BIN: &str = "/opt/overlay-network/nebula"; | ||
| const LIGHTHOUSE_PORT: u32 = 4242; // sync with WORKER_CONFIG_TEMPLATE | ||
| const CA_CERT_PATH: &str = "/tmp/nebula/ca.crt"; |
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 don't know if CDH is multi-threaded or if we will have support for multiple nebula VPNs, but if so it might be better to use tempdir_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.
maybe we handle this if/when we try to tackle multi-VPN?
| let mesh_ip = self.generate_mesh_ip()?; | ||
| let overlay_netmask: Ipv4Addr = self.config.overlay_netmask.parse()?; | ||
| let prefix_len: u32 = self.netmask_to_prefix_len(overlay_netmask); | ||
| let neb_cred_uri: String = format!( |
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.
IP and pod-name are required, but it also have some nice optional fields. I would be great if we can support them as well
https://github.com/confidential-containers/trustee/blob/main/kbs/src/plugins/implementations/nebula_ca.rs#L55
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.
That makes me thinkg if we should add the nebula version somewhere to be able to check for compatibility in the future
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 remember you added these fields. I'm seeing:
/// Optional: how long the cert should be valid for.
/// The default is 1 second before the signing cert expires.
/// Valid time units are seconds: "s", minutes: "m", hours: "h".
duration: Option<String>,
/// Optional: comma separated list of groups.
groups: Option<String>,
/// Optional: comma separated list of ipv4 address and network in CIDR notation.
/// Subnets this cert can serve for
subnets: Option<String>,
I'm wondering now if duration should even be something that the worker node requests. Should it instead be set up on the trustee side? For groups and subnets, I'm tempted to punt to a future PR. For example, I'm not clear on the constraints for the subnets option (and I haven't tested either of these features yet).
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.
That makes me think if we should add the nebula version somewhere to be able to check for compatibility in the future
I also wonder if nebula does this as part of its own protocol.
| message InitOverlayNetworkResponse { | ||
| int32 return_code = 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.
I think we probably want to enable this entirely via the cdh config / init-data rather than through the kata agent. You may still want to get the pod name from the agent, though.
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.
What is the thing that should actually kick the initialization of the overlay network now, though? The CDH is a driver that registers services to listen for requests. For example, it might receive a request to pull an image. Or, in the case of the overlay network, it would receive a request from the kata-agent to start. If kata-agent isn't doing that, what should? In other words, after we register the service here, who actually makes this API call?
f9f33de to
2c8bb5f
Compare
Signed-off-by: Chris Porter <[email protected]>
2c8bb5f to
c2550fb
Compare
RFC issue is here
This PR holds the main guest functionality for overlay network support with Nebula. The main additions are in confidential-data-hub/overlay-network.
Related PRs:
Additional support: