Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

tests: add test case for kata agent API configuration #4677

Merged
merged 5 commits into from
Apr 20, 2022
Merged

tests: add test case for kata agent API configuration #4677

merged 5 commits into from
Apr 20, 2022

Conversation

wainersm
Copy link
Contributor

@wainersm wainersm commented Apr 6, 2022

Added a e2e test to check that when a pod is created with an agent API
config that blocked certain endpoints that requests to these are rejected.

Fixes #4676
Signed-off-by: Wainer dos Santos Moschetta [email protected]

@katacontainersbot katacontainersbot added the size/large Task of significant size label Apr 6, 2022
@wainersm wainersm added rfc Requires input from the team and removed size/large Task of significant size labels Apr 6, 2022
@wainersm
Copy link
Contributor Author

wainersm commented Apr 6, 2022

Hi! I'm labeling 'rfc' this PR because so far my goal is to collect feedback.

I'm introducing a method (cp_to_guest_img()) to allow us to copy files to the guest image. Currently, on CCv0 branch we build the image with some files for testing included, but we likely must stop doing that when merge the branches. In a particular chat with @fidencio he suggested the use of volumes (IIUC pod volumes) but I should have the file inside the guest at the time the agent starts. Did you like that approach?

I noticed that when I enable the "CC" handler in containerd's configuration and I don't enable image offloading on kata side then it fails to start the container. Is it expected behavior? i.e. once you add the handler, you are completely disabling the image pulls from containerd (delegated to the kata agent) so it is mandatory to enable image offload on kata?

My last remark is that it seems we don't necessarily need to test agent API configuration with CC. If that is the case maybe I should try to merge that test case straight to the main branch. Or am I missing something?

@wainersm wainersm force-pushed the CCV0-agent_api-tests branch from cf64957 to a5380eb Compare April 11, 2022 22:56
@katacontainersbot katacontainersbot added the size/large Task of significant size label Apr 11, 2022
@wainersm
Copy link
Contributor Author

Updates to this pull request:

  • added code to save/restore the guest image.

@wainersm wainersm force-pushed the CCV0-agent_api-tests branch from a5380eb to 9aafec7 Compare April 12, 2022 20:58
@wainersm
Copy link
Contributor Author

Updates to this pull request:

  • Added a checker on logs to ensure the cause of the failure was the API blocked
  • Added a checker for the default case: no passing a configuration file, the exec API should work
  • Created a shared library (asserts.sh) to hold all assert functions
  • using those asserts in both agent_image and agent_api tests

wainersm added 4 commits April 13, 2022 18:07
@wainersm
tests: add test case for kata agent API configuration
b306e7a 
Added a e2e test to check that when a pod is created with an agent API
config that blocked certain endpoints that requests to these are rejected.

Fixes kata-containers#4676
Signed-off-by: Wainer dos Santos Moschetta <[email protected]>
@wainersm
integration/containerd: introduce the asserts.sh library
d9dadaa 
Moved the assert functions from confidential/agent_image.bats to a library file (asserts.sh)
so that they can be re-used in various tests files.

Signed-off-by: Wainer dos Santos Moschetta <[email protected]>
@wainersm
integration/containerd: use assert_logs_contain() in agent_api tests
df25db2 
In order to use the assert_logs_contain() in agent_api.bats, it was needed to change
the function so that journalctl grabs logs from "kata", "containerd" and "crio" identifiers.

Signed-off-by: Wainer dos Santos Moschetta <[email protected]>
@wainersm
integration/containerd: use container asserts on agent_api tests
1b09b93 
Created assert_can_exec_on_container() and refactored assert_container(), then
use both on agent_api tests.

Signed-off-by: Wainer dos Santos Moschetta <[email protected]>
stevenhorsman
stevenhorsman approved these changes Apr 14, 2022
View reviewed changes
Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

Hi Wainer, sorry I missed this PR. I think it looks good and having a method to add files to the guest image is going to be really helpful and something we should switch to using in ccv0.sh.
With respect to your question about where to test agent API - I think you are correct that the function went into main and your test case would be valid in a non CC environment.

integration/containerd/confidential/agent_api.bats
Comment on lines +12 to +26
start_date=$(date +"%Y-%m-%d %H:%M:%S")
sandbox_name="kata-cc-busybox-sandbox"
pod_config="${FIXTURES_DIR}/pod-config.yaml"
pod_id=""

echo "Delete any existing ${sandbox_name} pod"
crictl_delete_cc_pod_if_exists "$sandbox_name"

echo "Prepare containerd for Confidential Container"
SAVED_CONTAINERD_CONF_FILE="/etc/containerd/config.toml.$$"
configure_cc_containerd "$SAVED_CONTAINERD_CONF_FILE"

echo "Reconfigure Kata Containers"
clear_kernel_params
switch_image_service_offload on
Copy link
Member

Choose a reason for hiding this comment

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

As this code matches the agent_image.bats I wonder if there is merit in moving it to a shared library to avoid the duplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, It does make sense to remove that duplication. I wish it was Python (or any OO language) where we could use heritage. Mind if I send it in a follow up PR?

@wainersm wainersm force-pushed the CCV0-agent_api-tests branch from 9aafec7 to 0c2a2f6 Compare April 14, 2022 18:48
@wainersm
integration/containerd: add handler for initrd image on cp_to_guest_i…
40d7f5d 
…mg()

This added added the code to copy files into an initrd image.

Signed-off-by: Wainer dos Santos Moschetta <[email protected]>
@wainersm wainersm force-pushed the CCV0-agent_api-tests branch from 0c2a2f6 to 40d7f5d Compare April 14, 2022 18:50
@wainersm
Copy link
Contributor Author

Update this PR with what I think the last missing bits: added handler for initrd in cp_to_guest_img() (40d7f5d). Sending in a separate commit just to ease the reviews but I can squash into b306e7a before the merge.

@wainersm wainersm removed the rfc Requires input from the team label Apr 14, 2022
@wainersm
Copy link
Contributor Author

Hi @stevenhorsman ,

Hi Wainer, sorry I missed this PR. I think it looks good and having a method to add files to the guest image is going to be really helpful and something we should switch to using in ccv0.sh. With respect to your question about where to test agent API - I think you are correct that the function went into main and your test case would be valid in a non CC environment.

Can we adapt ccv0.sh after merging this one? Thinking aloud...maybe it can be done alongside the merge to main. Once we have #4675 merged and ported to CCv0, we can work on preparing the osbuilder (**) which, among other changes, it will need to remove the signature and config files being added in the image at the build time.

** it seems we don't have an issue to track that work. At least the closest that I found was about removing skopeo/umoci (kata-containers/kata-containers#3970)

Regarding where to merge this PR...I think it is too late to merge on main as it is too tied with the code in CCv0. Mind if I port it alongside the other CC tests when the time comes for #4628?

@wainersm
Copy link
Contributor Author

/test

@stevenhorsman
Copy link
Member

stevenhorsman commented Apr 19, 2022 •
edited
Loading

Hey Wainer, sorry for the lack of clarity, my comments weren't supposed to be merge stoppers, just something to consider.

Can we adapt ccv0.sh after merging this one? Thinking aloud...maybe it can be done alongside the merge to main. Once we have #4675 merged and ported to CCv0, we can work on preparing the osbuilder (**) which, among other changes, it will need to remove the signature and config files being added in the image at the build time.

Agreed, I meant this is great code we should try and switch to using in ccv0.sh in a separate PR after this as refactoring to remove the hard-coded policy files from the dev code will be great. I don't think that should be in this PR though :)

Regarding where to merge this PR...I think it is too late to merge on main as it is too tied with the code in CCv0. Mind if I port it alongside the other CC tests when the time comes for #4628?

Yep, that's fine by me. I think the scenario would be valid with the current main dev code, but can be ported as part of the existing plans to merge in CCv0.

@stevenhorsman
Copy link
Member

Hey @wainersm - as you mentioned that it was missing, I've created kata-containers/kata-containers#4111 to cover using the new cp_to_guest_img to inject the image container image signature verification files into the guest image. Please take a look and add an info & fix my mistakes. This might be something that someone at IBM can do soon, but no promises yet until we've done some planning :)

@wainersm wainersm merged commit b48c03b into kata-containers:CCv0 Apr 20, 2022
@wainersm wainersm mentioned this pull request Apr 20, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@amshinde amshinde amshinde left review comments

@stevenhorsman stevenhorsman stevenhorsman approved these changes

Assignees
No one assigned
Labels
area/agent area/confidential-containers area/testing size/large Task of significant size
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wainersm @stevenhorsman @amshinde @katacontainersbot