-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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: document missing entity 'ingress' #25665
docs: document missing entity 'ingress' #25665
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also needs to be backported to v1.12, but noting that the ingress
entity only applies to cluster external traffic in v1.12.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mhofstetter Good work. Some nits to fix, otherwise LGTM. I'm approving with the understanding that changes are required prior to merge.
The ingress entity represents the Cilium Envoy instance which handles ingress | ||
L7 traffic. Be aware that this also applies for pod-to-pod traffic within | ||
the same cluster when using ingress endpoints (hairpinning). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deep into the nitty weeds: s/which/that, some small clarification, italics when introducing a new term
The ingress entity represents the Cilium Envoy instance which handles ingress | |
L7 traffic. Be aware that this also applies for pod-to-pod traffic within | |
the same cluster when using ingress endpoints (hairpinning). | |
The ingress entity represents the Cilium Envoy instance that handles ingress | |
L7 traffic. Be aware that this also applies for pod-to-pod traffic within | |
the same cluster when using ingress endpoints (also known as *hairpinning*). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handy guidance for "which" vs. "that":
If your sentence has a clause but does not need it, use “which”; if the sentence does need the clause, use “that.”
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zacharysarah thanks for your review - i adapted your recommendations. special thanks for the which/that guidance - definitely an important one for me. will try to remember next time! 🧠
The entity `ingress` is missing from the list of pre-defined entities which are available when defining policies which `fromEntities` and `toEntities`. This commits fixes this. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
255d98f
to
2130a57
Compare
change in the documentation doesn't require running the full ci test suite -> adding label |
The entity
ingress
is missing from the list of pre-defined entities which are available when defining policies whichfromEntities
andtoEntities
.This PR documents the missing entity.
❗ backport info 1.12: note that the ingress entity only applies to cluster external traffic