API-1844: Update KMSConfig to support all external kms plugins #2622
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,54 +2,33 @@ package v1 | |||||
|
|
||||||
| // KMSConfig defines the configuration for the KMS instance | ||||||
| // that will be used with KMSEncryptionProvider encryption | ||||||
| type KMSConfig struct { | ||||||
| // managementModel defines how KMS plugins are managed. | ||||||
| // Valid values are "External". | ||||||
| // When set to External, encryption keys are managed by a user-deployed | ||||||
| // KMS plugin that communicates via UNIX domain socket using KMS V2 API. | ||||||
| // | ||||||
|
Contributor
There was a problem hiding this comment. For serialization safety and to adhere to v1 API guarantees, we do not allow removal of any fields directly, even if it was only accessible in tech preview. Instead, we ask to tombstone them (removal with a comment), see some examples #2256 and #2576 This also means that the field name may not be used again, so we should be certain that we're ok with the removal.
Member
Author
There was a problem hiding this comment. Thank you for review. That makes sense. I'll update this PR based on your comments, once we finalize the design. |
||||||
| // +kubebuilder:validation:Enum=External | ||||||
| // +kubebuilder:default=External | ||||||
|
Contributor
There was a problem hiding this comment. Could you help me understand why you'd like a default to be set here? We generally try not to set defaults via the API but instead through controllers and document the default behaviour via godocs, allowing for flexibility in the future. Since we only have one optional for now, is anything depending on this field to be explicitly set? For additional context, see: https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#defaulting |
||||||
| // +optional | ||||||
| ManagementModel ManagementModel `json:"managementModel,omitempty"` | ||||||
|
|
||||||
| // endpoint specifies the UNIX domain socket endpoint for communicating with the external KMS plugin. | ||||||
| // The endpoint must follow the format "unix:///path". | ||||||
|
Contributor
There was a problem hiding this comment.
Suggested change
|
||||||
| // Abstract Linux sockets (i.e. "unix:///@abstractname") are not supported. | ||||||
| // | ||||||
| // +kubebuilder:validation:MaxLength=120 | ||||||
|
Member
Author
There was a problem hiding this comment. Based on https://man7.org/linux/man-pages/man7/unix.7.html ("Address format" section), maximum length is 108. It is less for mac. So I decided to set it to 120. |
||||||
| // +kubebuilder:validation:MinLength=9 | ||||||
| // +kubebuilder:validation:XValidation:rule="self.matches('^unix:///[^@ ][^ ]*$')",message="endpoint must follow the format 'unix:///path'" | ||||||
| // +required | ||||||
| Endpoint string `json:"endpoint,omitempty"` | ||||||
| } | ||||||
|
|
||||||
| // ManagementModel describes how the KMS plugin is managed. | ||||||
| // Valid values are "External". | ||||||
| type ManagementModel string | ||||||
|
|
||||||
| const ( | ||||||
| // External represents a KMS plugin that is managed externally and accessed via unix domain socket | ||||||
| External ManagementModel = "External" | ||||||
| ) | ||||||
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.
Out of curiosity, what would potentially be other possible values in the future? Would there be any cloud-provided specific plugins supported here since you mentioned that AWS is obsolete? Just trying to understand what would be the api changes would look like in the future.