ingress: Migrate to networking.k8s.io/v1 #172

Miciah · 2021-03-16T03:03:28Z

Update the ingress-to-route controller and unit tests to use the networking.k8s.io/v1 API, implement IngressClass, and migrate routes created using older ingress API versions.

pkg/cmd/controller/ingress.go: Update to import and use networking/v1 packages.
(RunIngressToRouteController): Pass ingressclass client to NewController.
pkg/route/ingress/ingress.go: Update to import and use networking/v1 packages.
(Controller): Add ingressclassLister field.
(NewController): Add parameter for ingressclasses informer. Use it to get ingressclassLister.
(sync): Check the ingress's kubernetes.io/ingress.class annotation and spec.ingressClassName field. If an ingressclass is specified, look it up and check its spec.controller field value. If the ingress specifies an ingressclass that does not specify the openshift.io/ingress-to-route ingressclass, ignore the ingress. When patching an ingress, replace the existing ownerReferences field value in case the API version has changed.
(validOwnerRefAPIVersions): New variable.
(hasIngressOwnerRef): Use validOwnerRefAPIVersions to verify that the owner reference specifies some recognized API version.
(newRouteForIngress): Update to use the ingress's spec.backend.service field to get the target service. Use the networking.k8s.io/v1 API version in the returned route's owner reference.
(routeMatchesIngress): Update to use the ingress's spec.backend.service field. Verify that the route's owner reference uses the networking.k8s.io/v1 API version.
(targetPortForService): Replace the ingress path parameter with an ingress service backend parameter.
pkg/route/ingress/ingress_test.go: Update to use and import networking/v1 packages.
(ingressclassLister): New type. Implement a fake ingressclass lister.
(List, Get): New methods for ingressclassLister.
(complexIngress): Update to the v1 API.
(TestController_stabilizeAfterCreate): Use a fake ingressclass lister.
(TestController_sync): Define fake ingressclass lister with an ingressclass that specifies the OpenShift ingress controller. Update ingresses in test cases to the v1 API. Update test cases to expect the controller to patch ownerReferences. Add test cases: "ignores ingress with third-party ingressclass", "ignores ingress with unsupported path type", "create route - default ingresscontroller", "create route - custom ingresscontroller", "no-op - ignore route created for an ingress with a third-party class", and "update route with old owner reference". Augment the "create route" test case to verify correct behavior for the implemented path types.

This enhancement updates the [ingress-to-route](https://github.com/openshift/openshift-controller-manager/blob/master/pkg/route/ingress/ingress.go) controller to use the stable `networking.k8s.io/v1` version of the Ingress API. Although the API server already supports the v1 API version without this enhancement, the ingress-to-route controller requires changes to transition from using the v1beta1 client to using the v1 client and to support new v1 features. These new features include the `spec.pathType` and `spec.ingressClassType` fields as well as the associated IngressClass API. This enhancement does *not* extend the Route API to accommodate new features in the Ingress API; these are only supported to the extent that they are compatible with the Route API. --- Implementation PRs: - [ ] [openshift/cluster-ingress-operator#574 WIP: ingressclass: New controller.](openshift/cluster-ingress-operator#574) - [ ] [openshift/openshift-controller-manager#172 WIP: ingress: Migrate to networking.k8s.io/v1.](openshift/openshift-controller-manager#172) - [ ] TODO openshift/api PR for `openshift.io/ingress-to-route` ingressclass controller name. - [ ] TODO openshift/cluster-monitoring-operator PR for alerts.

sgreene570 · 2021-04-05T20:41:16Z

pkg/route/ingress/ingress.go

@@ -370,6 +372,27 @@ func (c *Controller) sync(key queueKey) error {
 		return err
 	}

+	// If the ingress specifies an ingressclass and the ingressclass does
+	// not specify OpenShift Router as its controller, ignore the ingress.


We should ignore ingresses that do not specify openshift.io/ingress-to-route as their controller, right? (OpenShift Router seems a little vague, but perhaps thats intentional?)

Right. An earlier version of these changes used the controller name "openshift.io/haproxy-router", which I changed to "openshift.io/ingress-to-route" (since it's really the ingress-to-route controller that is managing the ingresses), and I forgot to update the comment when I changed the controller name. Thanks for catching that!

sgreene570 · 2021-04-05T20:45:31Z

pkg/route/ingress/ingress.go

+	var ingressClassName *string
+	if v, ok := ingress.Annotations["kubernetes.io/ingress.class"]; ok {
+		ingressClassName = &v
+	} else if ingress.Spec.IngressClassName != nil {


Suggested change

} else if ingress.Spec.IngressClassName != nil {

} else {

No need to check for nil twice, right?

Right. Thanks!

sgreene570 · 2021-04-05T20:57:27Z

@Miciah CI builds for this PR don't look too hot.

Maybe try running go mod tidy and go mod vendor?

Miciah · 2021-04-05T22:12:51Z

@Miciah CI builds for this PR don't look too hot.

Maybe try running go mod tidy and go mod vendor?

I already did that. Hmm...

Miciah · 2021-04-05T22:36:30Z

@Miciah CI builds for this PR don't look too hot.
Maybe try running go mod tidy and go mod vendor?

I already did that. Hmm...

I had forgotten to git add some untracked files. Long day.

Miciah · 2021-04-06T01:57:31Z

=== RUN   TestUpdateOldStyleSecretWithKey
    docker_registry_service_test.go:274: Waiting for ready
I0405 22:40:17.195463   11282 docker_registry_service.go:145] Starting DockerRegistryServiceController controller
    docker_registry_service_test.go:281: Waiting to reach syncRegistryLocationHandler
    docker_registry_service_test.go:287: Waiting to update secret
    docker_registry_service_test.go:291: failed to call into syncSecret
--- FAIL: TestUpdateOldStyleSecretWithKey (45.01s)
I0405 22:41:02.198922   11282 docker_registry_service.go:163] Shutting down DockerRegistryServiceController controller

/test unit

Miciah · 2021-04-06T01:58:16Z

alert ClusterOperatorDegraded fired for 12 seconds with labels: {condition="Degraded", endpoint="metrics", instance="10.0.163.109:9099", job="cluster-version-operator", name="authentication", namespace="openshift-cluster-version", pod="cluster-version-operator-5b8c98dd98-cvvtp", reason="WellKnownReadyController_SyncError", service="cluster-version-operator", severity="critical"}
alert ClusterOperatorDown fired for 12 seconds with labels: {endpoint="metrics", instance="10.0.163.109:9099", job="cluster-version-operator", name="authentication", namespace="openshift-cluster-version", pod="cluster-version-operator-5b8c98dd98-cvvtp", service="cluster-version-operator", severity="critical", version="4.8.0-0.ci.test-2021-04-05-223956-ci-op-yw62ylij"}

/test e2e-aws

sgreene570 · 2021-04-06T02:28:20Z

fixups look good.

/lgtm

Miciah · 2021-04-07T15:53:42Z

/test e2e-aws

adambkaplan

How will upgrades work? What will happen to existing routes linked to v1beta1 Ingress instances?

adambkaplan · 2021-04-07T16:03:59Z

go.mod

-	k8s.io/api v0.20.0
-	k8s.io/apimachinery v0.20.0
+	k8s.io/api v0.21.0-rc.0
+	k8s.io/apimachinery v0.21.0-rc.0


It seems that the openshift/api bump tries to pull in the rebase to k8s 1.21. This doesn't actually happen in this PR because of the replaces below.

Hmm, should we drop the replaces? Who is responsible for fixing that?

adambkaplan · 2021-04-07T16:10:27Z

pkg/route/ingress/ingress.go

+		if err != nil {
+			return err
+		}
+		if ingressclass.Spec.Controller != routev1.IngressToRouteIngressClassControllerName {


Was this constant added before or after we started rebasing to k8s v1.21?

I'd rather not pull in go.mod changes that try to add k8s 1.21 deps in this PR. If this is the only reason that we need an openshift/api bump, can we hard-code the controller name here and mark a TODO to use the api constant?

It was merged after. https://github.com/openshift/api/commits/abd04f148867b326211c90f798235565572efd01

I dropped the openshift/api bump from this PR and replaced routev1.IngressToRouteIngressClassControllerName with "openshift.io/ingress-to-route", with a TODO comment per your suggestion.

adambkaplan · 2021-04-07T16:13:11Z

pkg/route/ingress/ingress.go

+var validOwnerRefAPIVersions = sets.NewString(
+	"networking.k8s.io/v1",
+	"networking.k8s.io/v1beta1",
+	"extensions.k8s.io/v1beta1",


If IIRC, extensions.k8s.io/v1beta1 has been removed in k8s, and was not previously allowed. Why are we allowing this apiVersion here?

We allow it because a route might have been created with an owner reference that uses the old apiVersion. See https://bugzilla.redhat.com/show_bug.cgi?id=1896678
for example.

Miciah · 2021-04-07T18:22:45Z

How will upgrades work? What will happen to existing routes linked to v1beta1 Ingress instances?

For existing routes, we tolerate old apiVersion values in owner references (thus the validOwnerRefAPIVersions definition that you commented on). I included a unit test for a route with an old apiVersion. Did you have further concerns?

Update the ingress-to-route controller and unit tests to use the networking.k8s.io/v1 API, implement IngressClass, and migrate routes created using older ingress API versions. * pkg/cmd/controller/ingress.go: Update to import and use networking/v1 packages. (RunIngressToRouteController): Pass ingressclass client to NewController. * pkg/route/ingress/ingress.go: Update to import and use networking/v1 packages. (Controller): Add ingressclassLister field. (NewController): Add parameter for ingressclasses informer. Use it to get ingressclassLister. (sync): Check the ingress's kubernetes.io/ingress.class annotation and spec.ingressClassName field. If an ingressclass is specified, look it up and check its spec.controller field value. If the ingress specifies an ingressclass that does not specify the openshift.io/ingress-to-route ingressclass, ignore the ingress. When patching an ingress, replace the existing ownerReferences field value in case the API version has changed. (validOwnerRefAPIVersions): New variable. (hasIngressOwnerRef): Use validOwnerRefAPIVersions to verify that the owner reference specifies some recognized API version. (newRouteForIngress): Update to use the ingress's spec.backend.service field to get the target service. Use the networking.k8s.io/v1 API version in the returned route's owner reference. (routeMatchesIngress): Update to use the ingress's spec.backend.service field. Verify that the route's owner reference uses the networking.k8s.io/v1 API version. (targetPortForService): Replace the ingress path parameter with an ingress service backend parameter. * pkg/route/ingress/ingress_test.go: Update to import and use networking/v1 packages. (ingressclassLister): New type. Implement a fake ingressclass lister. (List, Get): New methods for ingressclassLister. (complexIngress): Update to the v1 API. (TestController_stabilizeAfterCreate): Use a fake ingressclass lister. (TestController_sync): Define fake ingressclass lister with an ingressclass that specifies the OpenShift ingress controller. Update ingresses in test cases to the v1 API. Update test cases to expect the controller to patch ownerReferences. Add test cases: "ignores ingress with third-party ingressclass", "ignores ingress with unsupported path type", "create route - default ingresscontroller", "create route - custom ingresscontroller", "no-op - ignore route created for an ingress with a third-party class", and "update route with old owner reference". Augment the "create route" test case to verify correct behavior for the implemented path types.

Miciah · 2021-04-07T21:39:53Z

The e2e-aws-upgrade job failed because Multus failed to connect to the API during sandbox creation:

ns/e2e-k8s-sig-apps-deployment-upgrade-1803 pod/dp-5c7b6d4df4-bx5ns node/ip-10-0-165-53.us-east-2.compute.internal - never deleted - reason/FailedCreatePodSandBox Failed to create pod sandbox: rpc error: code = Unknown desc = failed to create pod network sandbox k8s_dp-5c7b6d4df4-bx5ns_e2e-k8s-sig-apps-deployment-upgrade-1803_59d8644d-16a3-4f12-866f-405bca01a09e_0(c32abc15ea162bca6231b3b43333d06c95e88e4cf01222440e9f5ebdd85d5d21): Multus: [e2e-k8s-sig-apps-deployment-upgrade-1803/dp-5c7b6d4df4-bx5ns]: error getting pod: Get "https://[api-int.ci-op-i8tr8bfk-3e1a2.origin-ci-int-aws.dev.rhcloud.com]:6443/api/v1/namespaces/e2e-k8s-sig-apps-deployment-upgrade-1803/pods/dp-5c7b6d4df4-bx5ns?timeout=1m0s": dial tcp 10.0.205.58:6443: connect: connection refused

/test e2e-aws-upgrade

bparees · 2021-04-08T17:01:51Z

/approve

sgreene570 · 2021-04-08T17:08:08Z

/lgtm

openshift-ci-robot · 2021-04-08T17:08:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, Miciah, sgreene570

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:

~~OWNERS~~ [bparees]

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 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 16, 2021

openshift-ci-robot requested review from adambkaplan and knobunc March 16, 2021 03:03

Miciah mentioned this pull request Mar 17, 2021

ingress: Add transition-ingress-from-beta-to-stable enhancement openshift/enhancements#697

Merged

4 tasks

Miciah force-pushed the ingress-migrate-to-networking.k8s.io-slash-v1 branch 2 times, most recently from 5c1311a to 396a918 Compare April 5, 2021 19:00

Miciah changed the title ~~WIP: ingress: Migrate to networking.k8s.io/v1~~ ingress: Migrate to networking.k8s.io/v1 Apr 5, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2021

sgreene570 reviewed Apr 5, 2021

View reviewed changes

Miciah force-pushed the ingress-migrate-to-networking.k8s.io-slash-v1 branch from 396a918 to 6d88436 Compare April 5, 2021 22:11

Miciah force-pushed the ingress-migrate-to-networking.k8s.io-slash-v1 branch from 6d88436 to fbf918e Compare April 5, 2021 22:35

openshift-ci-robot assigned sgreene570 Apr 6, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2021

adambkaplan suggested changes Apr 7, 2021

View reviewed changes

Miciah force-pushed the ingress-migrate-to-networking.k8s.io-slash-v1 branch from fbf918e to 0211318 Compare April 7, 2021 18:48

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2021

openshift-merge-robot merged commit 461fe64 into openshift:master Apr 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ingress: Migrate to networking.k8s.io/v1 #172

ingress: Migrate to networking.k8s.io/v1 #172

Miciah commented Mar 16, 2021

sgreene570 Apr 5, 2021

Miciah Apr 5, 2021

sgreene570 Apr 5, 2021

Miciah Apr 5, 2021

sgreene570 commented Apr 5, 2021

Miciah commented Apr 5, 2021

Miciah commented Apr 5, 2021

Miciah commented Apr 6, 2021

Miciah commented Apr 6, 2021

sgreene570 commented Apr 6, 2021

Miciah commented Apr 7, 2021

adambkaplan left a comment

adambkaplan Apr 7, 2021

Miciah Apr 7, 2021

adambkaplan Apr 7, 2021

Miciah Apr 7, 2021

Miciah Apr 7, 2021 •

edited

Loading

adambkaplan Apr 7, 2021

Miciah Apr 7, 2021

Miciah commented Apr 7, 2021

Miciah commented Apr 7, 2021

bparees commented Apr 8, 2021

sgreene570 commented Apr 8, 2021

openshift-ci-robot commented Apr 8, 2021

ingress: Migrate to networking.k8s.io/v1 #172

ingress: Migrate to networking.k8s.io/v1 #172

Conversation

Miciah commented Mar 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgreene570 commented Apr 5, 2021

Miciah commented Apr 5, 2021

Miciah commented Apr 5, 2021

Miciah commented Apr 6, 2021

Miciah commented Apr 6, 2021

sgreene570 commented Apr 6, 2021

Miciah commented Apr 7, 2021

adambkaplan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miciah Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miciah commented Apr 7, 2021

Miciah commented Apr 7, 2021

bparees commented Apr 8, 2021

sgreene570 commented Apr 8, 2021

openshift-ci-robot commented Apr 8, 2021

Miciah Apr 7, 2021 •

edited

Loading