kbs: Simplify deployments, split out IBM SE #521

stevenhorsman · 2024-10-01T14:27:35Z

The current "s390x" overlay support is currently very SE specific, whereas deploying on non-SE s390x with the sample KBS is still an important scenario for our testing without specialised hardware.

Let's separate out the ibmse overlay(s), and update deploy-kbs.sh to help make the s390x non-SE scenario easier.

stevenhorsman · 2024-10-01T16:58:04Z

I've tested this on s390x (non-SE):

trustee/kbs/config/kubernetes# ./deploy-kbs.sh
namespace/coco-tenant created
configmap/kbs-config-68g582hg7m created
configmap/policy-config-55tfg2kc22 created
secret/kbs-auth-public-key-h64bbgc98k created
secret/keys-4cbmd8h422 created
service/kbs created
deployment.apps/kbs created
root@sh-libvirt-s390x:~/go/src/github.com/confidential-containers/cloud-api-adaptor/src/cloud-api-adaptor/test/trustee/kbs/config/kubernetes# kubectl get pods -A
NAMESPACE                        NAME                                               READY   STATUS             RESTARTS       AGE
coco-tenant                      kbs-65f467957d-m86qc                               1/1     Running            0              6s

I don't have an SE LPAR, so @BbolroC can you test it on SE when you get a chance please? Thanks!

mythi · 2024-10-02T06:08:41Z

kbs/config/kubernetes/deploy-kbs.sh

@@ -22,18 +25,13 @@ kbs_cert="${k8s_cnf_dir}/base/kbs.pem"
    openssl pkey -in "${k8s_cnf_dir}/base/kbs.key" -pubout -out "${kbs_cert}"
 }

-if [ "${ARCH}" == "s390x" ]; then
-    if [ -n "${IBM_SE_CREDS_DIR:-}" ]; then
+if [ "${OVERLAY}" == "ibmse" ]; then


Suggested change

if [ "${OVERLAY}" == "ibmse" ]; then

if [ "${OVERLAY}" == "ibm-se" ]; then

Fixed - thanks for the spot!

mythi · 2024-10-02T06:23:14Z

The current "s390x" overlay support is currently very SE specific, whereas deploying on non-SE s390x

What are the arch specific settings in these deployments? It feels to me this s390x vs x86_64 vs common just makes things more complex than they should be and they should be removed altogether.

stevenhorsman · 2024-10-02T06:43:02Z

The current "s390x" overlay support is currently very SE specific, whereas deploying on non-SE s390x

What are the arch specific settings in these deployments? It feels to me this s390x vs x86_64 vs common just makes things more complex than they should be and they should be removed altogether.

That's fair - it looks like s390x doesn't need the same patch.yaml as x86, but otherwise they are very similar. The thought was to keep them together for flexibility, but I'm happy to try merging and having common and ibm-se if you'd prefer?

mythi · 2024-10-02T07:12:44Z

it looks like s390x doesn't need the same patch.yaml as x86,

AFAUI, this just means workload_key/key.bin Secret is not available on s390x.

try merging and having common and ibm-se

we can treat ibm-se and an independent overlay and restore overlays back to where it was. This would also make "Expose KBS using Ingress" instructions working again.

stevenhorsman · 2024-10-02T08:20:22Z

we can treat ibm-se and an independent overlay and restore overlays back to where it was. This would also make "Expose KBS using Ingress" instructions working again.

Hi Mikko, just to try and clarify what you are suggesting as I'm not sure what "back to where it was" is referencing. Is you suggestion that the "normal" deployment is like:

kubernetes
- overlays
  - ingress.yaml
  - kustomization.yaml (that points to ../base, lists the patch.yml and the secretGenerator)
  - patch.yaml (with the keys volumes mounts/volume)
    thereby removing the common and x86_64 overlays entirely

Then we'd have the ibm-se overlay still in kubernetes/overlays/ibm-se with:

kustomization.yaml (that is largely a duplicate, but points to pvc.yaml as well as ../../base, patch.yml and the secretGenerator)
patch.yaml, pv.yaml and pvc.yaml as they are in this PR?

Sorry if I've misunderstood, I'm not great with kustomize!

mythi · 2024-10-02T09:52:48Z

Is you suggestion that the "normal" deployment is like:

yes, this was exactly my thinking. "back to where it was" is referring to the point where we did not have any of that common/x86_64/s390x.

stevenhorsman · 2024-10-02T10:06:37Z

Is you suggestion that the "normal" deployment is like:

yes, this was exactly my thinking. "back to where it was" is referring to the point where we did not have any of that common/x86_64/s390x.

Cool - I'll work on this now. Thank you.

stevenhorsman · 2024-10-02T10:44:24Z

Ok, updated now and re-tested on s390x:

/trustee/kbs/config/kubernetes# ./deploy-kbs.sh
namespace/coco-tenant created
configmap/kbs-config-68g582hg7m created
configmap/policy-config-55tfg2kc22 created
secret/kbs-auth-public-key-h64bbgc98k created
secret/keys-ft5bc4tbth created
service/kbs created
deployment.apps/kbs created
root@sh-libvirt-s390x:~/go/src/github.com/confidential-containers/cloud-api-adaptor/src/cloud-api-adaptor/test/trustee/kbs/config/kubernetes# kubectl get pods -n coco-tenant
NAME                   READY   STATUS    RESTARTS   AGE
kbs-6fc45c745d-z6hxf   1/1     Running   0          10s

mythi

pls double check ita and custom_pccs for x86_64 specific paths

other than that, LGTM.

stevenhorsman · 2024-10-03T10:33:54Z

pls double check ita and custom_pccs for x86_64 specific paths

other than that, LGTM.

Sorry about this - whilst I was working on the refactor I added a mental note to fix them too, but it got paged out, so thanks for keeping me on it!

kbs/config/kubernetes/custom_pccs/kustomization.yaml

mythi

thanks!

stevenhorsman · 2024-10-03T13:50:44Z

thanks!

Thanks for you review advice and patience!

fitzthum

LGTM. One little suggestion.

You might also think about adding a note or two to the docs about using s390x. There is one mention of setting an env var, but I don't see anything else about what users should do. Maybe they don't need to do anything else but still could be useful to note that. Maybe here or here.

kbs/config/kubernetes/deploy-kbs.sh

stevenhorsman · 2024-10-08T11:14:06Z

You might also think about adding a note or two to the docs about using s390x. There is one mention of setting an env var, but I don't see anything else about what users should do. Maybe they don't need to do anything else but still could be useful to note that. Maybe here or here.

The IBM SE deployment, with an explanation of IBM_SE_CREDS_DIR is documented in the Kubernetes deployment readme: https://github.com/confidential-containers/trustee/blob/main/kbs/config/kubernetes/README.md#deploy-kbs. Maybe that could be linked elsewhere?

fitzthum · 2024-10-08T14:35:26Z

The IBM SE deployment, with an explanation of IBM_SE_CREDS_DIR is documented in the Kubernetes deployment readme: https://github.com/confidential-containers/trustee/blob/main/kbs/config/kubernetes/README.md#deploy-kbs. Maybe that could be linked elsewhere?

That's a good start. It's fine to put it off to another PR, but that doc doesn't mention what this dir is or when/why it should be setup. I assume the idea is that you need it if you want to verify an SE guest but not if you just want to run Trustee on s390x.

stevenhorsman · 2024-10-08T14:37:41Z

That's a good start. It's fine to put it off to another PR, but that doc doesn't mention what this dir is or when/why it should be setup. I assume the idea is that you need it if you want to verify an SE guest but not if you just want to run Trustee on s390x.

Yeah, that's not something I'm an expert on, but it is specifically for SE, so I can try and make that clearer in the doc.

Edit: I've added some more information and explanation, so hopefully it helps with the clarity?

The current "s390x" overlay support is currently very SE specific, whereas deploying on non-SE s390x with the sample KBS is still an important scenario for our testing without specialised hardware. This scenario deployment matches the x86_64 deployment scenario, so let's go back to having a standard deployment, with a special case for ibm-se rather to reduce duplication. Update the documentation to clarify when and where `IBM_SE_CREDS_DIR` is needed. Signed-off-by: stevenhorsman <[email protected]>

In confidential-containers/trustee#521 the overlays logic was modified to add non-SE s390x support and simplify non-ibm-se platforms. We need to update the logic in `kbs_k8s_deploy` to match and can remove the dummying of `IBM_SE_CREDS_DIR` for non-SE now Signed-off-by: stevenhorsman <[email protected]>

In confidential-containers#521 I re-worked the deploy-kbs script with overlays and didn't factor in that the key.bin needed to go to a different place for ibm-se, so it was causing: ``` trustee/kbs/config/kubernetes$ ls overlays/key.bin ls: cannot access 'overlays/key.bin': No such file or directory ``` on an SE system. I think the least bad way to resolve this is to move the ibm-se logic up before the key.bin check and rely on the updated `DEPLOYMENT_DIR` Signed-off-by: stevenhorsman <[email protected]>

In confidential-containers#521 I re-worked the deploy-kbs script with overlays and didn't factor in that the key.bin needed to go to a different place for ibm-se, so it was causing: ``` trustee/kbs/config/kubernetes$ ls overlays/key.bin ls: cannot access 'overlays/key.bin': No such file or directory ``` on an SE system. I think the least bad way to resolve this is to move the ibm-se logic up before the key.bin check and rely on the updated `DEPLOYMENT_DIR` Also update the deployment doc instructions to add the ibm-se case Signed-off-by: stevenhorsman <[email protected]>

stevenhorsman requested a review from a team as a code owner October 1, 2024 14:27

mythi reviewed Oct 2, 2024

View reviewed changes

stevenhorsman marked this pull request as draft October 2, 2024 07:54

stevenhorsman force-pushed the non-se-s390x-deployment-support branch 3 times, most recently from b1d8297 to 48b3a34 Compare October 2, 2024 10:36

stevenhorsman marked this pull request as ready for review October 2, 2024 10:44

stevenhorsman force-pushed the non-se-s390x-deployment-support branch from 48b3a34 to d709aa4 Compare October 2, 2024 11:12

stevenhorsman requested a review from mythi October 3, 2024 07:59

mythi reviewed Oct 3, 2024

View reviewed changes

stevenhorsman force-pushed the non-se-s390x-deployment-support branch from ecd7959 to 00be657 Compare October 3, 2024 10:33

mythi reviewed Oct 3, 2024

View reviewed changes

kbs/config/kubernetes/custom_pccs/kustomization.yaml Outdated Show resolved Hide resolved

stevenhorsman force-pushed the non-se-s390x-deployment-support branch from 00be657 to 171389d Compare October 3, 2024 12:57

mythi approved these changes Oct 3, 2024

View reviewed changes

stevenhorsman changed the title ~~kbs: Split out s390x, from IBM SE deployment~~ kbs: Simplify deployments, split out IBM SE Oct 4, 2024

fitzthum approved these changes Oct 7, 2024

View reviewed changes

kbs/config/kubernetes/deploy-kbs.sh Show resolved Hide resolved

stevenhorsman force-pushed the non-se-s390x-deployment-support branch from 171389d to c26d5fc Compare October 8, 2024 11:11

stevenhorsman force-pushed the non-se-s390x-deployment-support branch from c26d5fc to ce9e654 Compare October 8, 2024 14:46

fitzthum merged commit b96ccf6 into confidential-containers:main Oct 8, 2024
14 checks passed

stevenhorsman mentioned this pull request Oct 8, 2024

test: Update Trustee deployment to match the simplifed deployment overlays confidential-containers/cloud-api-adaptor#2099

Merged

stevenhorsman mentioned this pull request Oct 9, 2024

Kbs deploy overlays update kata-containers/kata-containers#10401

Merged

stevenhorsman mentioned this pull request Oct 9, 2024

kbs: Fix ibm-se deployment #525

Merged

stevenhorsman deleted the non-se-s390x-deployment-support branch October 9, 2024 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kbs: Simplify deployments, split out IBM SE #521

kbs: Simplify deployments, split out IBM SE #521

stevenhorsman commented Oct 1, 2024

stevenhorsman commented Oct 1, 2024

mythi Oct 2, 2024

stevenhorsman Oct 2, 2024

mythi commented Oct 2, 2024

stevenhorsman commented Oct 2, 2024

mythi commented Oct 2, 2024

stevenhorsman commented Oct 2, 2024

mythi commented Oct 2, 2024

stevenhorsman commented Oct 2, 2024

stevenhorsman commented Oct 2, 2024

mythi left a comment

stevenhorsman commented Oct 3, 2024

mythi left a comment

stevenhorsman commented Oct 3, 2024

fitzthum left a comment

stevenhorsman commented Oct 8, 2024

fitzthum commented Oct 8, 2024

stevenhorsman commented Oct 8, 2024 •

edited

Loading

	if [ "${OVERLAY}" == "ibmse" ]; then
	if [ "${OVERLAY}" == "ibm-se" ]; then

kbs: Simplify deployments, split out IBM SE #521

kbs: Simplify deployments, split out IBM SE #521

Conversation

stevenhorsman commented Oct 1, 2024

stevenhorsman commented Oct 1, 2024

mythi Oct 2, 2024

Choose a reason for hiding this comment

stevenhorsman Oct 2, 2024

Choose a reason for hiding this comment

mythi commented Oct 2, 2024

stevenhorsman commented Oct 2, 2024

mythi commented Oct 2, 2024

stevenhorsman commented Oct 2, 2024

mythi commented Oct 2, 2024

stevenhorsman commented Oct 2, 2024

stevenhorsman commented Oct 2, 2024

mythi left a comment

Choose a reason for hiding this comment

stevenhorsman commented Oct 3, 2024

mythi left a comment

Choose a reason for hiding this comment

stevenhorsman commented Oct 3, 2024

fitzthum left a comment

Choose a reason for hiding this comment

stevenhorsman commented Oct 8, 2024

fitzthum commented Oct 8, 2024

stevenhorsman commented Oct 8, 2024 • edited Loading

stevenhorsman commented Oct 8, 2024 •

edited

Loading