NE-2021: Support dual-stack IngressController on AWS#1940
NE-2021: Support dual-stack IngressController on AWS#1940alebedev87 wants to merge 6 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 |
enhancements/ingress/aws-dual-stack-support-for-ingresscontrollers.md
Outdated
Show resolved
Hide resolved
|
/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 its 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...?
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...?
Right, the scenario of changing ipFamilies day-2 cannot be supported for the reason of using RequireDualStack policy. Btw, this matches well the Infrastructure API which disallows day-2 changes of IPFamily in the status. I'll add a note to Implementation Details about day-2 changes to ipFamilies.
| 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
| 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.
There was a problem hiding this comment.
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.
The only way I know for disabling of a featuregate is via CustomNoUpgrade featureset. From what I understand, clusters which use Default featureset cannot disable features. I'm not sure we have to account for CustomNoUpgrade featureset. Do you know of any other supported way of disabling features?
|
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?
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)
Removing both fields (ipAddressFamily, ipFamilies) should still be allowed. However, this made me check my assumption about the ingress operator enforcement of the spec of the service. The operator stomps only on specific spec fields (logical as this avoids fights with other controllers). So after the downgrade both fields will remain set on the service, however the old CCO doesn't know about those fields.
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?
I'm wondering whether we even support downgrades for fresh installs. @tthvo: do you know whether a user can request a downgrade to 4.21.z if a cluster was created for a version 4.22.0?
There was a problem hiding this comment.
I'm wondering whether we even support downgrades for fresh installs. @tthvo: do you know whether a user can request a downgrade to 4.21.z if a cluster was created for a version 4.22.0?
I remember we don't support downgrades across minor versions (from this article) even for existing clusters. So, that would be a no for fresh install. cc @patrickdillon @sadasu.
There was a problem hiding this comment.
The operator stomps only on specific spec fields (logical as this avoids fights with other controllers). So after the downgrade both fields will remain set on the service, however the old CCO doesn't know about those fields.
I guess if the older ingress operator and also CCM do not know about these fields, the following mechanism in the EP is no longer true, right?
- The IngressController's publishing service will be reconciled and the `ipFamilies`
and `ipFamilyPolicy` fields will be removed if previously set, defaulting to IPv4-only.
- The AWS NLB hostname remains the same; AWS removes AAAA record.
- IPv4 connectivity continues to work; IPv6 connectivity is lost.
It should only be applicable if the service is re-created and the old IPv4 configuration is followed?
There was a problem hiding this comment.
I guess if the older ingress operator and also CCM do not know about these fields, the following mechanism in the EP is no longer true, right?
Yes. I didn't rephrase it yet as I'm trying to clarify whether we even need the downgrade part or it's out of scope as the feature is for fresh installs only.
There was a problem hiding this comment.
I remember we don't support downgrades across minor versions (from this article) even for existing clusters.
I added a caveat about the unsupported path and removed the inaccurate info about the removal of ipFamilies and ipFamilyPolicy. However I still need to clarify 2 things: 1) CCM behavior in case of a downgrade, 2) disabling of a featuregate.
| **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.
There was a problem hiding this comment.
Currently, I think the Infrastructure CR type definition only defines DualStackIPv4Primary, DualStackIPv6Primary, and IPv4.
Right, IPv6 only is not a valid value for the IPFamily field from the Infrastructure CR. The non goal was supposed to re-iterate on this. Also, there is a permalink to the Infrastructure CR in the proposal section which captures the state of the Infrastructure API with only 3 valid values: IPv4, DualStackIPv6Primary, DualStackIPv4Primary.
There was a problem hiding this comment.
I rephrased it slightly to 1) link the installer's API field (permalink too), 2) mention the 2 available fields for the dual-stack configuration.
| **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).
There was a problem hiding this comment.
I agree, more edge cases can be added. I'm not sure about a downgrade test though as it would require yet another CI job. But changing the lb type from NLB to CLB is an e2e test scenario I can think about.
enhancements/ingress/aws-dual-stack-support-for-ingresscontrollers.md
Outdated
Show resolved
Hide resolved
enhancements/ingress/aws-dual-stack-support-for-ingresscontrollers.md
Outdated
Show resolved
Hide resolved
| - 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: 😁
There was a problem hiding this comment.
Right. This made me think that I forgot to mention that the end-to-end testing implies a new CI job with a dualstack support. @tthvo: Do you have any updates to be done to the CI operator in openshift/release repository to support AWS dualstack installations?
There was a problem hiding this comment.
@alebedev87 Yes, as of now, we openned openshift/release#73270 to add 2 presubmits for AWS dualstack installation. The 2 jobs are are:
- pull-ci-openshift-installer-main-e2e-aws-ovn-dualstack-ipv4-primary-techpreview
- pull-ci-openshift-installer-main-e2e-aws-ovn-dualstack-ipv6-primary-techpreview
I think periodic jobs will be handled by our QEs later on.
There was a problem hiding this comment.
I think periodic jobs will be handled by our QEs later on.
Good, then the ingresscontroller's e2e test can be added to the origin repository as part of the required testing for AWSDualStackInstall featuregate.
I've added details about a new presubmit for the ingress operator and origin tests to Test Strategy chapter.
enhancements/ingress/aws-dual-stack-support-for-ingresscontrollers.md
Outdated
Show resolved
Hide resolved
enhancements/ingress/aws-dual-stack-support-for-ingresscontrollers.md
Outdated
Show resolved
Hide resolved
enhancements/ingress/aws-dual-stack-support-for-ingresscontrollers.md
Outdated
Show resolved
Hide resolved
- Use permalink for installer IP family API. - Improve wording about AWS DNS aliases. - Mention dual-stack IP family explicitly when needed.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@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. |
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