Read certificate files directly#1267
Conversation
Customers have found it inconvenient and error-prone to pass in the contents of certs and keys to the `certificate create` subcommand. To make this command easier to use, update its `key` and `cert` arguments to be treated as the path to their respective files, rather than their contents. We previously updated the SAML IdP creation command the same way with 11fe44b (Take paths instead of base64 for SAML creation (#1112), 2025-05-28).
| CliCommand::CertificateCreate => cmd | ||
| .mut_arg("cert", |arg| { | ||
| arg.value_name("cert-file") | ||
| .help("path to PEM-formatted string containing public certificate chain") | ||
| }) | ||
| .mut_arg("key", |arg| { | ||
| arg.value_name("key-file") | ||
| .help("path to PEM-formatted string containing public certificate chain") | ||
| }), |
There was a problem hiding this comment.
Add a comment?
We'd like users to provide a file rather than an inline string value. We avoid having the generated code that would normally handle these arguments misinterpret these as string by simply changing the names of the arguments.
There was a problem hiding this comment.
To be clear, the value_name calls are adding a placeholder value to the help text. I've capitalized these and the other value_names to be consistent with the generated code.
Options:
--cert <CERT-FILE>
path to a PEM-formatted file containing a public certificate chain
Added a comment explaining the change.
| matches: &clap::ArgMatches, | ||
| request: &mut oxide::builder::CertificateCreate, | ||
| ) -> anyhow::Result<()> { | ||
| let key_path = matches.get_one::<String>("key").unwrap(); |
There was a problem hiding this comment.
worth mentioning that the name of the arg is "key" but the actual parameter is "key-file"?
There was a problem hiding this comment.
I don't think so, the value_name is purely for display purposes in the help string.
There was a problem hiding this comment.
Oh, I misunderstood completely. So the generated code does do the wrong thing and then we fix it? Is that as-intended?
There was a problem hiding this comment.
It's not so much that the generated code is wrong, as that it's not a great user experience. We can make the user's life easier by reading the file for them.
There was a problem hiding this comment.
The generated code is wrong in that it first puts the file names in those body params
| *request = request | ||
| .to_owned() | ||
| .body_map(|body| body.key(key_bytes).cert(cert_bytes)); | ||
| Ok(()) |
There was a problem hiding this comment.
should we have a version of body_map we could use on a &mut?
There was a problem hiding this comment.
It is a little awkward using body_map like this, but not so much that it bothered me. Given the very small number methods we're overriding, I think the extra codegen from adding a new method isn't worth it.
| .with_context(|| format!("failed to read cert file {cert_path}"))?; | ||
|
|
||
| *request = request | ||
| .to_owned() |
There was a problem hiding this comment.
Why are we calling to_owned rather than, say, clone? I'm not asking you to change it, I'm asking why you made this choice.
There was a problem hiding this comment.
I was following the pattern used in the existing overrides like execute_networking_allow_list_update
Line 345 in 543d044
clone is probably slightly clearer here, since it's just a normal ref we're copying, but I don't feel strongly about it.
Fix help string Co-authored-by: Adam Leventhal <ahl@oxide.computer>
| .with_context(|| format!("failed to read cert file {cert_path}"))?; | ||
|
|
||
| *request = request | ||
| .to_owned() |
2 participants
Customers have found it inconvenient and error-prone to pass in the contents of certs and keys to the
certificate createsubcommand. To make this command easier to use, update itskeyandcertarguments to be treated as the path to their respective files, rather than their contents.We previously updated the SAML IdP creation command the same way with 11fe44b (Take paths instead of base64 for SAML creation (#1112), 2025-05-28).