Serving: Revision's k8s service reconciliation doesn't take defaulter into account when diffing

[jonjohnsonjr]

This is similar to #1637

Expected Behavior

The revision reconciler should ignore fields that are set by the defaulter and not reconcile on those fields.

Actual Behavior

The revision reconciler fights the defaulter, per logs:

2018-08-14T22:01:29.434Z    revision/cruds.go:146   Reconciling service diff (-desired, +observed): {v1.ServiceSpec}.Ports[0].Protocol:
    -: v1.Protocol("")
    +: v1.Protocol("TCP")
{v1.ServiceSpec}.Ports[0].NodePort:
    -: 0
    +: 32020
{v1.ServiceSpec}.SessionAffinity:
    -: v1.ServiceAffinity("")
    +: v1.ServiceAffinity("None")
{v1.ServiceSpec}.ExternalTrafficPolicy:
    -: v1.ServiceExternalTrafficPolicyType("")
    +: v1.ServiceExternalTrafficPolicyType("Cluster")

Steps to Reproduce the Problem

1. Run one of the examples.
2. Look at controller logs.

[yolocs]

/assign

[knative-prow-robot]

@yolo3301: GitHub didn't allow me to assign the following users: yolo3301.

Note that only knative members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign

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.

[jonjohnsonjr]

/assign @yolo3301

[knative-prow-robot]

@jonjohnsonjr: GitHub didn't allow me to assign the following users: yolo3301.

Note that only knative members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign @yolo3301

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.

[yolocs]

hate you @knative-prow-robot

[yolocs]

@jonjohnsonjr I did an initial fix. But I still see diff like:

{v1.ServiceSpec}.Ports[0].Protocol:
    -: v1.Protocol("")
    +: v1.Protocol("TCP")
{v1.ServiceSpec}.Ports[0].NodePort:
    -: 0
    +: 32020

Does k8s add additional port by default?

[jonjohnsonjr]

Yeah see Type NodePort here.

If you set the type field to NodePort, the Kubernetes master will allocate a port from a range specified by --service-node-port-range flag (default: 30000-32767), and each Node will proxy that port (the same port number on every Node) into your Service. That port will be reported in your Service’s .spec.ports[*].nodePort field.

That's the crux of the issue here, I think, since we are clobbering it.

[tcnghia]

I am not sure we need NodePort for Revision's Service. We should be fine with ClusterIP.

@mattmoor @evankanderson if you have objection here.

[mattmoor]

@tcnghia I think you are right. NodePort sounds like a superset of ClusterIP based on:

A ClusterIP service, to which the NodePort service will route, is automatically created.

Let's switch to ClusterIP, if things work.