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

Fix Cilium ENI ipam #14694

Merged
merged 2 commits into from
Dec 2, 2022
Merged

Conversation

olemarkus
Copy link
Member

Fixes #14575

The simplest working configuratin right now seems to be to enable BPF
masquerade and masquerade ipv4 traffic. The old setup with disabling
masquerade entirely no longer works.
@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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 30, 2022
@olemarkus
Copy link
Member Author

/test pull-kops-e2e-cni-cilium-eni

@olemarkus
Copy link
Member Author

/cc @johngmyers @rifelpet
what do you think of this config?

@olemarkus olemarkus changed the title WIP Fix Cilium ENI ipam Fix Cilium ENI ipam Nov 30, 2022
@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 Nov 30, 2022
@johngmyers
Copy link
Member

Isn't a point of ENI to disable masquerading so that pods are directly addressable from outside the cluster? Or is this what is needed in order to get services to work?

@olemarkus
Copy link
Member Author

The Pods are directly addressable from the VPC network. The masqing source NATs the Pod IPs behind the host IP (which is also a VPC address). I don't understand why the masqing is required, but with masqing disabled, I see packets going out the correct interface, but I don't see a single return packet. It's beyond me what drops the packets and why. The VPC flow logs suggests some NATing takes place even with masq disabled, but I couldn't make sense of it. I talked to Cilium as well and they didn't know what was happening here.

The default setup for EKS uses masq as well. The helm config looks like this:

  tunnel: disabled
  enable-endpoint-routes: "true"
  auto-create-cilium-node-resource: "true"
  ec2-api-endpoint: ""
  eni-tags: "{}"
  subnet-ids-filter: ""
  subnet-tags-filter: ""
  # Enables L7 proxy for L7 policy enforcement and visibility
  enable-l7-proxy: "true"

  enable-ipv4-masquerade: "true"
  enable-ipv6-masquerade: "true"
  egress-masquerade-interfaces: eth0

  enable-xt-socket-fallback: "true"
  install-iptables-rules: "true"
  install-no-conntrack-iptables-rules: "false"

@johngmyers
Copy link
Member

Maybe the packets are going out the wrong ENI and are getting caught by the src/dst checks? I've had the reverse problem with AWS VPC CNI.

@olemarkus
Copy link
Member Author

Nope. ens5 has the node IP and the EBS controllers have IPs also associated with ens5. So this is not the case here.

I am wondering if we should just merge this and then see if we can figure out a way of disabling masq later on. I'll also see if I can catch up to cilium 1.13 soon.

@johngmyers
Copy link
Member

This is sub-optimal in that it makes it more difficult to trace network traffic back to the workload that generated it. Could you add to the documentation this quirk?

@olemarkus
Copy link
Member Author

Yeah I'll update the docs on this.

Not sure how well IPs can be relied on as identity anyway though. The IPs change hands across Pods on the same host as they come and go.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 2, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers

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 Dec 2, 2022
@johngmyers
Copy link
Member

IP plus timestamp can be useful for finding things in audit logs. With NAT, not so much.

@k8s-ci-robot k8s-ci-robot merged commit 670de03 into kubernetes:master Dec 2, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/addons area/api cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
3 participants