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

daemon: initialize datapath before compiling sockops programs #24140

Merged
merged 1 commit into from
Mar 9, 2023

Conversation

jibi
Copy link
Member

@jibi jibi commented Mar 2, 2023

When egress gateway is enabled, we define the ENABLE_EGRESS_GATEWAY macro in the datapath, which in lib/common.h causes HAVE_ENCAP to be defined as well:

#if defined(ENCAP_IFINDEX) || defined(ENABLE_EGRESS_GATEWAY)
#define HAVE_ENCAP
//..
#endif

This doesn't work with sockops enabled, as bpf_sockops.c ends up including lib/overloadable_skb.h with HAVE_ENCAP defined and ENCAP_IFINDEX not defined.

This happens since in the daemon we currently:

  • create the node_config.h header file with createNodeConfigHeaderfile()
  • invoke the logic to compile the sockops programs with the sockops.Sockmap*() methods
  • initialize the datapath with Datapath().Loader().Reinitialize(), which is responsible among other things of invoking bpf/init.sh, which in turn is responsible for populating node_config.h with a few missing constants, one of which is ENCAP_IFINDEX

To fix this, simply invoke Datapath().Loader().Reinitialize() before compiling the sockops programs, in order to properly populate node_config.h

Fixes: #24090

@jibi jibi added kind/bug This is a bug in the Cilium logic. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium. feature/egress-gateway Impacts the egress IP gateway feature. needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Mar 2, 2023
@jibi
Copy link
Member Author

jibi commented Mar 2, 2023

/test

@jibi jibi marked this pull request as ready for review March 2, 2023 16:29
@jibi jibi requested a review from a team as a code owner March 2, 2023 16:29
@jibi jibi requested a review from thorn3r March 2, 2023 16:29
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Ideally sockops would be reinitialized inside the datapath Reinitialize() function, but I think that this code is about to be in flux anyway. LGTM.

When egress gateway is enabled, we define the ENABLE_EGRESS_GATEWAY
macro in the datapath, which in lib/common.h causes HAVE_ENCAP to be
defined as well:

    #if defined(ENCAP_IFINDEX) || defined(ENABLE_EGRESS_GATEWAY)
    #define HAVE_ENCAP
    //..
    #endif

This doesn't work with sockops enabled, as bpf_sockops.c ends up
including lib/overloadable_skb.h with HAVE_ENCAP defined and
ENCAP_IFINDEX not defined.

This happens since in the daemon we currently:

- create the node_config.h header file with createNodeConfigHeaderfile()
- invoke the logic to compile the sockops programs with the
  sockops.Sockmap*() methods
- initialize the datapath with Datapath().Loader().Reinitialize(), which
  is responsible among other things of invoking bpf/init.sh, which in
  turn is responsible for populating node_config.h with a few missing
  constants, one of which is ENCAP_IFINDEX

To fix this, simply invoke Datapath().Loader().Reinitialize() before
compiling the sockops programs, in order to properly populate
node_config.h

Fixes: #24090
Signed-off-by: Gilberto Bertin <jibi@cilium.io>
@jibi jibi force-pushed the pr/jibi/fix-egressgw-with-sockops branch from d7ebdff to b7f1cd0 Compare March 9, 2023 16:45
@jibi
Copy link
Member Author

jibi commented Mar 9, 2023

/test
(rebased)

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 9, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.13.1 Mar 9, 2023
@pchaigno pchaigno merged commit 8b20703 into master Mar 9, 2023
@pchaigno pchaigno deleted the pr/jibi/fix-egressgw-with-sockops branch March 9, 2023 23:27
@nebril nebril added this to Needs backport from master in 1.13.2 Mar 15, 2023
@nebril nebril removed this from Needs backport from master in 1.13.1 Mar 15, 2023
@NikAleksandrov NikAleksandrov mentioned this pull request Mar 23, 2023
29 tasks
@NikAleksandrov NikAleksandrov added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Mar 23, 2023
@tklauser tklauser mentioned this pull request Mar 28, 2023
9 tasks
@tklauser tklauser added affects/v1.11 This issue affects v1.11 branch affects/v1.12 This issue affects v1.12 branch labels Mar 30, 2023
@jibi jibi added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Apr 7, 2023
@gentoo-root gentoo-root moved this from Needs backport from master to Backport done to v1.13 in 1.13.2 Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects/v1.11 This issue affects v1.11 branch affects/v1.12 This issue affects v1.12 branch backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. feature/egress-gateway Impacts the egress IP gateway feature. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.13.2
Backport done to v1.13
Development

Successfully merging this pull request may close these issues.

Start hook failed
6 participants