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

KBS: Enable deployment for s390x #436

Merged

Conversation

BbolroC
Copy link
Member

@BbolroC BbolroC commented Jul 1, 2024

The following changes have been made to enable the KBS deployment for s390x:

  • Made the mount point /opa/confidential-containers/kbs writable (for both arches, reviving kbs: Revert support for configurable policy #431)
  • Differentiated the configuration for x86_64 and s390x in overlays and nodeport
  • Updated the documentation

Detailed explanations can be found in each commit message. Verification was performed on a test machine for each platform.

Signed-off-by: Hyounggyu Choi [email protected]

Using initContainers, we can make a previously read-only mount point writable.
This allows the test code using the set-policy endpoint to function correctly
while optimizing the use of configmap.

This commit configures the initContainers for the KBS deployment and adjusts
the overlays accordingly.

Signed-off-by: Hyounggyu Choi <[email protected]>
Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

Thanks @BbolroC . The init-container way seems work-able for both configMap and writable. I think also needs reviews from @mkulke

A question from my side is why not we marked policy as volume rather than configMap. What if a stateful volume? In this way we can probably store the policy in front of time (even for DBs).

Copy link
Contributor

@mkulke mkulke left a comment

Choose a reason for hiding this comment

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

why do we need to add a persistent volume? could we just mount the configmap into the init-container (read-only) and copy the file to the (writable) container storage?

@BbolroC
Copy link
Member Author

BbolroC commented Jul 1, 2024

why do we need to add a persistent volume? could we just mount the configmap into the init-container (read-only) and copy the file to the (writable) container storage?

Do you see any cons for using the PV? Why I chose the PV is that I just don't want to open any credentials used for a SE verifier to upstream. For me, a PV is the easiest and simplest way of encapsulating those data for kustomize.

@mkulke
Copy link
Contributor

mkulke commented Jul 1, 2024

why do we need to add a persistent volume? could we just mount the configmap into the init-container (read-only) and copy the file to the (writable) container storage?

Do you see any cons for using the PV? Why I chose the PV is that I just don't want to open any credentials used for a SE verifier to upstream. For me, a PV is the easiest and simplest way of encapsulating those data for kustomize.

why not use the kustomize secret generator?

Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

Seems good. Two comments.

@@ -9,7 +9,7 @@ We will see how to deploy KBS (with builtin Attestation Service) on a Kubernetes
Create a secret that you want to be served using this instance of KBS:

```bash
echo "This is my super secert" > overlays/key.bin
echo "This is my super secert" > overlays/$(uname -m)/key.bin
Copy link
Member

Choose a reason for hiding this comment

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

Should probably fix secert -> secret

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I will change it. Thanks!

- command:
- sh
- -c
- cp -r /config/$(dirname $(readlink /config/policy.rego))/* /opa/confidential-containers/kbs/
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't support updates at runtime or provisioning resources, right? Maybe this is fine in the short term.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it is just used for copying any data from a read-only mounting point to a writable one before creating a container. I agree with your opinion. Thanks!

@BbolroC BbolroC force-pushed the enable-kustomize-kbs-for-s390x branch from 8a6d0b3 to 39c7191 Compare July 1, 2024 16:47
@BbolroC
Copy link
Member Author

BbolroC commented Jul 1, 2024

A question from my side is why not we marked policy as volume rather than configMap. What if a stateful volume? In this way we can probably store the policy in front of time (even for DBs).

I'm not sure what the original idea behind the policy configuration was. We might be able to improve the configuration later after some investigation. Thanks.

@BbolroC
Copy link
Member Author

BbolroC commented Jul 1, 2024

why not use the kustomize secret generator?

The configuration is specific to s390x, and a way of configuring the credentials via PV aligns with the documentation at https://github.com/confidential-containers/trustee/tree/main/attestation-service/verifier/src/se. This means that once a user has prepared the credentials following the documentation, they can simply export IBM_SE_CREDS_DIR to point to the root directory of the credentials.

Forgot mentioning that a file size for one of the files is larger than 150Mb.

@BbolroC BbolroC force-pushed the enable-kustomize-kbs-for-s390x branch from 39c7191 to f2ef314 Compare July 1, 2024 17:19
BbolroC added 2 commits July 1, 2024 19:21
The following changes enable KBS deployment with a different configuration for s390x:

- Environment variable declaration: SE_SKIP_CERTS_VERIFICATION
- Persist volume/volume claim: required attestation credentials

This commit differentiates the {overlays, nodeport} configuration for KBS deployment
between x86_64 and s390x. It also includes updates to `deploy-kbs.sh`.

Signed-off-by: Hyounggyu Choi <[email protected]>
This commit updates the documentation to include instructions
for KBS deployment on s390x.

Signed-off-by: Hyounggyu Choi <[email protected]>
@BbolroC BbolroC force-pushed the enable-kustomize-kbs-for-s390x branch from f2ef314 to 0e229a7 Compare July 1, 2024 17:22
Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

LGTM. I think we may change back to the simple config map approach soon, but I guess it's fine to use the init container

@Xynnn007 Xynnn007 merged commit d5d03d9 into confidential-containers:main Jul 2, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants