Consolidate per-endpoint routes macros in the datapath

[CBVenkataReddy]

In the datapath,
we currently have several macros that all indicate whether per-endpoint routes are enabled or not.
ENABLE_ENDPOINT_ROUTES
!ENABLE_ROUTING
Now consolidated the first two, by keeping the first one ENABLE_ENDPOINT_ROUTES.
Also userspace necessary changes done by removing ENABLE_ROUTING.

Fixes: #15449

Signed-off-by: Venkata Reddy venkata.reddy@accuknox.com

Please ensure your pull request adheres to the following guidelines:

• For first time contributors, read Submitting a pull request
• All code is covered by unit and/or runtime tests where feasible.
• All commits contain a well written commit description including a title,
  description and a Fixes: #XXX line if the commit addresses a particular
  GitHub issue.
• All commits are signed off. See the section Developer’s Certificate of Origin
• Provide a title or release-note blurb suitable for the release notes.
• Thanks for contributing!

Fixes: #issue-number

<!-- Enter the release note text here if needed or remove this section! -->

[ti-mo]

There are 2 dangling references to ENABLE_ROUTING in bpf_host.c:

bpf/bpf_host.c:1135:#if defined(ENABLE_HOST_FIREWALL) && !defined(ENABLE_ROUTING)
bpf/bpf_host.c:1167:#endif /* ENABLE_HOST_FIREWALL && !ENABLE_ROUTING */

[qmonnet]

What's the logic behind the replacement? It looks like all conditions on ENABLE_ROUTING being defined OR undefined have been indistinctively replaced with a condition on ENABLE_ENDPOINT_ROUTING being defined. I'm also unsure where the change of conditions in the middle of bpf/bpf_host.c is coming from. Can you please elaborate?

[CBVenkataReddy]

In this PR we have consolidated "ENABLE_ENDPOINT_ROUTES" and "ENABLE_ROUTING"
to "ENABLE_ENDPOINT_ROUTES" also removed unnecessary references to ENABLE_ROUTING in user space.

In bpf/bpf_host.c file it handles packets to and from a local endpoints entering/leaving the host namespace and applies
ingress and egress host policies based on per-endpoint routes are disabled/enabled.

[qmonnet]

Thanks, but it does not answer my question. Seeing that with your PR we have ENABLE_ENDPOINT_ROUTES used for both routing and per-endoint routes:

	if e.RequireEndpointRoute() || e.RequireRouting() {
		fmt.Fprintf(fw, "#define ENABLE_ENDPOINT_ROUTES 1\n")
	}

... Then ENABLE_ENDPOINT_ROUTES will be defined where ENABLE_ROUTING previously was. So in my understanding, conditions like the one below:

-#if defined(ENABLE_HOST_FIREWALL) && !defined(ENABLE_ROUTING)
+#if defined(ENABLE_HOST_FIREWALL) && defined(ENABLE_ENDPOINT_ROUTES)

... are reversing the previous check. Why would they do that?

[CBVenkataReddy]

code commit changes are done considering ENABLE_ENDPOINT_ROUTES and ENABLE_ROUTING are same meaning.

I still have some doubts regarding these two macro's,

1. does these two 'ENABLE_ENDPOINT_ROUTES 'and 'ENABLE_ROUTING' macros same meaning? or
2. does these two 'ENABLE_ENDPOINT_ROUTES' and '!ENABLE_ROUTING' macros same meaning?
   ENDPOINT_ROUTE.txt

[qmonnet]

My understanding is that 2. is correct. ENABLE_ENDPOINT_ROUTES and !ENABLE_ROUTING should be treated the same. This is also what you have for all occurrences in your text file?

[CBVenkataReddy]

My understanding is that 2. is correct. ENABLE_ENDPOINT_ROUTES and !ENABLE_ROUTING should be treated the same. This is also what you have for all occurrences in your text file?

Done the changes w.r.to 2

[CBVenkataReddy]

Pls review PR

[CBVenkataReddy]

with "sudo make -C bpf go_prog_test" we see compilation issues.

Ok, thanks for checking. Just like Paul said, let's drop the option from the Makefile. Please make sure you completely remove the line in the LXC_OPTIONS in the Makefile, to avoid creating a duplicate line.

Yes taken care

[qmonnet]

All right, looks good to me this time, thanks!

[qmonnet]

test-me-please

[qmonnet]

Some CI flakes.

• GKE: CI: K8sServicesTest Checks service across nodes Tests NodePort BPF Tests with direct routing Tests NodePort with Maglev Tests NodePort #13839
• k8s-1.19-kernel-5.4: CI: Host firewall: unable to upgrade connection #15455

Couldn't find a documented flake for the Wireguard failure on k8s-1.16-kernel-netnext, giving it another try to see if it's consistently failing for the PR.

[qmonnet]

test-1.16-netnext

[CBVenkataReddy]

Should we wait #15974 issue to be fixed for this PR get merged?

[pchaigno]

Nit: extra newline can be removed.

[CBVenkataReddy]

Sure I will remove extra newline

[pchaigno]

I'm not sure we can just remove from the API. We might need to deprecate instead. @cilium/maintainers WDYT?

[joestringer]

Correct, removing this is an API-breaking change.

I don't understand why we're removing this from the API at all. The CNI component specifies this to the agent based on certain environment factors. Is there some description of why that logic is no longer necessary or what the motivation is here? It seems unrelated to the datapath-specific piece that this effort started out trying to address.

[nathanjsweet]

I'm converting this to a draft until the high-level comments are addressed.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has not seen any activity since it was marked stale. Closing.