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

Fix configuration file default value and make error information more detailed #726

Merged
merged 2 commits into from
Sep 26, 2024

Conversation

Xynnn007
Copy link
Member

Please see the commit message of this PR.

cc @stevenhorsman

@Xynnn007 Xynnn007 requested a review from a team as a code owner September 26, 2024 01:44
This default value of KBS cert would make the KBS client to read it as a
PEM cert, thus always fails to launch. Without this default value, the
`kbs_cert` field inside the KBS config will be a None, which means no
cert reading and parsing will be performed.

Signed-off-by: Xynnn007 <[email protected]>
This patch replaces all error info formating from `{e}` to `{e:?}`. In
rust, `{e:?}` would print a much more detailed information of an error
than `{e}`. This would help to gather more details when debugging.

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

@mkulke mkulke left a comment

Choose a reason for hiding this comment

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

thanks @Xynnn007 for looking into this so quickly, can we maybe also as a drive-by fix make a missing credentials setting in the CDH conf default to []?

at the moment users have to specify explicitly, even if a user does not use the feature.

credentials = []

@mythi
Copy link
Contributor

mythi commented Sep 26, 2024

This default value of KBS cert would make the KBS client to read it as a PEM cert, thus always fails to launch.

The unit tests probably need updating to get this path covered?

@Xynnn007
Copy link
Member Author

at the moment users have to specify explicitly, even if a user does not use the feature.

credentials = []

Without this section it could be fine. See the unit test for config example https://github.com/confidential-containers/guest-components/blob/main/confidential-data-hub/hub/src/config.rs#L249-L261

The unit tests probably need updating to get this path covered?

Updated. See https://github.com/confidential-containers/guest-components/pull/726/files#diff-51b4317c4f4b87e6825be8fe3761800028d718fff2c48542e78904a6fc82aba1R303

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

@fitzthum fitzthum merged commit c202203 into confidential-containers:main Sep 26, 2024
19 checks passed
stevenhorsman added a commit to stevenhorsman/cloud-api-adaptor that referenced this pull request Sep 26, 2024
- Bump to pick up the CDH config fix from
confidential-containers/guest-components#726

Signed-off-by: stevenhorsman <[email protected]>
@Xynnn007 Xynnn007 deleted the fix-kbs-client branch September 26, 2024 22:36
stevenhorsman added a commit to stevenhorsman/cloud-api-adaptor that referenced this pull request Sep 27, 2024
- Bump to pick up the CDH config fix from
confidential-containers/guest-components#726

Signed-off-by: stevenhorsman <[email protected]>
wainersm pushed a commit to confidential-containers/cloud-api-adaptor that referenced this pull request Sep 27, 2024
- Bump to pick up the CDH config fix from
confidential-containers/guest-components#726

Signed-off-by: stevenhorsman <[email protected]>
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.

4 participants