Add Ingress controller implementation and logic in route controller to reconcile ingresses by wtam2018 · Pull Request #4346 · knative/serving

wtam2018 · 2019-06-13T04:41:17Z

Add ingress controller for Fixes #3982. This is a part of the overall implementation if namespaced ingress. This PR covers the Ingress controller and its resources.

Proposed Changes

Create Ingress controller based on clusteringress
Add label keys for Ingress and IngressNamespace
ClusterIngress label will be "/clusteringress" instead of "/ingress"

Release Note

knative-prow-robot · 2019-06-13T04:41:32Z

Hi @wtam2018. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

knative-prow-robot

@wtam2018: 1 warning.

Details

In response to this:

Add ingress controller for Fixes #3982. This is a part of the overall implementation if namespaced ingress. This PR covers the Ingress controller and its resources.

Proposed Changes

Create Ingress controller based on clusteringress

Add label keys for Ingress and IngressNamespace

ClusterIngress label will be "/clusteringress" instead of "/ingress"

Release Note

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.

pkg/reconciler/ingress/ingress.go

wtam2018 · 2019-06-13T04:54:01Z

@vagababov I tried to split the PR into purely renaming and logic/other changes. However, I am not sure how feasible it is (to make it compiled and tested) because renamed objects are going to be used differently. I've broken down the PR into small ones. This PR is on the ingress controller side. It is mainly copied from its clusteringress counterpart. I hope it is more reviewable.

Co-Authored-By: mattmoor-sockpuppet <mattmoor+sockpuppet@google.com>

wtam2018 · 2019-06-18T22:44:59Z

/test pull-knative-serving-unit-tests

wtam2018 · 2019-06-19T00:43:47Z

/test pull-knative-serving-smoke-tests

wtam2018 · 2019-06-19T12:46:57Z

/test pull-knative-serving-upgrade-tests

markusthoemmes

Only flyby comments, this is still quite a huge diff to properly review. I'm certainly not a good reviewer for this.

markusthoemmes · 2019-06-19T12:59:40Z

pkg/apis/networking/register.go

+
+	// IngressNamespaceLabelKey is the label key attached to underlying network programming
+	// to indicate which namespace the Ingress was created in.
+	IngressNamespaceLabelKey = GroupName + "/ingressNamespace"


Why is this label needed?

These labels are put on virtual services to identify creators/owners. Ingress controller listens to Virtual Service Informer events come from resources with these labels attached.

markusthoemmes · 2019-06-19T13:00:47Z

pkg/reconciler/ingress/resources/virtual_service.go

+// VirtualServices for a given Ingress.
+func IngressVirtualServiceNamespace(i *v1alpha1.Ingress) string {
+	return i.Namespace
+}


This seems kinda redundant, why not use i.Namespace on the calling side directly?

This method has been consolidated to conditionally return i.Namespace or system.Namespace.

markusthoemmes · 2019-06-19T13:04:14Z

pkg/reconciler/ingress/controller.go

+
+	virtualServiceInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
+		FilterFunc: myFilterFunc,
+		Handler: controller.HandleAll(impl.EnqueueLabelOfNamespaceScopedResource(networking.IngressNamespaceLabelKey,


Shouldn't this controller only handle namespaced virtual services in the first place? As such the namespace of the VS should be == that of the ingress and thus the label wouldn't be needed.

This controller only handles namespaced Ingress (I think that is what you mean). Virtual Services have always been namespaced. Yes, Ingress and its VSes have same namespace. I think we still need these labels to add event handler and handle events to reconcile these children (VSes). These labels are used to find/delete children resources.

markusthoemmes · 2019-06-19T13:07:56Z

pkg/reconciler/route/controller.go


+	ingressInformer.Informer().AddEventHandler(controller.HandleAll(
+		impl.EnqueueLabelOfNamespaceScopedResource(
+			serving.RouteNamespaceLabelKey, serving.RouteLabelKey)))


The namespaces should match.

I think they do, right?

markusthoemmes · 2019-06-19T13:09:22Z

pkg/reconciler/route/reconcile_resources.go

+		return ingress, nil
+	}
+
+	// If that isn't found, then fallback on the legacy selector-based approach.


I'm pretty sure there's no legacy methods needed for the all-new ingress? I think all code below this is dead.

I think you are right. There is unit test that verifies the legacy method (targeted to clusteringress) which also applied to new ingress at the moment. We will clean this (Ingress) up and remove the obsolete unit test when we remove clusteringress.

markusthoemmes · 2019-06-19T13:12:13Z

pkg/reconciler/route/reconcile_resources.go

 	clusterIngress, err := c.getClusterIngressForRoute(r)
 	if apierrs.IsNotFound(err) {
+		// As an optimization, we don't create a cluster ingress if it does not exist already.  Ingress will take over
+		if !shouldCreateNew {


Is this set to true anywhere besides tests? If not, shall we ditch it?

It is set to true in some unit tests to exercise clusteringress reconciliation.

markusthoemmes · 2019-06-19T13:13:14Z

pkg/reconciler/route/resources/cluster_ingress.go


+// MakeIngress creates ClusterIngress to set up routing rules. Such ClusterIngress specifies
+// which Hosts that it applies to, as well as the routing rules.
+func MakeIngress(ctx context.Context, r *servingv1alpha1.Route, tc *traffic.Config, tls []v1alpha1.IngressTLS, ingressClass string) (*v1alpha1.Ingress, error) {


Should this be moved to a new file ingress.go in the same package?

markusthoemmes · 2019-06-19T13:14:29Z

pkg/reconciler/route/resources/service.go

-	ingressStatus := ingress.Status
-	if ingressStatus.LoadBalancer == nil || len(ingressStatus.LoadBalancer.Ingress) == 0 {
+func makeServiceSpec(ingressAccessor netv1alpha1.IngressAccessor) (*corev1.ServiceSpec, error) {
+	//ingressStatus := ingress.Status


Remove this line?

wtam2018 · 2019-06-19T13:19:13Z

/test pull-knative-serving-integration-tests

wtam2018 · 2019-06-20T12:20:34Z

/test pull-knative-serving-integration-tests

knative-metrics-robot · 2019-06-20T14:58:30Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/networking/v1alpha1/clusteringress_accessors.go	Do not exist	100.0%
pkg/apis/networking/v1alpha1/ingress_accessors.go	Do not exist	100.0%
pkg/reconciler/ingress/config/istio.go	Do not exist	96.9%
pkg/reconciler/ingress/config/store.go	Do not exist	100.0%
pkg/reconciler/ingress/controller.go	Do not exist	100.0%
pkg/reconciler/ingress/ingress.go	Do not exist	77.6%
pkg/reconciler/ingress/resources/gateway.go	Do not exist	95.2%
pkg/reconciler/ingress/resources/names/names.go	Do not exist	100.0%
pkg/reconciler/ingress/resources/secret.go	Do not exist	83.3%
pkg/reconciler/ingress/resources/virtual_service.go	Do not exist	100.0%
pkg/reconciler/route/reconcile_resources.go	85.3%	83.6%	-1.7
pkg/reconciler/route/resources/ingress.go	Do not exist	87.3%
pkg/reconciler/route/resources/service.go	85.7%	85.3%	-0.4
pkg/reconciler/route/route.go	84.7%	85.5%	0.9

wtam2018 · 2019-06-20T15:22:16Z

/test pull-knative-serving-upgrade-tests

wtam2018 · 2019-06-20T16:41:54Z

/test pull-knative-serving-upgrade-tests

wtam2018 · 2019-06-20T20:24:56Z

/test pull-knative-serving-upgrade-tests

knative-prow-robot · 2019-06-20T20:33:37Z

@wtam2018: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-knative-serving-upgrade-tests	`5dd349e`	link	`/test pull-knative-serving-upgrade-tests`

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.

wtam2018 · 2019-06-21T18:22:15Z

Close this PR and redo with smaller PRs.

wtam2018 added 6 commits June 12, 2019 15:57

move clusteringress ingress

136a092

mv clusteringress -> clusteringress-magic

83a41bd

trivitally merged

fb9f2bc

mv clusteringress-magic -> clusteringress

ccb60dc

rename clusteringress -> ingress

f8e56b2

add ingress controller

e4f2fca

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 13, 2019

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 13, 2019

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 13, 2019

knative-prow-robot reviewed Jun 13, 2019

View reviewed changes

pkg/reconciler/ingress/ingress.go Outdated Show resolved Hide resolved

mattmoor-sockpuppet reviewed Jun 13, 2019

View reviewed changes

knative-prow-robot added area/API API objects and controllers area/networking labels Jun 13, 2019

knative-prow-robot requested review from dgerd, lichuqiang and markusthoemmes June 13, 2019 04:41

eliminate else clause behind return statement

de613dc

mattmoor-sockpuppet reviewed Jun 13, 2019

View reviewed changes

Update pkg/reconciler/ingress/config/store.go

e20fac3

Co-Authored-By: mattmoor-sockpuppet <mattmoor+sockpuppet@google.com>

mattmoor-sockpuppet reviewed Jun 13, 2019

View reviewed changes

wtam2018 changed the title ~~Ingress8~~ Add Ingress controller implementation Jun 13, 2019

wtam2018 and others added 7 commits June 13, 2019 01:00

Update pkg/reconciler/ingress/config/doc.go

8ef8f88

Co-Authored-By: mattmoor-sockpuppet <mattmoor+sockpuppet@google.com>

Update pkg/reconciler/ingress/config/store_test.go

00c3f31

Co-Authored-By: mattmoor-sockpuppet <mattmoor+sockpuppet@google.com>

Update pkg/reconciler/ingress/resources/names/doc.go

3ec2583

Co-Authored-By: mattmoor-sockpuppet <mattmoor+sockpuppet@google.com>

Update pkg/reconciler/ingress/resources/names/names.go

36c60c2

Co-Authored-By: mattmoor-sockpuppet <mattmoor+sockpuppet@google.com>

Update pkg/reconciler/ingress/resources/virtual_service.go

d7534fe

Co-Authored-By: mattmoor-sockpuppet <mattmoor+sockpuppet@google.com>

Update pkg/reconciler/ingress/resources/virtual_service_test.go

2c08edd

Co-Authored-By: mattmoor-sockpuppet <mattmoor+sockpuppet@google.com>

Update pkg/reconciler/ingress/resources/names/names_test.go

5cbc353

Co-Authored-By: mattmoor-sockpuppet <mattmoor+sockpuppet@google.com>

moved resources from clusteringress 0> ingress folder

8e89d4b

knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Jun 18, 2019

remove -virtualservice suffix

2a91a8f

fixed tests

e8c40c5

route reconcller to reconcile namespaced ingress

6a63dd0

wtam2018 changed the title ~~Add Ingress controller implementation~~ WIP: Add Ingress controller implementation and logic in route controller to reconcile ingresses Jun 19, 2019

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 19, 2019

markusthoemmes reviewed Jun 19, 2019

View reviewed changes

wtam2018 added 3 commits June 20, 2019 01:23

refactored and got rid of *ForIngress/ForClusterIngress methods

82fe082

get rid of GetSecretForXXX methods()

89654c7

remove dead/commented line

6b4c1e7

wtam2018 added 3 commits June 20, 2019 09:08

move cluster_ingress -> ingress

baef811

add unit tests

8a96990

add unit test

5dd349e

wtam2018 changed the title ~~WIP: Add Ingress controller implementation and logic in route controller to reconcile ingresses~~ Add Ingress controller implementation and logic in route controller to reconcile ingresses Jun 20, 2019

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 20, 2019

wtam2018 closed this Jun 21, 2019

This was referenced Feb 27, 2026

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#6

Closed

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#11

Merged

Conversation

wtam2018 commented Jun 13, 2019

Proposed Changes

Uh oh!

knative-prow-robot commented Jun 13, 2019

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

Uh oh!

wtam2018 commented Jun 13, 2019

Uh oh!

wtam2018 commented Jun 18, 2019

Uh oh!

wtam2018 commented Jun 19, 2019

Uh oh!

wtam2018 commented Jun 19, 2019

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtam2018 commented Jun 19, 2019

Uh oh!

wtam2018 commented Jun 20, 2019

Uh oh!

knative-metrics-robot commented Jun 20, 2019

Uh oh!

wtam2018 commented Jun 20, 2019

Uh oh!

wtam2018 commented Jun 20, 2019

Uh oh!

wtam2018 commented Jun 20, 2019

Uh oh!

knative-prow-robot commented Jun 20, 2019

Uh oh!

wtam2018 commented Jun 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants