Image-rs & CDH | Refactoring and use the same ImageClient #708

Xynnn007 · 2024-09-06T10:03:33Z

This PR organizes the code of image-rs to make it more modular and allows CDH and image-rs to share the same image pull configuration file.

With the modular refactoring, we found some hidden bugs that caused by static/global variables. This refactoring tries to get rid of static/global variables to promote the reentrancy.

Please see each commit for a more structured view.

cc @fitzthum @ChengyuZhu6

fitzthum

A big improvement imo. I left a few comments.

image-rs/src/image.rs

image-rs/src/signature/mod.rs

image-rs/src/builder.rs

image-rs/src/resource/kbs/mod.rs

image-rs/src/signature/policy/mod.rs

fitzthum

LGTM

I think there are still some improvements to make in image-rs, but this is a step forward.

ChengyuZhu6

Thanks for the work @Xynnn007. Just a comment:

ChengyuZhu6 · 2024-09-14T01:09:06Z

image-rs/tests/image_decryption.rs

-    // lint from warning dead code.
-    common::clean_configs()
-        .await
-        .expect("Delete configs failed.");
    let mut image_client = image_rs::image::ImageClient::new(work_dir.path().to_path_buf());


@Xynnn007 Do we need to initialize the image_client instance using ClientBuilder ?

btw The ClientBuilder logic brings image policy, auth file, sigstore config fetching in the time of creation of ClientBuilder.build(), and supports to read them from local filesystem.

Got it. You mean that ClientBuilder.build() would read some configs by default that are irrelevant to image descryption?

Oh. I mean that image policy, auth file, sigstore config all are explicitly set in the config file. If any/all of them is set, the read will be performed by ClientBuilder.build()

Xynnn007 · 2024-09-14T01:32:20Z

I think there are still some improvements to make in image-rs, but this is a step forward.

Yes. Please give some ideas about this, and I will try to figure out if we can promote in this pr together

Refactor signature module from function way to object oriented. Also changed the logic a little to avoid duplicated get resource operation. Signed-off-by: Xynnn007 <[email protected]>

change the whole resource module's API into an object. This would help to make the modularation clear and better maintainance. Also, change the feature `getresource` to `kbs`. Because this feature only adds supports to find resources with `kbs:///` uri scheme. deletes useless lines in Cargo.toml Signed-off-by: Xynnn007 <[email protected]>

Now auth is a separate module with an object. This would help to maintain the internal status of each auth module. Signed-off-by: Xynnn007 <[email protected]>

With clientBuilder, we can get registry auth, sigstore config file and image policy when the client is built. This would help to avoid duplicated file getting. Also, combined CDH's image config with image-rs'. To leverage the new client builder, we mark the image pull object as a static one to be lazily initialized. Because before CDH is set up, the initialization steps of image client cannot pass when fetching resources from KBS. Signed-off-by: Xynnn007 <[email protected]>

Signed-off-by: Xynnn007 <[email protected]>

Now we use kbs to refer to the old feature getresource Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 · 2024-09-27T08:13:27Z

Rebased to resolve the conflicts.

ChengyuZhu6

LGTM thanks for the work @Xynnn007. BTW, I have a question that’s unrelated to this PR:
I’ve noticed that some comments in the code of the repo use three slashes ///, while others use two //. Are there specific scenarios for using these two types of comments? For example, is it recommended to use /// in one comment type and // in another comment type?

ChengyuZhu6 · 2024-09-27T12:43:42Z

image-rs/tests/image_decryption.rs

-    // lint from warning dead code.
-    common::clean_configs()
-        .await
-        .expect("Delete configs failed.");
    let mut image_client = image_rs::image::ImageClient::new(work_dir.path().to_path_buf());


Got it. You mean that ClientBuilder.build() would read some configs by default that are irrelevant to image descryption?

fitzthum · 2024-09-27T14:02:55Z

I've noticed that some comments in the code of the repo use three slashes ///, while others use two //. Are there specific scenarios for using these two types of comments? For example, is it recommended to use /// in one comment type and // in another comment type?

Three slash comments get used to generate the rustdocs. You can also use /** */ to do block doc comments. Ideally we should have one of these comments at the top of all our public functions in components that can be used as crates.

ChengyuZhu6 · 2024-09-27T14:20:19Z

I've noticed that some comments in the code of the repo use three slashes ///, while others use two //. Are there specific scenarios for using these two types of comments? For example, is it recommended to use /// in one comment type and // in another comment type?

Three slash comments get used to generate the rustdocs. You can also use /** */ to do block doc comments. Ideally we should have one of these comments at the top of all our public functions in components that can be used as crates.

Thanks Tobin. I noticed that some public structures or variables in the structure in the code are commented with //, while others are commented with ///. This seems curious to me.

ChengyuZhu6 · 2024-09-27T14:27:15Z

BTW, do we need to cut a minor release and bump guest-components and image-rs to the new minor release in kata, according to the community meeting? If so, I recommend holding off on merging this PR until the release is cut.

fitzthum · 2024-09-27T14:50:35Z

BTW, do we need to cut a minor release and bump guest-components and image-rs to the new minor release in kata, according to the community meeting? If so, I recommend holding off on merging this PR until the release is cut.

It sounds like we don't need another release, but we still might want to wait until next week to merge this stuff just in case.

fitzthum · 2024-09-27T15:00:27Z

I noticed that some public structures or variables in the structure in the code are commented with //, while others are commented with ///. This seems curious to me.

Yeah I think we have not been very consistent so far.

Xynnn007 · 2024-10-09T06:03:06Z

Added another commit to replace HTTPS_PROXY and NO_PROXY env setting with config to oci-client.

New dependency oci-client version supports to set HTTPS_PROXY and NO_PROXY without setting env of the whole process. Also, this commit marks the cosign module's client TODO to add a proxy configuration. Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 · 2024-10-12T09:34:17Z

Added another commit to support self-signed image registry certs in CDH/image-rs config.

Seeing that cosign will also need to fetch signatures from the same registry, I made a PR to support this in sigstore-rs crate. sigstore/sigstore-rs#392

After that PR is merged we can make the story complete

Fixes confidential-containers#525 Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 force-pushed the refact-config branch 8 times, most recently from db80b82 to c757c51 Compare September 11, 2024 03:40

Xynnn007 marked this pull request as ready for review September 11, 2024 04:15

Xynnn007 requested a review from a team as a code owner September 11, 2024 04:15

Xynnn007 force-pushed the refact-config branch from c757c51 to ee971ab Compare September 12, 2024 09:15

fitzthum reviewed Sep 13, 2024

View reviewed changes

Xynnn007 force-pushed the refact-config branch 2 times, most recently from 6e87742 to 8813e45 Compare September 13, 2024 08:26

fitzthum approved these changes Sep 13, 2024

View reviewed changes

ChengyuZhu6 reviewed Sep 14, 2024

View reviewed changes

Xynnn007 added 6 commits September 27, 2024 16:09

image-rs: refactor signature module

3fcd663

Refactor signature module from function way to object oriented. Also changed the logic a little to avoid duplicated get resource operation. Signed-off-by: Xynnn007 <[email protected]>

image-rs: refactor auth module

05266d8

Now auth is a separate module with an object. This would help to maintain the internal status of each auth module. Signed-off-by: Xynnn007 <[email protected]>

image-rs: fix integration test

5f2df22

Signed-off-by: Xynnn007 <[email protected]>

ci: update image-rs feature name getresource to kbs

9e75e02

Now we use kbs to refer to the old feature getresource Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 force-pushed the refact-config branch from 8813e45 to 9e75e02 Compare September 27, 2024 08:12

ChengyuZhu6 approved these changes Sep 27, 2024

View reviewed changes

Xynnn007 force-pushed the refact-config branch from 1fa5626 to 7c750ad Compare October 9, 2024 06:23

Xynnn007 force-pushed the refact-config branch from 281c722 to ca0c52a Compare October 14, 2024 03:06

image-rs: add support for self-signed image registry

d378288

Fixes confidential-containers#525 Signed-off-by: Xynnn007 <[email protected]>

Xynnn007 force-pushed the refact-config branch from ca0c52a to d378288 Compare October 14, 2024 06:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image-rs & CDH | Refactoring and use the same ImageClient #708

Image-rs & CDH | Refactoring and use the same ImageClient #708

Xynnn007 commented Sep 6, 2024 •

edited

Loading

fitzthum left a comment

fitzthum left a comment

ChengyuZhu6 left a comment

ChengyuZhu6 Sep 14, 2024

Xynnn007 Sep 14, 2024

Xynnn007 Sep 14, 2024

ChengyuZhu6 Sep 27, 2024

Xynnn007 Sep 27, 2024

Xynnn007 commented Sep 14, 2024

Xynnn007 commented Sep 27, 2024

ChengyuZhu6 left a comment

ChengyuZhu6 Sep 27, 2024

fitzthum commented Sep 27, 2024

ChengyuZhu6 commented Sep 27, 2024

ChengyuZhu6 commented Sep 27, 2024

fitzthum commented Sep 27, 2024

fitzthum commented Sep 27, 2024

Xynnn007 commented Oct 9, 2024

Xynnn007 commented Oct 12, 2024

Image-rs & CDH | Refactoring and use the same ImageClient #708

Are you sure you want to change the base?

Image-rs & CDH | Refactoring and use the same ImageClient #708

Conversation

Xynnn007 commented Sep 6, 2024 • edited Loading

fitzthum left a comment

Choose a reason for hiding this comment

fitzthum left a comment

Choose a reason for hiding this comment

ChengyuZhu6 left a comment

Choose a reason for hiding this comment

ChengyuZhu6 Sep 14, 2024

Choose a reason for hiding this comment

Xynnn007 Sep 14, 2024

Choose a reason for hiding this comment

Xynnn007 Sep 14, 2024

Choose a reason for hiding this comment

ChengyuZhu6 Sep 27, 2024

Choose a reason for hiding this comment

Xynnn007 Sep 27, 2024

Choose a reason for hiding this comment

Xynnn007 commented Sep 14, 2024

Xynnn007 commented Sep 27, 2024

ChengyuZhu6 left a comment

Choose a reason for hiding this comment

ChengyuZhu6 Sep 27, 2024

Choose a reason for hiding this comment

fitzthum commented Sep 27, 2024

ChengyuZhu6 commented Sep 27, 2024

ChengyuZhu6 commented Sep 27, 2024

fitzthum commented Sep 27, 2024

fitzthum commented Sep 27, 2024

Xynnn007 commented Oct 9, 2024

Xynnn007 commented Oct 12, 2024

Xynnn007 commented Sep 6, 2024 •

edited

Loading