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

tests: k8s: improve the Agent Policy tests #9398

Merged

Conversation

danmihai1
Copy link
Contributor

  • Don't blindly apply the "allow all" policy to all the test yaml files - even to files that don't need a policy (e.g., ConfigMap|LimitRange|Namespace|PersistentVolume|PersistentVolumeClaim|RuntimeClass|Secret|Service don't need policy)

  • Reduce the run_kubernetes_tests.sh complexity, by replacing all of its commands that are directly related to policy

  • Add the "allow all" policy just to test yaml files that need a policy (Pod|Deployment|Job|ReplicationController) and that are not being tested using a policy auto-generated by the genpolicy tool

Don't add the "allow all" policy to all the test YAML files anymore.

After this change, the k8s tests assume that all the Kata CI Guest
rootfs image files either:

- Don't support Agent Policy at all, or
- Include an "allow all" default policy.

This relience/assumption will be addressed in a future commit.

Fixes: kata-containers#9395

Signed-off-by: Dan Mihai <dmihai@microsoft.com>
Check from:

- k8s-exec-rejected.bats
- k8s-policy-set-keys.bats

if policy testing is enabled or not, to reduce the complexity of
run_kubernetes_tests.sh. After these changes, there are no policy
specific commands left in run_kubernetes_tests.sh.

add_allow_all_policy_to_yaml() is moving out of run_kubernetes_tests.sh
too, but it not used yet. It will be used in future commits.

Fixes: kata-containers#9395

Signed-off-by: Dan Mihai <dmihai@microsoft.com>
Use the "allow all" policy for k8s-empty-dirs.bats, instead of
relying on the Kata Guest image to use the same policy as its
default.

Signed-off-by: Dan Mihai <dmihai@microsoft.com>
Use the "allow all" policy for k8s-footloose.bats, instead of
relying on the Kata Guest image to use the same policy as its
default.

Signed-off-by: Dan Mihai <dmihai@microsoft.com>
Use the "allow all" policy for k8s-guest-pull-image.bats, instead of
relying on the Kata Guest image to use the same policy as its default.

Signed-off-by: Dan Mihai <dmihai@microsoft.com>
Use the "allow all" policy for k8s-inotify.bats, instead of relying
on the Kata Guest image to use the same policy as its default.

Signed-off-by: Dan Mihai <dmihai@microsoft.com>
Use the "allow all" policy for k8s-liveness-probes.bats, instead of
relying on the Kata Guest image to use the same policy as its default.

Signed-off-by: Dan Mihai <dmihai@microsoft.com>
Use the "allow all" policy for k8s-measured-rootfs.bats, instead of
relying on the Kata Guest image to use the same policy as its default.

Signed-off-by: Dan Mihai <dmihai@microsoft.com>
Use the "allow all" policy for k8s-optional-empty-secret.bats,
instead of relying on the Kata Guest image to use the same policy as
its default.

Signed-off-by: Dan Mihai <dmihai@microsoft.com>
Use the "allow all" policy for k8s-pod-quota.bats, instead of
relying on the Kata Guest image to use the same policy as its default.

Signed-off-by: Dan Mihai <dmihai@microsoft.com>
Use the "allow all" policy for k8s-projected-volume.bats, instead of
relying on the Kata Guest image to use the same policy as its default.

Signed-off-by: Dan Mihai <dmihai@microsoft.com>
Use the "allow all" policy for k8s-seccomp.bats, instead of relying
on the Kata Guest image to use the same policy as its default.

Signed-off-by: Dan Mihai <dmihai@microsoft.com>
Use the "allow all" policy for k8s-security-context.bats, instead of
relying on the Kata Guest image to use the same policy as its default.

Signed-off-by: Dan Mihai <dmihai@microsoft.com>
Use the "allow all" policy for k8s-sysctls.bats, instead of
relying on the Kata Guest image to use the same policy as its default.

Signed-off-by: Dan Mihai <dmihai@microsoft.com>
Use the "allow all" policy for k8s-volume.bats, instead of relying
on the Kata Guest image to use the same policy as its default.

Signed-off-by: Dan Mihai <dmihai@microsoft.com>
Use the "allow all" policy for k8s-nginx-connectivity.bats, instead of
relying on the Kata Guest image to use the same policy as its default.

Signed-off-by: Dan Mihai <dmihai@microsoft.com>
Use the "allow all" policy for k8s-sandbox-vcpus-allocation.bats,
instead of relying on the Kata Guest image to use the same policy
as its default.

Signed-off-by: Dan Mihai <dmihai@microsoft.com>
@danmihai1 danmihai1 force-pushed the danmihai1/policy-test-cleanup branch from f75853e to f800bd8 Compare April 3, 2024 03:03
Comment on lines +277 to +279
ALLOW_ALL_POLICY="${ALLOW_ALL_POLICY}" yq write -i "${yaml_file}" \
'metadata.annotations."io.katacontainers.config.agent.policy"' \
"${ALLOW_ALL_POLICY}"
Copy link
Member

Choose a reason for hiding this comment

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

@danmihai1 , I recommend to call set_metadata_annotation to apply annotation.

set_metadata_annotation() {
local yaml="${1}"
local key="${2}"
local value="${3}"
local metadata_path="${4:-}"
local annotation_key=""
[ -n "$metadata_path" ] && annotation_key+="${metadata_path}."
# yaml annotation key name.
annotation_key+="metadata.annotations.\"${key}\""
echo "$annotation_key"
# yq set annotations in yaml. Quoting the key because it can have
# dots.
yq write -i --style=double "${yaml}" "${annotation_key}" "${value}"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChengyuZhu6 thanks! I think that's a good idea but I think we should perform this additional cleanup in a separate PR. I explained my thoughts in #9405 .

Comment on lines +284 to +286
ALLOW_ALL_POLICY="${ALLOW_ALL_POLICY}" yq write -i "${yaml_file}" \
'spec.template.metadata.annotations."io.katacontainers.config.agent.policy"' \
"${ALLOW_ALL_POLICY}"
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above.

@danmihai1
Copy link
Contributor Author

/test

@danmihai1 danmihai1 merged commit f60c9ea into kata-containers:main Apr 8, 2024
295 of 299 checks passed
@danmihai1 danmihai1 deleted the danmihai1/policy-test-cleanup branch April 26, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test size/large Task of significant size
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants