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.

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

Introduce eks sync-core-components subcommand #96

Merged
merged 5 commits into from
Sep 29, 2020
Merged

Conversation

yorinasub17
Copy link
Contributor

This implements the logic in https://docs.aws.amazon.com/eks/latest/userguide/update-cluster.html , which we currently use a python script baked in the EKS module for. By moving the logic to kubergrunt, we not only make the module more flexible (e.g., you don't need to update to latest version everytime a new k8s version is released), but the logic can also be used with other methods of launching EKS, including the open source tf module.

See https://github.com/gruntwork-io/terraform-aws-eks/issues/174 for more details.

@yorinasub17
Copy link
Contributor Author

NOTE: there are no tests for the command because it is expensive to launch and manage an EKS cluster. Instead, we rely on our EKS repo for integration testing.

The related integration testing is being done here: https://github.com/gruntwork-io/terraform-aws-eks/pull/206

@yorinasub17
Copy link
Contributor Author

yorinasub17 commented Sep 21, 2020

Ah just realized there is a step missing, specifically the region manipulation for the VPC CNI... Addressed: 7c68ef6

bwhaley
bwhaley previously approved these changes Sep 22, 2020
Copy link
Contributor

@bwhaley bwhaley left a comment

Choose a reason for hiding this comment

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

Great addition! Just a couple minor NITs.

cmd/eks.go Outdated Show resolved Hide resolved
eks/sync.go Outdated
// Figure out the manifest URL based on region
// Reference: https://docs.aws.amazon.com/eks/latest/userguide/update-cluster.html
if strings.HasPrefix(region, "cn-") {
manifestPath = fmt.Sprintf("https://raw.githubusercontent.com/aws/amazon-vpc-cni-k8s/release-%s/config/v%s/aws-k8s-cni-cn.yaml", vpcCNIVersion, vpcCNIVersion)
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: It might be slightly more DRY and readable to Sprintf the URL once, like:

awsK8sCNIURL := fmt.Sprintf("https://raw.githubusercontent.com/aws/amazon-vpc-cni-k8s/release-%s/config/v%s/", vpcCNIVersion, vpcCNIVersion)
if strings.HasPrefix(region, "cn-") {
    manifestPath = awsK8sCNIURL + "aws-k8s-cni-cn.yaml"
} else if region == "us-gov-east-1" {
... etc etc ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point! Updated: 87ea8c4

Co-authored-by: Ben Whaley <bwhaley@gmail.com>
@yorinasub17
Copy link
Contributor Author

Ping: this is ready for rereview.

Copy link
Contributor

@bwhaley bwhaley left a comment

Choose a reason for hiding this comment

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

LGTM! Sorry that I missed the re-review notice!

@yorinasub17
Copy link
Contributor Author

Thanks for review! Merging and releasing now!

Sorry that I missed the re-review notice!

No worries! This wasn't urgent anyway. I was going to have fun to see how long it would take by repeating ping and pong every week.

@yorinasub17 yorinasub17 merged commit b88b70d into master Sep 29, 2020
@yorinasub17 yorinasub17 deleted the yori-upgrade-cmd branch September 29, 2020 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants