Skip to content
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

Consolidate oak containers protos into one ergonomic file for customer teams, and add relevant build targets #4663

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jul-sh
Copy link
Contributor

@jul-sh jul-sh commented Jan 17, 2024

All interfaces exposed to the application are now in proto/containers/application_interfaces.proto. Previously they were split across multiple different proto files.

Adds cpp and python build targets for them, so customer teams do not have to patch our build file to add them.

Also separates them from internal interfaces that customer teams don't need to be exposed to. Those are now part of proto/containers/launcher.proto.

@jul-sh jul-sh changed the title Consolidate oak containers protos into one ergonimic file for customer teams, and add relevant build targets Consolidate oak containers protos into one ergonomic file for customer teams, and add relevant build targets Jan 17, 2024
Copy link
Collaborator

@tiziano88 tiziano88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Juliette! Will this change be breaking, once imported into google3, or should no one depend on those protos that were moved?

@jul-sh
Copy link
Contributor Author

jul-sh commented Jan 17, 2024

Thanks Juliette! Will this change be breaking, once imported into google3, or should no one depend on those protos that were moved?

It will be breaking.

@tiziano88
Copy link
Collaborator

Could you do it in non-breaking steps, like #4661 ?

@jul-sh
Copy link
Contributor Author

jul-sh commented Jan 17, 2024

Could you do it in non-breaking steps, like #4661 ?

#4661 was infact breaking. Client teams depended on it in order to build grpc clients. See http://cs/h/quirk-internal/confidential-federated-compute/+/main:third_party/oak/BUILD.containers.patch

This change is inherently breaking, but should help avoid further breaking changes later.

@tiziano88
Copy link
Collaborator

Could you do it in non-breaking steps, like #4661 ?

#4661 was infact breaking. Client teams depended on it in order to build grpc clients. See http://cs/h/quirk-internal/confidential-federated-compute/+/main:third_party/oak/BUILD.containers.patch

How was that breaking? AFAICT it is only adding new protos. The idea being that clients can migrate one by one to the new protos, at which point we can remove the old ones.

@jul-sh
Copy link
Contributor Author

jul-sh commented Jan 17, 2024

How was that breaking? AFAICT it is only adding new protos. The idea being that clients can migrate one by one to the new protos, at which point we can remove the old ones.

Oh okay, understood. So you mean separating it into a non-breaking step, followed by a breaking step. We can do that, though we've already merged breaking changes to the files last week.

The PR that was breaking is #4626, since alters protos that are build deps for the interface used by applications.

@tiziano88
Copy link
Collaborator

How was that breaking? AFAICT it is only adding new protos. The idea being that clients can migrate one by one to the new protos, at which point we can remove the old ones.

Oh okay, understood. So you mean separating it into a non-breaking step, followed by a breaking step. We can do that, though we've already merged breaking changes to the files last week.

The PR that was breaking is #4626, since alters protos that are build deps for the interface used by applications.

Yes, I am well aware, and in hindsight we agreed that we should have done it in a non-breaking way.

I'd like us to avoid breaking changes going forward (hence why #4661 is structured like that). The definition of breaking change here being that it requires changing multiple client team code at the same time in google3.

According to this definition, even the last step as you described it would actually not be breaking (since no one would be using those protos any more at that point).

@jul-sh
Copy link
Contributor Author

jul-sh commented Jan 17, 2024

Updated to preserve the old files. As a nit, per the definition you quoted it would still be considered breaking, unless we include

I'd like us to avoid breaking changes going forward (hence why #4661 is structured like that). The definition of breaking change here being that it requires changing multiple client team code at the same time in google3.

According to this definition, even the last step as you described it would actually not be breaking (since no one would be using those protos any more at that point).

Worth noting that dependencies on files in repos on git on borg would still exist and be broken. We should account for this.

message GetApplicationConfigResponse {
// Arbitrary config that the container can retrieve from the orchestrator.
// Included in the attestation measurements conducted by the orchestrator.
bytes config = 1;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting choice of file that the git algorithm determined I modified and renamed.

For clarity: the old build file is deleted, as it was unused.

The new proto file is unrelated.

Copy link
Contributor

@nfallen nfallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'll defer to Brett's review as I will be OOO for the next few weeks and thus not able to migrate our repo to the new interfaces.

@@ -0,0 +1,62 @@
//
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should call it hostlib.proto


// Defines the service exposed by the launcher, that can be invoked by the stage1 and the
// orchestrator.
service Launcher {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also could be renamed to Hostlib (but in a separate PR)

Copy link
Contributor

@bmclarnon bmclarnon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for including me in this review -- and for consolidating these messages and services!

Comment on lines 19 to 23
// TODO(#4392): Remove this file once the migration is complete.
// This file is deprecated. The relevant functionality is available in
// `proto/containers/application_interfaces.proto` & `proto/containers/launcher.proto`. DO NOT
// MODIFY THIS FILE, DO NOT ADD NEW DEPENDENCIES ON IT.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: In theory, you could use "import public" (https://protobuf.dev/programming-guides/proto3/#importing) to import the symbols from application_interfaces.proto. But it might be simpler to just duplicate the definitions if you expect the migration to be quick -- especially since using "import public" would also cause OrchestratorCrypto to be exposed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, sounds like a reasonable option. Updated to use public imports

proto/containers/BUILD Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants