L7 Visibility Annotations for proxylib parsers

[trvll]

Translating VisibilityPolicy into PortNetworkPolicy to enable L7 visibility for pod annotations.

Creates an allow-all policy that has a PortNetworkPolicyRule derived from VisibilityPolicy.

Signed-off-by: Thales Paiva thales@accuknox.com

Fixes: #14072

Pod visibility annotations are now supported for Kafka and other policies implemented via Cilium Go extensions for Envoy.

[maintainer-s-little-helper]

Commit dd08f08a75785c4c6bd3d53975f5f47f25063b63 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[jrajahalme]

@trvll Please note that no PortNetworkPolicy is needed for the visibility annotations to work. The reason why the visibility annotations do not currently work with Network Policies is not the lack of translation from visibility annotation to PortNetworkPolicy (which is not needed), but the missing interplay with L3/L4 allow/deny policy processing for the BPF policy map entries.

There is a work-in-progress PR that adds the missing pieces: #16258

While the solution in the linked PR is correct, it is still missing unit testing making it fragile towards future regressions. I'll be working on that shortly.

EDIT: I did not consider proxylib parsers yet. It may be possible to make the proxylib network policy code behave like the Envoy side so that a missing port network policy is interpreted as an allow-all policy. For normal (non-annotation) policies we never redirect to the proxy unless a PortNetworkPolicy is successfully configured. So the only configuration where traffic is redirected without a PortNetworkPolicy is for visibility only (via the visibility annotations).

[jrajahalme]

Here's the code on the cilium-envoy side making the policy exception to support Istio sidecars, this same exception also allows visibility annotations to work without PortNetworkPolicies:

https://github.com/cilium/proxy/blob/master/cilium/network_policy.cc#L555-L560

[jrajahalme]

Corresponding code on the proxylib side is somewhat unfinished, maybe you try make it behave similarly to the C++ side linked above and test how it works out?

https://github.com/cilium/cilium/blob/master/proxylib/proxylib/policymap.go#L235-L245

Something like this here might work:

	if !(found || foundWc) {
		log.Debugf("NPDS::PortNetworkPolicies(port=%d, remoteId=%d): allowing traffic on port for which there is no policy, assuming L3/L4 has passed it! (%v)", port, remoteId, p)
		return true
	}
	return false

[trvll]

@jrajahalme thank you for this kind discussion.

@trvll Please note that no PortNetworkPolicy is needed for the visibility annotations to work.

AFAIU for annotations a PortNetworkPolicy will be used but it is an allow-all policy since there is no policy enforced:

cilium/pkg/envoy/server.go

Lines 1239 to 1242 in eb11c14

	if !policyEnforced {
	// Return an allow-all policy.
	return allowAllPortNetworkPolicy
	}

EDIT: I did not consider proxylib parsers yet. It may be possible to make the proxylib network policy code behave like the Envoy side so that a missing port network policy is interpreted as an allow-all policy. For normal (non-annotation) policies we never redirect to the proxy unless a PortNetworkPolicy is successfully configured. So the only configuration where traffic is redirected without a PortNetworkPolicy is for visibility only (via the visibility annotations).

In case of proxylib parsers, when receiving a new connection, the proxy will create a new instancy from proxylib and needs the protocol name information to do that. This information on Envoy side is retrieved from PortNetworkPolicyRule which comes to be empty because we are using an allow-all policy.

https://github.com/cilium/proxy/blob/e83275091139101e7ff29284ef35c56f4cd6bfc3/cilium/network_filter.cc#L124-L127

So my approach here was to create an allow-all policy that has a PortNetworkPolicyRule derived from VisibilityPolicy. This make sense to you?

[jrajahalme]

@trvll Right, I forgot that proxylib parser selection requires the l7 parser name in the PortNetworkPolicyRule :-)

This PR looks good to me to fix this for the current master and is fully orthogonal with #16258. Some CI testing would be needed though. Would it be possible to augment the existing proxylib and/or Kafka tests with visibility annotations to verify this works?

Additional work will be needed to fuse the visibility annotation with an actual policy when policy is enforced, though. #16258 does this for the bpf policy map rules, but fails to address this for proxylib parsers. IMO this can be left for a follow-up PR.

[jrajahalme]

test-me-please

[trvll]

@jrajahalme thanks for your review. Yes! I can go through the tests right now as the proposed solution is considered satisfactory.

I also agree with the additional work statement. Thank you so much!

[jrajahalme]

@trvll Do you still plan working on some test coverage? This has no test coverage as of now. At a minimum, some unit tests for the additional functionality of getNetworkPolicy() in pkg/envoy/server_test.go should be added.

[trvll]

@jrajahalme yes, I'll code the tests ASAP considering your suggestion for sure. Tks.

[trvll]

Hi @jrajahalme I've added a unit test for the added functionality. I used kafka as example which I think is enough to test the mechanism. Please let me know your opinion. Tks.

[aanm]

test-1.21-4.9

[aanm]

ci-awscni

[aanm]

ci-aks

[aanm]

ci-eks

[aanm]

ci-gke

[aanm]

@trvll it looks these changes is breaking CI. Can you to take a look at them?

[trvll]

@aanm for sure. I'm going to look on that. thx

[trvll]

I've patched the offending code. Basically we should create the PortNetworkPolicyRule with L7Proto set only for proxylib parsers.
I added a statement to avoid this setup for HTTP and DNS annotations.

[jrajahalme]

/test

[jrajahalme]

/test

[jrajahalme]

Build Wait for quay images timed out, have to close/open to trigger new build.

[jrajahalme]

@trvll Sorry for the delay with this PR! Due to changes in PR build process (#17623) this PR needs to be rebased for it to build successfully.

[trvll]

@jrajahalme "Travis CI - Pull Request" is failling to pull docker images:

Error response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit

Not related to PR changes.

[jrajahalme]

/test

[jrajahalme]

All the CI fails are unrelated, labeling ready-to-merge.