Repository: knative/pkg
Pull request #2510 (2022-05-03 05:09:52)

Fix timeout test on darwin and log dial destination on timeout.
Fixes: #2502

This adds extra logging so we can determine if a dial timeout like in #2502 was
hitting the expected port.

/assign @cardil
/cc @dprotaso


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

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

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

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:

- ~~[network/OWNERS](https://github.com/knative/pkg/blob/main/network/OWNERS)~~ [evankanderson]

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":[]} -->
knative-prow[bot]: @evankanderson: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
unit-tests_pkg_main | 7d0a79c7a67d141cad9b41f34d17f7f20c136212 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/2510/unit-tests_pkg_main/1521356348866957312) | true | `/test unit-tests_pkg_main`

[Your PR dashboard](https://prow.knative.dev/pr).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
evankanderson: FML; the second (backlog) argument to `listen` is handled _differently_ on darwin (MacOS) and Linux. On Mac, a backlog of `1` requires a single connection to enter the connection refused state for new connections, while on Linux, a backlog of `1` requires _two_ connections to enter the connection refused state. Even better, a backlog of `0` on MacOS seems to mean some large default, while it means _only allow one connection_ on Linux.

I'm going to switch to an infinite loop with a break statement in the backoff test.
evankanderson: /retest
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/2510?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
> Merging [#2510](https://codecov.io/gh/knative/pkg/pull/2510?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (c400d7a) into [main](https://codecov.io/gh/knative/pkg/commit/4fced0164c9a2ba173271a273662aa3e015d8073?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (4fced01) will **not change** coverage.
> The diff coverage is `100.00%`.

> :exclamation: Current head c400d7a differs from pull request most recent head dfe1a93. Consider uploading reports for the commit dfe1a93 to get more accurate results

```diff
@@           Coverage Diff           @@
##             main    #2510   +/-   ##
=======================================
  Coverage   81.76%   81.76%           
=======================================
  Files         163      163           
  Lines        9725     9725           
=======================================
  Hits         7952     7952           
  Misses       1535     1535           
  Partials      238      238           
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/2510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) | Coverage Δ | |
|---|---|---|
| [network/transports.go](https://codecov.io/gh/knative/pkg/pull/2510/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-bmV0d29yay90cmFuc3BvcnRzLmdv) | `71.95% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/pkg/pull/2510?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/pkg/pull/2510?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Last update [4fced01...dfe1a93](https://codecov.io/gh/knative/pkg/pull/2510?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).


Patch:
diff --git a/.github/workflows/knative-downstream.yaml b/.github/workflows/knative-downstream.yaml
index c0a24258f..31e1ddc15 100644
--- a/.github/workflows/knative-downstream.yaml
+++ b/.github/workflows/knative-downstream.yaml
@@ -25,47 +25,28 @@ jobs:
     strategy:
       fail-fast: false # Keep running if one leg fails.
       matrix:
-        include:
-          - repo: serving
-            org: knative
-          - repo: networking
-            org: knative
-          - repo: eventing
-            org: knative
-          - repo: operator
-            org: knative
-          - repo: client
-            org: knative
-          - repo: eventing-autoscaler-keda
-            org: knative-sandbox
-          - repo: eventing-ceph
-            org: knative-sandbox
-          - repo: eventing-github
-            org: knative-sandbox
-          - repo: eventing-gitlab
-            org: knative-sandbox
-          - repo: eventing-kafka
-            org: knative-sandbox
-          - repo: eventing-kafka-broker
-            org: knative-sandbox
-          - repo: eventing-natss
-            org: knative-sandbox
-          - repo: eventing-rabbitmq
-            org: knative-sandbox
-          - repo: eventing-redis
-            org: knative-sandbox
-          - repo: kn-plugin-admin
-            org: knative-sandbox
-          - repo: net-certmanager
-            org: knative-sandbox
-          - repo: net-contour
-            org: knative-sandbox
-          - repo: net-http01
-            org: knative-sandbox
-          - repo: net-istio
-            org: knative-sandbox
-          - repo: net-kourier
-            org: knative-sandbox
+        repo:
+          - knative/serving
+          - knative/networking
+          - knative/eventing
+          - knative/operator
+          - knative/client
+          - knative-sandbox/eventing-autoscaler-keda
+          - knative-sandbox/eventing-ceph
+          - knative-sandbox/eventing-github
+          - knative-sandbox/eventing-gitlab
+          - knative-sandbox/eventing-kafka
+          - knative-sandbox/eventing-kafka-broker
+          - knative-sandbox/eventing-natss
+          - knative-sandbox/eventing-rabbitmq
+          - knative-sandbox/eventing-redis
+          - knative-sandbox/kn-plugin-admin
+          - knative-sandbox/net-certmanager
+          - knative-sandbox/net-contour
+          - knative-sandbox/net-http01
+          - knative-sandbox/net-istio
+          - knative-sandbox/net-kourier
+
     runs-on: ubuntu-latest
 
     steps:
@@ -82,12 +63,12 @@ jobs:
       uses: actions/checkout@v3
       with:
         fetch-depth: 0
-        path: knative.dev/${{ github.event.repository.name }}
+        path: upstream
 
     - name: Merge upstream
       if: github.event_name == 'pull_request'
       shell: bash
-      working-directory: knative.dev/${{ github.event.repository.name }}
+      working-directory: upstream
       run: |
          if ! git config user.name > /dev/null; then
            git config user.name "Knative Action"
@@ -103,11 +84,11 @@ jobs:
       uses: actions/checkout@v3
       with:
         ref: ${{ github.base_ref }}
-        repository: ${{ matrix.org }}/${{ matrix.repo }}
-        path: ./knative.dev/${{ matrix.repo }}
+        repository: ${{ matrix.repo }}
+        path: downstream
 
     - name: Test downstream
       uses: knative/actions/go/downstream-test@main
       with:
-        upstream-module: knative.dev/${{ github.event.repository.name }}
-        downstream-module: knative.dev/${{ matrix.repo }}
+        upstream-path: upstream
+        downstream-path: downstream
diff --git a/.github/workflows/tekton-downstream.yaml b/.github/workflows/tekton-downstream.yaml
index bf5bd7ab8..233016c05 100644
--- a/.github/workflows/tekton-downstream.yaml
+++ b/.github/workflows/tekton-downstream.yaml
@@ -25,13 +25,10 @@ jobs:
     strategy:
       fail-fast: false # Keep running if one leg fails.
       matrix:
-        include:
-          - repo: pipeline
-            org: tektoncd
-          - repo: chains
-            org: tektoncd
-          - repo: triggers
-            org: tektoncd
+        repo: 
+          - tektoncd/pipeline
+          - tektoncd/chains
+          - tektoncd/triggers
 
     runs-on: ubuntu-latest
 
@@ -49,12 +46,12 @@ jobs:
       uses: actions/checkout@v3
       with:
         fetch-depth: 0
-        path: knative.dev/${{ github.event.repository.name }}
+        path: upstream
 
     - name: Merge upstream
       if: github.event_name == 'pull_request'
       shell: bash
-      working-directory: knative.dev/${{ github.event.repository.name }}
+      working-directory: upstream
       run: |
          if ! git config user.name > /dev/null; then
            git config user.name "Knative Action"
@@ -69,11 +66,11 @@ jobs:
     - name: Checkout Downstream
       uses: actions/checkout@v3
       with:
-        repository: ${{ matrix.org }}/${{ matrix.repo }}
-        path: github.com/${{ matrix.org }}/${{ matrix.repo }}
+        repository: ${{ matrix.repo }}
+        path: downstream
 
     - name: Test Downstream
       uses: knative/actions/go/downstream-test@main
       with:
-        upstream-module: knative.dev/${{ github.event.repository.name }}
-        downstream-module: github.com/${{ matrix.org }}/${{ matrix.repo }}
+        upstream-path: upstream
+        downstream-path: downstream
diff --git a/injection/sharedmain/main.go b/injection/sharedmain/main.go
index 0420606b4..a57956ba8 100644
--- a/injection/sharedmain/main.go
+++ b/injection/sharedmain/main.go
@@ -59,10 +59,16 @@ func init() {
 	maxprocs.Set()
 }
 
-// GetLoggingConfig gets the logging config from either the file system if present
-// or via reading a configMap from the API.
+// GetLoggingConfig gets the logging config from the (in order):
+// 1. provided context,
+// 2. reading from the API server,
+// 3. defaults (if not found).
 // The context is expected to be initialized with injection.
 func GetLoggingConfig(ctx context.Context) (*logging.Config, error) {
+	if cfg := logging.GetConfig(ctx); cfg != nil {
+		return cfg, nil
+	}
+
 	var loggingConfigMap *corev1.ConfigMap
 	// These timeout and retry interval are set by heuristics.
 	// e.g. istio sidecar needs a few seconds to configure the pod network.
@@ -79,8 +85,15 @@ func GetLoggingConfig(ctx context.Context) (*logging.Config, error) {
 	return logging.NewConfigFromConfigMap(loggingConfigMap)
 }
 
-// GetLeaderElectionConfig gets the leader election config.
+// GetLeaderElectionConfig gets the leader election config from the (in order):
+// 1. provided context,
+// 2. reading from the API server,
+// 3. defaults (if not found).
 func GetLeaderElectionConfig(ctx context.Context) (*leaderelection.Config, error) {
+	if cfg := leaderelection.GetConfig(ctx); cfg != nil {
+		return cfg, nil
+	}
+
 	leaderElectionConfigMap, err := kubeclient.Get(ctx).CoreV1().ConfigMaps(system.Namespace()).Get(ctx, leaderelection.ConfigMapName(), metav1.GetOptions{})
 	if apierrors.IsNotFound(err) {
 		return leaderelection.NewConfigFromConfigMap(nil)
diff --git a/leaderelection/config.go b/leaderelection/config.go
index 11010014a..ba6e0c60f 100644
--- a/leaderelection/config.go
+++ b/leaderelection/config.go
@@ -17,6 +17,7 @@ limitations under the License.
 package leaderelection
 
 import (
+	"context"
 	"fmt"
 	"os"
 	"strconv"
@@ -88,6 +89,24 @@ type Config struct {
 	LeaseNamesPrefixMapping map[string]string
 }
 
+type lecfg struct{}
+
+// WithConfig associates a leader election configuration with the
+// context.
+func WithConfig(ctx context.Context, cfg *Config) context.Context {
+	return context.WithValue(ctx, lecfg{}, cfg)
+}
+
+// GetConfig gets the leader election config from the provided
+// context.
+func GetConfig(ctx context.Context) *Config {
+	untyped := ctx.Value(lecfg{})
+	if untyped == nil {
+		return nil
+	}
+	return untyped.(*Config)
+}
+
 func (c *Config) GetComponentConfig(name string) ComponentConfig {
 	return ComponentConfig{
 		Component:               name,
diff --git a/logging/config.go b/logging/config.go
index 5dbf0a4e4..2cf81a822 100644
--- a/logging/config.go
+++ b/logging/config.go
@@ -17,6 +17,7 @@ limitations under the License.
 package logging
 
 import (
+	"context"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -136,6 +137,22 @@ type Config struct {
 	LoggingLevel  map[string]zapcore.Level
 }
 
+type lcfg struct{}
+
+// WithConfig associates a logging configuration with the context.
+func WithConfig(ctx context.Context, cfg *Config) context.Context {
+	return context.WithValue(ctx, lcfg{}, cfg)
+}
+
+// GetConfig gets the logging config from the provided context.
+func GetConfig(ctx context.Context) *Config {
+	untyped := ctx.Value(lcfg{})
+	if untyped == nil {
+		return nil
+	}
+	return untyped.(*Config)
+}
+
 func defaultConfig() *Config {
 	return &Config{
 		LoggingLevel: make(map[string]zapcore.Level),
diff --git a/network/transports.go b/network/transports.go
index c97d0052e..d48cd6215 100644
--- a/network/transports.go
+++ b/network/transports.go
@@ -113,7 +113,7 @@ func dialBackOffHelper(ctx context.Context, network, address string, bo wait.Bac
 		return c, nil
 	}
 	elapsed := time.Since(start)
-	return nil, fmt.Errorf("%w after %.2fs", ErrTimeoutDialing, elapsed.Seconds())
+	return nil, fmt.Errorf("%w %s after %.2fs", ErrTimeoutDialing, address, elapsed.Seconds())
 }
 
 func newHTTPTransport(disableKeepAlives, disableCompression bool, maxIdle, maxIdlePerHost int) http.RoundTripper {


Test patch:
diff --git a/injection/sharedmain/main_test.go b/injection/sharedmain/main_test.go
index 673bda005..d739fe2cf 100644
--- a/injection/sharedmain/main_test.go
+++ b/injection/sharedmain/main_test.go
@@ -17,11 +17,15 @@ limitations under the License.
 package sharedmain
 
 import (
+	"context"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"go.uber.org/zap/zapcore"
 
 	"knative.dev/pkg/injection"
+	"knative.dev/pkg/leaderelection"
+	"knative.dev/pkg/logging"
 )
 
 func TestEnabledControllers(t *testing.T) {
@@ -30,29 +34,26 @@ func TestEnabledControllers(t *testing.T) {
 		disabledControllers []string
 		ctors               []injection.NamedControllerConstructor
 		wantNames           []string
-	}{
-		{
-			name:                "zero",
-			disabledControllers: []string{"foo"},
-			ctors:               []injection.NamedControllerConstructor{{Name: "bar"}},
-			wantNames:           []string{"bar"},
+	}{{
+		name:                "zero",
+		disabledControllers: []string{"foo"},
+		ctors:               []injection.NamedControllerConstructor{{Name: "bar"}},
+		wantNames:           []string{"bar"},
+	}, {
+		name:                "one",
+		disabledControllers: []string{"foo"},
+		ctors:               []injection.NamedControllerConstructor{{Name: "foo"}},
+		wantNames:           []string{},
+	}, {
+		name:                "two",
+		disabledControllers: []string{"foo"},
+		ctors: []injection.NamedControllerConstructor{
+			{Name: "foo"},
+			{Name: "bar"},
 		},
-		{
-			name:                "one",
-			disabledControllers: []string{"foo"},
-			ctors:               []injection.NamedControllerConstructor{{Name: "foo"}},
-			wantNames:           []string{},
-		},
-		{
-			name:                "two",
-			disabledControllers: []string{"foo"},
-			ctors: []injection.NamedControllerConstructor{
-				{Name: "foo"},
-				{Name: "bar"},
-			},
-			wantNames: []string{"bar"},
-		},
-	}
+		wantNames: []string{"bar"},
+	}}
+
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			got := enabledControllers(tt.disabledControllers, tt.ctors)
@@ -70,3 +71,35 @@ func namesOf(ctors []injection.NamedControllerConstructor) []string {
 	}
 	return names
 }
+
+func TestWithLoggingConfig(t *testing.T) {
+	want := &logging.Config{
+		LoggingLevel: map[string]zapcore.Level{
+			"foo": zapcore.DebugLevel,
+		},
+	}
+	ctx := logging.WithConfig(context.Background(), want)
+
+	got, err := GetLoggingConfig(ctx)
+	if err != nil {
+		t.Fatalf("GetLoggingConfig() = %v", err)
+	}
+	if diff := cmp.Diff(got, want); diff != "" {
+		t.Errorf("(-got, +want) = %s", diff)
+	}
+}
+
+func TestWithLeaderElectionConfig(t *testing.T) {
+	want := &leaderelection.Config{
+		Buckets: 12,
+	}
+	ctx := leaderelection.WithConfig(context.Background(), want)
+
+	got, err := GetLeaderElectionConfig(ctx)
+	if err != nil {
+		t.Fatalf("GetLeaderElectionConfig() = %v", err)
+	}
+	if diff := cmp.Diff(got, want); diff != "" {
+		t.Errorf("(-got, +want) = %s", diff)
+	}
+}
diff --git a/network/transports_test.go b/network/transports_test.go
index 1ad34ebbf..b9283db43 100644
--- a/network/transports_test.go
+++ b/network/transports_test.go
@@ -120,11 +120,27 @@ func testDialWithBackoffTimeout(tlsConf *tls.Config, t testingT) {
 		t.Fatal("Unable to create listener:", err)
 	}
 	defer closer()
-	c1, err := net.Dial("tcp4", addr.String())
-	if err != nil {
-		t.Fatalf("Unable to connect to server on %s: %s", addr, err)
+
+	for {
+		// This seems really strange... we're listening with a backlog of one
+		// connection, and we keep creating connections and holding onto them
+		// until we get a connection timeout.
+		//
+		// It turns out that darwin (MacOS) and Linux implement the Listen
+		// backlog argument slightly differently, and MacOS needs one more
+		// connection than Linux to saturate the backlog. Rather than sniffing
+		// the OS, we simply ensure that the backlog is saturated.
+		c1, err := net.DialTimeout("tcp4", addr.String(), 10*time.Millisecond)
+		if err != nil {
+			var neterr net.Error
+			if errors.As(err, &neterr) && neterr.Timeout() {
+				// Waiting for a timeout
+				break
+			}
+			t.Fatalf("Unable to connect to server on %s: %s", addr, err)
+		}
+		defer closeOrFail(t, c1)
 	}
-	defer closeOrFail(t, c1)
 
 	// Since the backlog is full, the next request must time out.
 	dialer := newDialer(ctx, tlsConf)
@@ -240,7 +256,7 @@ func listenOne() (func(), *net.TCPAddr, error) {
 	if err = syscall.Bind(fd, sa); err != nil {
 		return nil, nil, newTestErr("Unable to bind", err)
 	}
-	if err = syscall.Listen(fd, 0); err != nil {
+	if err = syscall.Listen(fd, 1); err != nil {
 		return nil, nil, newTestErr("Unable to Listen", err)
 	}
 	closer := func() { _ = syscall.Close(fd) }