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

use oxnet::{ IpNet, Ipv4Net, Ipv6Net } #5810

Merged
merged 24 commits into from
May 24, 2024
Merged

use oxnet::{ IpNet, Ipv4Net, Ipv6Net } #5810

merged 24 commits into from
May 24, 2024

Conversation

ahl
Copy link
Contributor

@ahl ahl commented May 22, 2024

No description provided.

use std::net::{Ipv4Addr, Ipv6Addr};

#[test]
fn test_deserialize_allowed_source_ips() {
let parsed: AllowedSourceIps = serde_json::from_str(
r#"{"allow":"list","ips":["127.0.0.1","10.0.0.0/24","fd00::1/64"]}"#,
r#"{"allow":"list","ips":["127.0.0.1/32","10.0.0.0/24","fd00::1/64"]}"#,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnaecker this ok?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, that looks fine to me.

@ahl ahl marked this pull request as ready for review May 23, 2024 18:01
Copy link
Contributor

@andrewjstone andrewjstone left a comment

Choose a reason for hiding this comment

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

This looks great. Love the change from network to prefix. Just small nits from me.

@@ -331,6 +331,7 @@ omicron-certificates = { path = "certificates" }
omicron-passwords = { path = "passwords" }
omicron-workspace-hack = "0.1.0"
oxlog = { path = "dev-tools/oxlog" }
oxnet = { git = "https://github.com/oxidecomputer/oxnet", branch = "main" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we relying on the lockfile to keep this in sync? I usually don't like relying on a branch directly. Can we give it a 1.0 version tag or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now; I want to get to something resembling stable in oxnet, publish to crates.io and depend on that. We have several other branch = main deps

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, works for me.

@@ -419,33 +419,27 @@ impl TryFrom<types::ProducerEndpoint>
}
}

impl TryFrom<&omicron_common::api::external::Ipv4Net> for types::Ipv4Net {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we planning to get rid of the generated types versions at some point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes; going to be rooting that out here and from other places like dendrite and propolis

Copy link
Contributor

Choose a reason for hiding this comment

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

Great!

common/src/address.rs Outdated Show resolved Hide resolved
common/src/api/external/mod.rs Outdated Show resolved Hide resolved
@@ -1334,22 +1335,22 @@ mod test {

let external_dns_ip = IpAddr::V4(Ipv4Addr::new(1, 2, 3, 4));
let external_dns_pip = DNS_OPTE_IPV4_SUBNET
.nth(NUM_INITIAL_RESERVED_IP_ADDRESSES as u32 + 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Very happy to get rid of the use of as.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good news: I added more back in

Copy link
Contributor

Choose a reason for hiding this comment

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

lol

ahl and others added 2 commits May 23, 2024 12:18
@ahl ahl merged commit 8975897 into main May 24, 2024
22 checks passed
@ahl ahl deleted the ipnet branch May 24, 2024 00:40
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