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

bind kube-proxy to 127.0.0.1 #209

Merged
merged 1 commit into from
Apr 26, 2021
Merged

bind kube-proxy to 127.0.0.1 #209

merged 1 commit into from
Apr 26, 2021

Conversation

ettiee
Copy link
Contributor

@ettiee ettiee commented Apr 23, 2021

Following the advice from aws/amazon-vpc-cni-k8s#1078 (comment)
This fix should stop us hitting the bug around unstable aws-node startup.
I've made the bindAddress configurable via ipv4/6 usage as binding to 127.0.0.1 will not work for ipv6

@ettiee ettiee marked this pull request as ready for review April 23, 2021 14:56
@ettiee ettiee requested a review from a team as a code owner April 23, 2021 14:56
@ettiee ettiee requested review from demijonathon, dan-slinky-ckpd and kenju and removed request for a team April 23, 2021 14:56
kenju
kenju reviewed Apr 26, 2021
View reviewed changes
modules/cluster/variables.tf Outdated
variable "support_ipv6" {
type = bool
default = false
description = "Support requilred for IPV6."
Copy link

Choose a reason for hiding this comment

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

Suggested change
description = "Support requilred for IPV6."
description = "Support required for IPV6."

errm
errm requested changes Apr 26, 2021
View reviewed changes
modules/cluster/addons.tf Outdated
{ aws_region = data.aws_region.current.name },
{
aws_region = data.aws_region.current.name
bind_address = var.support_ipv6 ? "0.0.0.0" : "127.0.0.1"
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure the support_ipv6 == true part of this branch is appropriate...

As I understand the comment here aws/amazon-vpc-cni-k8s#1078 (comment) the idea is that we can bypass some detection for v4 vs v6 if we specify 127.0.0.1...

Since I don't think we are using ipv6 stack at all anywhere I am not sure how to test the behaviour ... I think it would be fine to hardcode this, and make a note in the readme that we don't currently support ipv6

mtpereira
mtpereira reviewed Apr 26, 2021
View reviewed changes
modules/cluster/addons.tf Outdated
{ aws_region = data.aws_region.current.name },
{
aws_region = data.aws_region.current.name
bind_address = var.support_ipv6 ? "0.0.0.0" : "127.0.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

❗ I don't think this is a great fix since we are allowing any host to connect under IPv4 as well. Seems like kube-proxy doesn't handle this configuration very gracefully!

Regardless, since we do not use IPv6 ATM and we are not supporting it (i.e., we haven't tested it at all), can we simply drop IPv6 for now?

Suggested change
bind_address = var.support_ipv6 ? "0.0.0.0" : "127.0.0.1"
bind_address = "127.0.0.1"

@errm
Copy link
Member

errm commented Apr 26, 2021

Having read what is going on here kubernetes/kubernetes#91725

I think we are ok to hardcode 127.0.0.1

It seems that the comments about incompatibility with a ipv6 network only apply to a single stack ipv6 setup.

On AWS VPC it is not possible to configure a single stack ipv6 network, you can only configure v4 single stack or dualstack.

@github-actions
Copy link

terraform validate Failed


Error: Not enough function arguments

  on ../../modules/cluster/addons.tf line 62, in module "kube_proxy":
  60: 
  61: 
  62:   )

Function "templatefile" expects 2 argument(s). Missing value for "vars".

Workflow: .github/workflows/ci.yml, Action: hashicorpterraform-github-actions3, Working Directory: examples/cluster, Workspace: default

@ettiee ettiee force-pushed the ee/kube-proxy-bind-address branch from c3a05cf to 8eab9af Compare April 26, 2021 11:01
@ettiee ettiee force-pushed the ee/kube-proxy-bind-address branch from 8eab9af to d4e37c2 Compare April 26, 2021 11:02
@ettiee
Copy link
Contributor Author

ettiee commented Apr 26, 2021

@errm @mtpereira ♻️ please 🙏

errm
errm approved these changes Apr 26, 2021
View reviewed changes
Copy link
Member

@errm errm left a comment

Choose a reason for hiding this comment

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

LGTM

@ettiee ettiee merged commit d18791b into master Apr 26, 2021
@ettiee ettiee changed the title bind kube-proxy to 127.0.0.1 when IPV6 is not required bind kube-proxy to 127.0.0.1 Apr 26, 2021
@errm errm deleted the ee/kube-proxy-bind-address branch August 16, 2021 10:28
@ettiee ettiee mentioned this pull request Dec 16, 2021
Closed
ettiee added a commit that referenced this pull request Dec 21, 2021
@ettiee
Apply kube-proxy daemonset by manifest not EKS addons
dc5394a 
The EKS addon for kube-proxy introduced regressions of #124 and #209.
We will apply the recommended overrides from aws/containers-roadmap#657 and aws/amazon-vpc-cni-k8s#1702 in the manifests until the EKS addon applies these by default or allows you to override config in the addon.
ettiee added a commit that referenced this pull request Dec 22, 2021
@ettiee
Apply kube-proxy daemonset by manifest not EKS addons
84a41b5 
The EKS addon for kube-proxy introduced regressions of #124 and #209.
We will apply the recommended overrides from aws/containers-roadmap#657 and aws/amazon-vpc-cni-k8s#1702 in the manifests until the EKS addon applies these by default or allows you to override config in the addon.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kenju kenju kenju left review comments

@mtpereira mtpereira mtpereira left review comments

@errm errm errm approved these changes

@demijonathon demijonathon Awaiting requested review from demijonathon

@dan-slinky-ckpd dan-slinky-ckpd Awaiting requested review from dan-slinky-ckpd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ettiee @errm @mtpereira @kenju