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

docs: Clarify that empty endpoint selectors implictly limit to namespace #14580

Merged
merged 1 commit into from
Jan 13, 2021
Merged

docs: Clarify that empty endpoint selectors implictly limit to namespace #14580

merged 1 commit into from
Jan 13, 2021

Conversation

twpayne
Copy link
Contributor

@twpayne twpayne commented Jan 11, 2021

As discussed on Slack with @kaworu, @aditighag, and @pchaigno.

@twpayne twpayne 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. labels Jan 11, 2021
@twpayne twpayne requested a review from a team as a code owner January 11, 2021 14:05
@twpayne twpayne requested review from a team, qmonnet and jrajahalme January 11, 2021 14:05
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Jan 11, 2021
@qmonnet qmonnet removed their assignment Jan 11, 2021
aditighag
aditighag requested changes Jan 11, 2021
View reviewed changes
Documentation/policy/language.rst Outdated
@@ -143,8 +143,9 @@ the label ``role=frontend``.
Egress Allow All
~~~~~~~~~~~~~~~~~

An empty `EndpointSelector` will select all endpoints, thus writing a rule that will allow
all egress traffic from an endpoint may be done as follows:
An empty `EndpointSelector` will select all endpoints in the pod's namespace,
Copy link
Member

Choose a reason for hiding this comment

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

in the pod's namespace

Well, technically, this would be the CNP namespace ("default" if not specified).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, PR updated.

@twpayne twpayne force-pushed the pr/twpayne/egress-allow-all-doc branch from 00d29a3 to 7686f26 Compare January 11, 2021 16:11
aditighag
aditighag reviewed Jan 11, 2021
View reviewed changes
Documentation/policy/language.rst Outdated
@@ -143,8 +143,9 @@ the label ``role=frontend``.
Egress Allow All
~~~~~~~~~~~~~~~~~

An empty `EndpointSelector` will select all endpoints, thus writing a rule that will allow
all egress traffic from an endpoint may be done as follows:
An empty `EndpointSelector` will select all endpoints in the pod's
Copy link
Member

@aditighag aditighag Jan 11, 2021 •
edited

Choose a reason for hiding this comment

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

Reads a bit awkward. How about this - "will select all egress endpoints from an endpoint based on the CNP namespace ("default" ...)"?

Suggested change
An empty `EndpointSelector` will select all endpoints in the pod's
An empty `EndpointSelector` will select all egress endpoints from an endpoint based on the `CiliumNetworkPolicy` namespace ("default" by default)

Copy link
Member

Choose a reason for hiding this comment

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

I think the first clause in that sentence is not specific to the following example. That is, EndpointSelector isn't specific to egress :-)

We could break into two sentences if the current form sounds weird.

Copy link
Member

Choose a reason for hiding this comment

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

Since it's listed under the Egress section, further emphasized by the second clause, it doesn't confuse me. So I'll leave the decision to @twpayne.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I've split this into two sentences which hopefully makes it clearer.

@twpayne twpayne force-pushed the pr/twpayne/egress-allow-all-doc branch from 7686f26 to 51deeb1 Compare January 11, 2021 17:19
kaworu
kaworu approved these changes Jan 11, 2021
View reviewed changes
Copy link
Member

@kaworu kaworu 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 for taking care of this!

@twpayne twpayne force-pushed the pr/twpayne/egress-allow-all-doc branch from 51deeb1 to 84de942 Compare January 12, 2021 09:15
@aditighag aditighag added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 13, 2021
@joestringer joestringer merged commit 4dc0a50 into master Jan 13, 2021
@joestringer joestringer deleted the pr/twpayne/egress-allow-all-doc branch January 13, 2021 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno approved these changes

@aditighag aditighag aditighag approved these changes

@kaworu kaworu kaworu approved these changes

@joestringer joestringer joestringer approved these changes

@qmonnet qmonnet qmonnet approved these changes

@jrajahalme jrajahalme Awaiting requested review from jrajahalme

Assignees

@jrajahalme jrajahalme

Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. 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
No open projects
1.10.0
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@twpayne @kaworu @joestringer @pchaigno @qmonnet @aditighag @jrajahalme