Skip to content

CDH/image-rs | promote error information printing #995

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

Merged
merged 11 commits into from
May 29, 2025

Conversation

Xynnn007
Copy link
Member

@Xynnn007 Xynnn007 commented May 9, 2025

This PR is mainly for move image-pull to cdh. It improves the log printing after CDH RPC call failure to keep the error information consistent with that when calling image-rs directly, and avoids a lot of useless debug trace information.

In addition, this PR includes an additional commit to abandon the Unknown Snapshot type. Please see the commit message for the specific reason. cc @ChengyuZhu6

@Xynnn007 Xynnn007 force-pushed the image-rs-error-prt branch 2 times, most recently from de128b6 to a6a6160 Compare May 12, 2025 02:27
@Xynnn007 Xynnn007 force-pushed the image-rs-error-prt branch from a6a6160 to d0df699 Compare May 12, 2025 09:48
@Xynnn007 Xynnn007 force-pushed the image-rs-error-prt branch 3 times, most recently from d1bb5dc to 4e800d7 Compare May 13, 2025 05:26
Xynnn007 added a commit to Xynnn007/kata-containers that referenced this pull request May 13, 2025
let's see if this whole bunch of things works.

together with PR

confidential-containers/guest-components#995

Signed-off-by: Xynnn007 <[email protected]>
@Xynnn007 Xynnn007 marked this pull request as ready for review May 13, 2025 07:58
@Xynnn007 Xynnn007 requested a review from a team as a code owner May 13, 2025 07:58
@Xynnn007
Copy link
Member Author

It's ready for review now!

Xynnn007 added a commit to Xynnn007/kata-containers that referenced this pull request May 13, 2025
let's see if this whole bunch of things works.

together with PR

confidential-containers/guest-components#995

Signed-off-by: Xynnn007 <[email protected]>
Xynnn007 added a commit to Xynnn007/kata-containers that referenced this pull request May 21, 2025
let's see if this whole bunch of things works.

together with PR

confidential-containers/guest-components#995

Signed-off-by: Xynnn007 <[email protected]>
Xynnn007 added a commit to Xynnn007/kata-containers that referenced this pull request May 21, 2025
let's see if this whole bunch of things works.

together with PR

confidential-containers/guest-components#995

Signed-off-by: Xynnn007 <[email protected]>
Xynnn007 added a commit to Xynnn007/kata-containers that referenced this pull request May 21, 2025
let's see if this whole bunch of things works.

together with PR

confidential-containers/guest-components#995

Signed-off-by: Xynnn007 <[email protected]>
Xynnn007 added a commit to Xynnn007/kata-containers that referenced this pull request May 21, 2025
let's see if this whole bunch of things works.

together with PR

confidential-containers/guest-components#995

Signed-off-by: Xynnn007 <[email protected]>
Xynnn007 added a commit to Xynnn007/kata-containers that referenced this pull request May 21, 2025
let's see if this whole bunch of things works.

together with CDH side PR. If the CI passes, we can merge the following
PR first.

confidential-containers/guest-components#995

Signed-off-by: Xynnn007 <[email protected]>
Xynnn007 added a commit to Xynnn007/kata-containers that referenced this pull request May 22, 2025
let's see if this whole bunch of things works.

together with CDH side PR. If the CI passes, we can merge the following
PR first.

confidential-containers/guest-components#995

Signed-off-by: Xynnn007 <[email protected]>
Previously, the snapshot type in the image-rs logic was specified by
the `default_snapshot` field in the configuration file. This type
remained constant throughout the entire lifecycle of the ImageClient.
However, the previous logic attempted to re-fetch the snapshot using
the `default_snapshot` from the current ImageClient's members each
time an imagePull request was made, subsequently performing related
image storage actions.

This commit simplifies the logic.

It ensures that initialization only creates one snapshot as specified
by `default_snapshot`, and this snapshot is directly referenced and
used throughout the entire lifecycle of the ImageClient.

It should be noted that if any Snapshot feature is not activated at
compile time, an Unknown type Snapshot may be initialized when creating
an ImageClient at runtime. This type actually has no effect and will
result in an error when the image is pulled. This commit will correct
this logic, completely abandon the Unknown type Snapshot, and check
whether there is an available Snapshot type at compile time, otherwise
an error will be reported directly.

Signed-off-by: Xynnn007 <[email protected]>
@Xynnn007 Xynnn007 force-pushed the image-rs-error-prt branch from 4e800d7 to e15a391 Compare May 23, 2025 03:45
Xynnn007 added a commit to Xynnn007/kata-containers that referenced this pull request May 23, 2025
let's see if this whole bunch of things works.

together with CDH side PR. If the CI passes, we can merge the following
PR first.

confidential-containers/guest-components#995

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

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

Functionality looks good. Made a few suggestions about the wording since these errors will be exposed to users.

@Xynnn007 Xynnn007 force-pushed the image-rs-error-prt branch from e15a391 to f41fa7c Compare May 24, 2025 09:19
@Xynnn007
Copy link
Member Author

@fitzthum Hi, I realized that the PR to help sigstore support http_proxy config has been merged, so I also updated the sigstore version in this PR. The latest sigstore version supports configuring http proxy, so we can configure both http and https proxy ​​at the CDH configuration file level.

To distinguish them, I changed the corresponding configuration items to our usual names, namely http_proxy, https_proxy and no_proxy. These new contents can all be configured in initdata, so I put them in this PR, ptal.

@Xynnn007 Xynnn007 force-pushed the image-rs-error-prt branch from f41fa7c to 484cfd6 Compare May 25, 2025 10:30
Xynnn007 added 3 commits May 29, 2025 09:21
Added thiserror for improved error handling in image-rs submodules. This
provides standardized and formatted error messages, aiding CDH in better
logging during image-pull RPC processes. Additionally, a minor refactor was
done for asynchronous image downloads in image-rs, organizing relevant
content into a stream submodule.

Signed-off-by: Xynnn007 <[email protected]>
This commit defines a more formatted error printing when RPC fails to be
called by a remote caller. Also, it includes a simple error reason for
image pulling failures.

Signed-off-by: Xynnn007 <[email protected]>
The function LayerBlockCipherHandler::new() will raise an error if
AESCTRBlockCipher::new returns error. Actually the latter will only
return error when the given parameter is not 256.

So in code it will never return error thus safe to unwrap here.

As this function now returns no error and receives no parameter, it's
recommended to mark it as default.

Signed-off-by: Xynnn007 <[email protected]>
Xynnn007 added 7 commits May 29, 2025 09:21
If the policy file has no rules for a specific image, the image should
be allowed rather than raise an error. This is common when there is no
default rules for the policy.

Signed-off-by: Xynnn007 <[email protected]>
This little refactor helps the higher caller to handle raised errors.

Signed-off-by: Xynnn007 <[email protected]>
ocicrypt_config.json is a configuration file that specifies the key
provider to decrypt images following ocicrypt standard. When using CDH
to provide image pull ability, this file must be used to specify the env
of CDH process.

This patch adds the file and docs for this.

Signed-off-by: Xynnn007 <[email protected]>
the new version of http_proxy supports to set http_proxy configuration,
this would help to have a common overall config inside image-rs/CDH
config.

Signed-off-by: Xynnn007 <[email protected]>
Note that now we have both http_proxy and https_proxy configurations for
image-rs. We use this widely acceptable variable name to replace the old
names.

As `no_proxy`, `https_proxy` and `http_proxy` are all proxy
configurations, thus we combine them altogether into one struct for
better maintainance.

Signed-off-by: Xynnn007 <[email protected]>
now `image_pull_proxy` is renamed to `https_proxy` and `http_proxy`,
thus we need some fixups to update the unit tests.

Signed-off-by: Xynnn007 <[email protected]>
new field `http_proxy` is introduced and we use the more common name
`no_proxy` and `https_proxy` for old `skip_proxy_ips` and
`image_pull_proxy` variable names.

Signed-off-by: Xynnn007 <[email protected]>
@Xynnn007 Xynnn007 force-pushed the image-rs-error-prt branch from 484cfd6 to ac3639a Compare May 29, 2025 01:21
Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

LGTM

@Xynnn007 Xynnn007 merged commit e13f89d into confidential-containers:main May 29, 2025
31 checks passed
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.

3 participants