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

policy: Add a bpf compiling option when enable-icmp-rules flag is set #17620

Merged
merged 1 commit into from Oct 22, 2021
Merged

policy: Add a bpf compiling option when enable-icmp-rules flag is set #17620

merged 1 commit into from Oct 22, 2021

Conversation

chez-shanpu
Copy link
Contributor

By this commit, bpf program is compiled with -DENABLE_ICMP_RULE option when enable-icmp-rules flag is set.

Signed-off-by: Tomoki Sugiura tomoki.sugiura@mail.shanpu.info

@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 Oct 15, 2021
@chez-shanpu chez-shanpu marked this pull request as ready for review October 15, 2021 15:33
@chez-shanpu chez-shanpu requested a review from a team as a code owner October 15, 2021 15:33
@chez-shanpu chez-shanpu changed the title policy: Fix bpf compiling options when enable-icmp-rules flag set policy: Fix bpf compiling options when enable-icmp-rules flag is set Oct 15, 2021
@chez-shanpu chez-shanpu changed the title policy: Fix bpf compiling options when enable-icmp-rules flag is set policy: Add a bpf compiling option when enable-icmp-rules flag is set Oct 15, 2021
@chez-shanpu chez-shanpu changed the title policy: Add a bpf compiling option when enable-icmp-rules flag is set policy: Add a bpf compiling option when enable-icmp-rules flag is set Oct 15, 2021
joestringer
joestringer requested changes Oct 19, 2021
View reviewed changes
Copy link
Member

@joestringer joestringer 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 the fix. Just one comment below, usually we we define these in the headerfiles instead of passing additional arguments directly to the compiler. Would be good to keep that consistent.

pkg/datapath/loader/compile.go Outdated Show resolved Hide resolved
joestringer
joestringer approved these changes Oct 20, 2021
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@joestringer
Copy link
Member

I'm seeing Image CI builds fail, example: https://github.com/cilium/cilium/actions/runs/1362491757

These errors do not look related to the PR. I'll close/reopen the PR to see if that retriggers the CI correctly.

@joestringer joestringer reopened this Oct 20, 2021
@joestringer
Copy link
Member

Looks like the github image build actions are still failing. I've raised a thread on Slack to follow up on this, since it still doesn't look related to your PR.

@joestringer
Copy link
Member

@chez-shanpu can you rebase your PR against the latest master tree? It looks like this is what is causing the image build failures.

@joestringer joestringer added dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. release-note/misc This PR makes changes that have no direct user impact. labels Oct 20, 2021
@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 Oct 20, 2021
@joestringer
Copy link
Member

Oh, one more thing - Does this mean that we don't currently have tests for this functionality? Otherwise we could have maybe found this out earlier.

@chez-shanpu
policy: Add a bpf compiling option when enable-icmp-rules flag is set
ddbab5c 
By this commit, bpf program is compiled with `-DENABLE_ICMP_RULE` option when `enable-icmp-rules` flag is set.

Signed-off-by: Tomoki Sugiura <tomoki.sugiura@mail.shanpu.info>
@chez-shanpu
Copy link
Contributor Author

Does this mean that we don't currently have tests for this functionality?

Yes.
When this ICMP rule is enabled with some options, eBPF verification CI fails because of kernel complexity issue. Therefore, I added e2e test at #17135, and I was going to merge it after this complexity issue is solved and ICMP rule is implemented totally.
But, I didn't try enabling this function in a e2e script and it's worth a try.
I'll do it :)

@joestringer
Copy link
Member

Given the code is not covered by CI, I think it's fine to merge this as-is. I would welcome followup to add coverage for this feature to CI somehow.

@joestringer joestringer added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 21, 2021
@chez-shanpu
Copy link
Contributor Author

Okay, I would open another PR (or use existing PR #17135) when I add an e2e test for this function.

@kkourt kkourt merged commit 1efd3c5 into cilium:master Oct 22, 2021
@chez-shanpu chez-shanpu deleted the fix-icmp-rule-bpf-compile branch October 22, 2021 06:37
@kkourt kkourt mentioned this pull request Oct 22, 2021
Merged
alexminder added a commit to rbkmoney/salt-common that referenced this pull request Jan 11, 2022
@alexminder
k8s: cilium: turn on "enable-icmp-rules" daemon flag
81efbde 
cilium/cilium#17620
cilium/cilium#16516
@alexminder alexminder mentioned this pull request Jan 11, 2022
Merged
alexminder added a commit to rbkmoney/salt-common that referenced this pull request Jan 11, 2022
@alexminder
k8s: cilium: turn on "enable-icmp-rules" daemon flag (#199)
9ded7fc 
cilium/cilium#17620
cilium/cilium#16516
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@ti-mo ti-mo ti-mo approved these changes

Assignees
No one assigned
Labels
dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@chez-shanpu @joestringer @ti-mo @kkourt