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

docs: Make ICMP rules for the Host Firewall easier to read/search #31900

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

qmonnet
Copy link
Member

@qmonnet qmonnet commented Apr 11, 2024

Two trivial improvements to the Host Firewall guide:

  • Use the explicit name for the EchoRequest in the example policy (instead of "type: 8"), for better readability.
  • Mention "ping" next to ICMP, to make it easier to search for this term in the document.

Fixes: #21986

Two trivial improvements to the Host Firewall guide:

- Use the explicit name for the EchoRequest in the example policy
  (instead of "type 8"), for better readability.
- Mention "ping" next to ICMP, to make it easier to search for this term
  in the document.

Signed-off-by: Quentin Monnet <qmo@qmon.net>
@qmonnet qmonnet added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. area/host-firewall Impacts the host firewall or the host endpoint. labels Apr 11, 2024
@qmonnet qmonnet requested a review from a team as a code owner April 11, 2024 14:12
@qmonnet qmonnet requested a review from learnitall April 11, 2024 14:12
@qmonnet
Copy link
Member Author

qmonnet commented Apr 11, 2024

/test

Copy link
Contributor

@learnitall learnitall left a comment

Choose a reason for hiding this comment

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

Great thank you!

@qmonnet qmonnet added this pull request to the merge queue Apr 11, 2024
Merged via the queue into cilium:main with commit 86cb479 Apr 11, 2024
62 checks passed
@qmonnet qmonnet deleted the pr/docs-hostfw-icmp branch April 11, 2024 21:51
@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 Apr 11, 2024
@Lenikas
Copy link

Lenikas commented May 17, 2024

Hello, @qmonnet

Could you please explain why you changed icmp.fields.type to a string value in the policy example?

According to the official API of CiliumClusterwideNetworkPolicy, the type of this field is integer.

This is confusing when trying to apply this demo policy to the cluster.

Thank you!

@qmonnet
Copy link
Member Author

qmonnet commented May 22, 2024

Hi @Lenikas, thanks for the report. I changed to align this example with what we use elsewhere in some CiliumNetworkPolicy examples, and make it easier to read.

As for the API, my understanding is that since commit 37d969c (merged for v1.16), this field is no longer an integer but an “ICMP-type”, which can be either an integer or the name of an ICMP type. See the relevant version of the API.

Does it answer the question?

@Lenikas
Copy link

Lenikas commented May 22, 2024

@qmonnet
Yes, I did not check the api version and the version used in my cluster when I asked

Thanks for the answer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. area/host-firewall Impacts the host firewall or the host endpoint. 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
Development

Successfully merging this pull request may close these issues.

[docs] Document enabling ping with host firewall
3 participants