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

option,helm: Add a flag enable-k8s-networkpolicy #23127

Merged
merged 1 commit into from Feb 16, 2023
Merged

option,helm: Add a flag enable-k8s-networkpolicy #23127

merged 1 commit into from Feb 16, 2023

Conversation

ChengyuanLiCY
Copy link
Contributor

@ChengyuanLiCY ChengyuanLiCY commented Jan 17, 2023 •
edited by qmonnet

This flag is for cilium agent to watch K8s NetworkPolicy. By default the value is true. User can set "enable-k8s-networkpolicy = false" to disable it.

option,helm: Add a flag to opt out from support for Kubernetes NetworkPolicy in Cilium

@ChengyuanLiCY ChengyuanLiCY requested review from a team as code owners January 17, 2023 06:07
@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 Jan 17, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jan 17, 2023
@ChengyuanLiCY ChengyuanLiCY force-pushed the k8s-networkpolicy branch 2 times, most recently from baf5c76 to 7c82b37 Compare January 17, 2023 09:24
gandro
gandro reviewed Jan 17, 2023
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks! I am curious, what's the use case for this? Overall I think Helm and flags look fine, I left a bit of feedback regarding the naming and description.

install/kubernetes/cilium/values.yaml Outdated Show resolved Hide resolved
install/kubernetes/cilium/values.yaml Outdated Show resolved Hide resolved
Documentation/cmdref/cilium-agent.md Outdated Show resolved Hide resolved
@ChengyuanLiCY
Copy link
Contributor Author

ChengyuanLiCY commented Jan 18, 2023 •
edited

@gandro Thanks for the review!

I am curious, what's the use case for this?

We are working on installing Cilium on existing kubernetes production clusters, we don't want to break anything in the production, so need to enable Cilium features little by little.

gandro
gandro requested changes Jan 18, 2023
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

The Go linter also failed with a legitimate error:

Error: /home/runner/work/cilium/cilium/src/github.com/cilium/cilium/pkg/k8s/watchers/watcher_test.go:196:3: cannot use &(fakeWatcherConfiguration literal) (value of type *fakeWatcherConfiguration) as WatcherConfiguration value in argument to NewK8sWatcher: *fakeWatcherConfiguration does not implement WatcherConfiguration (missing method K8sNetworkPolicyEnabled)

@gandro
Copy link
Member

gandro commented Jan 18, 2023

Doc lint and Travis failures are also legitimate:

Docs:

Running linter...
Validating documentation (syntax, spelling)...

Please fix the following spelling mistakes:
* Documentation/helm-values.rst:1376: (sNetworkPolicy)  k8sNetworkPolicy
* Documentation/helm-values.rst:1380: (sNetworkPolicy)  k8sNetworkPolicy.enabled
* Documentation/helm-values.rst:1376: (sNetworkPolicy)  k8sNetworkPolicy
* Documentation/helm-values.rst:1380: (sNetworkPolicy)  k8sNetworkPolicy.enabled

If the words are not misspelled, run:
Documentation/update-spelling_wordlist.sh sNetworkPolicy

Travis (this is related to #23127 (comment) )

  CHECK  Documentation cmdref
diff --git a/Documentation/cmdref/cilium-agent.md b/Documentation/cmdref/cilium-agent.md
index dc92b89b..a8afb8d1 100644
--- a/Documentation/cmdref/cilium-agent.md
+++ b/Documentation/cmdref/cilium-agent.md
@@ -114,7 +114,7 @@ cilium-agent [flags]
       --enable-k8s-api-discovery                                Enable discovery of Kubernetes API groups and resources with the discovery API
       --enable-k8s-endpoint-slice                               Enables k8s EndpointSlice feature in Cilium if the k8s cluster supports it (default true)
       --enable-k8s-event-handover                               Enable k8s event handover to kvstore for improved scalability
-      --enable-k8s-networkpolicy                                Enable support for K8s NetworkPolicy
+      --enable-k8s-networkpolicy                                Enable the K8s NetworkPolicy (default true)
       --enable-k8s-terminating-endpoint                         Enable auto-detect of terminating endpoint condition (default true)
       --enable-l2-neigh-discovery                               Enables L2 neighbor discovery used by kube-proxy-replacement and IPsec (default true)
       --enable-l7-proxy                                         Enable L7 proxy for L7 policy enforcement (default true)

HINT: to fix this, run 'make -C Documentation update-cmdref'

@ChengyuanLiCY ChengyuanLiCY requested a review from a team as a code owner January 19, 2023 00:13
@ChengyuanLiCY ChengyuanLiCY requested review from qmonnet and gandro and removed request for ldelossa, tommyp1ckles, qmonnet and thorn3r January 19, 2023 00:13
@gandro
Copy link
Member

gandro commented Jan 23, 2023 •
edited

@ChengyuanLiCY Please do not remove the assignees that were automatically requested to give a review. Those people are assigned for each code owner (e.g. docs, cli, k8s) to also give a review. Only once all code owners have approved this PR will it be merged.

@ChengyuanLiCY ChengyuanLiCY force-pushed the k8s-networkpolicy branch 2 times, most recently from e03f25f to ff01ea5 Compare January 31, 2023 06:56
@ChengyuanLiCY ChengyuanLiCY requested review from qmonnet and removed request for ldelossa, thorn3r and tommyp1ckles February 1, 2023 00:43
qmonnet
qmonnet approved these changes Feb 1, 2023
View reviewed changes
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.

Looks good, thank you!

@tommyp1ckles
Copy link
Contributor

tommyp1ckles commented Feb 1, 2023 •
edited

TravicCI failure looks like this flake: #23314

@qmonnet
Copy link
Member

qmonnet commented Feb 1, 2023 •
edited

@ChengyuanLiCY Please, stop removing review assignees without stating a reason. I see @gandro already asked. (Or is it by accident? But I'd be curious to understand what happens, in that case.)

@qmonnet qmonnet added sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Feb 1, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Feb 1, 2023
@qmonnet
Copy link
Member

qmonnet commented Feb 1, 2023

/test

@ChengyuanLiCY @chengyli
option,helm: Add a flag enable-k8s-networkpolicy
64982b2 
This flag is for Cilium to support K8s NetworkPolicy.
By default the value is true. User can set "enable-k8s-networkpolicy = false" to disable it.

Signed-off-by: Li Chengyuan <chengyuanli@hotmail.com>
pchaigno
pchaigno approved these changes Feb 3, 2023
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Thanks!

@pchaigno

This comment was marked as outdated.

Sign in to view
@maintainer-s-little-helper maintainer-s-little-helper bot mentioned this pull request Feb 4, 2023
Closed
@pchaigno
Copy link
Member

pchaigno commented Feb 16, 2023 •
edited

This CI job is new because we just removed 4.9 and shifted some CI jobs around. See message in Slack #testing channel.
/test-1.16-4.19

@pchaigno
Copy link
Member

k8s-1.16-kernel-4.19 and k8s-1.26-kernel-net-next hit known flake #22056. k8s-1.26-kernel-net-next also hit known flake #23637. Finally, k8s-1.16-kernel-4.19 failed with an existing flake (issue is new, but it's been happening for a while), #23833.

Given the above and the reviews, I'm merging.

@pchaigno pchaigno merged commit 4e08a90 into cilium:master Feb 16, 2023
@ChengyuanLiCY
Copy link
Contributor Author

Thanks @pchaigno !

@doniacld doniacld mentioned this pull request Apr 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@pchaigno pchaigno pchaigno approved these changes

@ldelossa ldelossa ldelossa approved these changes

@qmonnet qmonnet qmonnet approved these changes

@thorn3r thorn3r thorn3r approved these changes

Assignees
No one assigned
Labels
kind/community-contribution This was a contribution made by a community member. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@ChengyuanLiCY @gandro @tommyp1ckles @qmonnet @pchaigno @ldelossa @thorn3r