NE-2021: Support dual-stack IngressController on AWS#1940
NE-2021: Support dual-stack IngressController on AWS#1940alebedev87 wants to merge 2 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@alebedev87: This pull request references NE-2021 which is a valid jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
alebedev87
force-pushed
the
NE-2021-dual-stack-support-ingresscontroller
branch
2 times, most recently
from
c178e30 to
2d2a626
Compare
This enhancement enables automatic dual-stack (IPv4 and IPv6) IP address types for IngressController publishing services on AWS clusters using Network Load Balancers (NLB). This is a day-0 feature that automatically configures itself based on the cluster-wide cluster's IP family configuration
alebedev87
force-pushed
the
NE-2021-dual-stack-support-ingresscontroller
branch
from
2d2a626 to
61dffff
Compare
|
/retitle NE-2021: Support dual-stack IngressController on AWS |
openshift-ci
bot
changed the title
|
/assign |
|
|
||
| AWS NLBs support dual-stack IP address type, allowing | ||
| services to be accessible via both IPv4 and IPv6 addresses. However, | ||
| OpenShift Ingress does not currently support this capability. As IPv6 adoption increases and organizations require |
There was a problem hiding this comment.
"OpenShift Ingress" is ambiguous. Does this cover our gateway controller? I suggest using a more specific term or phrasing, such as "OpenShift does not support dual-stack NLBs with the IngressController API", and adding a non-goal if this does not cover our gateway controller.
|
/cc |
sadasu
left a comment
sadasu
left a comment
There was a problem hiding this comment.
Thanks for this detailing Ingress controller considerations for Day-0 dualstack support. A few comments inline.
| - For `DualStackIPv6Primary`: set `service.spec.ipFamilies: ["IPv6", "IPv4"]` | ||
| and `service.spec.ipFamilyPolicy: PreferDualStack` | ||
| - The AWS cloud provider (cloud-provider-aws) will read these service | ||
| fields and configure the NLB with dual-stack support accordingly. |
There was a problem hiding this comment.
Although, we are not making any updates to the Ingress API, I found this comment to be useful.
Looking at the documentation for dualstack configuration on k8s services (https://kubernetes.io/docs/concepts/services-networking/dual-stack/#services), since the the 2nd IP Family in service.spec.ipFamilies is mutable, should we be setting service.spec.ipFamilyPolicy to RequireDualStack so that we don't end up in a situation with ["IPv6", "IPv4"] where the 2nd IP Family "IPv4" is removed and we are left with single stack IPv6 configured on the LB service for the NLB.
There was a problem hiding this comment.
Although, we are not making any openshift/api#2663 to the Ingress API, I found openshift/api#2663 (comment) to be useful.
Right, this is the best we have for the moment to understand how CCM will interface with the user for the dual stack implementation.
Looking at the documentation for dualstack configuration on k8s services (https://kubernetes.io/docs/concepts/services-networking/dual-stack/#services), since the the 2nd IP Family in service.spec.ipFamilies is mutable, should we be setting service.spec.ipFamilyPolicy to RequireDualStack so that we don't end up in a situation with ["IPv6", "IPv4"] where the 2nd IP Family "IPv4" is removed and we are left with single stack IPv6 configured on the LB service for the NLB.
The Kubernetes doc doesn't precise that the secondary ipFamily cannot be removed if the policy is RequireDualStack. Also, if we are considering a use case independent from the IngressController publishing service, the user can change the policy too. However in the context of this EP, we are talking about a service which is managed by the ingress-operator, so the ipFamilies and ipFamilyPolicy fields will be enforced by the operator (unsolicited changes will be stomped).
Overall, I haven't fully made my mind about which policy the ingress operator has to enforce. It's difficult to do without an actual implementation which can be tested. However, now as I'm thinking of the policy RequireDualStack can be a good choice, for the reason of it's more deterministic failure mode - I suppose that the ingress hostname will not be added to the service's status if something goes side ways with CCM load balancer provisioning. Let me change it to RequireDualStack.
There was a problem hiding this comment.
The Kubernetes doc doesn't precise that the secondary ipFamily cannot be removed if the policy is RequireDualStack
I just wanted to add that if the policy RequiredDualStack, the ipFamilies must have both IPv4 and IPv6 entries and we can't change the order either like the doc said. Below is tested with a dualstack kinD cluster.
$ kubectl get svc/cryostat -o yaml | yq .spec.ipFamilyPolicy
RequireDualStack
$ $ kubectl get svc/cryostat -o yaml | yq .spec.ipFamilies
- IPv4
- IPv6
$ kubectl patch svc/cryostat --type=merge -p '{"spec":{"ipFamilies": ["IPv4"]}}'
The Service "cryostat" is invalid: spec.ipFamilyPolicy: Invalid value: "RequireDualStack": must be 'SingleStack' to release the secondary IP family
$ kubectl patch svc/cryostat --type=merge -p '{"spec":{"ipFamilies": ["IPv6", "IPv4"]}}'
The Service "cryostat" is invalid:
* spec.clusterIPs[0]: Invalid value: "10.96.17.234": expected an IPv6 value as indicated by `ipFamilies[0]`
* spec.clusterIPs[1]: Invalid value: "fd00:10:96::37f9": expected an IPv4 value as indicated by `ipFamilies[1]`And I agreed with using RequireDualStack policy, which opens less doors for user misconfigurations.
| to create both Route53 Alias A and Alias AAAA records when the cluster IP | ||
| family is dual-stack. The IP family is passed to the provider at | ||
| initialization time, similar to [AWS region](https://github.com/openshift/cluster-ingress-operator/blob/8afaffbf8ddbe65565bad52eea6267b615eceec2/pkg/dns/aws/dns.go). | ||
|
|
There was a problem hiding this comment.
Similarly, for DualStackIPv4Primary when service.spec.ipFamilies is set to ["IPv4", "IPv6"] , do we have to consider the Day-2 scenario where the cluster's IPFamily configuration remains DualStackIPv4Primary, but the 2nd IP Family in service.spec.ipFamilies removed, leaving us with just IPV4. Are we allowing this (by setting ipFamilyPolicy to PreferDualStack?
If yes, then we should also make sure to remove the AAAA DNS entry in Route53.
There was a problem hiding this comment.
As I mentioned in the previous comment, I don't see any confirmation that RequireDualStack policy will stop the user (cluster admin in this case) from removing a secondary ipFamily. However the operator will enforce the desired state to match the IP family specified in Infrastructure CR.
There was a problem hiding this comment.
I can quickly confirm that in #1940 (comment). And looks like we are leaning towards RequireDualStack policy so this scenario won't be supported...?
| about the need to recreate the service manually. Also, the message highlights the fact | ||
| that CLB does not support the cluster-wide dual-stack IP family. | ||
|
|
||
| 4. When the user proceeds with a service recreation, the ingress-operator creates the |
There was a problem hiding this comment.
I see 2 other alternatives:
- reporting error and ingress operator going into a
Degraded=Truestate - warning user that this change to CLB on Day-2 is not allowed and continue as DualStack.
I am sure these were considered and the current option was picked as the best outcome for the customer. Could you please add a brief reasoning for that choice?
There was a problem hiding this comment.
warning user that this change to CLB on Day-2 is not allowed and continue as DualStack.
This is what I described in the step before:
Also, the message highlights the fact that CLB does not support the cluster-wide dual-stack IP family.
reporting error and ingress operator going into a Degraded=True state
Right, this is an alternative. My reasoning was:
- This is a recorded user intention - to use CLB type even if a warning was given.
- This behavior is similar to what the ingress operator does with another Infrastructure field:
ResourceTags. Azure doesn't support custom tags on their load balancer, so the ingress operator don't do anything about it. - We can improve it in the future with an addition of
ipFamilyfield to IngressController API (CRD validation). For the moment (taking deadlines into account) this is the minimum effort we can do to put our foot into the "dual stack support" territory.
There was a problem hiding this comment.
I think letting the user know that the CLB isn't dual stack is appropriate.
One thing I don't see here is anything mentioning going back to Progressing=False once the user manually recreates the service. I'd expect this to self-heal once the user took corrective action.
There was a problem hiding this comment.
One thing I don't see here is anything mentioning going back to Progressing=False once the user manually recreates the service. I'd expect this to self-heal once the user took corrective action.
Right, I didn't go in too many details for this one as it's an existing behavior which doesn't need to be implemented. Let me explain that part though to make things clearer.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
- Clarify that dual-stack support is for IngressController publishing services specifically - Add non-goal about Gateway API not being covered - Change ipFamilyPolicy to RequireDualStack for more deterministic failure mode Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
alebedev87
force-pushed
the
NE-2021-dual-stack-support-ingresscontroller
branch
from
d99d94b to
cc616c3
Compare
|
@alebedev87: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
| The feature is day-0 only, only fresh installs are supported. | ||
|
|
||
| **Downgrade to version without feature:** | ||
| - On clusters installed with the `AWSDualStackInstall` feature gate and dual-stack IP family: |
There was a problem hiding this comment.
Reading this section made me think of docs.redhat.com/en/documentation/openshift_container_platform/4.21/html-2ngle/config_apis/index#status-featuregates and specifically
The enabled/disabled values for a particular version may change during the life of the cluster as various .spec.featureSet values are selected. Operators may choose to restart their processes to pick up these changes, but remembering past enable/disable lists is beyond the scope of this API and is the responsibility of individual operators.
That implies to me that it is possible to remove dual stack support from an AWS OCP cluster without a complete OCP downgrade, which has cascading effects on the resources, particularly if they have IPv6 configured to be the primary/default.
My suspicion is that most of the operations listed here remain the same - the IngressController will need to reconcile and recreate the relevant services, as well as updating the DNS records. That includes not reading the IP family information, since presumably that will be behind the feature gate.
|
This looks accurate from my understanding. |
| - On clusters installed with the `AWSDualStackInstall` feature gate and dual-stack IP family: | ||
| - The older ingress-operator version will not read the IP family from | ||
| the Infrastructure CR and will fall back to IPv4-only configuration. | ||
| - The IngressController's publishing service will be reconciled and the `ipFamilies` and |
There was a problem hiding this comment.
This seems in contraddiction with what we are reporting at line 317 related to the fact that Kubernetes doesn't allow switching the ipFamilies on an existing service (https://kubernetes.io/docs/concepts/services-networking/dual-stack/#services)
There was a problem hiding this comment.
I think this is a good point. This downgrade path from dualstack cluster to older IPv4 cluster requires conversion from dual-stack networking to IPv4-only networking (i.e. similar to day-2 conversion). This is something we do not plan to support right?
| **installer** is the OpenShift installer responsible for creating the | ||
| cluster and configuring the Infrastructure CR. | ||
|
|
||
| 1. The cluster administrator installs an OpenShift cluster on AWS with |
There was a problem hiding this comment.
The proposal handles DualStackIPv4Primary, DualStackIPv6Primary, and IPv4 (or unset), but the Infrastructure CR type definition likely also includes a single-stack IPv6 value. The Non-Goals section states "Implementing single-stack IPv6 IngressControllers" is a non-goal, but I think that it will be better to explicitly clarify what the operator does when it encounters ipFamily: IPv6: does it error, set a degraded status condition, fall back to IPv4, or ignore it?
There was a problem hiding this comment.
Currently, I think the Infrastructure CR type definition only defines DualStackIPv4Primary, DualStackIPv6Primary, and IPv4.
Maybe, we can make it clear in this enhancement that those are the only supported values.
| **Test Strategy:** | ||
| - Unit tests for operator logic that reads IP family from Infrastructure | ||
| CR and configures services accordingly. | ||
| - E2E tests verifying: |
There was a problem hiding this comment.
The E2E test list should also include negative and edge-case scenarios that are described in the proposal body but not covered here.
Specifically:
- Downgrade behavior validation (dual-stack cluster downgraded to a version without the feature -> IPv4-only connectivity preserved, expected status).
| cluster and configuring the Infrastructure CR. | ||
|
|
||
| 1. The cluster administrator installs an OpenShift cluster on AWS with | ||
| the `AWSDualStackInstall` feature gate enabled and [a dual-stack IP family](https://github.com/openshift/installer/pull/9930), |
There was a problem hiding this comment.
nit: we can reference this link instead: https://github.com/openshift/installer/blob/b0514c8e022d8445e57f303852487d3cd59c4a0a/pkg/types/aws/platform.go#L134-L144
Alternatively, we can use openshift/installer#10207. The openshift/installer#9930 is a test PR (for reviewers to do local testing), which won't be merged at all.
|
|
||
| 4. The ingress-operator does not create an Alias AAAA record for the wildcard domain. | ||
|
|
||
| 5. AWS provisions a standard IPv4-only NLB. |
There was a problem hiding this comment.
If the ipFamily is IPv4, CLB is also allowed. We should be clear that the type of LB will be based on the what the user configures?
| - Unit tests for operator logic that reads IP family from Infrastructure | ||
| CR and configures services accordingly. | ||
| - E2E tests verifying: | ||
| - On clusters installed with `AWSDualStackInstall` feature gate: |
There was a problem hiding this comment.
| - On clusters installed with `AWSDualStackInstall` feature gate: | |
| - On clusters installed with `AWSDualStackInstall` feature gate and a dual-stack IP family (i.e. field `platform.aws.ipFamily` in the install-config).: |
nit: 😁
| - DNS alias records of the wildcard domain point to the AWS NLB hostname. | ||
| - AWS NLB hostname resolves to both IPv4 and IPv6 addresses. |
There was a problem hiding this comment.
Since we are using Route53 alias record, those A/AAAA alias records should point directly to the NLB IP addressess, right?
The current wording seems to describe CNAME records instead?
| - When the installer's `AWSDualStackInstall` feature gate is enabled, | ||
| the ingress operator will automatically configure the dual-stack IP address type for | ||
| publishing services of IngressControllers. |
There was a problem hiding this comment.
nit: The ingress operator should only configure dual-stack IP address if the infrastructure status set ipFamily to one of the dual-stack variants, right?
| 8. Verify DNS alias is published: `dig <wildcard-domain>` should show | ||
| alias to AWS NLB hostname. |
There was a problem hiding this comment.
It should point to IP addresses of AWS NLB since it is an alias record, right?
9 participants
This enhancement enables automatic dual-stack (IPv4 and IPv6) IP address types for IngressController publishing services on AWS clusters using Network Load Balancers (NLB). This is a day-0 feature that automatically configures itself based on the cluster-wide cluster's IP family configuration