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

Pr/v1.9 backport 2020 10 27 #13786

Merged
merged 36 commits into from Oct 28, 2020

Conversation

aditighag
Copy link
Member

@aditighag aditighag commented Oct 27, 2020

#13747 -- bpf: redirect_neigh signature fix (@borkmann) -- #13747
#13724 -- loader: Use netlink lib instead of tc binary to delete filters (@pchaigno) -- #13724
#13734 -- Remove high cardinality port-distribution metric from default install (@jedsalazar) -- #13734
#13741 -- redirectpolicy: Check lrp type before restoring lrp service (@aditighag) -- #13741
#13666 -- VM Support Refinement (@jrajahalme) -- #13666
#13671 -- redirectpolicy: Move the feature behind feature flag and remove CCLRP related code (@aditighag) -- #13671
#13764 -- docs: update CODEOWNERS review process (@aanm) -- #13764
#13755 -- ci: skip tests involving L7 proxy if race detector is enabled (@tklauser) -- #13755
#13770 -- .github: Add action to build all BPF permutations (@joestringer) -- #13770
#12268 -- docs: document test-only ci command (@nebril) -- #12268
#13750 -- hubble: Fix reply state unknown being interpreted as false (@gandro) -- #13750
#13779 -- ci: Avoid null pointer exception in non-pr builds (@nebril) -- #13779
#13781 -- docs: Clarify bumping the runtime images step (@christarazi) -- #13781
#13549 -- helm: Add check for prometheus service monitoring CRDs (@sayboras) -- #13549
#13776 -- hubble/relay: flush old flows when the buffer drain timeout is reached (@rolinh) -- #13776

When you have backported the above commits, you can update the PR labels via this command:

$ for pr in 13747 13724 13734 13741 13666 13671 13764 13755 13770 12268 13750 13779 13781 13549 13776; do contrib/backporting/set-labels.py $pr done 1.9; done

borkmann and others added 30 commits October 27, 2020 19:55
[ upstream commit 90fae5f ]

Pull in latest BPF uapi redirect_neigh() fixes from Linus' tree.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit aa4031c ]

We had a redirect_neigh() helper signature update so that it can be used
together with the fib_lookup() lookup helper efficiently [0] for multi-dev
setups, that is, if the fib_lookup() fails neighbor resolution, it won't
populate the L2 addresses in struct bpf_fib_lookup, but it will populate
the L3 addresses in particular for nexthop and device ifindex. The latter
two can then be used for redirect_neigh(). It will avoid doing the nexthop
lookup twice but instead reuse the one fib_lookup() lookup. Right now, just
update the signature to the single device case where redirect_neigh() does
the nexthop lookup internally instead. For multi-dev we'll later compile
in the fib_lookup() for such case.

  [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ba452c9e996d8a4c347b32805f91abb70de5de7e
      https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=adfd272c4ccbe43d9761bb17dd8a4387d7815382

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit 7b66995 ]

This commit replaces the DeleteDatapath method which called the tc binary
by a new RemoveTCFilters function which relies on the netlink library.

Suggested-by: Martynas Pumputis <m@lambda.lt>
Signed-off-by: Paul Chaignon <paul@cilium.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit 8909c7f ]

Open to discussion: The port-distribution metric covers "Number of packets by destination port number" which results in a high cardinality metric with arguably minimal value from a default installation. We should consider removing the metric from the default metric GSG.

Signed-off-by: Jed Salazar jed@isovalent.com
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit c2b7b91 ]

This fixes the regression caused by commit b553f49, whereby not
checking the lrp type before restoring clusterIP service would
lead to nil pointer access for addressMatcher lrps that don't
have the serviceID field set in the lrp config. As there is no
backing k8s service for such lrps so they don't need restore.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit c864fd3 ]

Split IPAM bootstrap into two phases:
1. Configure IPAM (configureIPAM())
2. Begin using the IPAM configuration (startIPAM())

This allows overriding the IPAM configuration set in phase 1
before starting the use of IPAM configuration in phase 2.

Split the --join-cluster feature init into two phases:

1. Get config from clustermesh-apiserver by resetting node
registration to only contain the node name. clustermesh-apiserver
updates the key with configuration for this node derived from the
cluster state.

2. Update node registration after node IPAM is started with full node
information.

When node registration is started, the local cluster name may not be
known yet. To accomodate this only the node name is used to identify
the node registration.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit aae2d7a ]

CiliumExternalWorkload CRD is used to specify which VMs can join the
cluster. Only clustermesh-apiserver is watching for these.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit 23ca7f2 ]

Add PROVISION_EXTERNAL_WORKLOAD environment variable that if not
"false" will cause runtime VM compile and run Cilium in docker with
configuration suitable for external workload to join a Cilium cluster.
This is using the new 'test/provision/externalworkload_install.sh',
which builds and starts a Cilium docker image for a VM.

TLS certs for the VM must be extracted from the k8s cluster before
starting the VM.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit 4d64ba1 ]

Move clustermesh-apiserver to Cilium repo to ease development.

Add support scripts for mTLS config management to contrib/k8s.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit 03f4892 ]

Certgen image can be used for generating certificates for multiple
targets (not only hubble), so it makes sense to not nest it's values
under hubble.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit f95a1d6 ]

The helm partials for generating hubble certs are hubble specific, indicate that in their names.

Delete the unused 'ca.gen-certs'.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit e6b993b ]

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
…secrets

[ upstream commit 7785ef5 ]

Update ClusterRole to allow cilium-certgen to create and update
external workload secrets/configmaps.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
….yaml

[ upstream commit 2345e53 ]

Enable support for external workloads in the experimental install
using "cronJob" auto TLS method.

"helm" method would include the generated keys in
experimental-install.yaml which would be a bad idea in the repo as
then all users of experimental-install.yaml would be using the same
key.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit 9b5d9c8 ]

Minimize WaitForCRDsToRegister() dependencies by making it a
standalone function and by moving k8sResoucesSynced and k8sAPIGroups
from pkg/k8s/watchers to pkg/k8s/synced.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit aed3e59 ]

Add CiliumExternalWorkload CRD to the CRD waiter. Cilium agent does
not use CiliumExternalWorkload CRD but this is needed for the
clustermesh-apiserver, and including it also with Cilium agent does
not hurt.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit 2dc701e ]

CRD watchers fail if the CRD has not (yet) been registered. Add a wait
for CRDs before starting k8s watchers so that we get rid of the race
with cilium operator that registers the CRDs.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit c2ea2b7 ]

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit c03784c ]

If needed, it'll be supported in a future release.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit 912c8df ]

Also, warn users about potential service translation caused
loop issue on older kernel versions.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit a8f3b29 ]

Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit bea4427 ]

Signed-off-by: André Martins <andre@cilium.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit fd0d6d2 ]

This allows to determine whether the Cilium binaries under test were
built using the race detector enabled and the tests should be run
skipping any tests that fail with the race detector enabled.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit 267df0f ]

Running tests involving proxylib fail when using the race detector, so
skip them in that case.

Fixes cilium#13753

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit f7dfe02 ]

K8s tests involving the L7 proxy fail when using the race detector, so
skip them in that case.

Fixes cilium#13757

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit 7ada515 ]

The current 'all' target builds userspace programs like
cilium-map-migrate which rely on installed libraries, as well as the
various BPF programs which are self-contained using dependencies in the
tree.

Introduce a new target, bpf_all, which represents all BPF programs, and
only build these targets from the build_all target. This way, we can
build all BPF program permutations from the upcoming github action
without needing to install things like libelf.

Signed-off-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit ce8be45 ]

Signed-off-by: Maciej Kwiek <maciej@isovalent.com>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit eb12b1f ]

Signed-off-by: Maciej Kwiek <maciej@isovalent.com>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit ee16843 ]

This replaces the existing `reply` field, which suffers from false
negatives due to protobuf not being able to distinguish between a false
value and an absent value.

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit 0507c62 ]

This fixes a long-standing issue with Hubble not being able to
distingiush between the reply field being false and the reply field
being absent. To address the issue, the old `reply` field is replaced
with a tri-state `is_reply` field which can be either nil, true or
false. All uses of the old field are removed in the source tree, but the
field itself is preserved for backwards-compatibility.

This incidentally also fixes a bug where the `port-distribution` metric
suffered from the same confusion, which caused it to report source ports
as destination ports.

Ref: cilium#13248
Fixes: cilium#13744

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
nebril and others added 5 commits October 27, 2020 19:55
[ upstream commit 9ed9cc5 ]

Signed-off-by: Maciej Kwiek <maciej@isovalent.com>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit 42f19f1 ]

This commit clarifies the docs to mention the case where users are
simply interested in bumping the runtime images to update to the latest
package, and don't necessarily have "a branch with changes".

Signed-off-by: Chris Tarazi <chris@isovalent.com>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit beacc35 ]

Service Monitnor CRD (e.g. monitoring.coreos.com/v1) is no longer
managed and installed as part of cilium chart. This commit is to
add validation to check if monitoring.coreos.com/v1 CRD is required
and available before installation.

Relates: cilium#13513
Signed-off-by: Tam Mach <sayboras@yahoo.com>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit 195351a ]

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
[ upstream commit 7d56068 ]

Draining only 1 flow every time the buffer drain timeout is reached
proves to be insufficient for certain cases. The worst case typically
happens for a request with historic data, some filters and follow-mode.
Using the Hubble CLI, this means requests like this one:

    hubble observe --last=100 --follow --pod kube-system/coredns-f9fd979d6-66mfg

The behavior before this patch for a request like this, assuming 250
flows matching the requests are stored in Hubble instances ring buffers
and new flows matching the filter criteria are infrequent, would be to
forward 150 flows, then drain 1 flow to forward every time the drain
timeout is reached. This means that for a buffer size of 100, it would
take at least 100 seconds (with the default buffer drain timeout of 1s)
to drain the buffer (unless enough new flows are received in the
meantime and fill the buffer again).

The new code drains all flows in the queue for which the timestamp is
older than `now-bufferDrainTimeout`. In other words, this would be the
old behavior:

    ...
    QUEUE FULL draining flow 149
    QUEUE FULL draining flow 150
    TIMEOUT REACHED
    DRAINED 1 flow
    TIMEOUT REACHED
    DRAINED 1 flow
    TIMEOUT REACHED
    DRAINED 1 flow
    ...

And this is the new behavior:

    ...
    QUEUE FULL draining flow 149
    QUEUE FULL draining flow 150
    TIMEOUT REACHED
    DRAINED 100 flow
    TIMEOUT REACHED
    DRAINED 0 flow
    TIMEOUT REACHED
    DRAINED 0 flow
    ...

If new flows are received but not in enough numbers to fill the buffer
within the time window of `bufferDrainTimeout`, the behavior would look
something like this:

    ...
    QUEUE FULL draining flow 149
    QUEUE FULL draining flow 150
    TIMEOUT REACHED
    DRAINED 100 flow
    TIMEOUT REACHED
    DRAINED 34 flow
    TIMEOUT REACHED
    DRAINED 72 flow
    ...

In effect, this means that a time window of `bufferDrainTimeout`
(default to 1s) is considered to sort flows before forwarding them to
the client. This dramatically improves the query experience for requests
in follow-mode.

Suggested-by: Tom Payne <tom@isovalent.com>
Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag aditighag requested review from a team as code owners October 27, 2020 20:00
@maintainer-s-little-helper maintainer-s-little-helper bot added backport/1.9 kind/backports This PR provides functionality previously merged into master. labels Oct 27, 2020
[ upstream commit dd7d270 ]

Use "make -C bpf build_all" to validate the buildability of the datapath
in a variety of permutations of input configurations.

Signed-off-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag
Copy link
Member Author

test-me-please

Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

My PR looks good 👍

Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

looks good for my commit 💯

Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM for my commits

Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

My changes look good, thanks!

@jrajahalme
Copy link
Member

checkpatch seems unhappy with our backport commit style:

ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit aa4031c0c08c ("bpf: fix up redirect_neigh helper signature wrt nexthop")'
#6: 
[ upstream commit aa4031c0c08cd0f7b3a8faba315712b541d930e2 ]

No other errors in there so merging as-is.

@jrajahalme jrajahalme merged commit f822390 into cilium:v1.9 Oct 28, 2020
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet