v1.11 backports 2022-05-09

[kaworu]

• clustermesh-apiserver: fixed nil pointer dereference #18957 -- clustermesh-apiserver: fixed nil pointer dereference (@abocim)
• docs: move sitemap-index.xml to static directory #19681 -- docs: move sitemap-index.xml to static directory (@aanm)
• hubble/relay: Make Peer Service a K8s Service #18620 -- hubble/relay: Make Peer Service a K8s Service (@nathanjsweet)
• test/upgrade: use the unreleased helm chart of stable branches #19710 -- test/upgrade: use the unreleased helm chart of stable branches (@aanm)
• images/cilium: remove cilium group from Dockerfile #19711 -- images/cilium: remove cilium group from Dockerfile (@aanm)
• operator: Add cilium node garbage collector #19576 -- operator: Add cilium node garbage collector (@sayboras)
• bpf: Add missing packet trace in handle_nat_fwd #19158 -- bpf: Add missing packet trace in handle_nat_fwd (@YutaroHayakawa)

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

$ for pr in 18957 19681 18620 19710 19711 19576 19158; do contrib/backporting/set-labels.py $pr done 1.11; done

or with

$ make add-label BRANCH=v1.11 ISSUES=18957,19681,18620,19710,19711,19576,19158

[kaworu]

@YutaroHayakawa BPF check failure is due to TRACE_REASON_UNKNOWN being undefined. It has been introduced by this PR, let me know what we should do.

[YutaroHayakawa]

@kaworu (cc: @qmonnet) Hi, thanks for the heads up! I think just replacing TRACE_REASON_UNKNOWN with 0 should be enough in this case. We also have an option to backport #19226, but we anyways release v1.12 soon and we probably won't request the backporting of the tracepoint bug to v1.11 branch anymore because it is not a "major" bug (c.f. https://docs.cilium.io/en/stable/contributing/release/backports/#backport-criteria).

[kaworu]

@YutaroHayakawa replacing by 0 will effectively be the same as TRACE_REASON_POLICY (see CT_NEW) right? If correct, let me know if you prefer explicit 0 or TRACE_REASON_POLICY (can add a comment linking to #19226 either way).

[aanm]

My commits look good, thanks!

[sayboras]

my change looks good, but I got the below error while trying to verify it again locally

$ make -j $(nproc) build
... 
In file included from bpf_xdp.c:38:
/home/tammach/go/src/github.com/cilium/cilium/bpf/lib/nodeport.h:1020:19: error: variable has incomplete type 'enum trace_point'
        enum trace_point obs_point;
                         ^
/home/tammach/go/src/github.com/cilium/cilium/bpf/lib/nodeport.h:1020:7: note: forward declaration of 'enum trace_point'
        enum trace_point obs_point;
             ^
/home/tammach/go/src/github.com/cilium/cilium/bpf/lib/nodeport.h:1033:48: error: use of undeclared identifier 'TRACE_REASON_UNKNOWN'; did you mean 'TRACE_REASON_POLICY'?
        send_trace_notify(ctx, obs_point, 0, 0, 0, 0, TRACE_REASON_UNKNOWN, 0);
                                                      ^~~~~~~~~~~~~~~~~~~~
                                                      TRACE_REASON_POLICY
/home/tammach/go/src/github.com/cilium/cilium/bpf/lib/trace.h:49:2: note: 'TRACE_REASON_POLICY' declared here
        TRACE_REASON_POLICY = CT_NEW,
        ^
In file included from bpf_xdp.c:38:
/home/tammach/go/src/github.com/cilium/cilium/bpf/lib/nodeport.h:2092:19: error: variable has incomplete type 'enum trace_point'
        enum trace_point obs_point;
                         ^
/home/tammach/go/src/github.com/cilium/cilium/bpf/lib/nodeport.h:2092:7: note: forward declaration of 'enum trace_point'
        enum trace_point obs_point;
             ^
/home/tammach/go/src/github.com/cilium/cilium/bpf/lib/nodeport.h:2104:48: error: use of undeclared identifier 'TRACE_REASON_UNKNOWN'; did you mean 'TRACE_REASON_POLICY'?
        send_trace_notify(ctx, obs_point, 0, 0, 0, 0, TRACE_REASON_UNKNOWN, 0);
                                                      ^~~~~~~~~~~~~~~~~~~~
                                                      TRACE_REASON_POLICY
/home/tammach/go/src/github.com/cilium/cilium/bpf/lib/trace.h:49:2: note: 'TRACE_REASON_POLICY' declared here
        TRACE_REASON_POLICY = CT_NEW,

[qmonnet]

@YutaroHayakawa replacing by 0 will effectively be the same as TRACE_REASON_POLICY (see CT_NEW) right? If correct, let me know if you prefer explicit 0 or TRACE_REASON_POLICY (can add a comment linking to #19226 either way).

TL;DR: Yes, go with 0 instead.

The supposition above is correct. 0 was used both when the reason was unknown and for new conntrack connections. This was not convenient, and that's the reason why I changed it in #19226.

So 0 is the correct value to use, but there's an additional thing we need to check. Quoting a DM from Sebastian:

In Hubble, we maintain a list of trace points for which we know that they have the reason filed in send_trace_notify set. This is required because 0 is actually valid value for the reason field. So whenever Hubble reads a trace notify reason field and it's 0, we don't know if it's zero because it was a new connection or because the trace point statically sets it to zero. Thus this check here.
If you added all trace points to include the ct_reason, you can just add them to the list here.

In the case of Yutaro's PR, the context of the new traces will be either TRACE_TO_OVERLAY or TRACE_TO_NETWORK. Both contexts will cause TraceObservationPointHasConnState() (second link above) to return false, meaning that we will in fact consider the reason as unknown, meaning that Hubble won't pass it to the UI and won't cause flow parsing errors. So I'd say we're good with just using 0 here (cc @gandro just in case). Other trace points in v1.11 use 0 rather than TRACE_REASON_POLICY when the reason is not known, so let's stick with that.

[kaworu]

@qmonnet thanks a lot for the context, I've replaced TRACE_REASON_UNKNOWN by 0 as suggested and added a comment.

@sayboras the error you got was expected as we were discussing the fix, it should work now! 🙏

[sayboras]

Thanks, I still have the below error :(

$ git rev-parse HEAD      

8139b3940a680183c27570862c600b2618b099aa


make[1]: *** [Makefile:216: bpf_xdp.ll] Error 1
llc -march=bpf -mattr=dwarfris -mcpu=v2 -filetype=obj -o bytecount.o bytecount.ll
In file included from bpf_host.c:55:
/home/tammach/go/src/github.com/cilium/cilium/bpf/lib/nodeport.h:1020:19: error: variable has incomplete type 'enum trace_point'
        enum trace_point obs_point;
                         ^
/home/tammach/go/src/github.com/cilium/cilium/bpf/lib/nodeport.h:1020:7: note: forward declaration of 'enum trace_point'
        enum trace_point obs_point;
             ^
/home/tammach/go/src/github.com/cilium/cilium/bpf/lib/nodeport.h:2094:19: error: variable has incomplete type 'enum trace_point'
        enum trace_point obs_point;
                         ^
/home/tammach/go/src/github.com/cilium/cilium/bpf/lib/nodeport.h:2094:7: note: forward declaration of 'enum trace_point'
        enum trace_point obs_point;

[kaworu]

@YutaroHayakawa I've replaced usage of enum trace_point by int as the bpf code used to do before 6343f4c.

[sayboras]

Thanks, looks good for my commit :). Verified locally to make sure GC was done correctly.

$ ksyslo deployment/cilium-operator --timestamps | grep "subsys=watchers"
2022-05-09T13:52:42.607874474Z level=info msg="Starting to garbage collect stale CiliumNode custom resources" subsys=watchers
...
2022-05-09T13:57:42.608310224Z level=info msg="Add CiliumNode to garbage collector candidates" nodeName=dummy-one subsys=watchers
2022-05-09T13:57:42.608347044Z level=debug msg="CiliumNode is valid, no gargage collection required" nodeName=minikube subsys=watchers
...
2022-05-09T14:02:42.609286306Z level=debug msg="CiliumNode is valid, no gargage collection required" nodeName=minikube subsys=watchers
2022-05-09T14:02:42.609323065Z level=info msg="Perform GC for invalid CiliumNode" nodeName=dummy-one subsys=watchers
2022-05-09T14:02:42.615194899Z level=info msg="CiliumNode is garbage collected successfully" nodeName=dummy-one subsys=watchers

[nathanjsweet]

To not break CI, we need to kind of backport the following PRs:

• ci: Update Uninstall Command For Cilium CLI #19679
• ci: update master workflows to cilium-cli v0.11.4 #19665
• And wherever this line/commit came from

[aanm]

/test-backport-1.11

Job 'Cilium-PR-K8s-1.23-kernel-4.9' failed:

Click to show.

Test Name

K8sServicesTest Checks graceful termination of service endpoints Checks client terminates gracefully on service endpoint deletion

Failure Output

FAIL: Timed out after 60.001s.

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.23-kernel-4.9 so I can create one.

[aanm]

/test-1.23-4.9