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.9 backports 2020-11-30 #14212

Merged
merged 19 commits into from
Dec 2, 2020
Merged

v1.9 backports 2020-11-30 #14212

merged 19 commits into from
Dec 2, 2020

Conversation

aanm
Copy link
Member

@aanm aanm commented Nov 30, 2020

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

$ for pr in 13923 14134 14135 14112 14145 14104 14163 14182 13983 14090 14185 14171 14110 14165 14141 14191; do contrib/backporting/set-labels.py $pr done 1.9; done

mandarjog and others added 3 commits November 30, 2020 12:08
[ upstream commit 53f35fb ]

Signed-off-by: Mandar U Jog <mjog@google.com>
Signed-off-by: André Martins <andre@cilium.io>
[ upstream commit 625f82d ]

- fix reference for host-side path, use `hostPath` instead of `mountPath`
- add `type`

Fixes: #14132

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>
Signed-off-by: André Martins <andre@cilium.io>
[ upstream commit a3d1f02 ]

Update the tag for the checkpatch image in order to benefit from the
latest changes when running the GitHub actions: The latest image
suppresses reports for FILE_PATH_CHANGES to avoid checkpatch to complain
when files are added or moved under bpf/ directory.

See discussion at
#14088 (comment)

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Signed-off-by: André Martins <andre@cilium.io>
@aanm
Copy link
Member Author

aanm commented Nov 30, 2020

@jrajahalme @brb there were some minor conflicts in d60dbd8 please take a 2nd look.

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.

LGTM for my commit 💯

@aanm aanm marked this pull request as draft November 30, 2020 11:31
borkmann and others added 11 commits November 30, 2020 12:36
[ upstream commit 2a3e5d4 ]

The probe mode is expected to only run alongside kube-proxy as hybrid.
There was confusion that the kube-proxy log was throwing (harmless) warnings
to its log that it could not bind sockets to service ports in the hostns.
This is due to Cilium performing bind protection right out of the bind(2)
syscall with eBPF. To avoid this confusion, defer to kube-proxy to bind
sockets instead. This is less efficient and consuming more resources, but
if users want to avoid the overhead, they would run kube-proxy free in strict
mode anyway where Cilium does the bind protection by default anyway.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: André Martins <andre@cilium.io>
[ upstream commit acb2daa ]

The new K8sVerifier test compiles some Cilium binaries inside the VM,
which can lead to 'interrupted system call' errors. Using NFS should fix
it by speeding up the filesystem accesses.

This commit switches the test VMs to use NFS by default, thereby
enabling NFS in our CI.

NFS remains disabled in the CI's Runtime tests because it leads to
permission errors [1].

1 - https://jenkins.cilium.io/job/Cilium-PR-Runtime-4.9/2739/consoleFull
Signed-off-by: Paul Chaignon <paul@cilium.io>
Signed-off-by: André Martins <andre@cilium.io>
[ upstream commit 1b29044 ]

This introduces a check that we do not overwrite the numeric security
identity provided by the datapath trace point. Only if the datapath did
not provide an identity (i.e. in `FROM_LXC` trace points) do we want to
fall back on the identity from the user-space ip cache or endpoint
manager.

The numeric identity from the datapath can differ from the one we obtain
from user-space (e.g. the endpoint manager or the IP cache), because the
identity could have changed between the time the datapath event was
created and the time the event reaches the Hubble parser. To aid in
troubleshooting, we want to preserve what the datapath observed when it
made the policy decision.

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
Signed-off-by: André Martins <andre@cilium.io>
[ upstream commit f380dd3 ]

The overall structure for test K8sBandwidth looks to have been extracted
from K8sServices. It works fine but is more complex than necessary and
leads to unintended behavior when tests are skipped. This commit
simplifies the structure to have a single conditional Context
(conditioned on net-next kernel) inside which the three It tests are
run.

Cilium was also installed with the bandwidth manager enabled *before*
the conditional Context. That installation would therefore happen
regardless of whether bandwidth tests should actually be skipped,
sometimes even leading to flakes on 4.9 kernels [1].

Removing this initial installation of Cilium implies that the test pods
are now deployed (once for all tests) before Cilium is installed. We
therefore need to wait for the test pods, with a new helper
waitForTestPods(), after each re-installation of Cilium.

1 - https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-K8s/3740/testReport/junit/Suite-k8s-1/16/K8sBandwidthTest_Checks_Bandwidth_Rate_Limiting/
Signed-off-by: Paul Chaignon <paul@cilium.io>
Signed-off-by: André Martins <andre@cilium.io>
[ upstream commit 885a319 ]

Previously, the SetUpSuite() routine called netns.New(). It expected
that the latter only creates a new netns without setting it.  However,
according to the docs it's not the case:

    package netns // import "github.com/vishvananda/netns"

    func New() (ns NsHandle, err error)
        New creates a new network namespace, sets it as current and returns
        a handle to it.

This meant that we changed the netns before locking the OS thread which
could result in other Go runtime threads running in the test netns.

Fixes: b059c31 ("daemon: Add unit tests for device detection")
Signed-off-by: Martynas Pumputis <m@lambda.lt>
Signed-off-by: André Martins <andre@cilium.io>
[ upstream commit 60bd47f ]

Add a map for newly allocated identities to ipcache.AllocateCIDR
functions that the caller can use to upsert the IPs to ipcache later,
after affected endpoint policy maps have been updated.

Use this new functionality on the DNS proxy code path, that makes sure
that new policy map entries are in place before an IP received from a
DNS server is placed in ipcache. This is really straightforward as the
logic for waiting was already in place for delaying the forwarding of
the DNS response.

Policy update path is still allowing ipcache upserts at policy
ingestion time rather than waiting for the policy maps to be
updated. This means that new, more specific CIDRs (e.g., 10.0.0/24) in
policies can still cause momentary drops on traffic currently using a
less specific CIDR (e.g., 10.0/16).

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: André Martins <andre@cilium.io>
… regenerated by endpoints.

[ upstream commit 8f20d3b ]

Move ipcache CIDR upserts and releases to the policy reaction queue,
where upserts can be executed after regenerations have been completed,
i.e. after endpoint policy maps have been updated. This way IP
addresses are mapped to newly allocated identities only after endpoint
policy maps are ready to classify them.

Correspondingly, on deletes the to-be-deleted CIDR identities are
first deleted from ipcache so that when they are deleted from endpoint
policy maps they are no longer used in classification. Releases of
CIDR identities must still be serialized with ipcache upserts via the
policy reaction queue so that they are executed in the same order
w.r.t. ipcache upserts as policy deletes and adds.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: André Martins <andre@cilium.io>
[ upstream commit 19a6011 ]

Signed-off-by: Deepesh Pathak <deepshpathak@gmail.com>
Signed-off-by: André Martins <andre@cilium.io>
[ upstream commit 81dc19b ]

When we load a BPF program in the kernel, tc loads the entire object
file, meaning it attempts to load each BPF program found in the object
file. In some cases (e.g., ICMPv6 code in bpf_xdp.o), we include BPF
program as sections in the object file even though we never tail call to
them.

This commit fixes it by ensuring we only compile those sections if they
are needed. This also fixes a failure to load bpf_xdp on 4.19 when
compiled with our MAX_LB_OPTIONS options combination: ENABLE_IPV4
ENABLE_IPV6 ENABLE_HOST_SERVICES_TCP ENABLE_HOST_SERVICES_UDP
ENABLE_IPSEC.

Signed-off-by: Paul Chaignon <paul@cilium.io>
Signed-off-by: André Martins <andre@cilium.io>
[ upstream commit a77842b ]

Running the Runtime tests in CI with NFS enabled currently fails because
'install' reports a permission error when trying to change permissions
of cilium.conf.ginkgo. This commit switches 'install' for 'chmod' which
works fine.

The reason for this error is that 'install' relies on the fsetxattr(2)
system call to change the permissions and, as pointed by Quentin, there
is no support for Extended File Attributes in NFS [1]. 'install'
therefore fails whereas 'chmod', which relies on fchmodat(2) works fine.

That bug wasn't found when running the Runtime test with NFS locally
because, for local tests, a different implementation of
RenderTemplateToFile() is used, one that does not rely on 'install'.

1 - https://tools.ietf.org/html/rfc8276
Signed-off-by: Paul Chaignon <paul@cilium.io>
Signed-off-by: André Martins <andre@cilium.io>
[ upstream commit 8bf3ed8 ]

Signed-off-by: Paul Chaignon <paul@cilium.io>
Signed-off-by: André Martins <andre@cilium.io>
@aanm aanm force-pushed the pr/v1.9-backport-2020-11-30 branch from 1501d09 to bb290f3 Compare November 30, 2020 11:36
@aanm aanm marked this pull request as ready for review November 30, 2020 11:56
@aanm
Copy link
Member Author

aanm commented Nov 30, 2020

test-backport-1.9

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.

👍 for my PRs.

@pchaigno pchaigno removed their assignment Nov 30, 2020
Copy link
Member

@brb brb 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 LGTM, thanks.

@aanm aanm unassigned brb Nov 30, 2020
Copy link
Member

@fristonio fristonio 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 changes. 🚀

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.

Good, thank you.

Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

Lookin' good on my part.

@aanm aanm merged commit ce211d0 into v1.9 Dec 2, 2020
@aanm aanm deleted the pr/v1.9-backport-2020-11-30 branch December 2, 2020 10:44
@aanm aanm mentioned this pull request Dec 4, 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