v0.1.6-api review #260
v0.1.6-api review #260
Conversation
Signed-off-by: astoycos <[email protected]>
k8s-ci-robot
added
the
kind/api-change
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: astoycos The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
astoycos
added
do-not-merge/hold
k8s-ci-robot
added
the
size/L
|
@astoycos: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Unresolved discussions from #235 included We would like to get this release cut prior to cutting an official v0.2.0 👍 |
|
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-contributor-experience at kubernetes/community. /check-cla |
k8s-ci-robot
added
the
cncf-cla: yes
| // +patchStrategy=merge | ||
| // +listType=map | ||
| // +listMapKey=type | ||
| Conditions []metav1.Condition `json:"conditions" patchStrategy:"merge" patchMergeKey:"type"` |
There was a problem hiding this comment.
kubebuilder metadata copied from the comments on metav1.Condition
| // +optional | ||
| PortNumber *Port `json:"portNumber,omitempty"` | ||
|
|
||
| // NamedPort selects a port on a pod(s) based on name. | ||
| // | ||
| // Support: Extended |
There was a problem hiding this comment.
OK, apparently we still have "Extended". @tssurya ?
We should not have NamedPort support be optional. Either it's required, or it's out. (Note that if it's out then that makes it harder to write a generic "allow to DNS" policy.)
| // Exactly one of the selector pointers must be set for a given peer. If a | ||
| // consumer observes none of its fields are set, they must assume an unknown | ||
| // option has been specified and fail closed. |
There was a problem hiding this comment.
(we should try to get #252 in)
| Namespaces *NamespacedPeer `json:"namespaces,omitempty"` | ||
| Namespaces *metav1.LabelSelector `json:"namespaces,omitempty"` |
There was a problem hiding this comment.
We dropped SameLabels / NotSameLabels because we decided against that model of tenancy, so there was now only one kind of Namespace peer, so the subtype seemed superfluous.
| // To be selected a Namespace must have all of the labels defined in NotSameLabels, | ||
| // AND at least one of them must have different values than the subject of this policy. | ||
| // If NotSameLabels is empty then nothing is selected. | ||
| Nodes *metav1.LabelSelector `json:"nodes,omitempty"` |
There was a problem hiding this comment.
ok, the diff is ugly here, but the API change is the addition of Nodes to AdminNetworkPolicyEgressPeer.
The docs are inadequate; what does selecting Nodes mean? In particular, we had requests in the past for both "select the IPs of nodes" and "select all pod-network pods on a node".
I believe it means the former, but what exactly does "the IPs of nodes" mean? The primary IPs? All NodeAddresses? All IPs assigned to an interface in the host network namespace on the node, whether or not they are listed in any k8s.io API object?
| // | ||
| // Networks can have upto 25 CIDRs specified. | ||
| // | ||
| // Support: Extended |
There was a problem hiding this comment.
More "Extended". I think this was supposed to be "Experimental", because it's expected to eventually be Core but it's not widely implemented enough yet?
| } | ||
| // CIDR is an IP address range in CIDR notation (for example, "10.0.0.0/8" or "fd00::/8"). | ||
| // This string must be validated by implementations using net.ParseCIDR | ||
| // TODO: Introduce CEL CIDR validation regex isCIDR() in Kube 1.31 when it is available. |
There was a problem hiding this comment.
We can assume that now can't we?
What type of PR is this?
/kind api-change
Carry Over from #235 which closed and would not let me re-open, probably due to email change 😢 🤷 (Sorry about that)
What this PR does / why we need it:
This PR is a diff of /apis from v0.1.6 (main branch) to v0.1.1 (release-v0.1.1 branch).
Note: This PR is purely to facilitate review, it is not intended to merge.
Note2: This does not include FQDN support from from @rahulkjoshi in #233, which will come in our next 0.2.0 release
More specifially it includes api changes up until 8be8ff5