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

hubble/correlation: Support deny policies #31544

Merged

Conversation

gandro
Copy link
Member

@gandro gandro commented Mar 21, 2024

No description provided.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 21, 2024
@gandro gandro added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Mar 21, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 21, 2024
@gandro gandro added sig/hubble Impacts hubble server or relay dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Mar 21, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 21, 2024
@michi-covalent
Copy link
Contributor

just stopping by to say that i'm super excited about this. i'lll play around with it next week and let you know how it goes 🚀🙏

It seems that at some point the datapath changed under us, and some
flows now carry the verdict REDIRECTED. We actually are interested in
seeing these too.

Co-authored-by: David Bimmler <david.bimmler@isovalent.com>
Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
@gandro gandro force-pushed the pr/gandro/policy-correlation-deny-policies branch from ac7ff33 to f2b193d Compare March 27, 2024 15:15
@gandro
Copy link
Member Author

gandro commented Mar 27, 2024

Did some local testing. Seems to work okay, given deny policy precedence (which is not always intuitive, but I have not found any discrepancies between what the policy engine does and what we correlate with) . Also extended the unit tests.

@gandro gandro marked this pull request as ready for review March 27, 2024 15:17
@gandro gandro requested review from a team as code owners March 27, 2024 15:17
@gandro gandro requested review from squeed and glibsm March 27, 2024 15:17
@michi-covalent
Copy link
Contributor

cool! i'll try it out today. thanks sebastian!

@gandro
Copy link
Member Author

gandro commented Mar 27, 2024

/test

@michi-covalent
Copy link
Contributor

✅ i did a quick manual test:

minikube start --network-plugin=cni --cni=false
helm install -n kube-system cilium oci://quay.io/cilium-charts-dev/cilium --version 1.16.0-dev-dev.347-HEAD-f2b193d01d
cat << EOF | kubectl apply -f -
apiVersion: "cilium.io/v2"
kind: CiliumNetworkPolicy
metadata:
  name: "deny-world"
spec:
  endpointSelector: {}
  egressDeny:
  - toEntities:
    - "world"
EOF

then

kubectl exec -n kube-system ds/cilium -c cilium-agent bash -- hubble observe -f -t policy-verdict -n default

and generate some traffic to world:

kubectl run -it --rm curl --image=badouralix/curl-jq  -- sh
curl 1.1.1.1

and i see:

    "egress_denied_by": [
      {
        "name": "deny-world",
        "namespace": "default",
        "labels": [
          "k8s:io.cilium.k8s.policy.derived-from=CiliumNetworkPolicy",
          "k8s:io.cilium.k8s.policy.name=deny-world",
          "k8s:io.cilium.k8s.policy.namespace=default",
          "k8s:io.cilium.k8s.policy.uid=e5165fac-a66b-48fd-b3e6-73ee414b371b"
        ],
        "revision": "2"
      }

i wasn't seeing this field yesterday because i forgot to re-compile the hubble CLI with the new protobuf definitions 👻

gandro and others added 5 commits March 28, 2024 10:24
We get the endpoint ID handed to us in the flow, so let's use it to
directly look up the endpoint.

Co-authored-by: David Bimmler <david.bimmler@isovalent.com>
Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
Instead of attempting to fetch the realized policy rule labels multiple
times with a less and less specific key, use the policy match type that
the flow provides us with.

Co-authored-by: David Bimmler <david.bimmler@isovalent.com>
Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
Co-authored-by: David Bimmler <david.bimmler@isovalent.com>
Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
Save the policies which lead to the drop of a packet in the
`{Ingress,Egress}DeniedBy` field.

Co-authored-by: David Bimmler <david.bimmler@isovalent.com>
Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
ICMP and SCTP can also be part of the policy key. For ICMP, the policy
engine allows to match on the type.

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
@gandro gandro force-pushed the pr/gandro/policy-correlation-deny-policies branch from f2b193d to 04d9fea Compare March 28, 2024 09:25
@gandro gandro requested a review from a team as a code owner March 28, 2024 09:25
@gandro
Copy link
Member Author

gandro commented Mar 28, 2024

/test

@gandro gandro removed the request for review from squeed April 8, 2024 11:48
@gandro
Copy link
Member Author

gandro commented Apr 8, 2024

Reviews are in, merging

@gandro gandro added this pull request to the merge queue Apr 8, 2024
Merged via the queue into cilium:main with commit aa43768 Apr 8, 2024
62 checks passed
@gandro gandro deleted the pr/gandro/policy-correlation-deny-policies branch April 8, 2024 11:55
@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 Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/hubble Impacts hubble server or relay
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants