search

kubernetes-sigs / cluster-api-provider-aws

Kubernetes Cluster API Provider AWS provides consistent deployment and day 2 operations of "self-managed" and EKS Kubernetes clusters on AWS.
http://cluster-api-aws.sigs.k8s.io/
Apache License 2.0
636 stars 561 forks source link

Refactor `EncryptionConfig` for AWSManagedControlPlane #2540

Open richardcase opened 3 years ago

richardcase commented 3 years ago

/area provider/eks /kind api-change /kind refactor /milestone v0.7.0 /help /priority important-soon

Describe the solution you'd like You can optionally enable encryption for EKS by supplying details in AWSManagedControlPlane.Spec.EncryptionConfig. As its optional, it is a pointer and marked as optional and omitempty.

If you want to enable encryption, then you must supply the provider and resources. Currently, these are pointers and not marked as required. We should mark these as required using kubebuilder validation and remove the pointers/omitempty.

This was noticed whilst making a change for #2505

Anything else you would like to add: So perhaps something like this:

// EncryptionConfig specifies the encryption configuration for the EKS clsuter.
type EncryptionConfig struct {
    // Provider specifies the ARN or alias of the CMK (in AWS KMS)
    // +kubebuilder:validation:Required
    Provider string `json:"provider"`
    // Resources specifies the resources to be encrypted
    // +kubebuilder:validation:Required
    // +kubebuilder:validation:MinItems=1
    Resources []string `json:"resources"`
}

This will cause problems with the generated deepcopy and conversion functions which will need to be fixed.

Environment:

k8s-ci-robot commented 3 years ago

@richardcase: This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-help command.

In response to [this](https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues/2540): >/area provider/eks >/kind api-change >/kind refactor >/milestone v0.7.0 >/help >/priority important-soon > >**Describe the solution you'd like** >You can optionally enable encryption for EKS by supplying details in [AWSManagedControlPlane.Spec.EncryptionConfig](https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/main/controlplane/eks/api/v1alpha4/awsmanagedcontrolplane_types.go#L87). As its optional, it is a pointer and marked as optional and omitempty. > >If you want to enable encryption, then you must supply the provider and resources. Currently, these are [pointers and not marked as required](https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/028bf2b34a87b9aa6eea0f8104d4ac61bea0547e/controlplane/eks/api/v1alpha4/awsmanagedcontrolplane_types.go#L179:L184). We should mark these as required using kubebuilder validation and remove the pointers/omitempty. > >This was noticed whilst making a change for #2505 > >**Anything else you would like to add:** >So perhaps something like this: > >```go >// EncryptionConfig specifies the encryption configuration for the EKS clsuter. >type EncryptionConfig struct { > // Provider specifies the ARN or alias of the CMK (in AWS KMS) > // +kubebuilder:validation:Required > Provider string `json:"provider"` > // Resources specifies the resources to be encrypted > // +kubebuilder:validation:Required > // +kubebuilder:validation:MinItems=1 > Resources []string `json:"resources"` >} >``` > >This will cause problems with the generated deepcopy and conversion functions which will need to be fixed. > >**Environment:** > >- Cluster-api-provider-aws version: 0.6.6 Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
slayer321 commented 3 years ago

Hii @richardcase, I am new to open source and will like to contribute to this project so can you assign it to me.

richardcase commented 3 years ago

Welcome @slayer321, thanks for wanting to work on this issue. If you have any questions feel free to ask on here or in the slack channel.

/assign slayer321

slayer321 commented 3 years ago

Hii @richardcase, I made changes in the EncryptionConfig struct as you have mentioned above but there are some errors that I am getting in generated deepcopy and conversion file and I am a bit confused about what exact changes need to be done in that file.

Ps: I am new to Golang so it is getting a bit difficult for me to understand the problem.

richardcase commented 3 years ago

@slayer321 - feel free to ping me on slack and we can chat.

Sometimes with the auto generated code you need to make manual changes so that the code is regenerated properly.

k8s-triage-robot commented 3 years ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

richardcase commented 3 years ago

@slayer321 - how are you getting on with this?

slayer321 commented 3 years ago

Hii @richardcase, Sorry due to some college work I was not able to work on this issue. But now I'm looking to work on this issue.

As you mention ..... when I change the pointers in this struct there are some change in deepcopy and conversion function which I'm not able to understand can you point me to some resources where I can learn about it.

richardcase commented 3 years ago

@slayer321 - when there are changes to the API definitions (like this change) then you need to run make generate. This does many things, some steps run various code generation tools that handle creating deepcopy functions or API conversions. If you run this and then perhaps ping me on slack?

k8s-triage-robot commented 2 years ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

richardcase commented 2 years ago

/lifecycle frozen

richardcase commented 2 years ago

/remove-lifecycle frozen

k8s-triage-robot commented 1 year ago

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale