Repository: knative/serving
Pull request #11518 (2021-06-14 06:45:50)

Make sure the defaultExternalScheme only applies to external URLs
<!-- Please include the 'why' behind your changes if no issue exists -->
## Proposed Changes

As per title. This edge-case came to me in a fever dream :joy:.

**Release Note**

<!-- Enter your extended release note in the below block. If the PR requires
additional action from users switching to the new release, include the string
"action required". If no release note is required, write "NONE". -->

```release-note
NONE
```

/assign @julz @nak3 


knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/11518#" title="Author self-approved">markusthoemmes</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fserving).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[pkg/reconciler/route/OWNERS](https://github.com/knative/serving/blob/main/pkg/reconciler/route/OWNERS)~~ [markusthoemmes]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/11518?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
> Merging [#11518](https://codecov.io/gh/knative/serving/pull/11518?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (a2d6c39) into [main](https://codecov.io/gh/knative/serving/commit/0f94773b2c4ed32c325e2b82a046d8040f832c54?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (0f94773) will **decrease** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/serving/pull/11518/graphs/tree.svg?width=650&height=150&src=pr&token=MPpDfAj4Ui&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)](https://codecov.io/gh/knative/serving/pull/11518?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)

```diff
@@            Coverage Diff             @@
##             main   #11518      +/-   ##
==========================================
- Coverage   87.75%   87.75%   -0.01%     
==========================================
  Files         191      191              
  Lines        9262     9269       +7     
==========================================
+ Hits         8128     8134       +6     
  Misses        881      881              
- Partials      253      254       +1     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/11518?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/route.go](https://codecov.io/gh/knative/serving/pull/11518/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcm91dGUuZ28=) | `79.69% <100.00%> (+0.31%)` | :arrow_up: |
| [pkg/reconciler/route/traffic/traffic.go](https://codecov.io/gh/knative/serving/pull/11518/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvdHJhZmZpYy90cmFmZmljLmdv) | `92.92% <100.00%> (+0.14%)` | :arrow_up: |
| [pkg/reconciler/configuration/configuration.go](https://codecov.io/gh/knative/serving/pull/11518/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL3JlY29uY2lsZXIvY29uZmlndXJhdGlvbi9jb25maWd1cmF0aW9uLmdv) | `84.61% <0.00%> (-2.31%)` | :arrow_down: |
| [pkg/autoscaler/statforwarder/leases.go](https://codecov.io/gh/knative/serving/pull/11518/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL2F1dG9zY2FsZXIvc3RhdGZvcndhcmRlci9sZWFzZXMuZ28=) | `78.41% <0.00%> (+1.43%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/serving/pull/11518?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/serving/pull/11518?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Last update [0f94773...a2d6c39](https://codecov.io/gh/knative/serving/pull/11518?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/11518#pullrequestreview-682594541" title="LGTM">julz</a>*, *<a href="https://github.com/knative/serving/pull/11518#" title="Author self-approved">markusthoemmes</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fserving).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[pkg/reconciler/route/OWNERS](https://github.com/knative/serving/blob/main/pkg/reconciler/route/OWNERS)~~ [julz,markusthoemmes]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/reconciler/route/route.go b/pkg/reconciler/route/route.go
index dc9668509..bccdede71 100644
--- a/pkg/reconciler/route/route.go
+++ b/pkg/reconciler/route/route.go
@@ -369,8 +369,12 @@ func (c *Reconciler) updateRouteStatusURL(ctx context.Context, route *v1.Route,
 		return err
 	}
 
+	scheme := "http"
+	if !isClusterLocal {
+		scheme = config.FromContext(ctx).Network.DefaultExternalScheme
+	}
 	route.Status.URL = &apis.URL{
-		Scheme: config.FromContext(ctx).Network.DefaultExternalScheme,
+		Scheme: scheme,
 		Host:   host,
 	}
 
diff --git a/pkg/reconciler/route/traffic/traffic.go b/pkg/reconciler/route/traffic/traffic.go
index 64465ff64..62eded78f 100644
--- a/pkg/reconciler/route/traffic/traffic.go
+++ b/pkg/reconciler/route/traffic/traffic.go
@@ -101,14 +101,20 @@ func (cfg *Config) computeURL(ctx context.Context, r *v1.Route, tt *RevisionTarg
 		return nil, err
 	}
 
-	labels.SetVisibility(meta, cfg.Visibility[tt.Tag] == netv1alpha1.IngressVisibilityClusterLocal)
+	isClusterLocal := cfg.Visibility[tt.Tag] == netv1alpha1.IngressVisibilityClusterLocal
+	labels.SetVisibility(meta, isClusterLocal)
 
 	fullDomain, err := domains.DomainNameFromTemplate(ctx, *meta, hostname)
 	if err != nil {
 		return nil, err
 	}
 
-	return domains.URL(config.FromContext(ctx).Network.DefaultExternalScheme, fullDomain), nil
+	scheme := "http"
+	if !isClusterLocal {
+		scheme = config.FromContext(ctx).Network.DefaultExternalScheme
+	}
+
+	return domains.URL(scheme, fullDomain), nil
 }
 
 func (cfg *Config) targetToStatus(ctx context.Context, r *v1.Route, tt *RevisionTarget,


Test patch:
diff --git a/pkg/reconciler/route/table_test.go b/pkg/reconciler/route/table_test.go
index 7827b984b..ee754a479 100644
--- a/pkg/reconciler/route/table_test.go
+++ b/pkg/reconciler/route/table_test.go
@@ -1994,7 +1994,7 @@ func TestReconcile(t *testing.T) {
 		}},
 		Key: "default/tooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo-long",
 	}, {
-		Name: "overridden schemes",
+		Name: "overridden scheme",
 		Ctx:  context.WithValue(context.Background(), externalSchemeKey, "https"),
 		Objects: []runtime.Object{
 			Route("default", "steady-state", WithConfigTarget("config"),
@@ -2032,8 +2032,51 @@ func TestReconcile(t *testing.T) {
 				WithExternalName(pkgnet.GetServiceHostname("private-istio-ingressgateway", "istio-system"))),
 		},
 		Key: "default/steady-state",
+	}, {
+		Name: "overridden scheme (cluster-local)",
+		Ctx:  context.WithValue(context.Background(), externalSchemeKey, "https"),
+		Objects: []runtime.Object{
+			Route("default", "steady-state", WithConfigTarget("config"),
+				WithRouteLabel(map[string]string{network.VisibilityLabelKey: serving.VisibilityClusterLocal}),
+				WithLocalDomain, WithAddress, WithRouteConditionsAutoTLSDisabled,
+				MarkTrafficAssigned, MarkIngressReady, WithRouteGeneration(1), WithRouteObservedGeneration,
+				WithRouteFinalizer, WithStatusTraffic(
+					v1.TrafficTarget{
+						RevisionName:   "config-00001",
+						Percent:        ptr.Int64(100),
+						LatestRevision: ptr.Bool(true),
+					})),
+			cfg("default", "config",
+				WithConfigGeneration(1), WithLatestCreated("config-00001"), WithLatestReady("config-00001"),
+				// The Route controller attaches our label to this Configuration.
+				WithConfigLabel("serving.knative.dev/route", "steady-state"),
+			),
+			rev("default", "config", 1, MarkRevisionReady, WithRevName("config-00001")),
+			simpleIngress(
+				Route("default", "steady-state", WithConfigTarget("config"),
+					WithRouteLabel(map[string]string{network.VisibilityLabelKey: serving.VisibilityClusterLocal})),
+				&traffic.Config{
+					Targets: map[string]traffic.RevisionTargets{
+						traffic.DefaultTarget: {{
+							TrafficTarget: v1.TrafficTarget{
+								ConfigurationName: "config",
+								LatestRevision:    ptr.Bool(true),
+								RevisionName:      "config-00001",
+								Percent:           ptr.Int64(100),
+							},
+						}},
+					},
+					Visibility: map[string]netv1alpha1.IngressVisibility{
+						traffic.DefaultTarget: netv1alpha1.IngressVisibilityClusterLocal,
+					},
+				},
+				withReadyIngress,
+			),
+			simpleK8sService(Route("default", "steady-state", WithConfigTarget("config")),
+				WithExternalName(pkgnet.GetServiceHostname("private-istio-ingressgateway", "istio-system"))),
+		},
+		Key: "default/steady-state",
 	}}
-
 	// TODO(mattmoor): Revision inactive (direct reference)
 	// TODO(mattmoor): Revision inactive (indirect reference)
 	// TODO(mattmoor): Multiple inactive Revisions