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

cmd/observe: improve policy verdict output in compact mode #745

Merged
merged 1 commit into from
Jun 15, 2022

Conversation

rolinh
Copy link
Member

@rolinh rolinh commented Jun 14, 2022

The default output (compact) makes it hard for a user to tell the
different event types apart. In particular, it is hard for a user to
distinguish a trace event from a policy verdict one.

This commit modifies the output for policy verdict events in two ways.
First, instead of printing the drop reason (which is redundant as also
printed out with the dropped flow) the string 'policy-verdict' along
with the policy match type string for the event (L3-Only, L3-L4,
L4-Only, all, none), is displayed.
Additionally, flows that are forwarded or redirected are printed as
ALLOWED, those that are dropped or error as DENIED and the audit ones as
AUDITED.

Example output from #570:

$ cat sample.json | hubble observe --output compact
Jun 10 12:50:15.555: default/cronjob-1623329400-8mg2z:43006 (ID:124484) -> default/deployment-85c67465d6-tfgcp:8087 (ID:107099) policy-verdict:L3-Only ALLOWED (TCP Flags: SYN)
Jun 10 12:50:15.555: default/cronjob-1623329400-8mg2z:43006 (ID:124484) -> default/deployment-85c67465d6-tfgcp:8087 (ID:107099) to-stack FORWARDED (TCP Flags: SYN)
Jun 10 12:50:15.556: 10.8.14.163:43006 (world) <> default/deployment-85c67465d6-tfgcp:8087 (ID:107099) policy-verdict:none DENIED (TCP Flags: SYN)
Jun 10 12:50:15.556: 10.8.14.163:43006 (world) <> default/deployment-85c67465d6-tfgcp:8087 (ID:107099) Policy denied DROPPED (TCP Flags: SYN)
Jun 10 12:50:16.580: default/cronjob-1623329400-8mg2z:43006 (ID:124484) -> default/deployment-85c67465d6-tfgcp:8087 (ID:107099) policy-verdict:L3-Only ALLOWED (TCP Flags: SYN)
Jun 10 12:50:16.580: default/cronjob-1623329400-8mg2z:43006 (ID:124484) -> default/deployment-85c67465d6-tfgcp:8087 (ID:107099) to-endpoint FORWARDED (TCP Flags: SYN)

image

Closes: #570

@rolinh rolinh added ⌨️ area/cli Impacts the command line interface of any command in the repository. release-note/minor This PR introduces functionality that users may find relevant to operating Hubble. labels Jun 14, 2022
@rolinh rolinh requested review from pchaigno, a team and joamaki and removed request for a team June 14, 2022 14:39
rolinh added a commit to cilium/cilium that referenced this pull request Jun 14, 2022
From a user perspective, it is much easier to understand the flow type
when policy verdict events verdicts use the term ALLOWED instead of
REDIRECTED/FORWARDED and DENIED instead of DROPPED.

See also: cilium/hubble#745

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
@michi-covalent michi-covalent added this to the 1.0 milestone Jun 14, 2022
@michi-covalent
Copy link
Collaborator

michi-covalent commented Jun 14, 2022

lgtm! for ALLOWED verdict, it might still be useful to print match type:

policy-verdict ALLOWED:L3-L4

for DENIED verdict i guess it's redundant to print none because we know nothing matched.

edit:

also it might be useful to indicate direction (ingress/egress) 💭

policy-verdict egress ALLOWED:L3-L4
policy-verdict ingress DENIED

@gandro
Copy link
Member

gandro commented Jun 15, 2022

lgtm! for ALLOWED verdict, it might still be useful to print match type:

policy-verdict ALLOWED:L3-L4

Fully agreed. I think having the match type is very useful in audit mode, to narrow down what part of a policy allowed it.

The default output (compact) makes it hard for a user to tell the
different event types apart. In particular, it is hard for a user to
distinguish a trace event from a policy verdict one.

This commit modifies the output for policy verdict events in two ways.
First, instead of printing the drop reason (which is redundant as also
printed out with the dropped flow) the string 'policy-verdict' along
with the policy match type string for the event (L3-Only, L3-L4,
L4-Only, all, none), is displayed.
Additionally, flows that are forwarded or redirected are printed as
ALLOWED, those that are dropped or error as DENIED and the audit ones as
AUDITED.

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
@rolinh rolinh force-pushed the pr/rolinh/surface-event-type branch from 3b5627f to 53e2996 Compare June 15, 2022 11:24
@rolinh rolinh requested a review from gandro June 15, 2022 11:26
@rolinh
Copy link
Member Author

rolinh commented Jun 15, 2022

@gandro PTAL, I updated the output to also include the policy match type.

Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks!

@michi-covalent
Copy link
Collaborator

time to ship

@michi-covalent michi-covalent merged commit 4d51e2a into master Jun 15, 2022
@michi-covalent michi-covalent deleted the pr/rolinh/surface-event-type branch June 15, 2022 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⌨️ area/cli Impacts the command line interface of any command in the repository. release-note/minor This PR introduces functionality that users may find relevant to operating Hubble.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Surface the event type in observe -o compact
3 participants