-
Notifications
You must be signed in to change notification settings - Fork 40
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
Omdb networking #4147
Omdb networking #4147
Conversation
db0835b
to
cea99a3
Compare
dev-tools/omdb/tests/successes.out
Outdated
@@ -84,6 +84,24 @@ stderr: | |||
note: using database URL postgresql://root@[::1]:REDACTED_PORT/omicron?sslmode=disable | |||
note: database schema version matches expected (4.0.0) | |||
============================================= | |||
EXECUTING COMMAND: omdb ["db", "network"] |
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.
It seems like this output would go in the "usage" test (if you want to cover it). The point of the "successes" test is to sanity-check that the command produces reasonable-looking output against a real system. You might add omdb db network list-eips
to this test instead? Or if it's too annoying to get that to work (you might have to add more redactions for the output to be stable across runs), skip it altogether?
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.
Moved this test to usage.
|
||
/// Fetch all external IP addresses of any kind for the provided instance | ||
pub async fn lookup_external_ips( | ||
&self, | ||
opctx: &OpContext, | ||
) -> LookupResult<Vec<ExternalIp>> { | ||
use db::schema::external_ip::dsl; | ||
dsl::external_ip | ||
.filter(dsl::time_deleted.is_null()) | ||
.select(ExternalIp::as_select()) | ||
.get_results_async(self.pool_authorized(opctx).await?) | ||
.await | ||
.map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) | ||
} |
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 it's pretty dangerous to add a non-authz-gated datastore operation like this. If this is a generally useful function, I'd just do the authz check. If we don't think we'd need this kind of functionality in Nexus, or if authz is a problem, I think it's fine to just put this code straight into omdb (without an authz check).
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.
Sounds good. I just moved this into the omdb code.
dev-tools/omdb/src/bin/omdb/db.rs
Outdated
.await | ||
.context("loading requested instance")? | ||
.pop() | ||
.context("requested instance not found")?; |
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.
Not critical but I'd consider continuing rather than bailing out for this case (so that if there's an errant "owner" in the database, we can still use the tool to see as much as we can).
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 call. Done
dev-tools/omdb/src/bin/omdb/db.rs
Outdated
.await | ||
.context("loading requested project")? | ||
.pop() | ||
.context("requested project not found")?; |
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.
Same -- I'd considering continuing instead of bailing out here (but definitely not a blocker).
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.
Done
|
||
#[derive(Tabled)] | ||
enum Owner { | ||
Instance { project: String, name: String }, |
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 always wonder if the tool should report ids (which are immutable and unique across silos) or names like this that potentially save someone a separate lookup (or both, but that uses up valuable space too). It's all tradeoffs -- just mentioning it to see what you think.
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 explicitly chose names here - as my use case for this is mapping external IPs to something immediately tangible. I added an option for verbose output that provides the raw data structures from the DB that shows references based on UUID.
Thanks for doing this! This will be very useful. |
5934ba8
to
1e9179b
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.
Just spotted one stray commented-out block but otherwise looks good!
dev-tools/omdb/src/bin/omdb/db.rs
Outdated
/* | ||
let ips = datastore | ||
.lookup_external_ips(&opctx) | ||
.await | ||
.context("listing external ips")?; | ||
*/ |
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.
nit: probably want to remove this
I can't tell if this was here before because I can't access the incremental review. (Maybe a force push?)
Adds an
omdb
command to list external network IP addresses along with their parent.Also comes with a verbose mode to print raw datastore structures.