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

Add features structure type #184

Merged
merged 6 commits into from
Aug 14, 2024

Conversation

musaprg
Copy link
Contributor

@musaprg musaprg commented Jul 7, 2024

related to: youki-dev/youki#2837

This PR adds Features based on the specs-go implementation in the runtime spec. That's supposed to be used by features subcommand of runtime and will replace the local type defined in the runtime such as youki.

TODO

  • Add tests for required fields.
  • Add tests for all fields including optional ones

@musaprg musaprg marked this pull request as ready for review July 7, 2024 04:40
@musaprg musaprg marked this pull request as draft July 7, 2024 04:46
@musaprg musaprg force-pushed the support-features-structure branch 2 times, most recently from bb4c4f7 to 6f3e7e0 Compare July 29, 2024 17:12
@musaprg musaprg marked this pull request as ready for review July 29, 2024 17:12
@musaprg
Copy link
Contributor Author

musaprg commented Jul 30, 2024

The CI has failed due to missing labels, and I don't have permission to add them. Could anyone look into it? Thanks.

@musaprg musaprg force-pushed the support-features-structure branch from 6f3e7e0 to 6ecc08b Compare July 30, 2024 14:48
Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

Thanks! Let's consider reusing the already-defined structures.

use getset::{Getters, MutGetters, Setters};
use serde::{Deserialize, Serialize};

/// Features Structure that represents supported features of the runtime.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Features Structure that represents supported features of the runtime.
/// Features represents supported features of the runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in bb504b4

pub struct Linux {
/// The list of the recognized namespaces, e.g., "mount".
/// "None" means "unknown", not "no support for any namespace".
namespaces: Option<Vec<String>>,
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed to reuse existing types in e247914

enabled: Option<bool>,
actions: Option<Vec<String>>,
operators: Option<Vec<String>>,
archs: Option<Vec<String>>,
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@musaprg musaprg Aug 7, 2024

Choose a reason for hiding this comment

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

I've added the missing ScmpArchRiscv64 to the Arch enum.
You can check the value of SCMP_ARCH_RISCV64 (originally AUDIT_ARCH_RISCV64) from the libseccomp source, or calculated values are available on the golang.org/x/sys/unix package.

2325f34

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed to reuse existing types in e247914

#[serde(rename_all = "camelCase")]
pub struct Seccomp {
enabled: Option<bool>,
actions: Option<Vec<String>>,
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@musaprg musaprg Aug 7, 2024

Choose a reason for hiding this comment

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

I've added ScmpActKillThread into the LinuxSeccompAction enum. It's equivalent to ScmpActKill but needs to be defined explicitly to conform to the spec. I need to implement the From trait for u32 because we cannot define multiple enum items with the same value 0x00000000.

129d82a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed to reuse existing types in e247914

@utam0k utam0k self-requested a review July 31, 2024 12:03
Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

Thanks! Let's consider reusing the already-defined structures.

@@ -1051,32 +1051,50 @@ pub struct LinuxSeccomp {
#[derive(Clone, Copy, Debug, Deserialize, Eq, PartialEq, Serialize, StrumDisplay, EnumString)]
#[strum(serialize_all = "SCREAMING_SNAKE_CASE")]
#[serde(rename_all = "SCREAMING_SNAKE_CASE")]
#[repr(u32)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not confident whether this line is safe to delete.

@utam0k
Copy link
Member

utam0k commented Aug 9, 2024

Hi, thanks for your update. Since I'm on vacation this week, I'll revisit here after next week.

@saschagrunert If possible, can you take a look at this🙏

@musaprg
Copy link
Contributor Author

musaprg commented Aug 10, 2024

The CI (ci / lint-clippy) failed, but it doesn't seem relevant to this PR. I checked and the error also occurred on the main branch, so I submitted another PR to fix the linter error. Could you also check it?

#195

@musaprg musaprg requested a review from utam0k August 14, 2024 08:59
@utam0k
Copy link
Member

utam0k commented Aug 14, 2024

@musaprg Can you rebase this PR from the main branch to pass the CI?

This PR adds Features based on the specs-go implementation in the runtime spec. That's supposed to be used by features subcommand of runtime and will replace the local type defined in the runtime such as youki.

Signed-off-by: Kotaro Inoue <[email protected]>
@musaprg musaprg force-pushed the support-features-structure branch from e247914 to bda7b0c Compare August 14, 2024 13:15
@musaprg
Copy link
Contributor Author

musaprg commented Aug 14, 2024

@utam0k Hi, I've rebased the branch with the current main branch. Could you check it again? Thanks.

@utam0k utam0k merged commit cf455b4 into youki-dev:main Aug 14, 2024
14 checks passed
@musaprg musaprg deleted the support-features-structure branch August 14, 2024 15:21
@musaprg musaprg mentioned this pull request Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants