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

Add policy verdicts GSG #12165

Merged
merged 2 commits into from Jun 19, 2020
Merged

Conversation

joestringer
Copy link
Member

This is based on some v1.8 blog material I was working on, but I figured it might be useful as a beginner introduction to forming policy from monitor output.

@joestringer joestringer added release-note/misc This PR makes changes that have no direct user impact. needs-backport/1.8 labels Jun 18, 2020
@joestringer joestringer requested a review from a team as a code owner June 18, 2020 04:35
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Jun 18, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.0 Jun 18, 2020
@coveralls
Copy link

coveralls commented Jun 18, 2020

Coverage Status

Coverage decreased (-0.01%) to 37.092% when pulling 92e3b2c on joestringer:submit/policy-verdicts into 16907c4 on cilium:master.

@joestringer joestringer changed the title RFC: Add policy verdicts GSG Add policy verdicts GSG Jun 18, 2020
Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

thanks for documenting this @joestringer this is super educational!

one thing that's not obvious to me is how one would pick ingress vs egress policy. the example seems slightly arbitrary in a sense that it just happened to get policy-verdict for ingress traffic and proceeds to define an ingress policy. providing a bit more background might help, something like:

  1. i want to protect the deathstar pod by defining an ingress poilcy.
  2. right now i don't know which endpoints access deathstar on which ports.
  3. i can run cilium monitor -t policy-verdict on the node deathstar is running to discover who's accessing deathstar.
  4. then define an ingress policy. it might be even better if there are multiple endpoints accessing deathstar so that we can show both action allow and action drop.

cc @rolinh this can be a good demo scenario for hubble-relay to gather policy verdict notifications from all the nodes without having to exec into a specific cilium pod :)

Documentation/gettingstarted/policy-creation.rst Outdated Show resolved Hide resolved

# cilium monitor -t policy-verdict
...
Policy verdict log: flow 0x63113709 local EP ID 1121, remote ID 2986, dst port 80, proto 6, ingress true, action audit, match none, 10.29.210.187:54134 -> 10.29.50.40:80 tcp SYN
Copy link
Contributor

Choose a reason for hiding this comment

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

not directly related to the documentation, but kind of curious why policy-verdict shows endpoint id for local and security identity for remote.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great question. @ap4y do you recall why this is?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually this may just be historic in that all of the BPF notification types followed this approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely sure, @lzang probably knows more about the design decision for this one. AFAIK security identity is used during the verdict resolution so it makes sense to expose it in the policy log. Source is set automatically based on the datapath via a common header of the message payload:

cilium/bpf/lib/common.h

Lines 270 to 274 in 0fa8ac7

#define __notify_common_hdr(t, s) \
.type = (t), \
.subtype = (s), \
.source = EVENT_SOURCE, \
.hash = get_hash_recalc(ctx)
And we have source defined in the datapath implementation:
#define EVENT_SOURCE LXC_ID

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe makes sense for us to take another look at the CLI output and see whether there are some basic quality-of-life things like this to also print the source local identity.

Documentation/gettingstarted/policy-creation.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/policy-creation.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/policy-creation.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/policy-creation.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/policy-creation.rst Outdated Show resolved Hide resolved

# cilium monitor -t policy-verdict
...
Policy verdict log: flow 0x63113709 local EP ID 1121, remote ID 2986, dst port 80, proto 6, ingress true, action audit, match none, 10.29.210.187:54134 -> 10.29.50.40:80 tcp SYN
Copy link
Member

Choose a reason for hiding this comment

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

Once #12137 is merged, this example output should probably be updated to the new format.

$ kubectl create -f sw_l3_l4_policy.yaml
ciliumnetworkpolicy.cilium.io/rule1 created
$ kubectl -n kube-system exec -ti $(get_cilium_pod) cilium monitor -t policy-verdict
Policy verdict log: flow 0xabf3bda6 local EP ID 343, remote ID 2986, dst port 80, proto 6, ingress true, action allow, match L3-L4, 10.29.210.187:59824 -> 10.29.47.87:80 tcp SYN
Copy link
Member

Choose a reason for hiding this comment

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

Same here, this needs to be updated once #12137 is merged.

@aanm aanm added the area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. label Jun 18, 2020
@rolinh
Copy link
Member

rolinh commented Jun 18, 2020

cc @rolinh this can be a good demo scenario for hubble-relay to gather policy verdict notifications from all the nodes without having to exec into a specific cilium pod :)

Indeed! Maybe something to add to the new getting started with hubble guide.

The next commit will reuse this, so split it into a common file.

Signed-off-by: Joe Stringer <joe@cilium.io>
@joestringer
Copy link
Member Author

@michi-covalent thanks for the detailed review! I revamped/expanded tthe guide and hopefully this addresses your questions&feedback.

Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

went through the guide, it worked seamlessly 💯

maybe we can reference https://www.starwars.com/video/one-in-a-million-shot at the end of the guide, but it does mean the force was too strong with this one even for cilium to protect death star.

Documentation/gettingstarted/policy-creation.rst Outdated Show resolved Hide resolved
Signed-off-by: Joe Stringer <joe@cilium.io>
Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

lgtm! went through the Disable Policy Audit Mode section. just a few minor nits

@joestringer joestringer merged commit c4fd192 into cilium:master Jun 19, 2020
1.8.0 automation moved this from In progress to Merged Jun 19, 2020
@joestringer joestringer deleted the submit/policy-verdicts branch June 19, 2020 00:14
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.0 Jun 19, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.0 Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
1.8.0
Backport done to v1.8
Development

Successfully merging this pull request may close these issues.

None yet

9 participants