Skip to content

Conversation

@sprt
Copy link

@sprt sprt commented Apr 10, 2024

Merge Checklist
  • Followed patch format from upstream recommendation: https://github.com/kata-containers/community/blob/main/CONTRIBUTING.md#patch-format
    • Included a single commit in a given PR - at least unless there are related commits and each makes sense as a change on its own.
  • Aware about the PR to be merged using "create a merge commit" rather than "squash and merge" (or similar)
  • genPolicy only: Ensured the tool still builds on Windows
  • genPolicy only: Updated sample YAMLs' policy annotations, if applicable
  • The upstream-missing label (or upstream-not-needed) has been set on the PR.
Summary

Add support to genpolicy for our CC Azure Disk drivers.

Test Methodology

Manual testing

@sprt sprt force-pushed the genpolicy-azuredisk branch 2 times, most recently from 5921881 to 18d664b Compare April 12, 2024 21:06
let volume_pvc = yaml_volume.persistentVolumeClaim.as_ref().unwrap();
let is_blk_mount =
persistent_volume_claims.iter()
.find(|pvcResource| pvcResource.metadata.name.as_ref() == Some(&volume_pvc.claimName))
Copy link
Author

@sprt sprt Apr 13, 2024

Choose a reason for hiding this comment

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

I think we should log a warning here if we can't find the PVC: either it wasn't provided via -pvc or it's not included in the multi-resource spec file, but in either case we would not be able to determine if this is a block mount. Thoughts?

Choose a reason for hiding this comment

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

Warning sounds good - and perhaps return an Error value if we don't already.

Copy link
Author

@sprt sprt Apr 15, 2024

Choose a reason for hiding this comment

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

Done as a warning. Didn't implement it as an error as that would break existing users.

@sprt
Copy link
Author

sprt commented Apr 15, 2024

Still TODO: add a -pvc flag.

@danmihai1
Copy link

Still TODO: add a -pvc flag.

Let's try to reuse the older -c for all of ConfigMap, PVC, and in the future Secret input YAML files.

@danmihai1 danmihai1 self-requested a review April 15, 2024 15:18
@sprt sprt force-pushed the genpolicy-azuredisk branch 3 times, most recently from bc00d8c to 4d3ac86 Compare April 15, 2024 20:06
@sprt sprt added the upstream/missing PRs that are yet to be upstreamed label Apr 15, 2024
@sprt sprt force-pushed the genpolicy-azuredisk branch 3 times, most recently from 908a5b9 to 3277b9a Compare April 15, 2024 20:54
@sprt
Copy link
Author

sprt commented Apr 15, 2024

Final iteration ready for review. Changes since last review:

  • Addressed all comments above.
  • Added the new -a/--attached-resource-file flag in a way that is backward-compatible and easy to cherry-pick upstream.
  • Updated the genpolicy samples and tweaked the script to run in parallel.

@sprt sprt marked this pull request as ready for review April 15, 2024 21:13
@sprt sprt requested review from a team as code owners April 15, 2024 21:13

#[tokio::main]
async fn main() {
if env::var("RUST_LOG").is_err() {

Choose a reason for hiding this comment

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

Please confirm with Dan whether we want to introduce this behavior.

Choose a reason for hiding this comment

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

This doesn't make sense to me - users should control the value of RUST_LOG.

Copy link
Author

@sprt sprt Apr 16, 2024

Choose a reason for hiding this comment

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

So the issue here is that warnings are not printed unless RUST_LOG is set to at least the warn level. So this code defaults it to warn, unless the user sets it. Thoughts?

Choose a reason for hiding this comment

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

Users having to enable warnings before they can see the warnings makes sense to me.

Copy link
Author

Choose a reason for hiding this comment

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

Understood, I removed this. But I still think it's risky to have warnings behind an env var that users may not be aware of. It took myself some time to figure out why I wasn't seeing warnings when testing.


#[tokio::main]
async fn main() {
if env::var("RUST_LOG").is_err() {

Choose a reason for hiding this comment

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

This doesn't make sense to me - users should control the value of RUST_LOG.

secrets.push(secret);
}
// ConfigMap and Secret documents contain additional input for policy generation.
if kind.eq("ConfigMap") {

Choose a reason for hiding this comment

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

Does "make check" complain about not using match instead if/else/if/else ?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed the unnecessary whitespace diff.

pub fn new() -> Self {
let args = CommandLineOptions::parse();

if args.config_map_file.is_some() {

Choose a reason for hiding this comment

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

Let's keep the old -c and call all of these 3 types of files "configuration files" instead of "resource files".

Copy link
Author

Choose a reason for hiding this comment

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

Done

@sprt sprt force-pushed the genpolicy-azuredisk branch from 726b826 to a3f2f2f Compare April 16, 2024 01:20
@Redent0r
Copy link

get_empty_dir_mount_and_storage(settings, p_mounts, storages, yaml_mount, memory_medium);
} else if yaml_volume.persistentVolumeClaim.is_some() || yaml_volume.azureFile.is_some() {
} else if yaml_volume.persistentVolumeClaim.is_some() {
get_persistent_volume_claim_mount(settings, yaml_mount, yaml_volume, p_mounts, storages, persistent_volume_claims, "rprivate", "rw");

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Good catch, thanks! Done, and refactored related code to be consistent.

@sprt sprt force-pushed the genpolicy-azuredisk branch 3 times, most recently from ab09be9 to 3b82b70 Compare April 16, 2024 20:54
@sprt sprt force-pushed the genpolicy-azuredisk branch from 3b82b70 to 03d0da9 Compare April 16, 2024 21:12
sprt added 3 commits April 16, 2024 21:16
These are mostly refactoring changes in preparation for the meat of the
PR.
This allows passing config maps and secrets (as well as any other
resource kinds relevant in the future) using the -c flag.

Signed-off-by: Aurélien Bombo <[email protected]>
@sprt sprt force-pushed the genpolicy-azuredisk branch from 03d0da9 to 7c05324 Compare April 16, 2024 21:21
@sprt sprt changed the title genpolicy: adapt policy for cc-managed-csi genpolicy: add support for cc-managed-csi Apr 16, 2024
@sprt sprt force-pushed the genpolicy-azuredisk branch from dbe7d64 to 7c05324 Compare April 16, 2024 22:02
Copy link
Author

@sprt sprt left a comment

Choose a reason for hiding this comment

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

New iteration - addressed all above comments.

secrets.push(secret);
}
// ConfigMap and Secret documents contain additional input for policy generation.
if kind.eq("ConfigMap") {
Copy link
Author

Choose a reason for hiding this comment

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

Fixed the unnecessary whitespace diff.


#[tokio::main]
async fn main() {
if env::var("RUST_LOG").is_err() {
Copy link
Author

Choose a reason for hiding this comment

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

Understood, I removed this. But I still think it's risky to have warnings behind an env var that users may not be aware of. It took myself some time to figure out why I wasn't seeing warnings when testing.

get_empty_dir_mount_and_storage(settings, p_mounts, storages, yaml_mount, memory_medium);
} else if yaml_volume.persistentVolumeClaim.is_some() || yaml_volume.azureFile.is_some() {
} else if yaml_volume.persistentVolumeClaim.is_some() {
get_persistent_volume_claim_mount(settings, yaml_mount, yaml_volume, p_mounts, storages, persistent_volume_claims, "rprivate", "rw");
Copy link
Author

Choose a reason for hiding this comment

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

Good catch, thanks! Done, and refactored related code to be consistent.

pub fn new() -> Self {
let args = CommandLineOptions::parse();

if args.config_map_file.is_some() {
Copy link
Author

Choose a reason for hiding this comment

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

Done

Comment on lines -45 to -50
pub fn new(file: &str) -> anyhow::Result<Self> {
debug!("Reading ConfigMap...");
let config_map: ConfigMap = serde_yaml::from_reader(File::open(file)?)?;
debug!("\nRead ConfigMap => {:#?}", config_map);
Ok(config_map)
}

Copy link
Author

Choose a reason for hiding this comment

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

The compiler complains about this code being unused now.

@sprt sprt requested review from Redent0r and danmihai1 April 17, 2024 02:12
@sprt sprt force-pushed the genpolicy-azuredisk branch from 7c05324 to a942605 Compare April 17, 2024 02:53
Copy link
Author

@sprt sprt left a comment

Choose a reason for hiding this comment

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

Fixed last make check failure. Ready for review.

Copy link

@Redent0r Redent0r left a comment

Choose a reason for hiding this comment

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

LGTM and I just verified it builds on Windows

sprt added 3 commits April 17, 2024 17:48
This processes all samples in parallel rather than sequentially and
speeds up the whole process from ~10 mins to ~90 secs.

$ python3 update_policy_samples.py
   Compiling genpolicy v0.1.0 (/home/abombo/tmp/kata-containers/src/tools/genpolicy)
    Finished dev [unoptimized + debuginfo] target(s) in 5.57s
COMMAND: cargo build
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/fixtures/limits.yaml
Time taken: 0.05 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/fixtures/namespace.yaml
Time taken: 0.05 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/fixtures/quota.yaml
Time taken: 0.05 seconds
COMMAND: sudo target/debug/genpolicy -d -s -y ../../agent/samples/policy/yaml/webhook2/webhook-pod8.yaml
Time taken: 6.48 seconds
COMMAND: sudo target/debug/genpolicy -d -s -y ../../agent/samples/policy/yaml/webhook2/webhook-pod11.yaml
Time taken: 7.12 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/deployment/deployment-docker-busybox.yaml
Time taken: 9.15 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/replica-set/replica2.yaml
Time taken: 9.22 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/fixtures2/valid-pod.yaml
Time taken: 9.64 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/job/test-job2.yaml
Time taken: 9.9 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/deployment/deployment-busybox.yaml
Time taken: 10.06 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/configmap/pod-cm1.yaml
Time taken: 10.31 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/conformance/csi-hostpath-testing.yaml
Time taken: 10.46 seconds
[2024-04-15T21:01:52Z WARN  genpolicy::pod] Can't find the value of annotation batch.kubernetes.io/job-completion-index. Allowing any value.
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/job/test-job.yaml
Time taken: 10.61 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/pod/pod-lifecycle.yaml
Time taken: 10.71 seconds
COMMAND: sudo target/debug/genpolicy -d -s -y ../../agent/samples/policy/yaml/webhook2/webhook-pod10.yaml
Time taken: 10.71 seconds
COMMAND: sudo target/debug/genpolicy -d -s -y ../../agent/samples/policy/yaml/webhook2/webhook-pod9.yaml
Time taken: 14.71 seconds
COMMAND: sudo target/debug/genpolicy -d -s -y ../../agent/samples/policy/yaml/webhook2/webhook-pod13.yaml
Time taken: 14.81 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/conformance/hello-populator-deploy.yaml
Time taken: 14.94 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/conformance/conformance-e2e.yaml
Time taken: 14.99 seconds
COMMAND: sudo target/debug/genpolicy -d -s -y ../../agent/samples/policy/yaml/webhook/webhook-pod4.yaml
Time taken: 15.06 seconds
COMMAND: sudo target/debug/genpolicy -d -s -y ../../agent/samples/policy/yaml/webhook/webhook-pod5.yaml
Time taken: 15.08 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/replica-set/replica-busy.yaml
Time taken: 15.18 seconds
COMMAND: sudo target/debug/genpolicy -d -s -y ../../agent/samples/policy/yaml/webhook2/webhook-pod12.yaml
Time taken: 15.24 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/pod/pod-one-container.yaml
Time taken: 15.38 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/secrets/azure-file-secrets.yaml
Time taken: 15.49 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/fixtures/daemon.yaml
Time taken: 15.55 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/configmap/pod-cm3.yaml
Time taken: 15.65 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/fixtures/rc-noexist.yaml
Time taken: 15.79 seconds
COMMAND: sudo target/debug/genpolicy -d -s -y ../../agent/samples/policy/yaml/webhook/webhook-pod1.yaml
Time taken: 15.87 seconds
COMMAND: sudo target/debug/genpolicy -d -s -y ../../agent/samples/policy/yaml/webhook/webhook-pod2.yaml
Time taken: 15.93 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/pod/pod-persistent-volumes.yaml
Time taken: 16.02 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/pod/pod-exec.yaml
Time taken: 16.13 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/deployment/deployment-azure-vote-back.yaml
Time taken: 16.21 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/conformance2/ingress-static-ip-rc.yaml
Time taken: 16.23 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/conformance2/ingress-http2-rc.yaml
Time taken: 16.27 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/conformance/netexecrc.yaml
Time taken: 16.38 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/fixtures/rc-lastapplied.yaml
Time taken: 16.49 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/conformance2/ingress-http-rc.yaml
Time taken: 16.52 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/conformance2/ingress-multiple-certs-rc.yaml
Time taken: 16.63 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/stateful-set/web.yaml
Time taken: 16.68 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/fixtures/appsv1deployment.yaml
Time taken: 16.78 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/configmap/pod-cm2.yaml
Time taken: 16.92 seconds
COMMAND: sudo target/debug/genpolicy -d -s -y ../../agent/samples/policy/yaml/webhook/webhook-pod3.yaml
Time taken: 16.97 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/fixtures/replication.yaml
Time taken: 17.01 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/fixtures/multi-resource-yaml.yaml
Time taken: 17.07 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/pod/pod-ubuntu.yaml
Time taken: 17.17 seconds
COMMAND: sudo target/debug/genpolicy -d -s -y ../../agent/samples/policy/yaml/webhook/webhook-pod6.yaml
Time taken: 17.23 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/fixtures/deploy-clientside.yaml
Time taken: 17.31 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/stateful-set/web2.yaml
Time taken: 17.34 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/fixtures2/rc-service.yaml
Time taken: 17.42 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/pod/pod-same-containers.yaml
Time taken: 17.45 seconds
COMMAND: sudo target/debug/genpolicy -d -s -y ../../agent/samples/policy/yaml/webhook/webhook-pod7.yaml
Time taken: 17.47 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/conformance/etcd-statefulset.yaml
Time taken: 17.63 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/incomplete-init/controller.yaml
Time taken: 17.71 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/incomplete-init/cassandra-statefulset.yaml
Time taken: 19.18 seconds
Skipping link to non-file: etc/rc0.d/.wh.K01sendsigs
Skipping link to non-file: etc/rc0.d/.wh.K01sendsigs
Skipping link to non-file: etc/rc0.d/.wh.K01sendsigs
Skipping link to non-file: etc/rc0.d/.wh.K01sendsigs
Skipping link to non-file: etc/rc0.d/.wh.K01sendsigs
Skipping link to non-file: etc/rc0.d/.wh.K01sendsigs
Skipping link to non-file: etc/rc0.d/.wh.K01sendsigs
Skipping link to non-file: etc/rc0.d/.wh.K01sendsigs
Skipping link to non-file: etc/rc0.d/.wh.K01sendsigs
Skipping link to non-file: etc/rc0.d/.wh.K01sendsigs
[2024-04-15T21:02:06Z WARN  genpolicy::registry] Failed to parse www-data as u32, using uid = 0 - error invalid digit found in string
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/conformance2/ingress-nginx-rc.yaml
Time taken: 23.91 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/fixtures/rc-nginxhttps.yaml
Time taken: 27.22 seconds
[2024-04-15T21:02:09Z WARN  genpolicy::mount_and_storage] Unable to determine backing storage of persistent volume claim 'datadir'. Pass `-a <pvc.yaml>` to get rid of this warning.
[2024-04-15T21:02:09Z WARN  genpolicy::mount_and_storage] Unable to determine backing storage of persistent volume claim 'datadir'. Pass `-a <pvc.yaml>` to get rid of this warning.
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/incomplete-init/cockroachdb-statefulset.yaml
Time taken: 27.3 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/conformance/csi-hostpath-plugin.yaml
Time taken: 29.12 seconds
COMMAND: sudo target/debug/genpolicy -d -s -y ../../agent/samples/policy/yaml/webhook3/dns-test.yaml
Time taken: 55.68 seconds
COMMAND: sudo target/debug/genpolicy -d -s -y ../../agent/samples/policy/yaml/webhook3/many-layers.yaml
Time taken: 55.66 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/pod/pod-spark.yaml
Time taken: 59.81 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/kubernetes/fixtures/job.yaml
Time taken: 60.45 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/deployment/deployment-azure-vote-front.yaml
Time taken: 61.6 seconds
COMMAND: sudo target/debug/genpolicy -d -y ../../agent/samples/policy/yaml/pod/pod-three-containers.yaml
Time taken: 96.99 seconds
Total time taken: 97.0137689113617 seconds
@sprt sprt force-pushed the genpolicy-azuredisk branch from a942605 to 7c0206c Compare April 17, 2024 17:48
Copy link
Author

@sprt sprt left a comment

Choose a reason for hiding this comment

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

Removed leftover TODO comment (related to make check) - merging now.

@sprt sprt merged commit a2f0902 into microsoft:msft-main Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

upstream/missing PRs that are yet to be upstreamed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants