Update to the API changes for ClusterNetworkPolicy#303
Conversation
danwinship
left a comment
There was a problem hiding this comment.
need to update go.mod
| } | ||
| } else { | ||
| // If DestinationPort is nil, it means all TCP ports are allowed. | ||
| return true |
There was a problem hiding this comment.
If DestinationPort is nil, then the policy is invalid. In the future, if/when we add something like flags, this clause will still not be correct. So just remove this.
There was a problem hiding this comment.
Missed this comment, let me check that I incorporated.
There was a problem hiding this comment.
I think it makes sense now... if we add flags in the future, there'd be a second
if matchTCPFlags(policyProtocol.TCP.Flags, tcpFlags) {
return true
}here, which would also do "if nil { return true }". And then if either property was filled in, it would get checked, and if it wasn't, then it wouldn't.
849c9b4 to
74e1baf
Compare
74e1baf to
926433a
Compare
| } | ||
| } else { | ||
| // If DestinationPort is nil, it means all TCP ports are allowed. | ||
| return true |
There was a problem hiding this comment.
I think it makes sense now... if we add flags in the future, there'd be a second
if matchTCPFlags(policyProtocol.TCP.Flags, tcpFlags) {
return true
}here, which would also do "if nil { return true }". And then if either property was filled in, it would get checked, and if it wasn't, then it wouldn't.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bowei, danwinship 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 |
|
@bowei you need to update the policies in |
|
@bowei can you finish so we get the CI green? |
|
investigating the failed e2e |
|
it times out on SCTP
is it possible that is related to the protocol and we forget about SCTP and only aadded UDP and TCP? |
|
We were doing some experiments and it seems like the SCTP tests pass when run in isolation. Trying to pin down why it is failing. |
|
It seems like increasing the timeout fixed the tests. |
|
/test |
|
/test pull-kube-network-policies-iptracker |
previously it run in ~12 mins and now it runs in ~21 mins https://github.com/kubernetes-sigs/kube-network-policies/actions/workflows/.github/workflows/npa.yml what have we changed? did we add more tests? checking new https://github.com/kubernetes-sigs/kube-network-policies/actions/runs/22374201653/job/64760904901?pr=303 vs old https://github.com/kubernetes-sigs/kube-network-policies/actions/runs/21652851118/job/62421538562 gemini analysis
Run 1: Standard go test arguments. Run 2: Added --timeout 20m. Note: Without this addition, Run 2 might have reached the default Go test timeout (10m), causing a failure.
Run 1: StatefulSet rollout logs were at helper.go:123. Run 2: StatefulSet rollout logs moved to helper.go:137.
Run 1: Only monitored the gryffindor namespace until ready. Run 2: Included an extra log line for the ravenclaw namespace (1/2 replicas available) before declaring the cluster ready. Result: Run 2 took slightly longer to reach a "Ready" state for the base manifests. |
Sorry forgot to mention it in the PR description. |
|
/lgtm |
|
great |
No description provided.