Support SEV_SNP instance type configuration #1410

bgartzi · 2025-01-23T09:23:55Z

/kind feature

What this PR does / why we need it:
Only SEV machines could be configured by using the former
confidentialCompute Enabled/Disabled. GCP allows now to also configure
the confidential instance type as well by using the appropriate
parameter, see [0].

This commit introduces confidentialInstanceType, which lets users choose
between sev or sev-snp as their confidential computing technology of
choice.

Meanwhile, add c3d as a machine that supports AMD SEV.

[0] https://cloud.google.com/confidential-computing/confidential-vm/docs/create-a-confidential-vm-instance#rest

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1402

Special notes for your reviewer:
confidentialInstanceType overrides confidentialCompute. This was due to these reasons:

Backwards compatibility.
Imitating the gcp compute API behavior.

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Add confidentialInstanceType to the API

linux-foundation-easycla · 2025-01-23T09:23:59Z

The committers listed above are authorized under a signed CLA.

✅ login: bgartzi / name: Beñat Gartzia (ff8cf8e)

k8s-ci-robot · 2025-01-23T09:24:04Z

[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 chrischdi 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

k8s-ci-robot · 2025-01-23T09:24:04Z

Welcome @bgartzi!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-gcp 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-gcp has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-01-23T09:24:05Z

Hi @bgartzi. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

netlify · 2025-01-23T09:24:13Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-gcp ready!

Name	Link
🔨 Latest commit	`ff8cf8e`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-gcp/deploys/679ba30bfa9b0700084c6dd0
😎 Deploy Preview	https://deploy-preview-1410--kubernetes-sigs-cluster-api-gcp.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

damdo · 2025-01-23T11:38:47Z

/ok-to-test

bgartzi · 2025-01-23T16:37:40Z

/retest

damdo

First pass on the API design, left some comments

damdo · 2025-01-28T09:46:55Z

api/v1beta1/gcpmachine_types.go

+	// ConfidentialVMTechSEV sets AMD SEV as the VM instance's confidential computing technology of choice.
+	ConfidentialVMTechSEV ConfidentialVMTechnology = "sev"
+	// ConfidentialVMTechSEVSNP sets AMD SEV-SNP as the VM instance's confidential computing technology of choice.
+	ConfidentialVMTechSEVSNP ConfidentialVMTechnology = "sev-snp"


I think it is better to use the fully typed out version of this, so
ConfidentialVMTechnologySEV and ConfidentialVMTechnologySEVSNP

Adressed in the last pushed version.

damdo · 2025-01-28T09:49:59Z

api/v1beta1/gcpmachine_types.go

@@ -339,10 +352,18 @@ type GCPMachineSpec 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.


Looking at this condition, and considering what we stated in the API above, I'm not sure this is the best approach we could take.

Looking deeper at what we are trying to set here, it seems like we trying to build up the underlying GCP
compute.ConfidentialInstanceConfig field in the compute/instance GCP SDK API.

Which is defined here: https://pkg.go.dev/google.golang.org/api/compute/v1#ConfidentialInstanceConfig

So we are trying to set these two fields:

type ConfidentialInstanceConfig struct { // ConfidentialInstanceType: Defines the type of technology used by the // confidential instance. // // Possible values: // "CONFIDENTIAL_INSTANCE_TYPE_UNSPECIFIED" - No type specified. Do not use // this value. // "SEV" - AMD Secure Encrypted Virtualization. // "SEV_SNP" - AMD Secure Encrypted Virtualization - Secure Nested Paging. // "TDX" - Intel Trust Domain eXtension. ConfidentialInstanceType [string](https://pkg.go.dev/builtin#string) `json:"confidentialInstanceType,omitempty"` // EnableConfidentialCompute: Defines whether the instance should have // confidential compute enabled. EnableConfidentialCompute [bool](https://pkg.go.dev/builtin#bool) `json:"enableConfidentialCompute,omitempty"` [...] }

and since I don't see any mention on the ConfidentialInstanceType taking precedence over EnableConfidentialCompute, I assume the latter takes the overall precedence and flicks the switch on whether or not the whole feature is enabled or not.

As such I think we should do the same.

ConfidentialCompute = enabled, ConfidentialInstanceType not set => enable, default confidential instance type

ConfidentialCompute = disabled, ConfidentialInstanceType not set => disable

ConfidentialCompute = disabled, ConfidentialInstanceType set, fail webhooks validation (we could also have a discriminated union here I think, but it might break the existing API, so we could leverage the WH)

ConfidentialCompute = unknown, ConfidentialInstanceType not set => disable

ConfidentialCompute = unknown, ConfidentialInstanceType set => fail webhooks validation (as the default for unknown is disabled).

cc. @JoelSpeed for API expertise.

I agree with Dam, if we already have an api that controls this enablement, having a second field that implicitly enables the feature is not good. It should be a validation error for the instance type to exist when confidential compute is disabled, as mentioned by Dam

Yes, I agree with both of you. That would make more sense than the implementation I provided in this draft.
However, even if not documented explicitly that way, that's how the compute api behaves. I know after trying. I know it's a weak excuse, but I thought there might have been some reason for that, as it makes the former EnableConfidentialComputing somewhat redundant.
That's why I decided to propose this as the first draft; an API that behaved as the compute api did, and decide if the true-to-the-backend or behavior we all would expect was the correct one.
Now, if we all agree on the proposed implementation instead, I will provide the changes, I hope that soon.

Hey @bgartzi thanks for providing details on how the GCP compute API behaves.
I still think that our API should behave the way we all expect it to in this case, even if the GCP API doesn't. Our won't be surprising as users will be prompted by webhook failures that inform them why it didn't go through.

I just updated the API design and the webhook implementation (+tests) according to the design agreed in the comments above.

JoelSpeed · 2025-01-28T10:20:28Z

api/v1beta1/gcpmachine_types.go

+	// confidentialInstanceType determines the required type of confidential computing technology.
+	// confidentialInstanceType will precede 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;


Kube conventions state that enum values should be PascalCase. What does sev and sev-snp actually mean? These aren't particularly expressive and might be hard for a consumer of this API to understand which value they may want

And yes, I also agree with you on this: those names aren't expressive at all. They are acronyms that stand for AMD's "Secure Encrypted Virtualization" [0] and "Secure Encrypted Virtualization - Secure Nested Paging" [1].

However, using the short acronyms instead seem to be a standard. One example that comes in handy is the compute API documentation and supported values for the confidential instance type field [2] (as referenced in the comment above). Even after reading the GCloud documentation[3], users might expect to find SEV/SEVSNP? I'm not personally sure.

Checking on how acronyms are written in pascalcase, would Sev/SevSnp + an expansion on the documentation work for you? Or would you rather go for the whole SecureEncryptedVirtualization/Secure EncryptedVirtualizationSecureNestedPaging combo?

[0] https://www.amd.com/es/developer/sev.html
[1] https://www.amd.com/content/dam/amd/en/documents/epyc-business-docs/white-papers/SEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf
[2] https://pkg.go.dev/google.golang.org/api/compute/v1#ConfidentialInstanceConfig
[3] https://cloud.google.com/confidential-computing/confidential-vm/docs/confidential-vm-overview

What about renaming the field secureInstanceEncryptionType and having values of Virtualization and VirtualizationWithNestedPaging? They would be fairly descriptive and flow right?

Regarding secureInstanceEncryptionType, I think it would be harder from users coming from the GCloud documentation to find the proper field name, and vice versa.
In terms of the supported values, the problem I see on mentioning virtualization is that most of the proposed confidential computing technologies are based on virtualization (probably all except for intel's sgx?). As these are AMD's solutions, intel has their own (TDX) and other architectures as ARM (CCA), also do (although it hasn't made it to GCP yet). Yes, Virtualization is only referred from SEV's name currently. But I think calling it Virtualization could also bring some confusion, as it could refer to AMD's SEV as to any other confidential computing solution.
Although cryptic, these names are basically trademarks, so I think it's way to make sure they will exclude each other. I also have to admit I'm for sure biased as those are the ways I see these technologies are referenced in documentation/news and that I don't have many new ideas to propose at this moment.

After thinking about this a little bit more, what do you think about keeping confidentialInstanceType and admitting the following values:

EncryptedVirtualizationSev

EncryptedNestedPagingSevsnp

And possibly upcoming ones:

TrustedDomainTdx

A bit too repetitive perhaps?

I haven't addressed this in the last force-pushed version of the patch. I will be happy to do so when we reach an agreement.

uril · 2025-01-27T18:54:36Z

api/v1beta1/gcpmachine_webhook.go

@@ -108,15 +108,32 @@ func (m *GCPMachine) Default() {
 	clusterlog.Info("default", "name", m.Name)
 }

+func targetConfidentialType(tech *ConfidentialVMTechnology) (ConfidentialVMTechnology, error) {
+	if tech == nil || tech != nil && *tech == "" {


Thanks @bgartzi for adding SEV_SNP support.

I think this can be shorten to: if tech == nil || *tech == "" {

Thanks @uril! Well noted. I will apply this on the next proposal.

I couldn't address it (explicitly) as the API design change made update this piece of code. However, I took it into account in the new conditions I had to write, thanks!

bgartzi

@damdo, @JoelSpeed, @uril thanks a lot for the quick review! I will try to update the PR as soon as I confirm some of the questions.

bgartzi · 2025-01-29T15:14:34Z

api/v1beta1/gcpmachine_types.go

@@ -339,10 +352,18 @@ type GCPMachineSpec 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.


Yes, I agree with both of you. That would make more sense than the implementation I provided in this draft.
However, even if not documented explicitly that way, that's how the compute api behaves. I know after trying. I know it's a weak excuse, but I thought there might have been some reason for that, as it makes the former EnableConfidentialComputing somewhat redundant.
That's why I decided to propose this as the first draft; an API that behaved as the compute api did, and decide if the true-to-the-backend or behavior we all would expect was the correct one.
Now, if we all agree on the proposed implementation instead, I will provide the changes, I hope that soon.

bgartzi · 2025-01-29T15:22:55Z

api/v1beta1/gcpmachine_types.go

+	// confidentialInstanceType determines the required type of confidential computing technology.
+	// confidentialInstanceType will precede 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;


And yes, I also agree with you on this: those names aren't expressive at all. They are acronyms that stand for AMD's "Secure Encrypted Virtualization" [0] and "Secure Encrypted Virtualization - Secure Nested Paging" [1].

However, using the short acronyms instead seem to be a standard. One example that comes in handy is the compute API documentation and supported values for the confidential instance type field [2] (as referenced in the comment above). Even after reading the GCloud documentation[3], users might expect to find SEV/SEVSNP? I'm not personally sure.

Checking on how acronyms are written in pascalcase, would Sev/SevSnp + an expansion on the documentation work for you? Or would you rather go for the whole SecureEncryptedVirtualization/Secure EncryptedVirtualizationSecureNestedPaging combo?

[0] https://www.amd.com/es/developer/sev.html
[1] https://www.amd.com/content/dam/amd/en/documents/epyc-business-docs/white-papers/SEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf
[2] https://pkg.go.dev/google.golang.org/api/compute/v1#ConfidentialInstanceConfig
[3] https://cloud.google.com/confidential-computing/confidential-vm/docs/confidential-vm-overview

bgartzi · 2025-01-29T15:23:00Z

api/v1beta1/gcpmachine_types.go

+	// ConfidentialVMTechSEV sets AMD SEV as the VM instance's confidential computing technology of choice.
+	ConfidentialVMTechSEV ConfidentialVMTechnology = "sev"
+	// ConfidentialVMTechSEVSNP sets AMD SEV-SNP as the VM instance's confidential computing technology of choice.
+	ConfidentialVMTechSEVSNP ConfidentialVMTechnology = "sev-snp"


bgartzi · 2025-01-29T15:24:23Z

api/v1beta1/gcpmachine_webhook.go

@@ -108,15 +108,32 @@ func (m *GCPMachine) Default() {
 	clusterlog.Info("default", "name", m.Name)
 }

+func targetConfidentialType(tech *ConfidentialVMTechnology) (ConfidentialVMTechnology, error) {
+	if tech == nil || tech != nil && *tech == "" {


Thanks @uril! Well noted. I will apply this on the next proposal.

Only SEV machines could be configured by using the former confidentialCompute Enabled/Disabled. GCP allows now to also configure the confidential instance type as well by using the appropriate parameter, see [0]. This commit introduces confidentialInstanceType, which lets users choose between sev or sev-snp as their confidential computing technology of choice. The reason confidentialInstanceType will preceed confidentialCompute is to mimic GCP's behavior, and ensuring backwards compatibility. Meanwhile, add c3d as a machine that supports AMD SEV. [0] https://cloud.google.com/confidential-computing/confidential-vm/docs/create-a-confidential-vm-instance#rest

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Jan 23, 2025

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 23, 2025

k8s-ci-robot requested review from damdo and dims January 23, 2025 09:24

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 23, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 23, 2025

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 23, 2025

bgartzi mentioned this pull request Jan 23, 2025

Support SEV_SNP confidential type selection on GCP openshift/api#2165

Open

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 23, 2025

bgartzi force-pushed the gcp-sev_snp branch 2 times, most recently from 8269aa6 to 5cbf79b Compare January 23, 2025 15:46

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

damdo reviewed Jan 28, 2025

View reviewed changes

JoelSpeed reviewed Jan 28, 2025

View reviewed changes

uril reviewed Jan 29, 2025

View reviewed changes

bgartzi commented Jan 29, 2025

View reviewed changes

bgartzi force-pushed the gcp-sev_snp branch from 5cbf79b to ff8cf8e Compare January 30, 2025 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SEV_SNP instance type configuration #1410

Support SEV_SNP instance type configuration #1410

bgartzi commented Jan 23, 2025

linux-foundation-easycla bot commented Jan 23, 2025 •

edited

Loading

k8s-ci-robot commented Jan 23, 2025

k8s-ci-robot commented Jan 23, 2025

k8s-ci-robot commented Jan 23, 2025

netlify bot commented Jan 23, 2025 •

edited

Loading

damdo commented Jan 23, 2025

bgartzi commented Jan 23, 2025

damdo left a comment

damdo Jan 28, 2025

bgartzi Jan 29, 2025

bgartzi Jan 30, 2025

damdo Jan 28, 2025

JoelSpeed Jan 28, 2025

bgartzi Jan 29, 2025

damdo Jan 29, 2025

bgartzi Jan 30, 2025

JoelSpeed Jan 28, 2025

bgartzi Jan 29, 2025

JoelSpeed Jan 29, 2025

bgartzi Jan 29, 2025

bgartzi Jan 30, 2025

bgartzi Jan 30, 2025

uril Jan 27, 2025

bgartzi Jan 29, 2025

bgartzi Jan 30, 2025

bgartzi left a comment

bgartzi Jan 29, 2025

bgartzi Jan 29, 2025

bgartzi Jan 29, 2025

bgartzi Jan 29, 2025

Support SEV_SNP instance type configuration #1410

Are you sure you want to change the base?

Support SEV_SNP instance type configuration #1410

Conversation

bgartzi commented Jan 23, 2025

linux-foundation-easycla bot commented Jan 23, 2025 • edited Loading

k8s-ci-robot commented Jan 23, 2025

k8s-ci-robot commented Jan 23, 2025

k8s-ci-robot commented Jan 23, 2025

netlify bot commented Jan 23, 2025 • edited Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-gcp ready!

damdo commented Jan 23, 2025

bgartzi commented Jan 23, 2025

damdo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgartzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Jan 23, 2025 •

edited

Loading

netlify bot commented Jan 23, 2025 •

edited

Loading