Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add AWSFargateProfile resource #2265

Merged
merged 2 commits into from
Mar 23, 2021
Merged

Add AWSFargateProfile resource #2265

merged 2 commits into from
Mar 23, 2021

Conversation

michaelbeaumont
Copy link
Contributor

@michaelbeaumont michaelbeaumont commented Feb 23, 2021 •
edited
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #1786

Special notes for your reviewer:

Checklist:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Release note:

Adds the `AWSFargateProfile` resource for managing EKS Fargate profiles

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 23, 2021
@michaelbeaumont michaelbeaumont force-pushed the fargate branch 2 times, most recently from 78b0067 to 8bd7b32 Compare February 24, 2021 16:31
@michaelbeaumont michaelbeaumont changed the title WIP: Add AWSFargateProfile resource Add AWSFargateProfile resource Feb 24, 2021
@michaelbeaumont michaelbeaumont marked this pull request as ready for review February 24, 2021 17:20
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 24, 2021
richardcase
richardcase reviewed Feb 25, 2021
View reviewed changes
exp/api/v1alpha3/awsfargateprofile_webhook.go
const (
// maxProfileNameLength isn't defined in the AWS docs but is taken from the
// EKS cluster name max length
maxProfileNameLength = 100
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we define this once instead of the different variations with the same value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, it didn't feel like a given that profile, nodegroup and cluster names have the same max length (although I just checked they do in fact), so it felt better to have different variables.

exp/controllers/awsfargatepool_controller.go
// +kubebuilder:rbac:groups=core,resources=events,verbs=get;list;watch;create;patch
// +kubebuilder:rbac:groups=controlplane.cluster.x-k8s.io,resources=awsmanagedcontrolplanes;awsmanagedcontrolplanes/status,verbs=get;list;watch
// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=awsfargateprofiles,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=awsfargateprofiles/status,verbs=get;update;patch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also need // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusters;clusters/status,verbs=get;list;watch ?

Copy link
Contributor Author

@michaelbeaumont michaelbeaumont Feb 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure.. I've added it to be sure but it does work without it. It doesn't change the generated Role, I suppose because some other reconciler requests the permissions. The CAPI MachinePool controller doesn't have it listed either for example.

exp/api/v1alpha3/awsfargateprofile_types.go Outdated

// FargateProfileSpec defines the desired state of FargateProfile
type FargateProfileSpec struct {
ClusterName string `json:"clusterName,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? Could the cluster name come from the AWSManagedControlPlane instead?

Copy link
Contributor Author

@michaelbeaumont michaelbeaumont Feb 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment Spec.ClusterName is how I get at the AWSManagedControlPlane:

https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/eceb96da3384769ac7c3aefa70c628c7efa51145/exp/controllers/awsfargatepool_controller.go#L89

in the same way that MachinePools use MachinePoolSpec.ClusterName

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the missing description to the field

exp/api/v1alpha3/awsfargateprofile_types.go
// +kubebuilder:resource:path=awsfargateprofiles,scope=Namespaced,categories=cluster-api
// +kubebuilder:storageversion
// +kubebuilder:subresource:status
// +kubebuilder:printcolumn:name="Ready",type="string",JSONPath=".status.ready",description="AWSFargateProfile ready status"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps some additional items here like profilename, failure message. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added profileName and failureReason

@michaelbeaumont michaelbeaumont force-pushed the fargate branch 2 times, most recently from f1454eb to 17c07e5 Compare February 25, 2021 14:37
@richardcase richardcase mentioned this pull request Mar 22, 2021
Open
@richardcase
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2021
michaelbeaumont and others added 2 commits March 22, 2021 16:13
@michaelbeaumont @richardcase
feat: added fargate profile support
6ccea2f 
This PR adds support for setting up Fargate profiles
for EKS.
@richardcase
feat: added fargate permissions
0745e18 
The permissions required for fargate have been
added to the role for the controller.

Additionally the tests have been updated in light
of this change
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2021
@richardcase
Copy link
Member

For final approval:

/assign sedefsavas
/assign randomvariable

@richardcase
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2021
@sedefsavas
Copy link
Contributor

/approve

Is adding an e2e test useful here?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sedefsavas

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2021
@richardcase
Copy link
Member

/approve

Is adding an e2e test useful here?

Thanks @sedefsavas. Yes a e2e test is needed and i created #2317 for this.....this is in the 0.6.5 milestone so i plan to get it implemented this week

@k8s-ci-robot k8s-ci-robot merged commit 82c53e6 into kubernetes-sigs:master Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardcase richardcase richardcase left review comments

@dthorsen dthorsen Awaiting requested review from dthorsen

@rudoi rudoi Awaiting requested review from rudoi

Assignees

@richardcase richardcase

@randomvariable randomvariable

@sedefsavas sedefsavas

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EKS Fargate profiles support
5 participants
@michaelbeaumont @k8s-ci-robot @richardcase @sedefsavas @randomvariable