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

Fix remove ring #135

Merged
merged 3 commits into from
Apr 24, 2023
Merged

Fix remove ring #135

merged 3 commits into from
Apr 24, 2023

Conversation

Xynnn007
Copy link
Member

@Xynnn007 Xynnn007 commented Apr 7, 2023

Fixes: #134

@Xynnn007 Xynnn007 marked this pull request as draft April 7, 2023 04:59
@Xynnn007
Copy link
Member Author

Xynnn007 commented Apr 7, 2023

Night error issue #136

@Xynnn007
Copy link
Member Author

Xynnn007 commented Apr 23, 2023 •
edited
Loading

The errors of nightly and beta is tracked by #136

Hi @jiangliu @arronwy I've changed previous CI tests for different combinations of features. Please take a look if it looks good to you.

Hi @stevenhorsman @BbolroC This PR will help s390x to support cosign. FYI

@Xynnn007 Xynnn007 marked this pull request as ready for review April 23, 2023 03:08
arronwy
arronwy approved these changes Apr 24, 2023
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！

Cargo.toml Outdated Show resolved Hide resolved
Xynnn007 added 2 commits April 24, 2023 11:59
@Xynnn007
Dep: bring in rustls and native-tls features
6e61031 
`sigstore-rs` has features `native-tls` and `rustls-tls`. The
two features are leveraged to give two types of build:

`kata-cc-rustls-tls` and `enclave-cc-rustls-tls`: they depend on purely
rust implementation of crypto suites, including `ring` which can
not be built on `s390x`.

`kata-cc-native-tls` and `enclave-cc-native-tls`: they depend on `openssl`
for crypto suites. They can be built on `s390x`.

sync blocking is used because the current implementation of oci
client is not `Send` between threads.

Fixes: #134

Signed-off-by: Xynnn007 <[email protected]>
@Xynnn007
ci: update ci
42caee6 
*-cc-* features will cover the tests of signature-cosign-*, encryption
enclave-cc-* features will cover the tests of snapshot-unionfs

Only four basic feature tests are left:
- enclave-cc-rustls-tls: encalve-cc tests based on purely rust implementation
- enclave-cc-native-tls: enclave-cc tests based on openssl
- kata-cc-rustls-tls: kata-cc tests based on purely rust implementation
- kata-cc-native-tls: kata-cc tests based on openssl

Kata-cc tests are also combined with gRPC and ttrpc, while enclave-cc
tests are based on native.

Signed-off-by: Xynnn007 <[email protected]>
@arronwy arronwy merged commit 57480df into confidential-containers:main Apr 24, 2023
@Xynnn007 Xynnn007 deleted the fix-remove-ring branch April 24, 2023 04:50
@stevenhorsman
Copy link
Member

@Xynnn007 - just to double check - this mean we can remove the separate s390x dependency pulled in in the kata-agent now?

# Image pull/decrypt
[target.'cfg(target_arch = "s390x")'.dependencies]
image-rs = { git = "https://github.com/confidential-containers/image-rs", tag = "v0.5.1", default-features = false, features = ["kata-cc-s390x"] }

[target.'cfg(not(target_arch = "s390x"))'.dependencies]
image-rs = { git = "https://github.com/confidential-containers/image-rs", tag = "v0.5.1", default-features = false, features = ["kata-cc"] }

If so I'll try removing it and enabling the co-sign tests for s390x (we might have to mutli-arch enable/create the s390x versions of those to get them passing, but it will be good to understand and work on that early in the release cycle.

Thanks for the great work!

@Xynnn007
Copy link
Member Author

Hi @stevenhorsman

@Xynnn007 - just to double check - this mean we can remove the separate s390x dependency pulled in in the kata-agent now?

Yes. To make it easy we can choose kata-cc-native-tls by default for all artitectures.

@Xynnn007 Xynnn007 mentioned this pull request Apr 24, 2023
Closed
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

@sameo sameo Awaiting requested review from sameo

@jiangliu jiangliu Awaiting requested review from jiangliu

@lumjjb lumjjb Awaiting requested review from lumjjb

@jialez0 jialez0 Awaiting requested review from jialez0

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Get rid of ring dependency
3 participants
@Xynnn007 @stevenhorsman @arronwy