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.

Sign up for GitHub

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

Jump to bottom

bpf: Move to-stack trace after host firewall #18484

Merged
merged 1 commit into from
Jan 21, 2022
Merged

bpf: Move to-stack trace after host firewall #18484

merged 1 commit into from
Jan 21, 2022

Conversation

YutaroHayakawa
Copy link
Member

@YutaroHayakawa YutaroHayakawa commented Jan 14, 2022 •
edited

When host firewall feature is enable, to-stack trace can be generated
even if the packets are rejected by host firewall policies. Generate trace
only when the packets are actually delivered to the stack.

Fixes: #12562
Signed-off-by: Yutaro Hayakawa yutaro.hayakawa@isovalent.com

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!
Fix bug where Hubble flows report that a packet is both forwarded and dropped by host firewall. It will now only report the drop.

@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 14, 2022
@YutaroHayakawa
Copy link
Member Author

YutaroHayakawa commented Jan 17, 2022 •
edited

Looks like k8s-1.22-kernel-4.19 (test-1.22-4.19) , k8s-1.23-kernel-net-next (test-1.23-net-next) and runtime (test-runtime) tests are failing. I think this is related to #18493. I'll rebase after it is merged. Otherwise, I think this PR is ready for review.

@YutaroHayakawa YutaroHayakawa marked this pull request as ready for review January 17, 2022 08:44
@YutaroHayakawa YutaroHayakawa requested review from a team and joamaki January 17, 2022 08:44
@pchaigno pchaigno added area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Jan 17, 2022
@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 Jan 17, 2022
@pchaigno pchaigno added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. needs-backport/1.11 labels Jan 17, 2022
@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 Jan 17, 2022
@YutaroHayakawa
Copy link
Member Author

Rebased the branch based on the latest master since #18493 is merged now.

@YutaroHayakawa
Copy link
Member Author

YutaroHayakawa commented Jan 17, 2022 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-Runtime-net-next' hit: #18497 (97.23% similarity)

Job 'Cilium-PR-K8s-1.22-kernel-4.19' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sServicesTest Checks graceful termination of service endpoints Checks client terminates gracefully on service endpoint deletion

Failure Output

FAIL: Timed out after 60.001s.

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.22-kernel-4.19 so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.23-kernel-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sPolicyTest Multi-node policy test validates fromEntities policies Validates fromEntities all policy

Failure Output

FAIL: Can not connect to service "10.0.0.47" from outside cluster (1/1)

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.23-kernel-net-next so I can create a new GitHub issue to track it.

@maintainer-s-little-helper maintainer-s-little-helper bot mentioned this pull request Jan 17, 2022
Closed
@YutaroHayakawa
Copy link
Member Author

YutaroHayakawa commented Jan 19, 2022 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-Runtime-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

RuntimeConntrackInVethModeTest Conntrack-related configuration options for endpoints

Failure Output

FAIL: Cannot disable ConnTrack for the endpoint "2807"

If it is a flake, comment /mlh new-flake Cilium-PR-Runtime-net-next so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sKafkaPolicyTest Kafka Policy Tests KafkaPolicies

Failure Output

FAIL: Kubernetes DNS did not become ready in time

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.23-kernel-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sPolicyTest Multi-node policy test validates fromEntities policies Validates fromEntities all policy

Failure Output

FAIL: Can not connect to service "10.0.0.108" from outside cluster (1/1)

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.23-kernel-net-next so I can create a new GitHub issue to track it.

@YutaroHayakawa YutaroHayakawa mentioned this pull request Jan 20, 2022
Closed
2 tasks
@YutaroHayakawa
Copy link
Member Author

Failure in K8sPolicyTest Multi-node policy test validates fromEntities policies Validates fromEntities all policy seems similar to #18544. I'll rebase.

@YutaroHayakawa
bpf: Move to-stack trace after host firewall
2cad225 
When host firewall feature is enable, to-stack trace can be generated
even if the packets are rejected by host firewall policies. Generate trace
only when the packets are actually delivered to the stack.

Fixes: cilium#12562
Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@pchaigno
Copy link
Member

pchaigno commented Jan 20, 2022 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.23-kernel-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: failed to ensure kubectl version: failed to run kubectl version

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.23-kernel-net-next so I can create a new GitHub issue to track it.

@YutaroHayakawa
Copy link
Member Author

/test-1.23-net-next

@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 Jan 20, 2022
@joestringer
Copy link
Member

One minor, nit, I would try to create a user-facing release-note in the PR description, something like this should communicate the implications of the change correctly:

```release-note
Fix bug where Hubble flows report that a packet is both forwarded and dropped by host firewall. It will now only report the drop.
```

@kkourt kkourt merged commit ffe59b3 into cilium:master Jan 21, 2022
@kkourt kkourt mentioned this pull request Jan 26, 2022
Merged
@glibsm glibsm added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Jan 30, 2022
@joestringer joestringer mentioned this pull request Feb 23, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joamaki joamaki joamaki approved these changes

@joestringer joestringer joestringer approved these changes

@pchaigno pchaigno pchaigno approved these changes

Assignees
No one assigned
Labels
area/monitor Impacts monitoring, access logging, flow logging, visibility of datapath traffic. backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bpf_host "to-host" prog sends TRACE_TO_STACK prematurely with host firewall enabled
6 participants
@YutaroHayakawa @pchaigno @joestringer @joamaki @glibsm @kkourt