-
Notifications
You must be signed in to change notification settings - Fork 198
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
Add support for disk encryption key in GCPMachine #1137
Conversation
Welcome @bfournie! |
Hi @bfournie. 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 Once the patch is verified, the new status will be reflected by the 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/test-infra repository. |
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 bare API comments from @JoelSpeed
Thanks @bfournie
/ok-to-test |
/test ls |
@cpanato: The specified target(s) for
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/test-infra repository. |
/test pull-cluster-api-provider-gcp-conformance |
3f82a49
to
82942c4
Compare
82942c4
to
9c6f36a
Compare
// KMSKeyName is the name of the encryption key that is stored in Google Cloud KMS. For example: | ||
// "kmsKeyName": "projects/kms_project_id/locations/region/keyRings/key_region/cryptoKeys/key | ||
// +kubebuilder:validation:Required | ||
KMSKeyName string `json:"kmsKeyName,omitempty"` |
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.
Does GCP specify any constraints for the name of the KMS key? Upper case? Lower case? Certain special characters allowed? MInimum or maximum length? All of this could be validated at admission time to prevent errors down the line
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.
So unless I'm missing something I don't see constraints defined in the API https://pkg.go.dev/google.golang.org/api/compute/v1#CustomerEncryptionKey or in validations. I'm leery to add additional limit checks here unless its well defined.
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.
When trying to create a key, it gives me
Key names can contain letters, numbers, underscores (_), and hyphens (-). Keys can't be renamed or deleted.
I believe that is also true of project IDs and regions so we probably can limit to that selection plus the slashes required
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.
Thanks for testing that @JoelSpeed.
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.
Thanks, I'll add a check in the webhook.Validator.
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.
Did the rule cost come up before or after you added the maxlength? The maxlength is an important factor in the rule cost estimations
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.
@JoelSpeed - you make a good point about kubebuilder validations/CEL vs webhook validation. I think part of the current reliance on webhook validation in CAPI is historical because CEL wasn't available for more complex validation logic.
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 rule cost error came up when using the 3 XValidation rules. With MaxLength validation plus one XValidation it did not cause an error.
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 was speaking with @vincepri last week and we think there's merit in having a community wide conversation about API review and CEL vs webhook validations. In a couple of weeks I have some time to put together some ideas which I'll bring to the community call for discussion
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.
Sounds good. Hopefully we can get this merged and revisit the validations after the discussion.
a2d0fb3
to
3b0fe01
Compare
3b0fe01
to
28e4a9c
Compare
28e4a9c
to
f1f917b
Compare
✅ Deploy Preview for kubernetes-sigs-cluster-api-gcp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
f1f917b
to
b2d5674
Compare
// KMSKeyName is the name of the encryption key that is stored in Google Cloud KMS. For example: | ||
// "kmsKeyName": "projects/kms_project_id/locations/region/keyRings/key_region/cryptoKeys/key | ||
// +kubebuilder:validation:Required | ||
KMSKeyName string `json:"kmsKeyName,omitempty"` |
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 was speaking with @vincepri last week and we think there's merit in having a community wide conversation about API review and CEL vs webhook validations. In a couple of weeks I have some time to put together some ideas which I'll bring to the community call for discussion
b2d5674
to
2148ab3
Compare
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
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.
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
Let's see what @richardcase thinks
Looks good to me, thanks @bfournie . Good discussion on CEL as part of this 👍 /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bfournie, cpanato, richardcase 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 |
/unhold |
/retest |
Add support for the disk encryption key for the boot disk and and additional disks.
/retest |
2148ab3
to
0c780af
Compare
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
What type of PR is this?
What this PR does / why we need it:
Add support for the disk encryption key for the boot disk and and additional disks in the GCP machine.
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 #
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
TODOs:
Release note: