[release-4.9] Bug 2047676: UPSTREAM: <carry>: Give warning when ipFamilyPolicy implicitly set by andreaskaris · Pull Request #1151 · openshift/kubernetes

andreaskaris · 2022-01-27T18:04:09Z

UPSTREAM: : Give warning when ipFamilyPolicy implicitly set

In kube 1.21 and 1.22 (OCP 4.8 and 4.9), the apiserver would default
the value of ipFamilyPolicy to RequireDualStack if you created a
Service with two ipFamilies or two clusterIPs but no explicitly
specified ipFamilyPolicy. In 1.23/4.10, you must explicitly specify
either PreferDualStack or RequireDualStack for DualStack services.
Emit a warning in 4.8 and 4.9 to raise awareness about the upcoming
API changes.

OpenShift 4.8 and 4.9 only, BZ 2047676

What type of PR is this?

What this PR does / why we need it:

Warn users about an upcoming breaking API change.

Which issue(s) this PR fixes:

Fixes BZ 2045576

Special notes for your reviewer:

I initially thought I could write a (mutating) admission controller for release 4.9/4.8 to warn users loudly about the upcoming breaking change in OCP 4.10 if the user tried to create a DualStack service in a way which would be invalid starting with 4.10.

I tested it against an upstream OVN kind setup on Kubernetes 1.20, and it turns out that, no matter if I use a validating or a mutating webhook, the field service.Spec.IPFamilyPolicy is set by Kubernetes to IPFamilyPolicyRequireDualStack before the webhooks are called.

My initial idea was to return something like ...

        admissionReviewResponse := admission.AdmissionReview{
                TypeMeta: metav1.TypeMeta{
                        APIVersion: "admission.k8s.io/v1",
                        Kind:       "AdmissionReview",
                },
                Response: &admission.AdmissionResponse{
                        UID:     admissionReviewRequest.Request.UID,
                        Allowed: true,
                        Warnings: []string{
                                "Invalid warning message",
                        },
                },
        }

... if len(service.Spec.ClusterIPs) == 2 || len(service.Spec.IPFamilies) == 2 and if the ipfamilypolicy == nil.

However, for this service here which will be invalid starting with Kubernetes 1.23:

[root@ovnkubernetes ~]# cat nginx-dualstack.yaml
apiVersion: v1
kind: Service
metadata:
  name: nginx-dualstack
spec:
  type: ClusterIP
  selector:
    app: nginx
  ipFamilies:
         - IPv6
         - IPv4
  ports:
    - port: 80
      targetPort: 80

What I end up getting is this, even with a mutating webhook. Note How K8s apiserver sets "ipFamilyPolicy":"RequireDualStack before any of the webhooks is called:

I0126 21:37:22.716298  620556 ovnkube-admission-controller.go:49] Service: {"kind":"Service","apiVersion":"v1","metadata":{"name":"nginx-dualstack","namespace":"default","creationTimestamp":null,"managedFields":[{"manager":"kubectl-create","operation":"Update","apiVersion":"v1","time":"2022-01-26T21:37:22Z","fieldsType":"FieldsV1","fieldsV1":{"f:spec":{"f:ipFamilies":{},"f:ipFamilyPolicy":{},"f:ports":{".":{},"k:{\"port\":80,\"protocol\":\"TCP\"}":{".":{},"f:port":{},"f:protocol":{},"f:targetPort":{}}},"f:selector":{".":{},"f:app":{}},"f:sessionAffinity":{},"f:type":{}}}}]},"spec":{"ports":[{"protocol":"TCP","port":80,"targetPort":80}],"selector":{"app":"nginx"},"type":"ClusterIP","sessionAffinity":"None","ipFamilies":["IPv6","IPv4"],"ipFamilyPolicy":"RequireDualStack"},"status":{"loadBalancer":{}}}
I0126 21:37:22.716366  620556 ovnkube-admission-controller.go:53] Annotations: map[]
I0126 21:37:22.716382  620556 ovnkube-admission-controller.go:54] ipFamilyPolicy: %!s(*v1.IPFamilyPolicyType=0xc000279370)
I0126 21:37:22.716391  620556 ovnkube-admission-controller.go:55] ipFamilies: [IPv6 IPv4]
I0126 21:37:22.716399  620556 ovnkube-admission-controller.go:56] clusterIPs: []
I0126 21:37:22.716403  620556 ovnkube-admission-controller.go:57] ipFamilies: [IPv6 IPv4]
I0126 21:37:22.716408  620556 ovnkube-admission-controller.go:65] IPFamilyPolicyRequireDualStack

So, given that something within the Api Server sets service.Spec.IPFamilyPolicy and only after that it calls any of the admission webhooks, using a webhook to generate a warning does not work.

Instead, I am proposing that we introduce a tiny change in openshift/kubernetes downstream and that we generate the warning directly inside the API server.

Does this PR introduce a user-facing change?

Yes.

In Red Hat OpenShift Container Platform 4.8 and 4.9, the kube-apiserver will default the value of ipFamilyPolicy to RequireDualStack when such a service is created with two ipFamilies or two clusterIPs but no explicitly specified ipFamilyPolicy.
In Red Hat OpenShift Container Platform 4.10, administrators must explicitly specify either ipFamilyPolicy: PreferDualStack or ipFamilyPolicy: RequireDualStack for DualStack services.
Emit a warning in 4.8 and 4.9 to raise awareness about the upcoming API changes.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

openshift-ci · 2022-01-27T18:04:10Z

@andreaskaris: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Details

In response to this:

[DNM] UPSTREAM: : Give warning when ipFamilyPolicy implicitly set

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/test-infra repository.

openshift-ci-robot · 2022-01-27T18:04:19Z

@andreaskaris: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

9af7840|UPSTREAM: : Give warning when ipFamilyPolicy implicitly set: does not specify an upstream backport in the commit message

Comment /validate-backports to re-evaluate validity of the upstream PRs, for example when they are merged upstream.

openshift-ci · 2022-01-27T18:04:41Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: andreaskaris
To complete the pull request process, please assign mfojtik after the PR has been reviewed.
You can assign the PR to them by writing /assign @mfojtik in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

DOWNSTREAM_OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2022-01-27T21:37:24Z

@andreaskaris: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

9af7840|UPSTREAM: : Give warning when ipFamilyPolicy implicitly set: does not specify an upstream backport in the commit message

Comment /validate-backports to re-evaluate validity of the upstream PRs, for example when they are merged upstream.

openshift-ci · 2022-01-27T21:38:24Z

@andreaskaris: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Details

In response to this:

[DNM] UPSTREAM: : Give warning when ipFamilyPolicy implicitly set

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/test-infra repository.

In kube 1.21 and 1.22 (OCP 4.8 and 4.9), the apiserver would default the value of `ipFamilyPolicy` to `RequireDualStack` if you created a Service with two `ipFamilies` or two `clusterIPs` but no explicitly specified `ipFamilyPolicy`. In 1.23/4.10, you must explicitly specify either PreferDualStack or RequireDualStack for DualStack services. Emit a warning in 4.8 and 4.9 to raise awareness about the upcoming API changes. OpenShift 4.8 and 4.9 only, BZ 2045576

openshift-ci-robot · 2022-01-27T21:41:50Z

@andreaskaris: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

feb195e|UPSTREAM: : Give warning when ipFamilyPolicy implicitly set: does not specify an upstream backport in the commit message

Comment /validate-backports to re-evaluate validity of the upstream PRs, for example when they are merged upstream.

openshift-ci · 2022-01-28T09:24:33Z

@andreaskaris: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Details

In response to this:

[DNM] UPSTREAM: : Give warning when ipFamilyPolicy implicitly set

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/test-infra repository.

openshift-ci · 2022-01-28T09:27:40Z

@andreaskaris: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Details

In response to this:

[DNM] UPSTREAM: : Give warning when ipFamilyPolicy implicitly set

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/test-infra repository.

openshift-ci · 2022-01-28T09:29:16Z

@andreaskaris: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Details

In response to this:

[DNM] UPSTREAM: : Give warning when ipFamilyPolicy implicitly set

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/test-infra repository.

openshift-ci · 2022-01-28T09:34:24Z

@andreaskaris: This pull request references Bugzilla bug 2045576, which is invalid:

expected the bug to target the "4.9.z" release, but it targets "---" instead
expected Bugzilla bug 2045576 to depend on a bug targeting a release in 4.10.0 and in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but no dependents were found

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

(release-4.9] Bug 2045576: UPSTREAM: : Give warning when ipFamilyPolicy implicitly set

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/test-infra repository.

openshift-ci · 2022-01-28T09:38:42Z

@andreaskaris: This pull request references Bugzilla bug 2047676, which is invalid:

expected dependent Bugzilla bug 2045576 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is ASSIGNED instead
expected dependent Bugzilla bug 2045576 to target a release in 4.10.0, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

[release-4.9] Bug 2047676: UPSTREAM: : Give warning when ipFamilyPolicy implicitly set

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/test-infra repository.

openshift-ci · 2022-01-28T09:38:48Z

@andreaskaris: This pull request references Bugzilla bug 2047676, which is invalid:

expected dependent Bugzilla bug 2045576 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is ASSIGNED instead
expected dependent Bugzilla bug 2045576 to target a release in 4.10.0, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

[release-4.9] Bug 2047676: UPSTREAM: : Give warning when ipFamilyPolicy implicitly set

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/test-infra repository.

andreaskaris · 2022-01-28T09:39:51Z

/bugzilla refresh

openshift-ci · 2022-01-28T09:39:54Z

@andreaskaris: This pull request references Bugzilla bug 2047676, which is invalid:

expected dependent Bugzilla bug 2045576 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE), but it is ASSIGNED instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

/bugzilla refresh

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/test-infra repository.

andreaskaris · 2022-01-28T10:01:42Z

/retest-required

openshift-ci · 2022-01-28T11:46:07Z

@andreaskaris: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-csi	`feb195e`	link	false	`/test e2e-aws-csi`

Full PR test history. Your PR dashboard.

Details

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/test-infra repository. I understand the commands that are listed here.

sttts · 2022-02-04T10:47:04Z

pkg/registry/core/service/storage/rest.go

+// tryDefaultValidateServiceClusterIPFields will set service.Spec.IPFamilyPolicy before the webhooks have a chance to inspect
+// it. Therefore, we implement this downstream only warning message in OpenShift's kube-apiserver.
+// Carry this change forward only in 4.8 and 4.9 and drop this for all other versions.
+func warnDualStackIPFamilyPolicy(ctx context.Context, service *api.Service) {


move this into patch_rest.go

aojea · 2022-02-04T10:53:28Z

@andreaskaris please see Stefan comments, we can pursue this approach, but I still need to verify what is the best place to put the wanings, the Service REST in this version has 2 layers.

Another important thing, we should add a good release note and docs about the issue explaining it with examples so we can easily refer users to them, something like

apiVersion: v1
kind: Service
metadata:
  labels:
    app: test
  name: test
  namespace: default
spec:
  ipFamilies:
  - IPv4
  - IPv6
  ports:
  - name: "80"
    port: 80
    protocol: TCP
    targetPort: 80
  selector:
    app: test
  type: ClusterIP

will work in 4.8 and 4.9, for 4.10 onwards is mandatory to add the ipFamilyPolicy field, resulting in

apiVersion: v1
kind: Service
metadata:
  labels:
    app: test
  name: test
  namespace: default
spec:
  ipFamilies:
  - IPv4
  - IPv6
  ipFamilyPolicy: PreferDualStack
  ports:
  - name: "80"
    port: 80
    protocol: TCP
    targetPort: 80
  selector:
    app: test
  type: ClusterIP

aojea · 2022-02-04T10:55:41Z

pkg/registry/core/service/storage/rest.go

+			"without explicitly setting Service.Spec.IPFamilyPolicy is deprecated. " +
+			"This operation will fail starting with Red Hat OpenShift Platform 4.10. " +
+			"Make sure to set IPFamilyPolicy to PreferDualStack or RequireDualStack when configuring DualStack services."
+		klog.Warningf(msg)


I think we should remove the klog here, it is not adding much value here since there is no actionable action

andreaskaris · 2022-02-04T11:21:16Z

Thanks for the detailed feedback! I can't reopen this here because I had force-pushed something to the branch in the meantime. The follow-up is here: #1170

openshift-ci-robot added the backports/unvalidated-commits Indicates that not all commits come to merged upstream PRs. label Jan 27, 2022

openshift-ci bot requested review from deads2k and mfojtik January 27, 2022 18:04

andreaskaris force-pushed the bz2045576-4.9 branch from 9af7840 to 280e190 Compare January 27, 2022 21:37

andreaskaris force-pushed the bz2045576-4.9 branch from 280e190 to feb195e Compare January 27, 2022 21:41

andreaskaris changed the title ~~[DNM] UPSTREAM: <carry>: Give warning when ipFamilyPolicy implicitly set~~ (release-4.9] Bug 2045576: UPSTREAM: <carry>: Give warning when ipFamilyPolicy implicitly set Jan 28, 2022

openshift-ci bot added bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jan 28, 2022

andreaskaris changed the title ~~(release-4.9] Bug 2045576: UPSTREAM: <carry>: Give warning when ipFamilyPolicy implicitly set~~ [release-4.9] Bug 2045576: UPSTREAM: <carry>: Give warning when ipFamilyPolicy implicitly set Jan 28, 2022

andreaskaris changed the title ~~[release-4.9] Bug 2045576: UPSTREAM: <carry>: Give warning when ipFamilyPolicy implicitly set~~ [release-4.9] Bug 2047676: UPSTREAM: <carry>: Give warning when ipFamilyPolicy implicitly set Jan 28, 2022

andreaskaris closed this Jan 28, 2022

andreaskaris mentioned this pull request Jan 31, 2022

Bug 2045576: [CARRY][Downstream-only] Give warning when ipFamilyPolicy implicitly set openshift/ovn-kubernetes#938

Merged

sttts reviewed Feb 4, 2022

View reviewed changes

aojea reviewed Feb 4, 2022

View reviewed changes

andreaskaris mentioned this pull request Feb 4, 2022

[release-4.9] Bug 2050632: UPSTREAM: <drop>: Give warning when ipFamilyPolicy implicitly set #1170

Merged

Comments

Conversation

andreaskaris commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

openshift-ci bot commented Jan 27, 2022

Uh oh!

openshift-ci-robot commented Jan 27, 2022

Uh oh!

openshift-ci bot commented Jan 27, 2022

Uh oh!

openshift-ci-robot commented Jan 27, 2022

Uh oh!

openshift-ci bot commented Jan 27, 2022

Uh oh!

openshift-ci-robot commented Jan 27, 2022

Uh oh!

openshift-ci bot commented Jan 28, 2022

Uh oh!

openshift-ci bot commented Jan 28, 2022

Uh oh!

openshift-ci bot commented Jan 28, 2022

Uh oh!

openshift-ci bot commented Jan 28, 2022

Uh oh!

openshift-ci bot commented Jan 28, 2022

Uh oh!

openshift-ci bot commented Jan 28, 2022

Uh oh!

andreaskaris commented Jan 28, 2022

Uh oh!

openshift-ci bot commented Jan 28, 2022

Uh oh!

andreaskaris commented Jan 28, 2022

Uh oh!

openshift-ci bot commented Jan 28, 2022

Uh oh!

sttts Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

aojea commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aojea Feb 4, 2022

Choose a reason for hiding this comment

Uh oh!

andreaskaris commented Feb 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andreaskaris commented Jan 27, 2022 •

edited

Loading

aojea commented Feb 4, 2022 •

edited

Loading