-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[CNV#69839] Enable IBM Secure Execution on ocpvirt #99764
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
Conversation
jschintag
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you @SNiemann15
|
🤖 Fri Oct 03 10:31:36 - Prow CI generated the docs preview: |
0394064 to
b48529d
Compare
jschintag
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
b48529d to
c20adce
Compare
|
New changes are detected. LGTM label has been removed. |
| [id="virt-enabling-vms-ibm-secure-execution-ibm-z_{context}"] | ||
| = Enabling VMs to run {ibm-name} Secure Execution on {ibm-z-name} and {ibm-linuxone-name} | ||
|
|
||
| Before you can run {ibm-name} Secure Execution virtual machines (VMs) on {ibm-z-name} and {ibm-linuxone-name} you must prepare the cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Before you can run {ibm-name} Secure Execution virtual machines (VMs) on {ibm-z-name} and {ibm-linuxone-name} you must prepare the cluster. | |
| Before you can run {ibm-name} Secure Execution virtual machines (VMs) on {ibm-z-name} and {ibm-linuxone-name}, you must prepare the cluster. |
|
Hi @SNiemann15, just a few small comments! Otherwise, LGTM. |
51a4480 to
bbc50d4
Compare
|
/lgtm |
|
@nbziouec: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ousleyp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Silke, I have some comments and suggestions for you to peruse. :) Please let me know if you have any questions; thanks!
| [id="virt-enabling-vms-ibm-secure-execution-ibm-z_{context}"] | ||
| = Enabling VMs to run {ibm-name} Secure Execution on {ibm-z-name} and {ibm-linuxone-name} | ||
|
|
||
| Before you can run {ibm-name} Secure Execution virtual machines (VMs) on {ibm-z-name} and {ibm-linuxone-name}, you must prepare the cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Prepare the cluster" doesn't inform me of what I am about to do. I would suggest making this intro more specific, if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SNiemann15 , i would propose to say:
Before you can run {ibm-name} Secure Execution virtual machines (VMs) on {ibm-z-name} and {ibm-linuxone-name}, you need to fulfil the following prerequisite and follow the step bellow to enable SE feature on the computer nodes of the cluster
|
|
||
| .Procedure | ||
|
|
||
| . To run {ibm-name} Secure Execution VMs, you must enable the nodes using the kernel command line. To do this for all compute nodes, apply the following machine configuration: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion for clarity:
| . To run {ibm-name} Secure Execution VMs, you must enable the nodes using the kernel command line. To do this for all compute nodes, apply the following machine configuration: | |
| . To run {ibm-name} Secure Execution VMs, you must enable the nodes using the kernel command line. To enable all compute nodes, create a file named `secure-execution.yaml` containing the following machine config manifest: |
I'm also unclear about what "enable the nodes using the kernel command line" means, and can't quite tell if that phrasing needs to be adjusted. Maybe you could explain this a bit in the intro, since it's the main action to be taken (other than the feature gate)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proposal :
"To be able to run {ibm-name} Secure Execution VMs, you have to enable the following parameter (prot_virt=1) on the kernel level of each compute node. Apply the following machine configuration on each compute node : "
| spec: | ||
| kernelArguments: | ||
| - prot_virt=1 | ||
| ---- | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of the YAML seems self-explanatory, but I would love context on this parameter. Suggestion:
| spec: | |
| kernelArguments: | |
| - prot_virt=1 | |
| ---- | |
| spec: | |
| kernelArguments: | |
| - prot_virt=1 | |
| ---- | |
| + | |
| where: | |
| `prot_virt=1`:: Specifies <explanation of what this parameter does/why it is here> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prot_virt=1:: enabling this parameter on the kernel of the host allows the ultravisor to store in memory the required security information.
more details : https://libvirt.org/kbase/s390_protected_virt.html#host-requirements
| .Prerequisites | ||
|
|
||
| * Your cluster has logical partition (LPAR) nodes running on IBM z15 or later. | ||
| * You have {ibm-name} Secure Execution workloads available to run on the cluster. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .Prerequisites | |
| * Your cluster has logical partition (LPAR) nodes running on IBM z15 or later. | |
| * You have {ibm-name} Secure Execution workloads available to run on the cluster. | |
| .Prerequisites | |
| * Your cluster has logical partition (LPAR) nodes running on IBM z15 or later. | |
| * You have {ibm-name} Secure Execution workloads available to run on the cluster. | |
| * You have installed the {oc-first}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SNiemann15 i just caught this , we are refering to z15 but not linuxone version , i would propose to also add :
- Your cluster has logical partition (LPAR) nodes running on IBM z15 or later IBM Z machine, or IBM LinuxONE III or later LinuxONE machine
| + | ||
| [source,terminal] | ||
| ---- | ||
| oc edit -n openshift-cnv HyperConverged kubevirt-hyperconverged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| oc edit -n openshift-cnv HyperConverged kubevirt-hyperconverged | |
| $ oc edit -n openshift-cnv HyperConverged kubevirt-hyperconverged |
|
|
||
| .Procedure | ||
|
|
||
| . Apply the following machine configuration: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- For a one-step procedure, there should be a
*(unordered list) instead of a.(ordered list). But this might be a two step procedure. (Confirm, but I think we need a second step to apply the changes, even if that step is just saving the file.) - This is a VM manifest, not a machine config :)
| . Apply the following machine configuration: | |
| * Apply the following `VirtualMachine` manifest to the cluster: |
| apiVersion: kubevirt.io/v1 | ||
| kind: VirtualMachine | ||
| metadata: | ||
| labels: | ||
| kubevirt.io/vm: f41-se | ||
| name: f41-se | ||
| spec: | ||
| runStrategy: Always | ||
| template: | ||
| metadata: | ||
| labels: | ||
| kubevirt.io/vm: f41-se | ||
| spec: | ||
| domain: | ||
| launchSecurity: {} | ||
| devices: | ||
| disks: | ||
| - disk: | ||
| bus: virtio | ||
| name: rootfs | ||
| machine: | ||
| type: "" | ||
| resources: | ||
| requests: | ||
| memory: 4Gi | ||
| terminationGracePeriodSeconds: 0 | ||
| volumes: | ||
| - name: rootfs | ||
| dataVolume: | ||
| name: f41-se |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very specific VM manifest; is it always these exact parameters? If not, please update it (using the description list format) so that it is clear which parameters need to be changed by the customer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the point from this VM manifest example is to point that we need to add spec.domain.launchSecurity element/part in the manifest : this part :
spec:
domain:
launchSecurity: {}
those are the exact parameters everytime , the rest can vary depends on the VM spec (other parameters in the manifest are described in other parts of the doc i believe for all archs )
| As the memory of the VM is protected, {ibm-name} Secure Execution VMs are not live migratable. The VMs | ||
| can only be migrated offline. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ISG nit; also removing hard wrap.
| As the memory of the VM is protected, {ibm-name} Secure Execution VMs are not live migratable. The VMs | |
| can only be migrated offline. | |
| Because the memory of the VM is protected, {ibm-name} Secure Execution VMs are not live migratable. The VMs can only be migrated offline. |
| :_mod-docs-content-type: ASSEMBLY | ||
| include::_attributes/common-attributes.adoc[] | ||
| [id="virt-configuring-ibm-secure-execution-vms-ibm-z"] | ||
| = Configuring {ibm-title} Secure Execution virtual machines on {ibm-z-title} and {ibm-linuxone-title} | ||
| :context: virt-configuring-ibm-secure-execution-vms-ibm-z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need a couple blank lines here (source: https://github.com/openshift/openshift-docs/blob/main/contributing_to_docs/doc_guidelines.adoc#general-file-guidelines).
| :_mod-docs-content-type: ASSEMBLY | |
| include::_attributes/common-attributes.adoc[] | |
| [id="virt-configuring-ibm-secure-execution-vms-ibm-z"] | |
| = Configuring {ibm-title} Secure Execution virtual machines on {ibm-z-title} and {ibm-linuxone-title} | |
| :context: virt-configuring-ibm-secure-execution-vms-ibm-z | |
| :_mod-docs-content-type: ASSEMBLY | |
| include::_attributes/common-attributes.adoc[] | |
| [id="virt-configuring-ibm-secure-execution-vms-ibm-z"] | |
| = Configuring {ibm-title} Secure Execution virtual machines on {ibm-z-title} and {ibm-linuxone-title} | |
| :context: virt-configuring-ibm-secure-execution-vms-ibm-z |
|
|
||
| toc::[] | ||
|
|
||
| You can configure {ibm-name} Secure Execution virtual machines (VMs) on {ibm-z-name} and {ibm-linuxone-name}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be out of scope for this change, but I would love to see some more conceptual information regarding what a Secure Execution VM is, or some additional resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would propose to use the same information as @jschintag did for the documentation upstream for kubevirt : https://kubevirt.io/user-guide/cluster_admin/confidential_computing/#ibm-secure-execution-for-linux-secure-execution
"IBM Secure Execution for Linux is a s390x security technology that is introduced with IBM z15 and LinuxONE III. It protects data of workloads that run in a KVM guest from being inspected or modified by the server environment.
In particular, no hardware administrator, no KVM code, and no KVM administrator can access the data in a guest that was started as an IBM Secure Execution guest.
For more details please read the official documentation."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we can take that he took the text from ibm.docs page.
bbc50d4 to
f772c8a
Compare
| + | ||
| where: | ||
| + | ||
| `prot_virt=1`:: Specifies that the ultravisor can use memory-protection hardwrarehost supports guests in {ibm-name} Secure Execution mode and allow the . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SNiemann15 fyi, this line has some weird things going on :D
|
|
||
| .Procedure | ||
|
|
||
| . Apply the following `VirtualMachine` manifest to the cluster: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're keeping this as one step, please change the . to *
| where: | ||
| + | ||
| [source,yaml] | ||
| ---- | ||
| spec: | ||
| domain: | ||
| launchSecurity: | ||
| ---- | ||
| + | ||
| Is required to launch {ibm-name} Secure Execution VMs, the other values can vary depending on the setup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| where: | |
| + | |
| [source,yaml] | |
| ---- | |
| spec: | |
| domain: | |
| launchSecurity: | |
| ---- | |
| + | |
| Is required to launch {ibm-name} Secure Execution VMs, the other values can vary depending on the setup. | |
| To launch {ibm-name} Secure Execution VMs, you must include the following YAML in the manifest: | |
| + | |
| [source,yaml] | |
| ---- | |
| spec: | |
| domain: | |
| launchSecurity: | |
| ---- | |
| + | |
| The rest of the VM manifest is variable depending on your setup. |
ousleyp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't do a full re-review but commented on a few things that stood out to me. Let me know when you're ready for another round! :)
f772c8a to
e7437c9
Compare
e7437c9 to
01cdda9
Compare
|
@SNiemann15: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Thanks so much for implementing my comments! It's probably a good idea to double check the feature gate implementation later, but for now it LGTM. |
|
/cherrypick enterprise-4.20 |
|
@ousleyp: new pull request created: #100013 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Version(s): 4.20+
Issue:
https://issues.redhat.com/browse/CNV-69839
Link to docs preview:
https://99764--ocpdocs-pr.netlify.app/openshift-enterprise/latest/virt/creating_vm/virt-configuring-ibm-secure-execution-vms-ibm-z.html
QE review:
Additional information: