snp: added snp unencrypted test by ryansavino · Pull Request #5594 · kata-containers/tests

ryansavino · 2023-04-08T08:08:15Z

added makefile target as well
moved common functions out of sev.bats
few minor fixes in separate commits

Signed-Off-By: Ryan Savino ryan.savino@amd.com

zvonkok · 2023-04-09T21:19:58Z

+  )
+  if [[ -z "${measurement}" ]]; then return 1; fi
+  echo ${measurement}
+}


No newline at EOF

fitzthum

A few comments.

There may be some more code that could be shared with the non-tee tests, but I'm not sure it's worth getting into that given the impending merge and certain organizational issues with the tests.

I'm not very familiar with the stuff in .ci. Hopefully somoene else can help review those changes.

We will also need a new case in tests_runner.sh in the operator repo.

fitzthum · 2023-04-10T13:15:28Z

+  # Delete all data with 'id = 10'
+  mysql -u${KBS_DB_USER} -p${KBS_DB_PW} -h ${kbs_db_host} -D ${KBS_DB} <<EOF
+    DELETE FROM secrets WHERE id = 10;
+    DELETE FROM keysets WHERE id = 10;


We shouldn't really have anything relating to keyses in the code anymore. I guess deleting them is fine as a precaution.

fitzthum · 2023-04-10T13:17:49Z

+
+  NAME="${name}" IMAGE="${image}" RUNTIMECLASS="${RUNTIMECLASS}" \
+    KBS_URI="${kbs_ip}:44444" \
+    POLICY="${policy}" \


Setting the policy and kbs_uri via annoation is not supported for SNP. The annotations will just be ignored so it isn't too bad, but this function isn't quite as general as it might seem.

fitzthum · 2023-04-10T13:29:11Z

+    docker-compose \
+    cpuid
+  pip install sev-snp-measure
+  "${TESTS_REPO_DIR}/.ci/install_yq.sh" >&2


Not all of these are required currently. I don't think we use cpuid yet, for instance, since we don't calculate launch measurement. On the other hand maybe we should make these deps generic and move them to the common file.

I like the idea of maintaining the packages to be installed in the script that needs them. I added these packages in anticipation for when attestation tests would be added, but for now I've trimmed them down to what's required.

fitzthum · 2023-04-10T13:30:52Z

+  # Extract name from the file name
+  local name=$(basename "${service_yaml%.*}")
+
+  # Default policy is 3:


Only for SEV. SNP policies have different layout, but they don't get set here anyway (see below).

Resolved by other fix from above.

fitzthum

LGTM with one little note.

Also, I think the lib code is probably in the wrong place, but I'm not sure what the optimal location is.

fitzthum · 2023-04-18T14:58:46Z

-  local initrd_path="$(esudo /opt/confidential-containers/bin/kata-runtime \
-    --config ${SEV_CONFIG} kata-env --json | jq -r .Initrd.Path)"
+# Set the annotations for the simple-kbs URI and policy in the encrypted service yaml
+encrypted_yaml_set_kbs_annotations() {


This can be used for unencrypted images as well. It might be best to just have a yaml generation function for SEV and one for SNP. That is currently the fundamental difference in what annotations are available.

Are there any annotations we need to apply for the unencrypted?

The policy annotation is totally separate from whether the image is encrypted or not. The policy enables SEV-ES. It is totally valid to use -ES with an unencrypted image. The KBS URI isn't directly tied to encryption either, actually. You need to set that for signed images as well. So none of the annotations are directly related to whether the image is encrypted. That is determined by the format of the image itself. SEV and SNP do have different annotations as of today, though.

Updated. Check it out now. I think this way might be better. Created a k8s_yaml_set_annotation method in common.bash and removed the specific method from sev.bats.

New approach is good.

UnmeshDeodhar · 2023-05-02T23:46:34Z

/test

UnmeshDeodhar · 2023-05-08T22:03:09Z

/test-sev

UnmeshDeodhar · 2023-05-12T15:48:20Z

/test

UnmeshDeodhar · 2023-05-12T16:29:48Z

/test

This line is duplicate with line 19 Signed-Off-By: Ryan Savino <ryan.savino@amd.com> Remove duplicate line

containerd config in /etc/containerd/config.toml requires sudo access Signed-Off-By: Ryan Savino <ryan.savino@amd.com>

ariel-adam · 2023-07-05T12:49:36Z

@ryansavino is this on track for 0.7.0 (feature freeze 12th of July) or should we move it to 0.8.0?

wainersm · 2023-07-05T20:17:06Z

 		info "Running Confidential Containers tests for AMD SEV"
 		sudo -E PATH="$PATH" CRI_RUNTIME="containerd" bash -c "make cc-sev-kubernetes"
 		;;
+	"CC_SNP_CRI_CONTAINERD_K8S")


Sorry, just notice this CC_SNP_CRI_CONTAINERD_K8S.... usually we have CC_CRI_CONTAINERD_K8S_SOME_SPECIFIC_CONFIG. So it should be CC_CRI_CONTAINERD_K8S_SNP.

CC_SEV_CRI_CONTAINERD_K8S above seems wrong but better not change it right now.

@wainersm changing this will need changes to be made in the CI repo as well. So do you think it is safer to do that in a different PR? I can open one as soon as this gets merged. I already have a PR open in ci repo. What do you think?

@UnmeshDeodhar Indeed it is safer to do that change on a follow up PR.

UnmeshDeodhar · 2023-07-07T14:24:44Z

@ariel-adam, @ryansavino and I were on vacation this week so did not get a chance to reply. This is on track and I will incorporate all the feedback and get it ready before the merge. Will post something in the slack if I need someone's help to get it merged.

common methods from sev.bats moved into common.bash service.yaml.in generalized Signed-Off-By: Ryan Savino <ryan.savino@amd.com> Signed-Off-By: Unmesh Deodhar <udeodhar@amd.com>

UnmeshDeodhar · 2023-07-07T17:41:04Z

Thanks @wainersm for the review. I have addressed all your comments. Please let me know if this looks good now.

UnmeshDeodhar · 2023-07-07T17:41:10Z

/test

wainersm · 2023-07-07T20:39:37Z


 	echo "Create the test sandbox"
 	echo "Pod config is: $pod_config"
-	kubernetes_create_cc_pod $pod_config


Hi @UnmeshDeodhar , I prefer the other way around: change the name of the functions that you are introducing on this PR to Kubernetes_, and leave the existing non-snp tests untouched. I want to avoid any regressions on the exsiting tests at this point :)

Changed all the functions back to kubernetes_*

wainersm · 2023-07-07T20:49:24Z


-QEMU_PATH="/opt/kata/bin/qemu-system-x86_64-experimental"
+if [ "${KATA_BUILD_QEMU_TYPE}" == "snp" ]; then
+	QEMU_PATH="/opt/confidential-containers/bin/qemu-system-x86_64-snp"


In a VM where I ran this script I got the qemu for snp installed with another name:

$ crudini --get /opt/confidential-containers/share/defaults/kata-containers/configuration-qemu-snp.toml hypervisor.qemu path "/opt/confidential-containers/bin/qemu-system-x86_64-snp-experimental" $ ls /opt/confidential-containers/bin/ containerd-shim-kata-v2 kata-collect-data.sh kata-monitor kata-runtime qemu-system-x86_64-snp-experimental

Maybe some code on this install_qemu_experimental.sh script is bogus?

Good catch @wainersm.
I have removed the bogus code in build_qemu_experimental.sh

added makefile target as well Fixes: kata-containers#5593 Signed-Off-By: Ryan Savino <ryan.savino@amd.com> Signed-Off-By: Unmesh Deodhar <udeodhar@amd.com>

Fixes: kata-containers#5593 Signed-off-by: Unmesh Deodhar <udeodhar@amd.com>

Fixes: kata-containers#5593 Signed-Off-By: Unmesh Deodhar <udeodhar@amd.com>

SNP requires generic OVMF. Thus adding a code that builds generic OVMF. Fixes: kata-containers#5593 Signed-off-by: Unmesh Deodhar <udeodhar@amd.com>

Using pod annotation instead of changing the config file. Fixes: kata-containers#5593 Signed-Off-By: Unmesh Deodhar <udeodhar@amd.com>

Renaming k8s_* functions to kubernetes_* to match the rest of the functions. Fixes: kata-containers#5593 Signed-Off-By: Unmesh Deodhar <udeodhar@amd.com>

UnmeshDeodhar · 2023-07-07T22:38:03Z

/test

UnmeshDeodhar · 2023-07-07T22:51:20Z

/test

fitzthum · 2023-07-10T15:03:10Z

Last week there was some discussion about organizing this PR in a way that is easier to review or possibly splitting it into multiple. At this point I'm not really sure what would be most efficient, but let me make a few notes to help anyone who looks at this.

Commits 1 and 2 fix small miscellaneous issues in the SEV tests.
Commit 3 (the majority of the changes in this PR) extracts functions from the SEV tests that will also be used in the SNP test and moves them alongside some of the common code for the non-tee tests. This doesn't close all the gaps between the SEV tests and the non-tee tests, but it's a good start.
Commit 4 actually adds the SNP test, which is relatively simple.
Commits 5-9 fix SNP issues that snuck in during the merge from main to CCv0. It might have been better to put these in a separate PR. Since they are here, SNP will be broken until we can merge this. We could still move these into another PR, but it is getting pretty late in the day either way.

wainersm · 2023-07-10T15:42:28Z

I see that in http://jenkins.katacontainers.io/job/tests-CCv0-ubuntu-20.04_snp-x86_64-CC_SNP_CRI_CONTAINERD_K8S-PR/64/consoleFull the SNP test introduced on this PR ran fine:

20:17:54 INFO: Run tests
20:17:54 1..1
20:18:21 ok 1 [cc][kubernetes][containerd][snp] Test SNP unencrypted container launch success
20:18:24 ~/workspace/tests-CCv0-ubuntu-20.04_snp-x86_64-CC_SNP_CRI_CONTAINERD_K8S-PR/go/src/github.com/kata-containers/tests

BTW, that job was manually created just to test this PR. It will be automatically created again once we get kata-containers/ci#547 merged (which depends on this PR).

One last thing before we merge this.... the TDX job failed. @fidencio could you please tell us the failure is related with this PR (or not)?

wainersm · 2023-07-10T17:42:55Z

/test-tdx

wainersm

@UnmeshDeodhar @ryansavino I think it can be merged now. Any improvement and fixes made to follow up PRs. Thanks!

katacontainersbot added the size/huge Largest and most complex task (probably needs breaking into small pieces) label Apr 8, 2023

ryansavino force-pushed the add-snp-test branch from c535204 to 1d0271c Compare April 9, 2023 06:33

katacontainersbot added size/tiny Smallest and simplest task and removed size/huge Largest and most complex task (probably needs breaking into small pieces) labels Apr 9, 2023

zvonkok reviewed Apr 9, 2023

View reviewed changes

fitzthum reviewed Apr 10, 2023

View reviewed changes

ryansavino force-pushed the add-snp-test branch 3 times, most recently from 6de094d to b258a59 Compare April 10, 2023 23:51

ryansavino mentioned this pull request Apr 11, 2023

Add support to provision SNP components confidential-containers/operator#146

Closed

fitzthum mentioned this pull request Apr 18, 2023

CC: Add image signature tests for SEV #5578

Open

fitzthum approved these changes Apr 18, 2023

View reviewed changes

ryansavino force-pushed the add-snp-test branch from b258a59 to 4f77038 Compare April 18, 2023 21:46

katacontainersbot added size/huge Largest and most complex task (probably needs breaking into small pieces) and removed size/tiny Smallest and simplest task labels Apr 18, 2023

ryansavino force-pushed the add-snp-test branch from 4f77038 to af9a990 Compare April 18, 2023 21:54

UnmeshDeodhar force-pushed the add-snp-test branch 2 times, most recently from ec9ff8b to a08386a Compare May 2, 2023 22:15

UnmeshDeodhar force-pushed the add-snp-test branch from f093a53 to a77e884 Compare May 3, 2023 00:17

katacontainersbot added size/tiny Smallest and simplest task size/huge Largest and most complex task (probably needs breaking into small pieces) and removed size/huge Largest and most complex task (probably needs breaking into small pieces) size/tiny Smallest and simplest task labels May 3, 2023

UnmeshDeodhar force-pushed the add-snp-test branch from 510d263 to d1ff5fe Compare May 3, 2023 22:17

UnmeshDeodhar force-pushed the add-snp-test branch from df90192 to 86c6947 Compare May 12, 2023 16:51

katacontainersbot removed the size/huge Largest and most complex task (probably needs breaking into small pieces) label May 12, 2023

UnmeshDeodhar force-pushed the add-snp-test branch from 48f397a to bb9fb6f Compare June 28, 2023 19:57

ryansavino added 2 commits June 29, 2023 11:38

libs: remove duplicate line

e579c8d

This line is duplicate with line 19 Signed-Off-By: Ryan Savino <ryan.savino@amd.com> Remove duplicate line

config: add sudo to cp command

a2f8709

containerd config in /etc/containerd/config.toml requires sudo access Signed-Off-By: Ryan Savino <ryan.savino@amd.com>

UnmeshDeodhar force-pushed the add-snp-test branch 2 times, most recently from e657bc1 to 3b54502 Compare June 29, 2023 17:18

ariel-adam assigned ryansavino Jul 5, 2023

wainersm reviewed Jul 5, 2023

View reviewed changes

UnmeshDeodhar force-pushed the add-snp-test branch from 3b54502 to 1bab0a2 Compare July 7, 2023 14:58

katacontainersbot added size/tiny Smallest and simplest task and removed size/huge Largest and most complex task (probably needs breaking into small pieces) size/tiny Smallest and simplest task labels Jul 7, 2023

sev: abstract common methods to common source file

a4017f6

common methods from sev.bats moved into common.bash service.yaml.in generalized Signed-Off-By: Ryan Savino <ryan.savino@amd.com> Signed-Off-By: Unmesh Deodhar <udeodhar@amd.com>

wainersm reviewed Jul 7, 2023

View reviewed changes

ryansavino and others added 6 commits July 7, 2023 17:32

snp: added snp unencrypted test

da31671

added makefile target as well Fixes: kata-containers#5593 Signed-Off-By: Ryan Savino <ryan.savino@amd.com> Signed-Off-By: Unmesh Deodhar <udeodhar@amd.com>

ci: Add snp kernel build support to install_kata_kernel

625f300

Fixes: kata-containers#5593 Signed-off-by: Unmesh Deodhar <udeodhar@amd.com>

ci: Adding SNP case when building k8s test

c58061a

Fixes: kata-containers#5593 Signed-Off-By: Unmesh Deodhar <udeodhar@amd.com>

ci: Build generic OVMF for SNP

330f0fc

SNP requires generic OVMF. Thus adding a code that builds generic OVMF. Fixes: kata-containers#5593 Signed-off-by: Unmesh Deodhar <udeodhar@amd.com>

confidential: Using pod annotation

c040815

Using pod annotation instead of changing the config file. Fixes: kata-containers#5593 Signed-Off-By: Unmesh Deodhar <udeodhar@amd.com>

kubernetes: Renaming kubernetes prefix

280e0f2

Renaming k8s_* functions to kubernetes_* to match the rest of the functions. Fixes: kata-containers#5593 Signed-Off-By: Unmesh Deodhar <udeodhar@amd.com>

wainersm approved these changes Jul 10, 2023

View reviewed changes

Conversation

ryansavino commented Apr 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fitzthum left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fitzthum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryansavino Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UnmeshDeodhar commented May 2, 2023

Uh oh!

UnmeshDeodhar commented May 8, 2023

Uh oh!

UnmeshDeodhar commented May 12, 2023

Uh oh!

UnmeshDeodhar commented May 12, 2023

Uh oh!

ariel-adam commented Jul 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UnmeshDeodhar commented Jul 7, 2023

Uh oh!

UnmeshDeodhar commented Jul 7, 2023

Uh oh!

UnmeshDeodhar commented Jul 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UnmeshDeodhar commented Jul 7, 2023

Uh oh!

UnmeshDeodhar commented Jul 7, 2023

Uh oh!

fitzthum commented Jul 10, 2023

Uh oh!

wainersm commented Jul 10, 2023

Uh oh!

fitzthum left a comment •

edited

Loading

ryansavino Apr 18, 2023 •

edited

Loading