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.

Sign up for GitHub

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

Jump to bottom

Support SEV_SNP confidential type selection on GCP #2165

Open

bgartzi wants to merge 1 commit into openshift:master from bgartzi:gcp-sev_snp

+54 −26

bgartzi commented Jan 23, 2025

GCP compute API permits not only specifying whether a machine has to be confidential or not, but also determining the technology backing that up see [0]. This commit adds an api parameter into the GCPMachineProviderSpec to support that.

That way, we should be able to configure SEV_SNP machines as well as SEV machines as before.

[0] https://cloud.google.com/confidential-computing/confidential-vm/docs/supported-configurations

Upstream/CAPG PR: kubernetes-sigs/cluster-api-provider-gcp#1410

openshift-ci bot added the size/S label

openshift-ci bot requested review from deads2k and mandre

January 23, 2025 10:05

openshift-ci bot added the needs-ok-to-test label

Contributor

openshift-ci bot commented Jan 23, 2025

Hello @bgartzi! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

Contributor

openshift-ci bot commented Jan 23, 2025

Hi @bgartzi. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Contributor

openshift-ci bot commented Jan 23, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bgartzi
Once this PR has been reviewed and has the lgtm label, please assign spadgett for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This was referenced Jan 23, 2025

DRAFT: Support installation on SEV SNP confidential nodes openshift/installer#9395

Open

Support SEV_SNP confidential machine provisioning openshift/machine-api-provider-gcp#107

Open

Support AMD SEV_SNP machines on GCP openshift/machine-api-operator#1324

Open

bgartzi force-pushed the gcp-sev_snp branch from c6513bb to c0fbf0e Compare

January 27, 2025 14:42


          Support SEV_SNP confidential type selection on GCP

232b41e

GCP compute API permits not only specifying whether a machine has to be
confidential or not, but also determining the technology backing that
up see [0]. This commit adds an api parameter into the
GCPMachineProviderSpec to support that.

That way, we should be able to configure SEV_SNP machines as well as SEV
machines as before.

[0] https://cloud.google.com/confidential-computing/confidential-vm/docs/supported-configurations

bgartzi force-pushed the gcp-sev_snp branch from c0fbf0e to 232b41e Compare

January 27, 2025 15:20

everettraven reviewed

View reviewed changes

machine/v1beta1/types_gcpprovider.go

@@ @@ -146,9 +155,16 @@ type GCPMachineProviderSpec struct { @@
               	// confidentialCompute Defines whether the instance should have confidential compute enabled.
               	// If enabled OnHostMaintenance is required to be set to "Terminate".
               	// If omitted, the platform chooses a default, which is subject to change over time, currently that default is false.
+              	// If confidentialInstanceType is configured, even if confidentialCompute is Disabled, a confidential compute instance will be configured.

everettraven Jan 27, 2025

This feels like it could be ripe for confusion. Would it make more sense to make it so you can only set confidentialInstanceType when confidentialCompute is set to Enabled?

Author

bgartzi Jan 29, 2025

Yes, I agree with you: it would make more sense. I've been also told the same in the ongoing discussion on https://github.com/kubernetes-sigs/cluster-api-provider-gcp/pull/1410/files#r1931842698

I will change this soon.

machine/v1beta1/types_gcpprovider.go

+              	// confidentialInstanceType will preceed confidentialCompute. That is, if confidentialCompute is "Disabled" but a valid confidentialInstanceType is specified, a confidential instance will be configured.
+              	// If confidentialInstanceType isn't set and confidentialCompute is "Enabled" the platform will set the default, which is subject to change over time. Currently the default is "sev" for "c2d", "c3d", and "n2d" machineTypes. For the other machine cases, a valid confidentialInstanceType must be specified.
+              	// +kubebuilder:validation:Enum=sev;sev-snp;
+              	// +optional

everettraven Jan 27, 2025

Mention that this field is optional in the godoc.

Author

bgartzi Jan 29, 2025

I will!

machine/v1beta1/types_gcpprovider.go

+              	// confidentialInstanceType determines the required type of confidential computing technology.
+              	// confidentialInstanceType will preceed confidentialCompute. That is, if confidentialCompute is "Disabled" but a valid confidentialInstanceType is specified, a confidential instance will be configured.
+              	// If confidentialInstanceType isn't set and confidentialCompute is "Enabled" the platform will set the default, which is subject to change over time. Currently the default is "sev" for "c2d", "c3d", and "n2d" machineTypes. For the other machine cases, a valid confidentialInstanceType must be specified.
+              	// +kubebuilder:validation:Enum=sev;sev-snp;

everettraven Jan 27, 2025

Include some information in the godoc about the allowed values and what they represent.

Author

bgartzi Jan 29, 2025

Noted.

bgartzi commented

View reviewed changes

Author

bgartzi left a comment

Hi @everettraven, thanks for the quick reply. I will work on the updates and try to send an updated proposal soon!

machine/v1beta1/types_gcpprovider.go

@@ @@ -146,9 +155,16 @@ type GCPMachineProviderSpec struct { @@
               	// confidentialCompute Defines whether the instance should have confidential compute enabled.
               	// If enabled OnHostMaintenance is required to be set to "Terminate".
               	// If omitted, the platform chooses a default, which is subject to change over time, currently that default is false.
+              	// If confidentialInstanceType is configured, even if confidentialCompute is Disabled, a confidential compute instance will be configured.

Author

bgartzi Jan 29, 2025

Yes, I agree with you: it would make more sense. I've been also told the same in the ongoing discussion on https://github.com/kubernetes-sigs/cluster-api-provider-gcp/pull/1410/files#r1931842698

I will change this soon.

machine/v1beta1/types_gcpprovider.go

+              	// confidentialInstanceType determines the required type of confidential computing technology.
+              	// confidentialInstanceType will preceed confidentialCompute. That is, if confidentialCompute is "Disabled" but a valid confidentialInstanceType is specified, a confidential instance will be configured.
+              	// If confidentialInstanceType isn't set and confidentialCompute is "Enabled" the platform will set the default, which is subject to change over time. Currently the default is "sev" for "c2d", "c3d", and "n2d" machineTypes. For the other machine cases, a valid confidentialInstanceType must be specified.
+              	// +kubebuilder:validation:Enum=sev;sev-snp;

Author

bgartzi Jan 29, 2025

Noted.

machine/v1beta1/types_gcpprovider.go

+              	// confidentialInstanceType will preceed confidentialCompute. That is, if confidentialCompute is "Disabled" but a valid confidentialInstanceType is specified, a confidential instance will be configured.
+              	// If confidentialInstanceType isn't set and confidentialCompute is "Enabled" the platform will set the default, which is subject to change over time. Currently the default is "sev" for "c2d", "c3d", and "n2d" machineTypes. For the other machine cases, a valid confidentialInstanceType must be specified.
+              	// +kubebuilder:validation:Enum=sev;sev-snp;
+              	// +optional

Author

bgartzi Jan 29, 2025

I will!

eranco74 reviewed

View reviewed changes

machine/v1beta1/types_gcpprovider.go

@@ @@ -65,6 +65,15 @@ const ( @@
               	ConfidentialComputePolicyDisabled ConfidentialComputePolicy = "Disabled"
               )
+              type ConfidentialVMTechnology string

Contributor

eranco74 Feb 9, 2025 •

edited

Loading

ConfidentialComputePolicy was added as an enum for the case that GCP will add another potential value, see upstream discussion:
kubernetes-sigs/cluster-api-provider-gcp#809 (comment)
Consider adding the new options ("sev", "sev-snp") to the ConfidentialComputePolicy, in case the user set it to "Enabled" - use the default.

Author

bgartzi Feb 10, 2025

Thanks for pointing this out @eranco74, I think it's worth considering. I've just brought this discussion topic into the upstream patch as well (see kubernetes-sigs/cluster-api-provider-gcp#1410 (comment)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ok-to-test size/S