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(k3s): add back the flag to disable network policies #16755

Merged
merged 1 commit into from
Jul 13, 2021

Conversation

rio
Copy link
Contributor

@rio rio commented Jul 2, 2021

This was actually added in #13783 but got removed again after some other changes.
That flag is required because otherwise any liveness or readiness probes will be classified as world traffic and get
dropped if network policies are installed.

Signed-off-by: Rio Kierkels riokierkels@gmail.com

This was actually added in cilium#13783 but got removed again after some other changes.
That flag is required because otherwise any liveness or readiness probes will be classified as *world* traffic and get
dropped if network policies are installed.

Signed-off-by: Rio Kierkels <riokierkels@gmail.com>
@rio rio requested a review from a team as a code owner July 2, 2021 09:47
@rio rio requested a review from qmonnet July 2, 2021 09:47
@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 Jul 2, 2021
@qmonnet
Copy link
Member

qmonnet commented Jul 2, 2021

The commit that removed this flag described it as “unrecognized”? #15503
@aanm do you have more context?

@rio
Copy link
Contributor Author

rio commented Jul 2, 2021

I just now notice that it's removing it from the agent and it was never set for the server. In that case that commit is correct because the agent doesn't support that flag only the server does.

@pchaigno pchaigno 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 Jul 2, 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 Jul 2, 2021
@aanm
Copy link
Member

aanm commented Jul 3, 2021

@qmonnet yes, I just copy-paste the original PR from #15191

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Ok, if this is a matter of passing the option to the correct entity then fine by me.

/Cc @seanmwinn for a quick review, as he is more familiar with the topic.

@seanmwinn
Copy link
Contributor

I just now notice that it's removing it from the agent and it was never set for the server. In that case that commit is correct because the agent doesn't support that flag only the server does.

Ok, if this is a matter of passing the option to the correct entity then fine by me.

/Cc @seanmwinn for a quick review, as he is more familiar with the topic.

I'm not particularly familiar with this topic as many of the flags for k3s have changed since we initially published a k3s getting started guide. I would need to test this to validate the reported behavior as well as the fix.

@qmonnet
Copy link
Member

qmonnet commented Jul 12, 2021

@seanmwinn Thanks, I thought maybe you knew about that option. If you need to run tests but don't have cycles for this at the moment, I think we should just go ahead and merge the PR. After all, this is simply a documentation change adding one option back.

I'll mark the PR as ready-to-merge. Thanks!

@qmonnet qmonnet added needs-backport/1.10 ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Jul 12, 2021
@kkourt kkourt merged commit 7bd301b into cilium:master Jul 13, 2021
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. 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.

None yet

6 participants