-
Notifications
You must be signed in to change notification settings - Fork 62
Filter VPC by VNI #1906
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
Filter VPC by VNI #1906
Conversation
7b81d51 to
1a96ebb
Compare
|
The test failure on |
|
I'm not sure the Do we need to update the |
Yeah, I'm not wild about the name either. But we need an enum like this (though maybe called something different) that is either an IP address/network or a VNI indicating a VPC. Alternatively, we could (as OPTE did) push the VNI address type down into
The latter (i.e., no update is needed). Any host filter of Rule ID 0 is |
Okay. For me I read That said, this seems good to me. |
bnaecker
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.
Looks good, thanks! A suggestion to help resolve the naming issue @rzezeski brought up, but otherwise great.
common/src/api/external/mod.rs
Outdated
| /// or an entire VPC (identified by its VNI). | ||
| #[derive(Clone, Debug, Deserialize, Serialize, PartialEq, JsonSchema)] | ||
| #[serde(tag = "type", content = "value", rename_all = "snake_case")] | ||
| pub enum VpcAddress { |
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 I understand why this enum exists, to cover the ways we ascribe a firewall rule to a set of hosts. Would a name more like HostIdentifier be appropriate?
I also wonder whether this needs to live in the external API. There are no external calls that should see this, AFAICT, so common/src/api/internal/mod.rs might be more appropriate. There may be some Rust visibility or Cargo dependency that makes that less attractive, but it might be nice to investigate.
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.
HostIdentifier is a great suggestion, thanks! And good call on the internal API; I always forget that we have that. Renamed and moved in 47361a5.
(Minor follow-up question: after much playing around with building, rebuilding, and running the various _openapi tests with EXPECTORATE=overwrite, I still can't figure out why HostIdentifier shows up in sled-agent.json but not nexus-internal.json. The build seems happy, so I'm not going to sweat it, but I'm still a bit confused.)
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 might be a question for @ahl! It's possible that types only show up in the OpenAPI output when they are actually needed to make requests to a given server. Since this type is technically only used by Nexus making calls to the sled agent, only including needed names could explain it. But that's a WAG :)
Fixes #1756 (using option 3) and fixes #1821 (using new OPTE default actions and proper VPC/VNI host filters). Depends on #1904.