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

Resolve transparent release binary filename issues #5004

Closed
wants to merge 5 commits into from

Conversation

jul-sh
Copy link
Contributor

@jul-sh jul-sh commented Apr 5, 2024

The tag under which provenances are uploaded must match the binary names defined in kokoro/build_binaries_rust.sh. Our existing logic derives the tag using the filename of the binary artifact. This creates an implicit, global, and unchangeable namespace for the filename of binary artifacts we import with TR.

Recent prior PRs had renamed a binary artifact of the restricted kernel.

As @thmsbinder correctly identified in #5001, this inadvertently led to an invisible collision in this implicit namespace.

This PR remedies this, by restoring the original filename of the binary artifact.

Change-Id: I586074edf736b1a886bd228c1148fcf3a060caaf
@jul-sh jul-sh requested review from tiziano88 and thmsbinder April 5, 2024 18:55
kokoro/build_binaries_rust.sh Outdated Show resolved Hide resolved
@@ -42,8 +42,8 @@ readonly generated_binaries=(
./enclave_apps/target/x86_64-unknown-none/release/oak_orchestrator
)
readonly binary_names=(
oak_restricted_kernel_simple_io_wrapper_bin
oak_restricted_kernel_simple_io_init_rd_wrapper_bin
oak_restricted_kernel_simple_io_bzimage
Copy link
Collaborator

Choose a reason for hiding this comment

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

This won't work, as the binary names are already locked down (use internal codesearch).

But I don't understand how this resolves the problem discussed - in artifacts_path we have the * (wildcard) which matches three files. One of the files (name needs to be configurable) is special, since it is the one for the internal TR binary package. I don't see a fundamentally different way than #5001 to fix this.

Copy link
Contributor Author

@jul-sh jul-sh Apr 5, 2024

Choose a reason for hiding this comment

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

This won't work, as the binary names are already locked down (use internal codesearch).

Good catch, there's internal references to these names. I personally think we should probably rename the internal binary names to use the bzimage suffix for clarity. But I agree, let's not do this change right now.

Updated this PR to match established internal binary names.

Copy link
Contributor Author

@jul-sh jul-sh Apr 5, 2024

Choose a reason for hiding this comment

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

But I don't understand how this resolves the problem discussed - in artifacts_path we have the * (wildcard) which matches three files. One of the files (name needs to be configurable) is special, since it is the one for the internal TR binary package. I don't see a fundamentally different way than #5001 to fix this.

With this PR:

With the additional files (image and setup data) there's now some additional tags under which the provenance will be uploaded. But TR can ignore those.

There's now multiple subjects in the provenance, but TR can find the right one by name.

I might be missing something here, but I don't think there's a fundamentally breaking change that means we need to add new data to the config. We used filenames as identifiers in the past, and we still do so.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK I get it now. The bzImage file name is constructed such that it coincides with the package name. Good. So assume we can remove binary_file everywhere in #5001. But how about buildconfigs/oak_containers_kernel_bzimage.toml? This also needs a split, in exactly the same way The package name is oak_containers_kernel.

To get the package_name into the reusable_provenance.yaml we could also use the basename of the buildconfig (need to rename buildconfigs appropriately). Or we add it to the matrix in provenance.yaml. I can find a better way here.

Copy link
Contributor Author

@jul-sh jul-sh Apr 5, 2024

Choose a reason for hiding this comment

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

The bzImage file name is constructed such that it coincides with the package name. Good. So assume we can remove binary_file everywhere in #5001.

yup!

But how about buildconfigs/oak_containers_kernel_bzimage.toml? This also needs a split, in exactly the same way The package name is oak_containers_kernel.

Yes, eventually we'll want to extend that provenance to include the image and setup data also. I'm not sure I understand why we'd need the changes in #5001 to do that, but I'd be happy to sync more on Monday. :)

Right now there haven't yet been any changes to the containers kernel.

jul-sh added 2 commits April 5, 2024 19:08
Change-Id: Ie6e47bd929c2a309accf819da2dbc0172bede21c
Change-Id: I831ddbda48d0b61e934e7d940e630c4f4f6705ad
kokoro/build_binaries_rust.sh Outdated Show resolved Hide resolved
jul-sh added 2 commits April 5, 2024 20:53
Change-Id: I3155f4957479de1a034f3065bab9e122b7ab82bc
Change-Id: I0af3062ba209037758ab91fd7738f49a55e5e596
@jul-sh jul-sh changed the title Resolve transparent release binary naming issues Resolve transparent release binary filename issues Apr 5, 2024
@jul-sh jul-sh closed this Apr 17, 2024
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.

2 participants