Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
| flag.BoolVar(&adminNetworkPolicy, "admin-network-policy", false, "If set, enable Admin Network Policies (default false)") | ||
| flag.BoolVar(&baselineAdminNetworkPolicy, "baseline-admin-network-policy", false, "If set, enable Baseline Admin Network Policies (default false)") | ||
| flag.BoolVar(&disableNRI, "disable-nri", false, "If set, disable the NRI functionality to get Pod IP information from the container runtime directly (default false)") | ||
| flag.BoolVar(&networkpolicies, "network-policy", true, "If set, enable Network Policy GA APIs (default true)") |
There was a problem hiding this comment.
So is the idea here that kindnet will not have built-in support for ANP/CNP until it becomes more stable, but people can run kindnetd with --network-policy=false and then run k-n-p separately if they want kindnet+ANP?
There was a problem hiding this comment.
yeah,
I think that kube-network-policies deployed as addon for the bleeding edge and kindnet to be the downstream more stable is a good combo and it seems less disruptive, WDYT?
There was a problem hiding this comment.
to be cler, I do not have strong opinion, so feel free to suggest changes, is just that keep adding flags for admin-network-policy and CNP didn't make sense in the update ... removing the flag later is a breaking change
There was a problem hiding this comment.
that makes sense; I just wanted to make sure I understood why you were removing it
|
|
||
| // Logging evaluator must go first if enabled. | ||
| if klog.V(2).Enabled() { | ||
| evaluators = append(evaluators, networkpolicy.NewLoggingPolicy()) |
There was a problem hiding this comment.
This should not be hardcoding the fact that networkpolicy.LoggingPolicy uses V(2). You should either make it so that networkpolicy.LoggingPolicy always logs the packets if you create the policy (so the caller alone decides what verbosity level causes packet logging to be enabled), or else make it so that networkpolicy.NewLoggingPolicy() returns a no-op policy if the verbosity level wasn't high enough (so LoggingPolicy alone decides what verbosity level causes packet logging to be enabled).
(This requires a change to k-n-p so it doesn't have to be fixed in this PR.)
There was a problem hiding this comment.
This should not be hardcoding the fact that
networkpolicy.LoggingPolicyusesV(2)
that is an oversight,
ou should either make it so that networkpolicy.LoggingPolicy always logs the packets
that was the idea, we need to fix it
There was a problem hiding this comment.
The intention is that the consumer uses the logging handler at the log level they please, by inserting it in the pipeline. Hence, the logging handler should not make any assumption about verbosity levels Spotted by @danwinship in kubernetes-sigs/kindnet#11 (comment)
|
/lgtm |
k8s-ci-robot
added
the
lgtm
This require additional changes: - bump golang version on docker image - remove anp flags to adapt to the new kube-network-policies architecture We only use stable Network Policies APIs that can be enabled or disabled via a flag, so users can deploy kube-network-policies to have the different Network Policies we are developing in the subproject.
k8s-ci-robot
removed
the
lgtm
|
New changes are detected. LGTM label has been removed. |
squashed and fixed teh hack/ci/script |
3 participants
Uh oh!
There was an error while loading. Please reload this page.