reconcile ClusterIngress into VirtualService #2189

lichuqiang · 2018-10-09T02:59:13Z

Part of #1963
Follow-up #2151

Proposed Changes

update ingress defaulting and validation
update test dependency
add ingress reconciler to reconcile ClusterIngress into VirtualService

Release Note

reconcile ClusterIngress into VirtualService

/area networking

knative-prow-robot

@lichuqiang: 10 warnings.

In response to this:

Part of #1963
Follow-up #2151

Proposed Changes

update ingress defaulting and validation

update test dependency

add ingress reconciler to reconcile ClusterIngress into VirtualService

Release Note
reconcile ClusterIngress into VirtualService
/area networking

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/v1alpha1/testing/listers.go

pkg/reconciler/v1alpha1/clusteringress/resources/virtual_service.go

pkg/reconciler/v1alpha1/clusteringress/clusteringress.go

pkg/reconciler/v1alpha1/clusteringress/resources/names/names.go

knative-prow-robot · 2018-10-09T02:59:24Z

pkg/apis/networking/v1alpha1/clusteringress_defaults.go

+)
+
+var (
+	DefaultTimeout    = 60 * time.Second


Golint comments: exported var DefaultTimeout should have comment or be unexported. More info.

pkg/apis/networking/v1alpha1/clusteringress_types.go

knative-prow-robot

@lichuqiang: 3 unresolved warnings and 2 new warnings.

In response to this:

/lint

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 · 2018-10-09T04:01:16Z

pkg/apis/networking/v1alpha1/clusteringress_defaults.go

+)
+
+const (
+	// Default to 60 seconds if timeout not specified.


Golint comments: comment on exported const DefaultTimeout should be of the form "DefaultTimeout ...". More info.

knative-prow-robot · 2018-10-09T04:01:16Z

pkg/apis/networking/v1alpha1/clusteringress_defaults.go

+const (
+	// Default to 60 seconds if timeout not specified.
+	DefaultTimeout = 60 * time.Second
+	// Default to retry for 3 times if not specified.


Golint comments: comment on exported const DefaultRetryCount should be of the form "DefaultRetryCount ...". More info.

knative-prow-robot

@lichuqiang: 3 unresolved warnings and 0 new warnings.

In response to this:

/lint

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.

lichuqiang · 2018-10-09T07:22:06Z

/assign @tcnghia @mattmoor

mattmoor · 2018-10-09T22:20:39Z

/test pull-knative-serving-integration-tests

mattmoor

Mostly nits throughout.

I'd like @tcnghia or @ZhiminXiang to do a pass over this before I approve, but I think chunks of this could sail through if you have any interest in breaking it up.

Also, I'd be fine with adding the reconciler to ./cmd/controller/main.go now, since it makes trying these resources manually practical.

mattmoor · 2018-10-10T16:23:41Z

pkg/apis/networking/register.go

+
+	// RouteLabelKey is the label key attached to ClusterIngress resources
+	// to indicate which Route triggered their creation.
+	RouteLabelKey = GroupName + "/route"


This feels like a layering violation. I think I'd expect:

serving.knative.dev/route: foo

I would like to avoid the Ingress resources knowing anything about Routes or our other resources.

Aha, I ignored the fact that ingress and route resource are not in a same group.
So move this definition to the register file of route?

Yeah that SGTM

mattmoor · 2018-10-10T16:24:03Z

pkg/apis/networking/register.go

+
+	// IngressLabelKey is the label key attached to underlying network programming
+	// resources to indicate which ClusterIngress triggered their creation.
+	IngressLabelKey = GroupName + "/clusteringress"


By contract, this is perfectly fine :)

*contrast. I blame the bouncy shuttle this morning

mattmoor · 2018-10-10T16:26:44Z

pkg/apis/networking/v1alpha1/clusteringress_defaults.go

+	}
+	if p.Retries.PerTryTimeout == nil {
+		p.Retries.PerTryTimeout = &metav1.Duration{Duration: DefaultTimeout}
+	}


Do you want to split these (./pkg/apis/networking/...) changes into a separate PR? They LGTM, thanks for the cleanup.

Sure, will do

mattmoor · 2018-10-10T16:32:02Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress.go

+		FilterFunc: controller.Filter(v1alpha1.SchemeGroupVersion.WithKind("ClusterIngress")),
+		Handler: cache.ResourceEventHandlerFuncs{
+			AddFunc:    impl.EnqueueControllerOf,
+			UpdateFunc: controller.PassNew(impl.EnqueueControllerOf),


There isn't enough information in OwnerReference for this to work properly.

See also: #1259

Thanks for reminding this.
Will construct a selection func instead, via label I think.

mattmoor · 2018-10-10T16:34:29Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress_test.go

+
+var (
+	ingressRules = []v1alpha1.ClusterIngressRule{
+		{


nit: if you move this to the previous line: {{ it will reduce indentation.

mattmoor · 2018-10-10T16:34:39Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress_test.go

+			},
+			HTTP: &v1alpha1.HTTPClusterIngressRuleValue{
+				Paths: []v1alpha1.HTTPClusterIngressPath{
+					{


same nit as above.

mattmoor · 2018-10-10T16:34:47Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress_test.go

+				Paths: []v1alpha1.HTTPClusterIngressPath{
+					{
+						Splits: []v1alpha1.ClusterIngressBackendSplit{
+							{


mattmoor · 2018-10-10T16:35:05Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress_test.go

+						},
+					},
+					Conditions: duckv1alpha1.Conditions{
+						{


I'm gonna stop now :)

mattmoor · 2018-10-11T03:25:37Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress.go

-			AddFunc:    impl.EnqueueControllerOf,
-			UpdateFunc: controller.PassNew(impl.EnqueueControllerOf),
+			AddFunc:    c.enqueueOwnerIngress(impl),
+			UpdateFunc: controller.PassNew(c.enqueueOwnerIngress(impl)),


I think I mistook this for the Route -> ClusterIngress relationship, which can't use OwnerReference.

Since ClusterIngress isn't namespaced, I believe my prior comment was false here (but accurate for when we move to ClusterIngress in the Route controller).

I apologize for the randomization, but you should be able to revert this change.

Yup, it can work from a functional standpoint, as we don't need namespace info for cluster-scoped resources.
But seeing it from a design perspective, the OwnerReference is supposed for "resources of same namespace", but not for cluster-scoped resources.
So I'm thinking if we should just get rid of this, to avoid possible semantic changes in the future.

That way, in EnqueueControllerOf, the key enqueued is indeed "virtualservice namespace+ingress name“: https://github.com/knative/pkg/blob/master/controller/controller.go#L129

And in reconciler we just throw away the namespace to make it work, I think the behavior a little "opportunistic", WDYT?

I'm happy to leave this, if you are. We could probably use a generalized impl.EnqueueLabvelOf(key) method in knative/pkg/controller to generalize this pattern a bit (I've now seen it a handful of places). Not for this PR.

We could probably use a generalized impl.EnqueueLabvelOf(key) method in knative/pkg/controller to generalize this pattern a bit (I've now seen it a handful of places)

That makes sense, we'll also need the kind of func in route->ingress.
I'll keep this in mind, and make it a cleanup after all the things are done

bbrowning · 2018-10-11T14:12:56Z

I'm curious how we see alternative ClusterIngress implementations being slotted in here. The ClusterIngress reconciler and the Serving Controller have explicit knowledge of VirtualService. Do we expect these resources to also have direct knowledge of other ClusterIngress implementations? Or is there some future work to separate out this knowledge so that ClusterIngress isn't directly tied to VirtualService?

mattmoor · 2018-10-11T14:30:12Z

@bbrowning The basic idea is to start decoupling by creating this abstraction to go through. Even with this implementation "bundled" (for now), I am hoping that we can quickly reach a point where alternative implementations may reconcile these resource by running a parallel controller that handles:

annotations:
  networking.knative.dev/ingress.class: their-class

Annotations should be passed through from the Route (and the Service to the Route), so annotating those should be able to override default Ingress selection. I also think that we will want a config setting for the default implementation we use. It'll take a few more steps to reach that point.

Ultimately, I could see us splitting off knative/networking for our Type definitions (like knative/caching) and knative/net-istio for the Istio implementation. You can see that here. This is likely further out.

mattmoor · 2018-10-11T14:31:38Z

/assign @ZhiminXiang

for LGTM, and then I'll approve

bbrowning · 2018-10-11T14:52:19Z

@mattmoor Thanks for the explanation. This is definitely a step in the right direction to allow that pluggability 👍

ZhiminXiang

Reviewed some of the files. Will try to finish the rest of files within this week.

pkg/reconciler/v1alpha1/clusteringress/clusteringress.go

ZhiminXiang · 2018-10-11T23:30:01Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress.go

+	original, err := c.clusterIngressLister.Get(name)
+	if apierrs.IsNotFound(err) {
+		// The resource may no longer exist, in which case we stop processing.
+		c.Logger.Errorf("clusteringress %q in work queue no longer exists", key)


Is there any particular reason to not use the logger in the context?

logger = logging.FromContext(ctx)

Ah, not really.
Will update this for consistency

ZhiminXiang · 2018-10-11T23:38:38Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress.go

+	return err
+}
+
+// Update the Status of the route.  Caller is responsible for checking


nit: s/route/clusteringress

ZhiminXiang

Still reviewing...

ZhiminXiang · 2018-10-12T04:45:41Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress.go

+}
+
+// Reconcile compares the actual state with the desired, and attempts to
+// converge the two. It then updates the Status block of the Service resource


nit: s/Service/ClusterIngress

ZhiminXiang · 2018-10-12T05:54:49Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress.go

+	})
+
+	virtualServiceInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
+		AddFunc:    c.enqueueOwnerIngress(impl),


Could we use EnqueueControllerOf and follow the pattern ?
If this is feasible, then we don't need enqueueOwnerIngress function here.

I had a discussion with @mattmoor about this above: #2189 (comment)

Would like to hear your opinion on the point as well :)

ah. Sorry I didn't see your discussion when I reviewed this part.
I get your point now. Agree that we probably need a generalized impl.EnqueueLabvelOf(key) method. we can do it later.

pkg/reconciler/v1alpha1/clusteringress/resources/virtual_service.go

ZhiminXiang

Thanks for working on this @lichuqiang ! Generally this PR looks good to me. Do you have any yaml example for ClusterIngress and its corresponding VirtualService? It would be great if we can paste an example here.

ZhiminXiang · 2018-10-12T20:42:08Z

pkg/reconciler/v1alpha1/clusteringress/resources/virtual_service.go

+}
+
+func timeoutToString(timeout time.Duration) string {
+	return fmt.Sprintf("%ds", int(timeout.Truncate(time.Second).Seconds()))


I am a bit worried about using Truncate with Second here. For example, if the original timeout is 10.5 seconds, it will be truncated as 10 seconds, which seems not accurate enough.
I am wondering if Istio accepts milliseconds as timeout unit. If it does, maybe we can consider to truncate the timeout with milliseconds.

Hmm, can we just serialize the time.Duration? timeout.String() should do it.

The type of the field in istio is indeed of protobuf.Duration, which is made up of Seconds and Nanos:

type Duration struct { // Signed seconds of the span of time. Must be from -315,576,000,000 // to +315,576,000,000 inclusive. Note: these bounds are computed from: // 60 sec/min * 60 min/hr * 24 hr/day * 365.25 days/year * 10000 years Seconds int64 `protobuf:"varint,1,opt,name=seconds,proto3" json:"seconds,omitempty"` // Signed fractions of a second at nanosecond resolution of the span // of time. Durations less than one second are represented with a 0 // `seconds` field and a positive or negative `nanos` field. For durations // of one second or more, a non-zero value for the `nanos` field must be // of the same sign as the `seconds` field. Must be from -999,999,999 // to +999,999,999 inclusive. Nanos int32 `protobuf:"varint,2,opt,name=nanos,proto3" json:"nanos,omitempty"` XXX_NoUnkeyedLiteral struct{} `json:"-"` XXX_unrecognized []byte `json:"-"` XXX_sizecache int32 `json:"-"` }

Have not looked in to that deep, bu I'm afraid it can not understand time duration formatted string so "smartly", and to support milliseconds in timeout, we might need to make it a float num for seconds, e.g "1.00123s"

Aha, I underestimated the compatibility of protobuf, it makes use of built-in time functions when unmarshal: https://github.com/gogo/protobuf/blob/master/jsonpb/jsonpb.go#L870,
so it can recognize time duration formatted string. Let's keep the passed in duration as is, and remove the extra truncation.

lichuqiang · 2018-10-13T09:53:17Z

Since previous comments have been addressed, I've squashed the commits generated when addressing the comments, for reading/reviewing convenience :)

ZhiminXiang · 2018-10-13T16:25:12Z

/lgtm
/approve

knative-prow-robot · 2018-10-15T02:33:05Z

@lichuqiang: GitHub didn't allow me to request PR reviews from the following users: for, approve.

Note that only knative members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @mattmoor @tcnghia for approve

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.

lichuqiang · 2018-10-15T15:34:48Z

pkg/reconciler/v1alpha1/clusteringress/resources/virtual_service.go

+	vs.Labels[networking.IngressLabelKey] = ci.Name
+
+	ingressLabels := ci.Labels
+	vs.Labels[serving.RouteLabelKey] = ingressLabels[serving.RouteLabelKey]


I meant to make the label value "namespace/name", but when modifying the route reconciler, I realized "/" is not allowed in label value. So I think here we'll need another label for route namespace.

tcnghia

/approve

tcnghia · 2018-10-15T22:29:37Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress_test.go

+		Key:                     "foo/not-found",
+		SkipNamespaceValidation: true,
+	}, {
+		Name:                    "create VirtualService matching ClisterIngress",


typo: ClusterIngress

tcnghia · 2018-10-15T22:48:19Z

/assign @evankanderson

who can approve changes in /pkg/apis/...

knative-metrics-robot · 2018-10-16T01:43:09Z

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
pkg/reconciler/v1alpha1/clusteringress/clusteringress.go	Do not exist	61.5%
pkg/reconciler/v1alpha1/clusteringress/resources/names/names.go	Do not exist	100.0%
pkg/reconciler/v1alpha1/clusteringress/resources/virtual_service.go	Do not exist	100.0%

vaikas · 2018-10-19T15:48:32Z

/lgtm
/approve

knative-prow-robot · 2018-10-19T15:48:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lichuqiang, tcnghia, vaikas-google, ZhiminXiang

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:

~~cmd/controller/OWNERS~~ [tcnghia,vaikas-google]
~~pkg/apis/OWNERS~~ [vaikas-google]
~~pkg/reconciler/OWNERS~~ [tcnghia,vaikas-google]

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

vaikas · 2018-10-19T15:32:40Z

pkg/apis/serving/register.go

 	RouteLabelKey = GroupName + "/route"

+	// RouteNamespaceLabelKey is the label key attached to a ClusterIngress indicating by


This reads little funny. Maybe:
RouteNamespaceLabelKey is the label key attached to a ClusterIngress by a Route to indicate which namespace the Route was created in.

I'll address this in a follow-up PR

vaikas · 2018-10-19T15:44:21Z

pkg/reconciler/v1alpha1/clusteringress/clusteringress.go

+		return existing, nil
+	}
+	existing.Status = ci.Status
+	// TODO: for CRD there's no updatestatus, so use normal update.


We should (unless we already have) make a tracking bug since CRDs now have /status:
https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/

Not blocking this PR, just jotting it down here.

I've also noticed this before, but have not got time to look deeper.
Not sure it's just cruft due to historic reasons, or currently there's still bug in UpdateStatus for CRDs that I'm not aware of .
Will open an issue for further discussion anyway.

vaikas · 2018-10-19T17:45:23Z

oops, was fine to merge, but had couple of comments that I had failed to submit. My bad.

* update test dependency * helper funcs to construct virtualservice * reconcile ClusterIngress into VirtualService * enable ingress reconciler in controller * add route namespace label

knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/networking labels Oct 9, 2018

knative-prow-robot requested review from jonjohnsonjr and tcnghia October 9, 2018 02:59

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 9, 2018

knative-prow-robot reviewed Oct 9, 2018

View reviewed changes

lichuqiang force-pushed the ingress_reconciler branch from a814fc8 to 6fd1711 Compare October 9, 2018 03:15

knative-prow-robot reviewed Oct 9, 2018

View reviewed changes

lichuqiang force-pushed the ingress_reconciler branch from 1dc61dc to e7611c1 Compare October 9, 2018 06:13

lichuqiang changed the title ~~WIP: reconcile ClusterIngress into VirtualService~~ reconcile ClusterIngress into VirtualService Oct 9, 2018

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 Oct 9, 2018

knative-prow-robot assigned mattmoor and tcnghia Oct 9, 2018

lichuqiang force-pushed the ingress_reconciler branch from 3c4dec1 to d95741e Compare October 10, 2018 09:10

mattmoor reviewed Oct 10, 2018

View reviewed changes

mattmoor reviewed Oct 11, 2018

View reviewed changes

lichuqiang mentioned this pull request Oct 11, 2018

ingress api cleanup #2214

Merged

lichuqiang force-pushed the ingress_reconciler branch from a7d630f to a845852 Compare October 11, 2018 08:37

knative-prow-robot assigned ZhiminXiang Oct 11, 2018

ZhiminXiang reviewed Oct 12, 2018

View reviewed changes

lichuqiang force-pushed the ingress_reconciler branch from 6f169da to da9e3f4 Compare October 12, 2018 10:28

ZhiminXiang reviewed Oct 12, 2018

View reviewed changes

lichuqiang added 3 commits October 13, 2018 16:01

helper funcs to construct virtualservice

8685b8e

reconcile ClusterIngress into VirtualService

079160e

enable ingress reconciler in controller

de1b00f

lichuqiang force-pushed the ingress_reconciler branch from 7d872e5 to de1b00f Compare October 13, 2018 08:04

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2018

knative-prow-robot requested a review from mattmoor October 15, 2018 02:33

lichuqiang commented Oct 15, 2018

View reviewed changes

tcnghia approved these changes Oct 15, 2018

View reviewed changes

knative-prow-robot assigned evankanderson Oct 15, 2018

add route namespace label

4b27af1

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 16, 2018

lichuqiang mentioned this pull request Oct 17, 2018

Update route reconciler to create ClusterIngress instead of VirtualService #2243

Merged

knative-prow-robot assigned vaikas Oct 19, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2018

knative-prow-robot merged commit db2efd8 into knative:master Oct 19, 2018

vaikas reviewed Oct 19, 2018

View reviewed changes

		RouteLabelKey = GroupName + "/route"

		// RouteNamespaceLabelKey is the label key attached to a ClusterIngress indicating by

reconcile ClusterIngress into VirtualService #2189

reconcile ClusterIngress into VirtualService #2189

Conversation

lichuqiang commented Oct 9, 2018

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Choose a reason for hiding this comment

knative-prow-robot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-prow-robot left a comment

Choose a reason for hiding this comment

lichuqiang commented Oct 9, 2018

mattmoor commented Oct 9, 2018

mattmoor 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

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

lichuqiang Oct 10, 2018 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbrowning commented Oct 11, 2018

mattmoor commented Oct 11, 2018

mattmoor commented Oct 11, 2018

bbrowning commented Oct 11, 2018

ZhiminXiang 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

ZhiminXiang 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

ZhiminXiang 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

Choose a reason for hiding this comment

lichuqiang commented Oct 13, 2018 • edited Loading

ZhiminXiang commented Oct 13, 2018

knative-prow-robot commented Oct 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcnghia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcnghia commented Oct 15, 2018

knative-metrics-robot commented Oct 16, 2018

vaikas commented Oct 19, 2018

knative-prow-robot commented Oct 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lichuqiang Oct 20, 2018 • edited Loading

Choose a reason for hiding this comment

lichuqiang Oct 10, 2018 •

edited

Loading

lichuqiang commented Oct 13, 2018 •

edited

Loading

lichuqiang Oct 20, 2018 •

edited

Loading