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

feat/policy-names: Add network policy names when they are known. #727

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kgtw
Copy link

@kgtw kgtw commented Nov 26, 2023

Introduces new Egress allowed by policies and Ingress allowed by policies info blocks within the the respective flows detailed sidebar information.

Policy names are only shown when they have successfully been correlated to a flow by cilium-agent. In a handful of known scenarios where cilium is allowing traffic internally (like allowing localhost access) we try to map the policy to a human friendly name with the value taken from the reserved:io.cilium.policy.derived-from label.

Fixes: cilium/hubble#1100

Example screenshot demonstrating a custom ingress policy, as well as showing the cilium internal policy which is prefixed with <cilium-internal>/.

Screenshot 2023-11-27 at 00 31 13

Signed-off-by: Kris Gambirazzi <[email protected]>
Signed-off-by: Kris Gambirazzi <[email protected]>
@kgtw kgtw requested a review from a team as a code owner November 26, 2023 13:46
@kgtw kgtw requested review from yandzee and removed request for a team November 26, 2023 13:46
@kimstacy kimstacy self-requested a review December 7, 2023 17:06
@kimstacy
Copy link
Member

kimstacy commented Dec 9, 2023

@kgtw Thank you for your contribution! Will you please provide the custom policies used to test this feature?

@kgtw
Copy link
Author

kgtw commented Dec 10, 2023

Hey @kimstacy, these were the policies that were used to test this change with.

---
apiVersion: cilium.io/v2
kind: CiliumNetworkPolicy
metadata:
  name: custom-ingress
  namespace: default
spec:
  endpointSelector: {}
  ingress:
    - fromEntities:
      - world
      - cluster

---
apiVersion: cilium.io/v2
kind: CiliumNetworkPolicy
metadata:
  name: custom-egress
  namespace: default
spec:
  endpointSelector: {}
  egress:
    - toEntities:
      - world
      - cluster

---
apiVersion: cilium.io/v2
kind: CiliumClusterwideNetworkPolicy
metadata:
  name: cluster-wide-custom-policy
spec:
  endpointSelector: {}
  egress:
    - toEntities:
      - all
  ingress:
    - fromEntities:
      - all

@kimstacy
Copy link
Member

Thank you! LGTM.

@kimstacy
Copy link
Member

kimstacy commented Dec 15, 2023

Hi @kgtw 👋

There seems to be a minor CI issue. The first relates to CodeQL:

2023/12/13 08:09:18 Error running go tooling: err: exit status 1: stderr: go: errors parsing go.mod:
/home/runner/work/hubble-ui/hubble-ui/backend/go.mod:3: invalid go version '1.21.0': must match format 1.23
/home/runner/work/hubble-ui/hubble-ui/backend/go.mod:5: unknown directive: toolchain
  
2023/12/13 08:09:18 Extraction failed: exit status 1
Error: We were unable to automatically build your code. Please replace the call to the autobuild action with your custom build steps. Encountered a fatal error while running "/opt/hostedtoolcache/CodeQL/2.15.4/x64/codeql/go/tools/autobuild.sh". Exit code was 1 and last log line was: 2023/12/13 08:09:18 Extraction failed: exit status 1. See the logs for more details.

For the second, per test / backend check's output, would you please try running ./ctl.sh update-proto and resubmitting your changes?

…e known and correlated to flows.

Fixes: cilium/hubble#1100
Signed-off-by: Kris Gambirazzi <[email protected]>
@kgtw
Copy link
Author

kgtw commented Dec 18, 2023

Hi @kimstacy, I'm hoping the latest commit should have resolved the issues with the CI.

I'm not entirely sure why it is happening, but when using ./ctl.sh update-proto with the current go.mod version of 1.21 it was adding the problematic toolchain go1.21.4 line.

@geakstr
Copy link
Collaborator

geakstr commented Dec 20, 2023

Hi @kgtw, thanks for your contribution! There was some strange issue with go version and dependencies indeed, so I updated them and pushed the commit. CI is happy now but there are some merge conflicts. Could you rebase and resolve them? I think we are good to go after that.

@geakstr geakstr mentioned this pull request Jan 24, 2024
@mbounaceur
Copy link

Hi, this feature is very useful. I was looking for it

Any update please @kgtw ?

Thanks !

@geakstr geakstr self-assigned this Apr 2, 2024
@kgtw
Copy link
Author

kgtw commented Apr 5, 2024

Apologies for the delay, I've just returned from a sabbatical from work and will be looking to update this PR in the coming days.

kgtw added 2 commits April 6, 2024 21:40
Signed-off-by: Kris Gambirazzi <[email protected]>
Signed-off-by: Kris Gambirazzi <[email protected]>
@kgtw
Copy link
Author

kgtw commented Apr 6, 2024

@geakstr this should be ready for another review now. Sorry for the slow response times.

Something I did notice while re-testing PR is that policy correlation is only happening on the first SYN packet for the connection. I haven't had time to investigate if this is a bug or expected within cilium-agent.

Update 15/04: The above makes sense as policy evaluation is a costly action. I'm wondering if we should make additional UX improvements to help identify the initial packet flow that was associated with the policy in a way which doesn't lead to more confusion.

Copy link
Contributor

@yannikmesserli yannikmesserli left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good. I've left suggestion to improve the code, e.g. div instead span, but I'll approuve this PR as this isn't substantial changes IMHO. Up to you if you want to adopt them.

Something I did notice while re-testing PR is that policy correlation is only happening on the first SYN packet for the connection

Yes, we do populate the policy field only on verdict events. @gandro implemented originally this feature, he can shed some lights about the reason behind it.

I don't think it would be sane that UI try fix this somehow, so maybe we can either open a feature on the Cilium repo, or document this somehow? WDYT @geakstr @kgtw ?

@@ -242,3 +242,29 @@ export const PodEntry = memo<PodItemProps>(function FlowsTableSidebarPodEntry(pr
</span>
);
});

export interface PolicyEntryProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export interface PolicyEntryProps {
export interface PoliciesListProps {

allowedBy: string[];
}

export const PolicyEntry = memo<PolicyEntryProps>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export const PolicyEntry = memo<PolicyEntryProps>(
export const PoliciesList = memo<PoliciesListProps>(

name: string;
}

export const PolicyEntryItem = memo<PolicyEntryItemProps>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export const PolicyEntryItem = memo<PolicyEntryItemProps>(
export const PoliciesListItem = memo<PoliciesListItemProps>(

},
);

export interface PolicyEntryItemProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export interface PolicyEntryItemProps {
export interface PoliciesListItemProps {


export const PolicyEntryItem = memo<PolicyEntryItemProps>(
function FlowsTableSidebarPolicyEntry(props) {
return <span className={css.policy}>{props.name}</span>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return <span className={css.policy}>{props.name}</span>;
return <div>{props.name}</div>;

Comment on lines +240 to +245
.policies {
.policy {
display: block;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.policies {
.policy {
display: block;
}
}

Let's use a div, so no need of this.

export const PolicyEntry = memo<PolicyEntryProps>(
function FlowsTableSidebarLabelsEntry(props) {
return (
<div className={css.policies}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div className={css.policies}>
<div>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Add network policy names in hubble
5 participants