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

api/hubble: add AUDIT policy verdict #14785

Merged

Conversation

jaffcheng
Copy link
Contributor

@jaffcheng jaffcheng commented Jan 29, 2021

Add a new policy verdict value AUDIT to distinguish whether a packet
is allowed due to the audit mode or because it complies with the security
policy.
Need to bump vendor in hubble repo accordingly.

This is a PR for API change, changes for hubble logic and tests will be in a separate PR once this gets merged and bumped in test VM hubble CLI.

@jaffcheng jaffcheng requested a review from a team as a code owner January 29, 2021 08:02
@jaffcheng jaffcheng requested a review from a team January 29, 2021 08:02
@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 Jan 29, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Jan 29, 2021
@jaffcheng
Copy link
Contributor Author

Runtime tests check for hubble observe output so this requires a newer hubble binary to pass, shall I split this change into two PRs, with one adding enum to flow API and the next one changes hubble output?

@jaffcheng jaffcheng force-pushed the add-audit-verdict-to-hubble-flow-upstream branch from afc07df to 5f11f1d Compare January 29, 2021 13:59
@jaffcheng jaffcheng requested a review from a team as a code owner January 29, 2021 13:59
@jaffcheng jaffcheng requested review from a team and kkourt January 29, 2021 13:59
@aanm aanm added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/hubble Impacts hubble server or relay labels Jan 30, 2021
Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

LGTM, but let's wait to hear from people more familiar with hubble than me :)

Thanks.

@rolinh rolinh removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 3, 2021
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.

Looks straight forward, thanks!

@gandro
Copy link
Member

gandro commented Feb 3, 2021

Runtime tests check for hubble observe output so this requires a newer hubble binary to pass, shall I split this change into two PRs, with one adding enum to flow API and the next one changes hubble output?

I missed this comment before. Yes, I think we first need to get the API change, update the CLI in test VM image, and then update the Hubble logic and the tests.

Add a new policy verdict value `AUDIT` to distinguish whether a packet
is allowed due to the audit mode or because it complies with the security
policy.
Need to bump vendor in hubble repo accordingly.

Signed-off-by: Jaff Cheng <jaff.cheng.sh@gmail.com>
@jaffcheng jaffcheng force-pushed the add-audit-verdict-to-hubble-flow-upstream branch from 5f11f1d to e771be3 Compare February 3, 2021 10:35
@jaffcheng
Copy link
Contributor Author

Removed hubble and test changes from this PR, will send that in a separate PR.

@gandro gandro removed the request for review from glibsm February 3, 2021 11:42
@gandro
Copy link
Member

gandro commented Feb 3, 2021

test-me-please

@glibsm
Copy link
Member

glibsm commented Feb 3, 2021

So in this case AUDIT means would be dropped otherwise?

@jaffcheng
Copy link
Contributor Author

jaffcheng commented Feb 4, 2021

So in this case AUDIT means would be dropped otherwise?

Hi, yes, we already have audit action in cilium monitor -t policy-verdict so I think we should have this in hubble observe as well.
https://docs.cilium.io/en/stable/gettingstarted/policy-creation/#observe-policy-verdicts

@gandro
Copy link
Member

gandro commented Feb 4, 2021

GKE provisioning failed

retest-gke

@gandro
Copy link
Member

gandro commented Feb 4, 2021

Unrelated failure in net-next: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.13-net-next/669/

retest-net-next

@gandro
Copy link
Member

gandro commented Feb 8, 2021

Marking this ready to merge. The API change itself is very small.

@gandro gandro added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 8, 2021
@borkmann borkmann merged commit 178af66 into cilium:master Feb 9, 2021
@jaffcheng jaffcheng deleted the add-audit-verdict-to-hubble-flow-upstream branch February 10, 2021 03:56
rolinh added a commit to cilium/hubble that referenced this pull request Feb 10, 2021
Bump Cilium to latest master to include protobuf definitions for AUDIT
policy verdict (see[0]).

While here, also bump spf13/cobra to v1.1.2 and stretchr/testify to
v1.7.0.

[0]: cilium/cilium#14785

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
gandro pushed a commit to cilium/hubble that referenced this pull request Feb 10, 2021
Bump Cilium to latest master to include protobuf definitions for AUDIT
policy verdict (see[0]).

While here, also bump spf13/cobra to v1.1.2 and stretchr/testify to
v1.7.0.

[0]: cilium/cilium#14785

Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
gandro added a commit to cilium/packer-ci-build that referenced this pull request Feb 10, 2021
This pulls in the lastest master version of the Hubble CLI. This is
needed to test the API changes to embedded Hubble in Cilium master,
specifcially cilium/cilium#14785

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
gandro added a commit to cilium/packer-ci-build that referenced this pull request Feb 10, 2021
This pulls in the lastest master version of the Hubble CLI. This is
needed to test the API changes to embedded Hubble in Cilium master,
specifcially cilium/cilium#14785

The pulled docker image remains at the released 0.7.1 version, as this
is what the Cilium master Dockerfile pulls in.

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
gandro added a commit to cilium/packer-ci-build that referenced this pull request Feb 10, 2021
This pulls in the lastest master version of the Hubble CLI. This is
needed to test the API changes to embedded Hubble in Cilium master,
specifcially cilium/cilium#14785

The pulled docker image remains at the released 0.7.1 version, as this
is what the Cilium master Dockerfile pulls in.

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
gandro added a commit to cilium/packer-ci-build that referenced this pull request Feb 11, 2021
This pulls in the lastest master version of the Hubble CLI. This is
needed to test the API changes to embedded Hubble in Cilium master,
specifcially cilium/cilium#14785

The pulled docker image remains at the released 0.7.1 version, as this
is what the Cilium master Dockerfile pulls in.

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
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
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants