-
Notifications
You must be signed in to change notification settings - Fork 623
✨ feat: Support setting EKS authentication mode #5578
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
base: main
Are you sure you want to change the base?
Changes from all commits
d224304
092811a
b289e83
32efafb
52d0e7c
277cd1a
7b51517
67241e7
f5554f1
910914e
7cbcc9f
d9d523c
0b3fa84
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -192,6 +192,10 @@ type AWSManagedControlPlaneSpec struct { //nolint: maligned | |
// +optional | ||
OIDCIdentityProviderConfig *OIDCIdentityProviderConfig `json:"oidcIdentityProviderConfig,omitempty"` | ||
|
||
// AccessConfig specifies the access configuration information for the cluster | ||
// +optional | ||
AccessConfig *AccessConfig `json:"accessConfig,omitempty"` | ||
|
||
// VpcCni is used to set configuration options for the VPC CNI plugin | ||
// +optional | ||
VpcCni VpcCni `json:"vpcCni,omitempty"` | ||
|
@@ -248,6 +252,15 @@ type EndpointAccess struct { | |
Private *bool `json:"private,omitempty"` | ||
} | ||
|
||
// AccessConfig represents the access configuration information for the cluster | ||
type AccessConfig struct { | ||
// AuthenticationMode specifies the desired authentication mode for the cluster | ||
// Defaults to CONFIG_MAP | ||
// +kubebuilder:default=CONFIG_MAP | ||
// +kubebuilder:validation:Enum=CONFIG_MAP;API;API_AND_CONFIG_MAP | ||
AuthenticationMode EKSAuthenticationMode `json:"authenticationMode,omitempty"` | ||
} | ||
|
||
Comment on lines
+256
to
+263
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we are creating a AccessConfig, I suggest we add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
(For additional context, #5583 adds AccessEntry definitions to this struct but now I'm second-guessing where those should live.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is what I think, Similary, we should have separate @joshfrench Let me know what you think? Also, @AndiDog Please share your thoughts. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @punkwalker Agreed that To clarify, you are suggesting we add type AccessConfig struct {
// +kubebuilder:default=CONFIG_MAP
// +kubebuilder:validation:Enum=CONFIG_MAP;API;API_AND_CONFIG_MAP
AuthenticationMode EKSAuthenticationMode `json:"authenticationMode,omitempty"`
// +kubebuilder:default=true
BootstrapClusterCreatorAdminPermissions bool `json:"bootstrapClusterCreatorAdminPermissions"`
} That feels right to me, since it matches the shape of the EKS API. My question was what we should do if the user changes So if a user changes
1 feels bad, 2 is low friction, 3 is the most explicit. Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. 2 feels a golden midway instead of failing. But it should be documented explicitly in the CR. |
||
// EncryptionConfig specifies the encryption configuration for the EKS clsuter. | ||
type EncryptionConfig struct { | ||
// Provider specifies the ARN or alias of the CMK (in AWS KMS) | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
(and JSON field name
access
)All YAML is "config", so I'd remove that word unless it's part of "official" AWS terminology, for instance.
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 upstream types and API docs both use
AccessConfig
.