Conversation
charludo
requested review from
burgerdev,
katexochen,
m1ghtym0,
sespiros and
thomasten
burgerdev
left a comment
burgerdev
left a comment
There was a problem hiding this comment.
Thanks a lot, this is a great doc and I really like the approach!
| # ... | ||
| ``` | ||
|
|
||
| However, the need for this seems questionable: for all options shared between the commands, values aren't expected to differ. |
There was a problem hiding this comment.
I agree with that judgement, with the minor difference that verify might be called very differently than the other commands, because verify is a data owner API while the others are workload owner APIs. However, I'd expect the data owner and the workload owner to use (slightly) different configuration files anyway because of filesystem paths.
There was a problem hiding this comment.
Hm, good point. Unfortunately, using slightly differing config files for two roles is also a bit inconvenient, since then large parts need to be maintained in duplicate by the user.
We could:
- support nested TOML structs as shown above, i.e. setting
cli.verify.x, where that value, if present, takes precedence beforecli.x; that would probably be "nicest", but significantly complicate the Go side of the configuration, - make
--configrepeatable, later file contents overwriting earlier one's, but I don't think I've ever seen that used and it might be counterintuitive, - instruct users to use cli flags with verify for the values in question, but that partially defeats the purpose of the file
Or do you have additional ideas here?
| } | ||
| ``` | ||
|
|
||
| Here, `AddArgs` takes the provided (default) `Config` and the `cobra.Command`, as well as a slice of argument names matching the ones used in the `toml` annotations. |
There was a problem hiding this comment.
The additional slice of arguments is a kind of filter, so that not all arguments are passed to all subcommands, right? I wonder whether we could solve that via annotation as well.
There was a problem hiding this comment.
The additional slice of arguments is a kind of filter, so that not all arguments are passed to all subcommands, right?
Yes, exactly. Clarified it in the text.
I wonder whether we could solve that via annotation as well.
Something like:
type Config struct {
CLI struct {
// ...
ManifestPath string `toml:"manifest" short:"m" comment:"path to manifest (.json) file" for="generate,set,verify,recover"`
// ...
}
}you mean? Sure, why not. I initially thought it might be too long/unreadable, but that looks not unreasonable. Let me know if you'd rather go with this, then I'll make the adjustments here!
| Here, `AddArgs` takes the provided (default) `Config` and the `cobra.Command`, as well as a slice of argument names matching the ones used in the `toml` annotations. | ||
| It then adds a `cobra.Command` argument for each one of those names, using the metadata from the `toml` annotations for short names and help texts. | ||
|
|
||
| Validation of the provided arguments continues to work just as it currently does (for example `parseGenerateFlags`), with the only difference being that this also runs on the new `Config` struct. |
There was a problem hiding this comment.
It might make sense to move that logic to the Config struct, too, so that we have all the logic in one place.
There was a problem hiding this comment.
Yeah this formulation is really ambiguous. I think what you suggested was my intention back then, but I'm not sure. Edited.
| A version field may be included in the configuration file to allow explicit handling of breaking changes: | ||
|
|
||
| ```toml | ||
| version = 1.17 | ||
| ``` |
There was a problem hiding this comment.
My gut feeling is that we should version the format from the beginning, since adding a version tag later is usually a source of (minor) headaches. I'd also tend towards decoupling the config file format version from the Contrast version, that way it's a bit easier to maintain implementations for multiple.
There was a problem hiding this comment.
we should version the format from the beginning
Sounds good to me.
I'd also tend towards decoupling the config file format version from the Contrast version, that way it's a bit easier to maintain implementations for multiple.
I'm not sure I understand this. Wouldn't making a change to the config format require releasing a new contrast version anyways? And in turn, the migration from one Contrast version to another, if nothing was changed in the config format, would just be noop/identity?
| This would allow us to perform compatibility checks when loading older configurations. | ||
| However, versioning is considered optional at this stage and may be introduced later if required. | ||
|
|
||
| ### Backward and Forward Compatibility |
There was a problem hiding this comment.
Nit: forward compat is mentioned here, but not addressed. I think we're already good with a versioned file format, but we need to ensure that we can migrate older configs to current ones. It may be enough to ensure that only the most stable flags are configurable.
There was a problem hiding this comment.
It may be enough to ensure that only the most stable flags are configurable.
Not sure I agree, that also somewhat defeats the purpose, if some flags still need to be communicated out of band, so to speak. But yes, re-reading this now, there's a noticeable lack of details on how the migrations should work 😅 I'll have a think on that.
| The actual values here are JSON-patches, in keeping with how we handle our own reference value patches. | ||
|
|
||
| Again, these sections should be completely optional. | ||
| Either passing `--reference-values` to the CLI *or* setting `cli.reference_values` in the CLI section of the configuration file should preclude the use of these values. |
There was a problem hiding this comment.
But these are different: --reference-values is used for things beyond the users' control, such as the trusted measurement of the guest VM, whereas the things filled in by the user are usually depend on the target machines. I think both should be respected, in order.
There was a problem hiding this comment.
Oh, right. Would you say we should limit the patches to e.g. the mintcb values?
2 participants
rendered version