-
Notifications
You must be signed in to change notification settings - Fork 90
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
KBS: refactor code structure #430
Conversation
a4756c4
to
58973f7
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.
I think this is a big improvement. A few comments.
One high-level thing is that I wonder if we want to have kbs/kbs
and attestation-service/attestation-service
dirs. It seems like we could move Cargo.toml
and build.rs
up a dir and get rid of the second kbs/as directory. wdyt?
|
fc09e81
to
8a28850
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.
Ok two more notes but I think this is very close.
We should probably wait until Trustee meeting on monday to merge since it is a pretty big change.
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 verifier could also potentially move a level up. It's not a primary component, but it is some of the most significant code in the project.
I don't feel super strongly about this, but my hunch would be to just get rid of deps.
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.
although verifier is important code, if moved to a level up it would be somehow weird to me as it can not work independently. When we add more binaries in future, i prefer to have all root level directories be runable binaries, or a sub directory. If we want to show the importance of the crate, probably we could change the name deps
to some else.
Or, another way for this is to make verifier crate a sub module of attestation service. This is somehow a regression. We once do the separation to make it possible for the crate to be imported by other projects.
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.
Yeah I'm not exactly sure what's best here. I think it's a little weird to have it in deps but idk where else to put it. maybe @mkulke has a great idea
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 guess this is bikeshedding to some degree, but my personal pref would be that a ./deps
folder only contains modules that are shared among one of the top-level projects, i.e. kbs
,attestation-service
,rvps
that work as individual units (kbs-client could be a bin
target of kbs maybe).
In Rust the folder structure doesn't imply a hierarchy of modules and crates. so it's completely fine IMO to have attestation-service/verifier
as a workspace member if we want to associate the verifier
code logically with attestation-service, but for $reasons want to put it in its own crate.
Putting modules in their own crate doesn't help with compile times and often you have to expose the module-crate's feature flags on the "parent crate", which is chore and a source of errors. so $reasons should be grounded in a real use case, not just modularization in anticipation of such a use case.
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.
kbs-client could be a bin target of kbs maybe
It is hard to... although the name is "client of kbs", they two have difinitely different deps and especially features. kbs-client has a bunch of *-attester
feature while kbs does not, because it does not care about this.
The ideal place to put as a bin is guest-components/attestation-agent/kbs_protocol
, but it potentially would be another annoying topic.
Putting modules in their own crate doesn't help with compile times and often you have to expose the module-crate's feature flags on the "parent crate", which is chore and a source of errors.
Yes. We are doing this for many parts of CoCo code, like attester/kbs_protocol/attestation-service in guest-components.
so $reasons should be grounded in a real use case, not just modularization in anticipation of such a use case.
Agreed. Currently we are lacking of use case, so probably leave them as-is, i.e. leaving verifier
a separate crate, until some promotion ideas come.
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 changed the code structure to
.
├── attestation-service
├── kbs
├── rvps
├── deps
│ └── verifier
└── tools
└── kbs-client
This would still need some discussion and thinking. Please feel free to share any ideas or give it a approve when it is fine
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.
Yeah one option would be to have the verifiers not be their own package and move them under the kbs. I would be fine with this, but keeping them under deps is ok too. I don't really care either way.
This commit will tidy the code under kbs directory. It deletes api-server crate, and move all logic of api-server to KBS crate, also mark kbs as a binary of the crate. This commit also makes some common dependencies of sub-crates to use the same version of workspace. Signed-off-by: Xynnn007 <[email protected]>
actions-rs is now out of date and will be reported as an error by actionlint. GHA runners have rustup installed already, thus we only need to set the toolchain to the version we want. Signed-off-by: Xynnn007 <[email protected]>
Combined Azure vTPM ci yamls. Also rename the ci pipelines with a unified name format for better preference. Signed-off-by: Xynnn007 <[email protected]>
if oras push failed, the original ci would still return true, which is not expected. Signed-off-by: Xynnn007 <[email protected]>
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
This commit will tidy the code under kbs directory. It deletes api-server crate, and move all logic of api-server to KBS crate, also mark kbs as a binary of the crate.
This commit also makes some common dependencies of sub-crates to use the same version of workspace.
Let's mark this PR draft until the release and other PRs, or it will cause a bunch of rebases.