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.6 backports 2020-06-03 #11883

Merged
merged 12 commits into from Jun 4, 2020
Merged

v1.6 backports 2020-06-03 #11883

merged 12 commits into from Jun 4, 2020

Conversation

joestringer
Copy link
Member

@joestringer joestringer commented Jun 4, 2020

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

$ for pr in 10839 11471 11541 11633 11580 11666 11701 11764 11750 11753; do contrib/backporting/set-labels.py $pr done 1.6; done

Skipped due to non-trivial conflicts:

joestringer and others added 4 commits June 3, 2020 19:06
[ 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>
[ upstream commit 1047583 ]

[ Backporter's notes: Minor conflict in daemon/cmd/ refactor; also
                      dropped the cilium-cni gops include ]

In all executables including the gops agent, make sure the gops agent is
properly teared down on exit and all the temporary files in
$HOME/.config created by it are cleaned up on shutdown by calling
gops/agent.Close() at the appropriate place.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Signed-off-by: Joe Stringer <joe@cilium.io>
[ upstream commit 003efe4 ]

'ackedVersions' was accessed in DeleteNode() without taking the mutex
which can lead to mutating the map while readers are accessing it.

Fixes: #11535
Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: Joe Stringer <joe@cilium.io>
[ upstream commit 054dd16 ]

DNS servers may request a list of root nameservers by forming an NS
request for ".". We have received reports that when applying a
visibility policy with the DNS matchPattern "*", DNS requests of this
kind were being dropped in the proxy.

Fix this by extending the visibility match "*" to explicitly match on
either "[validdnscharacters].", or ".". If the matchPattern is more
complicated than simply "*", do not match on ".".

Signed-off-by: Joe Stringer <joe@cilium.io>
@joestringer joestringer requested a review from a team as a code owner June 4, 2020 02:11
@joestringer joestringer added backport/1.6 kind/backports This PR provides functionality previously merged into master. labels Jun 4, 2020
djboris9 and others added 5 commits June 3, 2020 19:25
… not at max

[ v1.7 commit 63fff9f ]

[ Backporter's notes: Added the dep to Gopkg.lock input-imports to
                      satisfy dep ]

Previously, a `Key allocation attempt failed` warning was logged for
`identity does not exist` errors even if maxAllocAttempts wasn't
reached. This also increased the cilium_errors_warnings_total metric.

This patch logs a `Key allocation attempt failed` warning for non existent
identities only if maxAllocAttempts is reached.

The error type is added to `pkg/allocator/allocator.go` in order to
prevent a cyclic import with `pkg/k8s/identitybackend/identity.go`.

`github.com/pkg/errors` is used for error wrapping instead of `errors` because
this patch will be backported to v1.6 that uses Go 1.12.

This patch will also be backported to v1.6.

Fixes: #11487

Signed-off-by: Boris Djurdjevic <boris@djurdjevic.ch>

fixup! pkg/allocator: Do not log `identity does not exist` if the attempt is not at max
[ upstream commit 8d3a142 ]

In CreateOrUpdateRedirect(), the redir.mutex is not release in the case
where redir.implementation.UpdateRules() fails. Although failure in that
function looks unlikely, let's release the mutex before we leave to
avoid any bad surprise.

Fixes: 5c08761 ("proxy: Add RedirectImplementation.UpdateRules")
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Signed-off-by: Joe Stringer <joe@cilium.io>
[ upstream commit c6e34f5 ]

Update the comment for removeRedirect(): Since commit d65adf8
("bpf: Use iptables TPROXY and shared proxy listeners"), this is not
p.mutex but proxyPortsMutex which is used in the returned finalize
function.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Signed-off-by: Joe Stringer <joe@cilium.io>
[ upstream commit 8bd3cea ]

[ Backporter's notes: Several minor conflicts, easily resolved ]

Add a new daemon CLI argument, "--iptables-lock-timeout" to specify the
iptables "--wait" argument when invoking the iptables CLI binary
directly from cilium-agent.

Fixes: #11608

Signed-off-by: Joe Stringer <joe@cilium.io>
[ upstream commit a899305 ]

[ Backporter's notes: Minor conflict with named ports ]

Cilium defines a precedence ordering between sources for mappings from
IP to Identity, defined in pkg/source/source.go. These are used to
determine which Identity should be used for an IP when multiple sources
provide conflicting reports of the Identity to be associated with an IP.

This ordering was handled in the case of source.Generated CIDR/FQDN
-sourced identities by inserting potentially two overlapping entries
into the ipcache.IPIdentityCache when an endpoint's IP is the same as a
CIDR mapping:

* An endpoint Identity would be inserted with the key 'w.x.y.z', and
* A CIDR Identity would be inserted with the key 'w.x.y.z/32'. (IPv6: /128)

During Upsert() and Delete(), when overlapping entries existed in the
map, this overlap would be resolved by directly checking whether another
entry exists with/without the `/32` suffix (IPv6: /128) and either
hiding the update from listeners (when a shadowed mapping is upserted),
or converting the delete to an update (when a shadowing entry is
deleted, revealing the underlying shadowed entry).

During DumpToListenerLocked() however, this shadowing would not be
resolved and instead both entries would be dumped to the caller in an
arbitrary order. This is particularly notable on Linux 4.11 to 4.15
where LPM support is available but deletion is not supported. In these
cases, Cilium periodically dumps the ipcache to a listener using this
function, and populates the BPF ipcache map using this dump. Any time this
dump occurs (default 5 minutes), it would be possible for the ipcache
mapping to be modified to map the IP to either of the cached identities.
Depending on the Go runtime in use by the version of Cilium, this may or
may not consistently provide particular dump ordering.

Resolve this issue by keeping track of shadowed entries with an
explicit boolean field in the value of the map, and avoiding dumping
such entries in the DumpToListenerLocked() function.

Fixes: #11517

Reported-by: Will Deuschle <wdeuschle@palantir.com>
Suggested-by: Jarno Rajahalme <jarno@cilium.io>
Signed-off-by: Joe Stringer <joe@cilium.io>
@joestringer
Copy link
Member Author

test-backport-1.6

joestringer and others added 3 commits June 3, 2020 19:42
[ upstream commit d909b14 ]

[ Backporter's notes: Minor conflict with named ports feature ]

These unit tests validate the bug fixed by the prior commit where
entries dumped from the ipcache may not consistently map IPs to the
correct Identity. Note that there is a potential Golang map dump
ordering aspect to these tests so depending on the Go version used they
may/may not consistently fail. They consistently fail for me prior to
the fix (eg v1.8.0-rc1), and consistently pass with the fix, but YMMV.

Signed-off-by: Joe Stringer <joe@cilium.io>
[ upstream commit 3430980 ]

If the node is under heavy CPU load, the status check may take longer
than 5 seconds. Increase it to 10 seconds and also make it configurable
with an environment variable.

Signed-off-by: Thomas Graf <thomas@cilium.io>
Signed-off-by: Joe Stringer <joe@cilium.io>
[ upstream commit 2a61989 ]

[ Backporter's notes: Minor conflict in CreateOrUpdateRedirect function
                      signature ]

Proxy port reference count is incremented only when an ACK has been received from
all proxies in a specific policy update. If any of the proxies fail to ACK in time,
the revert function is called. Proxy port reference counts must not be decremented
at this time, as they have not been incremented yet.

Fixes: #11637
Fixes: #6921
Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: Joe Stringer <joe@cilium.io>
@joestringer
Copy link
Member Author

test-backport-1.6

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.

LGTM regarding my change.

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Ack for the changes from #11666.

@aanm aanm merged commit f7be163 into v1.6 Jun 4, 2020
@aanm aanm deleted the pr/v1.6-backport-2020-06-03 branch June 4, 2020 12:28
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

7 participants