Repository: knative/serving
Pull request #9744 (2020-10-08 21:00:56)

Make docs about cluster local domains more accurate
The current config map example docs [are only true if the default cluster domain is being used](https://github.com/knative/serving/blob/7398ed7b7e45bb751e6aa475a8d2f2ab15398e44/pkg/reconciler/route/visibility/visibility.go#L68).

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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/9744#" title="Author self-approved">julz</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **markusthoemmes** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @markusthoemmes` in a comment when ready.

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

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

- **[config/OWNERS](https://github.com/knative/serving/blob/master/config/OWNERS)**

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":["markusthoemmes"]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/9744?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@e246e56`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/knative/serving/pull/9744/graphs/tree.svg?width=650&height=150&src=pr&token=MPpDfAj4Ui)](https://codecov.io/gh/knative/serving/pull/9744?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##             master    #9744   +/-   ##
=========================================
  Coverage          ?   87.74%           
=========================================
  Files             ?      180           
  Lines             ?     8492           
  Branches          ?        0           
=========================================
  Hits              ?     7451           
  Misses            ?      792           
  Partials          ?      249           
```



------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/serving/pull/9744?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/serving/pull/9744?src=pr&el=footer). Last update [e246e56...01bdef3](https://codecov.io/gh/knative/serving/pull/9744?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/config/core/configmaps/domain.yaml b/config/core/configmaps/domain.yaml
index 9accfead6..197c453d8 100644
--- a/config/core/configmaps/domain.yaml
+++ b/config/core/configmaps/domain.yaml
@@ -20,7 +20,7 @@ metadata:
   labels:
     serving.knative.dev/release: devel
   annotations:
-    knative.dev/example-checksum: "f8e5beb4"
+    knative.dev/example-checksum: "74c3fc6a"
 data:
   _example: |
     ################################
@@ -49,9 +49,10 @@ data:
       selector:
         app: nonprofit
 
-    # Routes having domain suffix of 'svc.cluster.local' will not be exposed
-    # through Ingress. You can define your own label selector to assign that
-    # domain suffix to your Route here, or you can set the label
+    # Routes having the cluster domain suffix (by default 'svc.cluster.local')
+    # will not be exposed through Ingress. You can define your own label
+    # selector to assign that domain suffix to your Route here, or you can set
+    # the label
     #    "serving.knative.dev/visibility=cluster-local"
     # to achieve the same effect.  This shows how to make routes having
     # the label app=secret only exposed to the local cluster.
diff --git a/pkg/reconciler/domainmapping/resources/ingress.go b/pkg/reconciler/domainmapping/resources/ingress.go
index 92750d36b..0bc99491e 100644
--- a/pkg/reconciler/domainmapping/resources/ingress.go
+++ b/pkg/reconciler/domainmapping/resources/ingress.go
@@ -25,6 +25,7 @@ import (
 	"knative.dev/networking/pkg/apis/networking"
 	netv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1"
 	"knative.dev/pkg/kmeta"
+	pkgnetwork "knative.dev/pkg/network"
 	servingv1alpha1 "knative.dev/serving/pkg/apis/serving/v1alpha1"
 )
 
@@ -38,7 +39,7 @@ func MakeIngress(dm *servingv1alpha1.DomainMapping, ingressClass string) *netv1a
 	// This assumes the target is a KSvc. To support arbitrary addressables
 	// we will need to resolve the reference to a URL in the same way that
 	// eventing does.
-	targetHostName := targetServiceName + "." + targetServiceNamespace + ".svc.cluster.local"
+	targetHostName := pkgnetwork.GetServiceHostname(targetServiceName, targetServiceNamespace)
 
 	return &netv1alpha1.Ingress{
 		ObjectMeta: metav1.ObjectMeta{
diff --git a/pkg/reconciler/route/traffic/traffic.go b/pkg/reconciler/route/traffic/traffic.go
index 8413ab18c..59adaac6a 100644
--- a/pkg/reconciler/route/traffic/traffic.go
+++ b/pkg/reconciler/route/traffic/traffic.go
@@ -291,11 +291,25 @@ func (cb *configBuilder) addRevisionTarget(tt *v1.TrafficTarget) error {
 	return nil
 }
 
+// This find the exact revision+tag pair and if so, just adds the percentages.
+// This expects single digit lists, so just does an O(N) search.
+func mergeIfNecessary(rts RevisionTargets, rt RevisionTarget) RevisionTargets {
+	for i := range rts {
+		if rts[i].Tag == rt.Tag && rts[i].RevisionName == rt.RevisionName &&
+			*rt.LatestRevision == *rts[i].LatestRevision {
+			rts[i].Percent = ptr.Int64(*rts[i].Percent + *rt.Percent)
+			return rts
+		}
+	}
+	return append(rts, rt)
+}
+
 func (cb *configBuilder) addFlattenedTarget(target RevisionTarget) {
 	name := target.TrafficTarget.Tag
-	cb.revisionTargets = append(cb.revisionTargets, target)
+	cb.revisionTargets = mergeIfNecessary(cb.revisionTargets, target)
 	cb.targets[DefaultTarget] = append(cb.targets[DefaultTarget], target)
 	if name != "" {
+		// This should always have just a single entry at most.
 		cb.targets[name] = append(cb.targets[name], target)
 	}
 }


Test patch:
diff --git a/.github/workflows/kind-e2e.yaml b/.github/workflows/kind-e2e.yaml
index 9e1b5543a..1eb78dfe9 100644
--- a/.github/workflows/kind-e2e.yaml
+++ b/.github/workflows/kind-e2e.yaml
@@ -169,8 +169,8 @@ jobs:
           kubectl apply -f -
 
         # This tells the tests what namespace to look in for our kingress LB.
-        echo "::set-env name=GATEWAY_OVERRIDE::envoy"
-        echo "::set-env name=GATEWAY_NAMESPACE_OVERRIDE::contour-external"
+        echo "GATEWAY_OVERRIDE=envoy" >> $GITHUB_ENV
+        echo "GATEWAY_NAMESPACE_OVERRIDE=contour-external" >> $GITHUB_ENV
 
     - name: Install kingress provider (Kourier)
       if: matrix.kingress == 'kourier'
@@ -184,8 +184,8 @@ jobs:
           kubectl apply -f -
 
         # This tells the tests what namespace to look in for our kingress LB.
-        echo "::set-env name=GATEWAY_OVERRIDE::kourier"
-        echo "::set-env name=GATEWAY_NAMESPACE_OVERRIDE::kourier-system"
+        echo "GATEWAY_OVERRIDE=kourier" >> $GITHUB_ENV
+        echo "GATEWAY_NAMESPACE_OVERRIDE=kourier-system" >> $GITHUB_ENV
 
     - name: Install kingress provider (Istio)
       if: matrix.kingress == 'istio'
@@ -206,7 +206,7 @@ jobs:
         export KIND=true
         install_istio $PATCHED_YAML
 
-        echo "::set-env name=GATEWAY_NAMESPACE_OVERRIDE::istio-system"
+        echo "GATEWAY_NAMESPACE_OVERRIDE=istio-system" >> $GITHUB_ENV
 
     - name: Upload Test Images
       run: |
diff --git a/pkg/reconciler/route/traffic/traffic_test.go b/pkg/reconciler/route/traffic/traffic_test.go
index c1c482fb0..c2cecf4ff 100644
--- a/pkg/reconciler/route/traffic/traffic_test.go
+++ b/pkg/reconciler/route/traffic/traffic_test.go
@@ -19,10 +19,12 @@ package traffic
 import (
 	"context"
 	"errors"
+	"fmt"
 	"os"
 	"testing"
 	"time"
 
+	"github.com/davecgh/go-spew/spew"
 	"github.com/google/go-cmp/cmp"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -324,6 +326,51 @@ func TestBuildTrafficConfigurationTwoConfigs(t *testing.T) {
 	}
 }
 
+func TestBuildTrafficConfigurationTwoEntriesSameConfig(t *testing.T) {
+	expected := &Config{
+		Targets: map[string]RevisionTargets{
+			DefaultTarget: {{
+				TrafficTarget: v1.TrafficTarget{
+					ConfigurationName: niceConfig.Name,
+					RevisionName:      niceNewRev.Name,
+					Percent:           ptr.Int64(100),
+					LatestRevision:    ptr.Bool(true),
+				},
+				Active:   true,
+				Protocol: net.ProtocolH2C,
+			}},
+		},
+		revisionTargets: []RevisionTarget{{
+			TrafficTarget: v1.TrafficTarget{
+				ConfigurationName: niceConfig.Name,
+				RevisionName:      niceNewRev.Name,
+				Percent:           ptr.Int64(100),
+				LatestRevision:    ptr.Bool(true),
+			},
+			Active:   true,
+			Protocol: net.ProtocolH2C,
+		}},
+		Configurations: map[string]*v1.Configuration{
+			niceConfig.Name: niceConfig,
+		},
+		Revisions: map[string]*v1.Revision{
+			niceNewRev.Name: niceNewRev,
+		},
+	}
+	if tc, err := BuildTrafficConfiguration(configLister, revLister, testRouteWithTrafficTargets(WithSpecTraffic(v1.TrafficTarget{
+		ConfigurationName: niceConfig.Name,
+		Percent:           ptr.Int64(90),
+	}, v1.TrafficTarget{
+		ConfigurationName: niceConfig.Name,
+		Percent:           ptr.Int64(10),
+	}))); err != nil {
+		t.Error("Unexpected error", err)
+	} else if got, want := tc, expected; !cmp.Equal(want, got, cmpOpts...) {
+		fmt.Println(spew.Sdump(got))
+		t.Error("Unexpected traffic diff (-want +got):", cmp.Diff(want, got, cmpOpts...))
+	}
+}
+
 // Splitting traffic between a fixed revision and the latest revision (canary).
 func TestBuildTrafficConfigurationCanary(t *testing.T) {
 	expected := &Config{