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

WIP feat: expose additional EKS bootstrap options #2854

Closed
wants to merge 2 commits into from
Closed

WIP feat: expose additional EKS bootstrap options #2854

wants to merge 2 commits into from

Conversation

richardcase
Copy link
Member

@richardcase richardcase commented Oct 15, 2021 •
edited
Loading

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

This change exposes extra EKS bootstrap options for configuration by the user with EKSConfig.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2853

Special notes for your reviewer:

Checklist:

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

Release note:

Expose additional EKS bootstrap options via `EKSConfig`

@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. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 15, 2021
@k8s-ci-robot
Copy link
Contributor

@richardcase: This issue is currently awaiting triage.

If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 15, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from richardcase after the PR has been reviewed.

The full list of commands accepted by this bot can be found 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

codablock
codablock reviewed Oct 20, 2021
View reviewed changes
bootstrap/eks/internal/userdata/args.go
{{- if .PauseContainerAccount }} --pause-container-account {{.PauseContainerAccount}}{{- end -}}
{{- if .PauseConatinerVersion }} --pause-container-version {{.PauseConatinerVersion}}{{- end -}}
{{- if .DNSClusterIP }} --dns-cluster-ip {{.DNSClusterIP}}{{- end -}}
{{- if .DockerConfigJson }} --docker-config-json {{.DockerConfigJson}}{{- end -}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you need to single-quote this? It might contain new-lines and double-quotes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe even full-blown shell escaping?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes i think we will need that to be safe. We didn't have that previously with the kubelet extra args. I will add.

Copy link
Contributor

Choose a reason for hiding this comment

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

The difference here is that the arg is meant to contain the content of a whole file, which adds quite some requirements to quoting/escaping. Other args (like kubelet args) usually don't have file contents but instead file paths inside.

codablock
codablock reviewed Oct 20, 2021
View reviewed changes
bootstrap/eks/internal/userdata/args.go
{{- if .UseMaxPods }} --use-max-pods {{.UseMaxPods}}{{- end -}}
{{- if .APIRetryAttempts }} --aws-api-retry-attempts {{.APIRetryAttempts}}{{- end -}}
{{- if .PauseContainerAccount }} --pause-container-account {{.PauseContainerAccount}}{{- end -}}
{{- if .PauseConatinerVersion }} --pause-container-version {{.PauseConatinerVersion}}{{- end -}}
Copy link
Contributor

@codablock codablock Oct 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

typo, should be PauseConatainerVersion ;) (incl. all other places)

Copy link
Member Author

Choose a reason for hiding this comment

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

Doh! Will change, thanks.

@randomvariable
Copy link
Member

Can we add the extra commands for #2771 as that will unblock the ability to add the CAPI API upgrade tests ?

@richardcase
Copy link
Member Author

Can we add the extra commands for #2771 as that will unblock the ability to add the CAPI API upgrade tests ?

Yeah good call. I will add that as part of this instead of the launch template change i have been playing with.

@codablock
Copy link
Contributor

codablock commented Oct 20, 2021 •
edited
Loading

@richardcase I'm planning to use containerd in our EKS clusters which makes this PR a requirement for me. At the same time, I need to specify containerd options to enable docker.io mirroring (thanks to api limits). To make this possible, I created awslabs/amazon-eks-ami#790 to add support for this to bootstrap.sh.

At the same time, I prepared a commit that you could cherry-pick into this PR to add support for --containerd-extra-config-toml support for EKSConfig. That commit however lacks code generation changes as I had some issues with uncompilable code which I'm unable to fix due to missing experience in Go and k8s api machinery. If the PR into amazon-eks-ami takes too long, I can also pick this up later in a separate PR.

EDIT: Fixed commit link

@randomvariable
Copy link
Member

@richardcase I'm planning to use containerd in our EKS clusters which makes this PR a requirement for me. At the same time, I need to specify containerd options to enable docker.io mirroring (thanks to api limits). To make this possible, I created awslabs/amazon-eks-ami#790 to add support for this to bootstrap.sh.

Not now, but I think we should consider a common configuration binary that handles this sort of stuff that can be used by either EKS or Cluster API, as we seem to duplicating common setup tasks across different vendors.

@richardcase richardcase mentioned this pull request Oct 21, 2021
Closed
@richardcase
Copy link
Member Author

Not now, but I think we should consider a common configuration binary that handles this sort of stuff that can be used by either EKS or Cluster API, as we seem to duplicating common setup tasks across different vendors.

Yes i would agree that it needs to be sorted so any provider can use it.

@randomvariable randomvariable mentioned this pull request Oct 21, 2021
Open
@richardcase
started work on eks bootstrap config changes
9b81ed1 
Signed-off-by: Richard Case <richard@weave.works>
@richardcase richardcase mentioned this pull request Oct 29, 2021
Closed
@richardcase
wip
0c8c26d 
Signed-off-by: Richard Case <richard@weave.works>
@k8s-ci-robot
Copy link
Contributor

@richardcase: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2021
@richardcase
Copy link
Member Author

Closing in favor of #2965

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codablock codablock codablock left review comments

@randomvariable randomvariable Awaiting requested review from randomvariable

@shivi28 shivi28 Awaiting requested review from shivi28

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. needs-priority needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support additional EKS config options
4 participants
@richardcase @k8s-ci-robot @randomvariable @codablock