-
Notifications
You must be signed in to change notification settings - Fork 3
Make fake BORD and SIGN values a little more realistic #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
Conversation
davepacheco
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.
LGTM but someone more familiar with tufaceous ought to weigh in, too.
|
Ug, just noticed this means the gimlet RoT and gimlet RoT bootloader artifact are bit-identical. I'll push a change to perturb one or the other of those a bit. EDIT: change pushed. |
karencfv
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.
Thanks! This will certainly help our testing in Omicron
| // have. | ||
| KnownArtifactKind::GimletRot | ||
| | KnownArtifactKind::GimletRotBootloader => { | ||
| Some("sign-gimlet".to_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.
Optional nit: Would be nice to make these fake sign values constants so we can use them in tests in Omicron.
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's actually why I made this a method! We can call it from Omicron tests.
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.
🤦♀️ lol of course
| | KnownArtifactKind::SwitchRotBootloader | ||
| | KnownArtifactKind::GimletRot | ||
| | KnownArtifactKind::PscRot | ||
| | KnownArtifactKind::SwitchRot => "SimRot", |
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.
No need to change anything here, but just curious. What are the consequences of using the real board names here instead of "SimRot", "SimPscSp", etc?
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.
We'd have to change the sp-sim to match for tests to work, I think. I could see an argument for that, but to date we've kept them separate. I think it shouldn't matter much either way, except that it's nice to be able to immediately tell whether something is fake?
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 shouldn't matter much either way, except that it's nice to be able to immediately tell whether something is fake?
Yeah, that makes sense. I guess we can revisit this if we find we need more fidelity to a real environment.
…TufArtifactMeta::board`) (#8872) When trying Reconfigurator-based updates on a racklette today with a real TUF repo, the planner skipped all the RoT bootloader and RoT updates because none of the artifact `name`s matched the `board`s reported in inventory. Our tests were assuming these were the same (and in prod they _are_ the same on the SP), but in real TUF repos these don't match: all the RoT and bootloaders report a `board` of `oxide-rot-1`, but the artifact names are pretty varied. This PR adds a new `board` value to the `tuf_artifact` table and the `TufArtifactMeta` struct. Outside of tests, the changes are almost identical to those from #8729 that added a `sign` field in this same spot. This allows us to stop assuming that `artifact.name` == `inventory_caboose.board`, and instead check `artifact.board` == `inventory_caboose.board`. It depends on oxidecomputer/tufaceous#40, which changes the way our fake data is generated to be more consistent with the state of production devices and artifacts. This perturbs some of the reconfiguartor-cli tests a bit, which significantly inflates the diff; the actual code changes here are pretty small.
When trying Reconfigurator-based updates on a racklette today with a real TUF repo, the planner skipped all the RoT bootloader and RoT updates because none of the artifact
names matched the artifactboards. Artifact name matching board is a property that's true for the SP in both fake data and production, but is only true for the bootloader and RoT in the fake data generated by tufaceous.This PR changes the fake data to be more consistent with real data:
namedoes not necessarily match hubris caboosenameBORDSIGNvalues despite theBORDvalues matchingAs written this would break a bunch of omicron tests (which it should! things aren't working in prod either). I'll open a PR with the corresponding changes there shortly.