Add dual-stack API support for Egress - #8244
Conversation
| return raw | ||
| } | ||
|
|
||
| func mutateEgress(egress *crdv1beta1.Egress, mutate func(*crdv1beta1.Egress)) *crdv1beta1.Egress { |
There was a problem hiding this comment.
This helper just invokes the mutator and returns the object unchanged — it adds indirection without adding value, turning each test case into nested marshal(mutateEgress(newEgress(...), func(e){ ... })) calls. The repo's test style favors explicit builder functions.
Suggest defining a builder for the dual-stack cases instead, e.g.:
func newDualStackEgress(name, pool string, egressIPs []string, ipFamilies []corev1.IPFamily) *crdv1beta1.Egressand call it directly from the test table.
There was a problem hiding this comment.
Thanks for the suggestion, added newDualStackEgress and refactored the implementation.
| - prefixLength | ||
| - required: | ||
| - ipFamilySubnets | ||
| x-kubernetes-validations: |
There was a problem hiding this comment.
The x-kubernetes-validations list indentation is inconsistent within this PR: egress.yaml keeps list items at the same level as the key (matching the existing ippool.yaml style), while externalippool.yaml indents them by 2 extra spaces (see also line 76). Suggest unifying on the same-level style.
| ip net.IP | ||
| } | ||
|
|
||
| func ipFamilyForIP(ip net.IP) corev1.IPFamily { |
There was a problem hiding this comment.
ipFamilyForIP here and ipFamilyForAddress in pkg/controller/validation/ippool.go are identical in logic. Consider consolidating into a single shared helper (e.g. in pkg/controller/validation or pkg/util/ip) so the two sites don't drift apart semantically — for example, when handling IPv4-mapped IPv6 addresses in the future.
There was a problem hiding this comment.
I added IPFamilyForAddress in pkg/util/ip, PTAL.
689aab6 to
b644afa
Compare
|
|
||
| // SubnetInfo specifies subnet attributes for IP Range. | ||
| // SubnetInfo specifies subnet attributes for IP ranges. | ||
| type SubnetInfo struct { |
There was a problem hiding this comment.
Could we add a new struct, and so IPPool wont be impacted?
There was a problem hiding this comment.
Sure, I named the new struct ExternalIPPoolSubnetInfo.
| PrefixLength int32 `json:"prefixLength,omitempty"` | ||
| // IPFamilySubnets specifies subnet attributes by IP family. At most one entry is allowed for each IP family. | ||
| // It cannot be set together with Gateway or PrefixLength. | ||
| IPFamilySubnets []IPFamilySubnetInfo `json:"ipFamilySubnets,omitempty"` |
There was a problem hiding this comment.
I would just call the field Gateways and the struct something like SubnetGateway. I hope you can think of better names though :)
| AppliedTo AppliedTo `json:"appliedTo"` | ||
| // EgressIP specifies the SNAT IP address for the selected workloads. | ||
| // If ExternalIPPool is empty, it must be specified manually. | ||
| // EgressIP is the legacy field that specifies one SNAT IP address for the selected workloads. |
There was a problem hiding this comment.
I would keep the original comment - EgressIP specifies a single SNAT IP address for the selected workloads.
If we want to start deprecating the field, we can add: "// +deprecated:warning="EgressIP is deprecated, use EgressIPs instead."
There was a problem hiding this comment.
EgressIP is still used for single-stack Egress, I have updated related comments.
| ExternalIPPools []string `json:"externalIPPools,omitempty"` | ||
| // IPFamilies specifies the IP families for which Egress IPs should be allocated. At most one entry is allowed for | ||
| // each IP family. It may be omitted for a single-stack ExternalIPPool or when EgressIP or EgressIPs is specified. | ||
| IPFamilies []corev1.IPFamily `json:"ipFamilies,omitempty"` |
There was a problem hiding this comment.
Use IPFamilyPolicy instead? Even for dual-stack IPPool, we should default to dual-stack allocation.
There was a problem hiding this comment.
Make sense to me, I have updated it to IPFamilyPolicy. The default value is PreferDualStack.
73dcef7 to
b5cfbb2
Compare
|
|
||
| // IPFamilyForAddress returns the Kubernetes IP family for the provided address. | ||
| func IPFamilyForAddress(address netip.Addr) corev1.IPFamily { | ||
| if address.Unmap().Is4() { |
There was a problem hiding this comment.
Should IPv4-mapped IPv6 addresses be treated as IPv4 here, or should we reject them during API validation? @jianjuns @luolanzone
| EgressIP string `json:"egressIP,omitempty"` | ||
| // EgressIPs specifies multiple SNAT IP addresses for the selected workloads. | ||
| // Cannot be set with EgressIP. | ||
| // EgressIPs specifies the IPv4 and IPv6 SNAT IP addresses for the selected workloads. It must contain exactly two |
There was a problem hiding this comment.
Can we support single stack allocation in the same version too, even if not in this PR?
There was a problem hiding this comment.
Can we support single stack allocation in the same version too, even if not in this PR?
Maybe we can discuss the egressIP/egressIPs unification and compatibility checks after all dual-stack Egress PRs are merged? For now, I‘d prefer to make sure all legacy single-stack Egress behavior remains consistent.
There was a problem hiding this comment.
We can discuss separately, but I am not saying you must change the existing EgressIP implementation. I am saying we should support single stack allocation with ExternalIPs too. I would avoid changing API behaviors again in the next Antrea version.
There was a problem hiding this comment.
@jianjuns Do you mean supporting single-stack allocation with egressIPs as well? Given Antonin's point about the version bump, I'm now thinking of using spec.egressIPs and status.egressIPs for both single-stack and dual-stack Egresses and removing spec.egressIP and status.egressIP in v1beta2, since the API compatibility concerns would be much less significant. cc @antoninbas @luolanzone @tnqn
| // Cannot be set with ExternalIPPool. | ||
| ExternalIPPools []string `json:"externalIPPools,omitempty"` | ||
| // IPFamilyPolicy specifies whether the Egress is single-stack or dual-stack. It defaults to PreferDualStack. | ||
| IPFamilyPolicy *corev1.IPFamilyPolicy `json:"ipFamilyPolicy,omitempty"` |
There was a problem hiding this comment.
Humm.. seems IPFamilyPolicy cannot specify v4 or v6 for a single stack allocation. Probably let us still use ipFamilies.
Could we by default use the ExternalIPPool's IP families when the filed not set?
I assume this field should be ignored when EgressIP or EgressIPs are specified?
There was a problem hiding this comment.
The explicitly specified EgressIP or EgressIPs determines the actual IP family configuration. However, I don't think IPFamilyPolicy should be silently ignored: we should reject contradictory combinations, such as EgressIP with RequireDualStack or EgressIPs with SingleStack. When no explicit IP is provided, the policy controls automatic allocation from the ExternalIPPool.
For single stack allocation, I think we can still get the IP family info from the externalIPPool or egressIP? For the default value of preferDualStack, a single-stack ExternalIPPool can still determine the IP family based on its own configuration. Do you still think there are any blockers to using IPFamilyPolicy? @jianjuns
There was a problem hiding this comment.
I am fine if you prefer a strict validation to make EgressIP(s) and specified IP stack(s) consistent.
For single stack allocation, I think we can still get the IP family info from the externalIPPool or egressIP? For the default value of preferDualStack, a single-stack ExternalIPPool can still determine the IP family based on its own configuration. Do you still think there are any blockers to using IPFamilyPolicy?
How to indicate I want to allocate a v6 IP from a dual-stack pool?
There was a problem hiding this comment.
How to indicate I want to allocate a v6 IP from a dual-stack pool?
For IPFamilyPolicy in this case, I would let it select the IP family based on the first entry in ipRanges of the dual-stack pool. If you think this is too tricky and we need to support this case explicitly, we probably have to revert to using the ipFamilies field. Could you share more thoughts? @jianjuns
Extend ExternalIPPool subnet configuration to support IPv4 and IPv6 subnets with a shared VLAN. Add Egress IP family selection and explicit dual-stack Egress IPs while preserving the legacy single-stack fields. Add CRD and admission validation for IP family consistency, subnet matching, mutually exclusive representations, and immutable pool IP families. Remove the unused externalIPPools field. Signed-off-by: Shuyang Xin <xin_shuyang@hotmail.com>
b5cfbb2 to
dc3ca6d
Compare
antoninbas
left a comment
There was a problem hiding this comment.
Even if we assume the egressIPs and externalIPPools fields were not technically used, I feel like we have to be a bit more careful with the API change, and I think we should bump up the API version to v1beta2.
Bumping the API version allows us to do the following 2 things, which otherwise we should not do:
- remove the
externalIPPoolsfield - change the validation rules for the
egressIPsfield
And if we introduce a new API version, we have more freedom when it comes to the new ExternalIPPoolSubnetInfo struct. We can drop gateway and prefixLength for example.
It can also be argued that if we don't bump the API version, we must make sure the top-level gateway and prefixLength fields should match gateways[0]: https://github.com/kubernetes/community/blob/main/contributors/devel/sig-architecture/api_changes.md#making-a-singular-field-plural
The conversion webhook from v1beta1 to v1beta2 should be pretty straightforward to implement.
TLDR; I would strongly recommend introducing a new v1beta2 API version, unless someone can make a valid argument against it.
| // If both ExternalIPPool and EgressIP are non-empty, the IP must be in the pool. | ||
| // EgressIP specifies a single SNAT IP address for the selected workloads. It is used only for single-stack Egresses. | ||
| // If ExternalIPPool is not specified, the EgressIP field must be specified manually. If ExternalIPPool is specified, | ||
| // EgressIP field is optional, and an IP will be automatically assigned by Antrea automatically when it is not specified. If both |
There was a problem hiding this comment.
Could you wrap lines at a fixed length?
| // EgressIP specifies a single SNAT IP address for the selected workloads. It is used only for single-stack Egresses. | ||
| // If ExternalIPPool is not specified, the EgressIP field must be specified manually. If ExternalIPPool is specified, | ||
| // EgressIP field is optional, and an IP will be automatically assigned by Antrea automatically when it is not specified. If both | ||
| // ExternalIPPool and EgressIP are specified, the IP must be in the pool. |
There was a problem hiding this comment.
Probably move EgressIP to be before ExternalIPPool -> "EgressIP and ExternalIPPool are.."
| EgressIP string `json:"egressIP,omitempty"` | ||
| // EgressIPs specifies multiple SNAT IP addresses for the selected workloads. | ||
| // Cannot be set with EgressIP. | ||
| // EgressIPs specifies the IPv4 and IPv6 SNAT IP addresses for the selected workloads. It must contain exactly two |
There was a problem hiding this comment.
We can discuss separately, but I am not saying you must change the existing EgressIP implementation. I am saying we should support single stack allocation with ExternalIPs too. I would avoid changing API behaviors again in the next Antrea version.
| // If it is non-empty, the EgressIP will be assigned to a Node specified by the pool automatically and will failover | ||
| // to a different Node when the Node becomes unreachable. | ||
| // ExternalIPPool specifies the IP Pool that the EgressIP(s) should be allocated from. If it is not set, the specified EgressIP(s) | ||
| // should be assigned to a Node manually. If it is specified, the EgressIP(s) will be assigned to a Node specified by the pool |
There was a problem hiding this comment.
If ExternalIPPool is provided
| // If it is empty, the specified EgressIP must be assigned to a Node manually. | ||
| // If it is non-empty, the EgressIP will be assigned to a Node specified by the pool automatically and will failover | ||
| // to a different Node when the Node becomes unreachable. | ||
| // ExternalIPPool specifies the IP Pool that the EgressIP(s) should be allocated from. If it is not set, the specified EgressIP(s) |
| // If both ExternalIPPool and EgressIP are non-empty, the IP must be in the pool. | ||
| // EgressIP specifies a single SNAT IP address for the selected workloads. It is used only for single-stack Egresses. | ||
| // If ExternalIPPool is not specified, the EgressIP field must be specified manually. If ExternalIPPool is specified, | ||
| // EgressIP field is optional, and an IP will be automatically assigned by Antrea automatically when it is not specified. If both |
There was a problem hiding this comment.
EgressIP field -> the EgressIP field
| // Cannot be set with ExternalIPPool. | ||
| ExternalIPPools []string `json:"externalIPPools,omitempty"` | ||
| // IPFamilyPolicy specifies whether the Egress is single-stack or dual-stack. It defaults to PreferDualStack. | ||
| IPFamilyPolicy *corev1.IPFamilyPolicy `json:"ipFamilyPolicy,omitempty"` |
There was a problem hiding this comment.
I am fine if you prefer a strict validation to make EgressIP(s) and specified IP stack(s) consistent.
For single stack allocation, I think we can still get the IP family info from the externalIPPool or egressIP? For the default value of preferDualStack, a single-stack ExternalIPPool can still determine the IP family based on its own configuration. Do you still think there are any blockers to using IPFamilyPolicy?
How to indicate I want to allocate a v6 IP from a dual-stack pool?
So, in which Antrea version |
|
Closed by mistake... |
@jianjuns AFAIK, it was first introduced in this PR three years ago: #4603 |
Extend ExternalIPPool subnet configuration to support IPv4 and IPv6 subnets with a shared VLAN. Add Egress IP family selection and explicit dual-stack Egress IPs while preserving the legacy single-stack fields.
Add CRD and admission validation for IP family consistency, subnet matching, mutually exclusive representations, and immutable pool IP families. Remove the unused externalIPPools field.