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.

Sign up for GitHub

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

Jump to bottom

Optimize error for guest-components #543

Merged
merged 4 commits into from
Apr 22, 2024
Merged

Optimize error for guest-components #543

merged 4 commits into from
Apr 22, 2024

Conversation

Xynnn007
Copy link
Member

@Xynnn007 Xynnn007 commented Apr 17, 2024 •
edited
Loading

This PR does the following

  1. Reorganize error handling and information in logs in CDH/AA. Only a brief error information will be responsed to caller, and detailed information will be printed in the log of CDH/AA. Close Promote the error print of guest-components #541
  2. ASR now will response an internal error (HTTP 500) when failed to execute API call. Close api-server-rest: Responded with 200 OK although providing error in the body #529
  3. Based on the code change, we bring out the duplicated code for unseal_secret in storage plugin

@Xynnn007 Xynnn007 marked this pull request as ready for review April 17, 2024 06:05
fitzthum
fitzthum reviewed Apr 17, 2024
View reviewed changes
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.

Great stuff. Made a few comments

attestation-agent/attestation-agent/src/bin/grpc-aa/server.rs Outdated Show resolved Hide resolved
confidential-data-hub/hub/src/bin/server/mod.rs Outdated Show resolved Hide resolved
confidential-data-hub/image/src/annotation_packet/v2.rs Outdated Show resolved Hide resolved
confidential-data-hub/secret/src/lib.rs Outdated Show resolved Hide resolved
confidential-data-hub/image/src/error.rs Outdated Show resolved Hide resolved
confidential-data-hub/secret/src/lib.rs Outdated Show resolved Hide resolved
@Xynnn007
Copy link
Member Author

@fitzthum Fixed as suggestions. Thanks. PTAL

fitzthum
fitzthum approved these changes Apr 18, 2024
View reviewed changes
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

Do you want to fixup this trace as well? I saw something about this in the slack.

There are a couple of places where you use the same format string in this PR that you might want to double check.

@Xynnn007
Copy link
Member Author

Do you want to fixup this trace as well? I saw something about this in the slack.

There are a couple of places where you use the same format string in this PR that you might want to double check.

I think it is enough now. {e:?} (concrete error information with backtrace) only shows in CDH/AA's own stdout/err and will not be delivered to kata-agent. This would help to debug.

@Xynnn007
Copy link
Member Author

cc @mkulke might be interested.

mkulke
mkulke reviewed Apr 19, 2024
View reviewed changes
confidential-data-hub/secret/src/secret/mod.rs Show resolved Hide resolved
confidential-data-hub/secret/src/error.rs Outdated Show resolved Hide resolved
confidential-data-hub/image/src/error.rs Outdated Show resolved Hide resolved
confidential-data-hub/image/src/error.rs Outdated Show resolved Hide resolved
confidential-data-hub/image/src/error.rs Outdated Show resolved Hide resolved
confidential-data-hub/image/src/error.rs Outdated Show resolved Hide resolved
confidential-data-hub/image/src/error.rs Outdated Show resolved Hide resolved
confidential-data-hub/image/src/annotation_packet/v2.rs Outdated Show resolved Hide resolved
@Xynnn007
CDH: optimize error information
cb16b62 
This commit re-organize the error information of CDH.

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
@Xynnn007
AA: optimize log printing
7b70b5f 
`anyhow` crate provides '{:?}' macro to print multiple line error
information.

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
@Xynnn007
ASR: return internal error HTTP response when failed to call API
685188b 
Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
@Xynnn007
CDH/storage: bring duplicate sealed secret things out
2633fdb 
Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>
arronwy
arronwy approved these changes Apr 22, 2024
View reviewed changes
Copy link
Member

@arronwy arronwy 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 LGTM!

@Xynnn007 Xynnn007 merged commit 2dd4a9f into confidential-containers:main Apr 22, 2024
10 checks passed
@Xynnn007 Xynnn007 deleted the fix-error branch April 22, 2024 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arronwy arronwy arronwy approved these changes

@fitzthum fitzthum fitzthum approved these changes

@mkulke mkulke mkulke approved these changes

@jialez0 jialez0 Awaiting requested review from jialez0

@sameo sameo Awaiting requested review from sameo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@Xynnn007 @mkulke @fitzthum @arronwy