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

v1.7 backports 2020-04-07 #10884

Merged
merged 2 commits into from Apr 9, 2020
Merged

v1.7 backports 2020-04-07 #10884

merged 2 commits into from Apr 9, 2020

Conversation

christarazi
Copy link
Member

Once this PR is merged, you can update the PR labels via:

$ for pr in 10839 10841; do contrib/backporting/set-labels.py $pr done 1.7; done

@maintainer-s-little-helper maintainer-s-little-helper bot added backport/1.7 kind/backports This PR provides functionality previously merged into master. labels Apr 7, 2020
@christarazi christarazi marked this pull request as ready for review April 7, 2020 21:24
@christarazi christarazi requested a review from a team as a code owner April 7, 2020 21:24
@christarazi
Copy link
Member Author

christarazi commented Apr 7, 2020

never-tell-me-the-odds

Edit: need to update go-bindata

@joestringer
Copy link
Member

Ah, caveat for backports for the moment: You'll need to manually run make -C daemon apply-bindata and add the changes into any patches that make changes to the bpf/ directory.

@christarazi
Copy link
Member Author

christarazi commented Apr 7, 2020

never-tell-me-the-odds

Edit: my version of go-bindata is too high, need to be on 1.13.9

@christarazi
Copy link
Member Author

never-tell-me-the-odds

[ upstream commit a5e289d ]

Newer microk8s requires the yaml arg to be specified as `--yaml`, not
`-o yaml`. This breakage was backported to all microk8s release series.
Fix the target.

Related: canonical/microk8s#1042
Signed-off-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi force-pushed the pr/v1.7-backport-2020-04-07 branch 2 times, most recently from 2492615 to 0bdeae0 Compare April 7, 2020 23:17
@christarazi
Copy link
Member Author

Not sure what's happening here. I've updated the bindata and ensured that I am using 1.13.9 Golang toolchain version.

❯ go version && go-bindata -version
go version go1.13.9 linux/amd64
go-bindata 3.1.3 (Go runtime go1.13.9).
Copyright (c) 2010-2013, Jim Teeuwen.
❯ make -j $(nproc) -C daemon check-bindata
make: Entering directory '/tmp/cilium/cilium/daemon'
  CHECK contrib/scripts/bindata.sh
../contrib/scripts/bindata.sh dea0831f973ad5498312d149ce4d911469968ba9
make: Leaving directory '/tmp/cilium/cilium/daemon'

@joestringer
Copy link
Member

@christarazi any chance you have other files lying around the bpf/ directory or something? When I pulled this tree I hit this:

 $ make -C daemon check-bindata
  GEN   daemon/ bindata.go
  CHECK contrib/scripts/bindata.sh
bindata.go: FAILED
sha1sum: WARNING: 1 computed checksum did NOT match
########################################################################

                  ERROR: bindata.go is out of date.

 This can happen for two reasons:
 1. You are using a go-bindata binary compiled with a different version
    of golang (not 1.13.9). If so, please up/downgrade.

 2. You have made changes to the bpf/ directory. Please run the
    following command to update the SHA in daemon/bpf.sha:

    $ make -C daemon apply-bindata

########################################################################
make: *** [Makefile:54: check-bindata] Error 1
 $ make -C daemon apply-bindata
  GEN   daemon/ go-bindata
  GEN   daemon/bpf.sha
 $ git diff
diff --git a/daemon/bpf.sha b/daemon/bpf.sha
index 24598fee550f..31577c10a171 100644
--- a/daemon/bpf.sha
+++ b/daemon/bpf.sha
@@ -1,2 +1,2 @@
-GO_BINDATA_SHA1SUM=dea0831f973ad5498312d149ce4d911469968ba9
+GO_BINDATA_SHA1SUM=2c4c55273c6523e9936a1980e1c1bd49c779f396
 BPF_FILES=../bpf/COPYING ../bpf/Makefile ../bpf/Makefile.bpf ../bpf/bpf_alignchecker.c ../bpf/bpf_features.h ../bpf/bpf_hostdev_ingress.c ../bpf/bpf_ipsec.c ../bpf/bpf_lxc.c ../bpf/bpf_netdev.c ../bpf/bpf_network.c ../bpf/bpf_overlay.c ../bpf/bpf_sock.c ../bpf/bpf_xdp.c ../bpf/cilium-map-migrate.c ../bpf/filter_config.h ../bpf/include/bpf/api.h ../bpf/include/elf/elf.h ../bpf/include/elf/gelf.h ../bpf/include/elf/libelf.h ../bpf/include/iproute2/bpf_elf.h ../bpf/include/linux/bpf.h ../bpf/include/linux/bpf_common.h ../bpf/include/linux/byteorder.h ../bpf/include/linux/byteorder/big_endian.h ../bpf/include/linux/byteorder/little_endian.h ../bpf/include/linux/icmp.h ../bpf/include/linux/icmpv6.h ../bpf/include/linux/if_arp.h ../bpf/include/linux/if_ether.h ../bpf/include/linux/if_packet.h ../bpf/include/linux/in.h ../bpf/include/linux/in6.h ../bpf/include/linux/ioctl.h ../bpf/include/linux/ip.h ../bpf/include/linux/ipv6.h ../bpf/include/linux/perf_event.h ../bpf/include/linux/swab.h ../bpf/include/linux/tcp.h ../bpf/include/linux/type_mapper.h ../bpf/include/linux/udp.h ../bpf/init.sh ../bpf/lib/arp.h ../bpf/lib/common.h ../bpf/lib/config.h ../bpf/lib/conntrack.h ../bpf/lib/conntrack_map.h ../bpf/lib/conntrack_test.h ../bpf/lib/csum.h ../bpf/lib/dbg.h ../bpf/lib/drop.h ../bpf/lib/encap.h ../bpf/lib/eps.h ../bpf/lib/eth.h ../bpf/lib/events.h ../bpf/lib/icmp6.h ../bpf/lib/identity.h ../bpf/lib/ipv4.h ../bpf/lib/ipv6.h ../bpf/lib/ipv6_test.h ../bpf/lib/l3.h ../bpf/lib/l4.h ../bpf/lib/lb.h ../bpf/lib/lxc.h ../bpf/lib/maps.h ../bpf/lib/metrics.h ../bpf/lib/nat.h ../bpf/lib/nat46.h ../bpf/lib/nodeport.h ../bpf/lib/policy.h ../bpf/lib/signal.h ../bpf/lib/tailcall.h ../bpf/lib/trace.h ../bpf/lib/utils.h ../bpf/lib/xdp.h ../bpf/lxc_config.h ../bpf/netdev_config.h ../bpf/node_config.h ../bpf/probes/raw_change_tail.t ../bpf/probes/raw_fib_lookup.t ../bpf/probes/raw_insn.h ../bpf/probes/raw_invalidate_hash.t ../bpf/probes/raw_lpm_map.t ../bpf/probes/raw_lru_map.t ../bpf/probes/raw_main.c ../bpf/probes/raw_max_insn.t ../bpf/probes/raw_sock_cookie.t ../bpf/run_probes.sh ../bpf/sockops/Makefile ../bpf/sockops/bpf_redir.c ../bpf/sockops/bpf_sockops.c ../bpf/sockops/bpf_sockops.h ../bpf/sockops/sockops_config.h

@christarazi
Copy link
Member Author

Hmm, nope. It's a clean repo. I uninstalled all of Golang on my machine and reinstalled 1.13.9.

I even tried cloning this tree and I still get the same SHA. I also tried cloning a brand new repo, switching to v1.7 branch and doing the cherry-picks all over again...still no luck. Strange...

@joestringer
Copy link
Member

Maybe it's the go-bindata binary? Here's my version:

$ go-bindata -version
go-bindata 3.1.0 (Go runtime go1.9).
Copyright (c) 2010-2013, Jim Teeuwen.

@christarazi
Copy link
Member Author

Switched my go-bindata version to match yours...

❯ go-bindata -version
go-bindata 3.1.0 (Go runtime go1.13.9).
Copyright (c) 2010-2013, Jim Teeuwen.

The only difference I see between yours and mine is the Golang toolchain version. However, my toolchain version matches Travis' version. But we don't know what version of go-bindata it uses.

@pchaigno
Copy link
Member

pchaigno commented Apr 8, 2020

@christarazi Are you using Cilium's fork of go-bindata? I generated the go-bindata file from inside the dev. VM to avoid any such issues when I did my round of backports.

[ upstream commit b9ef15f ]

While testing recently, I've noticed the case where we start to use
the 169.254.42.1 loopback address for requests from outside:

  # tcpdump -i any port 30042 or 80 -n
  [...]
  09:50:15.835618 IP 192.168.178.28.80 > 169.254.42.1.32882: Flags [S.], seq 689888661, ack 2221057376, win 65160, options [mss 1460,sackOK,TS val 3644467179 ecr 3644467179,nop,wscale 7], length 0
  09:50:16.863069 IP 192.168.178.28.80 > 169.254.42.1.32882: Flags [S.], seq 689888661, ack 2221057376, win 65160, options [mss 1460,sackOK,TS val 3644468207 ecr 3644467179,nop,wscale 7], length 0
  [...]

This can happen if the backend IP (192.168.178.28) is the same IP that
is doing the request to the frontend:

  # ./cilium/cilium service list
  ID   Frontend               Service Type   Backend
  1    192.168.178.29:30042   NodePort       1 => 192.168.178.28:80

Then we're hitting the codepath where we replace the 192.168.178.28 src
address with 169.254.42.1 (IPV4_LOOPBACK) and try to send it back out
the NodePort device, which is just wrong. That code was only intended
for node local Pod to Pod ClusterIP handling we had before socket LB.

For NodePort requests from outside the node this should not be done. We
can handle this situation just fine in case of BPF-based SNAT, and in
case of DSR it is expected to not work and we should not try to do any
special NAT'ing or such. For east-west traffic on Cilium-managed nodes,
we are simply using the socket LB anyway for everything, so we won't
run into this corner case either.

There is a unused DISABLE_LOOPBACK_LB define, to compile this out. Fix
it by specifying this define from init.sh.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi
Copy link
Member Author

christarazi commented Apr 8, 2020

never-tell-me-the-odds

Thanks @pchaigno , that was it! I wasn't aware we had a fork of it.

Edit: provisioning failure

@christarazi
Copy link
Member Author

never-tell-me-the-odds

@pchaigno
Copy link
Member

pchaigno commented Apr 9, 2020

Looks like provisioning error.

test-with-kernel

@christarazi
Copy link
Member Author

Cilium-Tests-With-Kernel keeps failing because it needs jenkinsfiles/ginkgo-kernel.Jenkinsfile which doesn't exist in this tree.

@pchaigno
Copy link
Member

pchaigno commented Apr 9, 2020

@christarazi Ah, right! Forgot this was a backport :/

@christarazi christarazi added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 9, 2020
@joestringer joestringer merged commit 3e7ed98 into v1.7 Apr 9, 2020
@joestringer joestringer deleted the pr/v1.7-backport-2020-04-07 branch April 9, 2020 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/backports This PR provides functionality previously merged into master. ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants