CORS-4188: AWS - Add support to AMD SEV-SNP confidential VMs #10012
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@fangge1212: This pull request references CORS-4188 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
fangge1212
force-pushed
the
aws_sev_snp
branch
2 times, most recently
from
d4990ea to
55c2984
Compare
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
When I tried to bump openshift/client-go to db0dee36 and cluster-api to v1.11.2, I encountered more dependency issues: $ (edit go.mod to remove the replace lines for k83.io/apimachinery and cluster-api) $ go mod tidy |
|
Hi @mtulio @patrickdillon |
The module paths changed in cluster-api v1.11.2. It seems like a big change, so should we bump cluster-api in another PR? |
|
@fangge1212 Bumping cluster-api (capi) to v1.11 is a significant change in the installer as we will need to bump all providers to versions that is compatible with capi v1.11 😞 We have opened a card https://issues.redhat.com/browse/CORS-4262 here to track with more details. |
|
/cc |
|
@fangge1212 Currently working on #10060 to unblock the issues you're facing |
fangge1212
force-pushed
the
aws_sev_snp
branch
3 times, most recently
from
fa1e5a0 to
54f0c53
Compare
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@fangge1212 some of the unit tests are failing. PTAL. Unfortunately prow is down at the moment and I can't review the results at this time. |
pkg/asset/machines/aws/machines.go
Outdated
| Placement: machineapi.Placement{Region: in.region, AvailabilityZone: in.zone}, | ||
| SecurityGroups: securityGroups, | ||
| CPUOptions: &machineapi.CPUOptions{ | ||
| ConfidentialCompute: ptr.To(machineapi.AWSConfidentialComputePolicy(*in.cpuOptions.ConfidentialCompute)), |
There was a problem hiding this comment.
unit tests are failing because of a panic. You need to check if in.cpuOptions is set
pkg/types/aws/machinepool.go
Outdated
| // More details can be checked at https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/sev-snp.html | ||
| // When omitted, this means no opinion and the AWS platform is left to choose a reasonable default, | ||
| // which is subject to change without notice. The current default is Disabled. | ||
| // +kubebuilder:validation:Enum=Disabled;AMDEncryptedVirtualizationNestedPaging |
There was a problem hiding this comment.
| // +kubebuilder:validation:Enum=Disabled;AMDEncryptedVirtualizationNestedPaging |
I think we can remove this +kubebuilder:validation marker as we already define one on the enum type on line 152.
Otherwise, install.openshift.io_installconfigs.yaml:186-194, there's a redundant allOf with duplicate enum definitions:
allOf:
- enum:
- Disabled
- AMDEncryptedVirtualizationNestedPaging
- enum:
- Disabled
- AMDEncryptedVirtualizationNestedPagin| field.Invalid( | ||
| fldPath.Child("confidentialCompute"), | ||
| p.CPUOptions.ConfidentialCompute, | ||
| fmt.Sprintf("Allowed values are %s, %s and omitted", aws.ConfidentialComputePolicyDisabled, aws.ConfidentialComputePolicySEVSNP), | ||
| ), |
There was a problem hiding this comment.
| field.Invalid( | |
| fldPath.Child("confidentialCompute"), | |
| p.CPUOptions.ConfidentialCompute, | |
| fmt.Sprintf("Allowed values are %s, %s and omitted", aws.ConfidentialComputePolicyDisabled, aws.ConfidentialComputePolicySEVSNP), | |
| ), | |
| field.NotSupported(fldPath.Child("confidentialCompute"), | |
| p.CPUOptions.ConfidentialCompute, | |
| []aws.ConfidentialComputePolicy{aws.ConfidentialComputePolicyDisabled, aws.ConfidentialComputePolicySEVSNP}, | |
| ), |
nit: we can use field.NotSupported here to complain about unsupported values 😁
| err: `^test-path.cpuOptions: Invalid value: "{}": At least one field must be set if cpuOptions is provided$`, | ||
| }} | ||
| for _, test := range cases { | ||
| t.Run("", func(t *testing.T) { |
There was a problem hiding this comment.
For each test case, we should give it a name... In case, there is failure in the future, we can trace back which test is failing.
| CPUOptions: capa.CPUOptions{ | ||
| ConfidentialCompute: capa.AWSConfidentialComputePolicy(*mpool.CPUOptions.ConfidentialCompute), | ||
| }, |
There was a problem hiding this comment.
You will also need check for nil here first before setting the CPU options. See #10012 (comment)
I think due to #10012 (comment) and #10012 (comment), there is a panic in all of the aws e2e jobs because they don't have the new |
fangge1212
force-pushed
the
aws_sev_snp
branch
3 times, most recently
from
f86e677 to
c25a423
Compare
|
@fangge1212 ci/prow/verify-codegen is not happy at the moment. We just need another quick |
|
@fangge1212: The The following commands are available to trigger optional jobs: Use 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. |
|
/retest-required |
|
/verified by yalzhang together with openshift/installer#10107 |
openshift-ci-robot
added
the
verified
|
@yalzhang: This PR has been marked as verified by 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/test verify-codegen |
|
@fangge1212 can you please rebase and run |
|
/retest-required |
fangge1212
force-pushed
the
aws_sev_snp
branch
from
69577af to
a026fea
Compare
openshift-ci-robot
removed
the
verified
Signed-off-by: Fangge Jin <[email protected]>
This will allow configuring confidential computing on AWS platform, only AMD SEV-SNP is supported for now. Signed-off-by: Fangge Jin <[email protected]>
fangge1212
force-pushed
the
aws_sev_snp
branch
from
a026fea to
64a524b
Compare
|
/verified by yalzhang manually by installing a confidential cluster on aws |
|
@yalzhang: 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/verified by yalzhang |
openshift-ci-robot
added
the
verified
|
@yalzhang: This PR has been marked as verified by 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
|
/skip |
|
@fangge1212: The following tests failed, say
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. |
|
/test e2e-gcp-ovn |
5 participants
This PR adds support to AMD SEV-SNP confidential VMs on AWS platform.
Related PRs:
openshift/api: openshift/api#2424
machine-api-operator: openshift/machine-api-operator#1420
machine-api-provider-aws: openshift/machine-api-provider-aws#141
upstream: kubernetes-sigs/cluster-api-provider-aws#5605