Merge pull request #66008 from smarterclayton/serving_test

Automatic merge from submit-queue (batch tested with PRs 66038, 65992, 66008). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Convert TestServerRunWithSNI to subtests to isolate flake

This test is flaking - make it easier to pin down where and why by
converting to subtests and making cleanup logic simpler. Also turn an
ignored listen error into a "fatal".

Make the test run in parallel to speed up individual runs and hopefully
flush out issues.

Noticed and reported in OpenShift, https://github.com/openshift/origin/issues/20220

@deads2k / @sttts

staging/src/k8s.io/apiserver/pkg/server/options/serving_test.go

@@ -398,68 +398,63 @@ func TestServerRunWithSNI(t *testing.T) {
 		return strings.Replace(name, "*", "star", -1)
 	}
 
-NextTest:
+	for title := range tests {
-	for title, test := range tests {
+		test := tests[title]
-		// create server cert
+		t.Run(title, func(t *testing.T) {
-		certDir := "testdata/" + specToName(test.Cert)
+			t.Parallel()
-		serverCertBundleFile := filepath.Join(certDir, "cert")
+			// create server cert
-		serverKeyFile := filepath.Join(certDir, "key")
+			certDir := "testdata/" + specToName(test.Cert)
-		err := getOrCreateTestCertFiles(serverCertBundleFile, serverKeyFile, test.Cert)
+			serverCertBundleFile := filepath.Join(certDir, "cert")
-		if err != nil {
+			serverKeyFile := filepath.Join(certDir, "key")
-			t.Errorf("%q - failed to create server cert: %v", title, err)
+			err := getOrCreateTestCertFiles(serverCertBundleFile, serverKeyFile, test.Cert)
-			continue NextTest
-		}
-		ca, err := caCertFromBundle(serverCertBundleFile)
-		if err != nil {
-			t.Errorf("%q - failed to extract ca cert from server cert bundle: %v", title, err)
-			continue NextTest
-		}
-		caCerts := []*x509.Certificate{ca}
-
-		// create SNI certs
-		var namedCertKeys []utilflag.NamedCertKey
-		serverSig, err := certFileSignature(serverCertBundleFile, serverKeyFile)
-		if err != nil {
-			t.Errorf("%q - failed to get server cert signature: %v", title, err)
-			continue NextTest
-		}
-		signatures := map[string]int{
-			serverSig: -1,
-		}
-		for j, c := range test.SNICerts {
-			sniDir := filepath.Join(certDir, specToName(c.TestCertSpec))
-			certBundleFile := filepath.Join(sniDir, "cert")
-			keyFile := filepath.Join(sniDir, "key")
-			err := getOrCreateTestCertFiles(certBundleFile, keyFile, c.TestCertSpec)
 			if err != nil {
-				t.Errorf("%q - failed to create SNI cert %d: %v", title, j, err)
+				t.Fatalf("failed to create server cert: %v", err)
-				continue NextTest
+			}
+			ca, err := caCertFromBundle(serverCertBundleFile)
+			if err != nil {
+				t.Fatalf("failed to extract ca cert from server cert bundle: %v", err)
+			}
+			caCerts := []*x509.Certificate{ca}
+
+			// create SNI certs
+			var namedCertKeys []utilflag.NamedCertKey
+			serverSig, err := certFileSignature(serverCertBundleFile, serverKeyFile)
+			if err != nil {
+				t.Fatalf("failed to get server cert signature: %v", err)
+			}
+			signatures := map[string]int{
+				serverSig: -1,
+			}
+			for j, c := range test.SNICerts {
+				sniDir := filepath.Join(certDir, specToName(c.TestCertSpec))
+				certBundleFile := filepath.Join(sniDir, "cert")
+				keyFile := filepath.Join(sniDir, "key")
+				err := getOrCreateTestCertFiles(certBundleFile, keyFile, c.TestCertSpec)
+				if err != nil {
+					t.Fatalf("failed to create SNI cert %d: %v", j, err)
+				}
+
+				namedCertKeys = append(namedCertKeys, utilflag.NamedCertKey{
+					KeyFile:  keyFile,
+					CertFile: certBundleFile,
+					Names:    c.explicitNames,
+				})
+
+				ca, err := caCertFromBundle(certBundleFile)
+				if err != nil {
+					t.Fatalf("failed to extract ca cert from SNI cert %d: %v", j, err)
+				}
+				caCerts = append(caCerts, ca)
+
+				// store index in namedCertKeys with the signature as the key
+				sig, err := certFileSignature(certBundleFile, keyFile)
+				if err != nil {
+					t.Fatalf("failed get SNI cert %d signature: %v", j, err)
+				}
+				signatures[sig] = j
 			}
 
-			namedCertKeys = append(namedCertKeys, utilflag.NamedCertKey{
+			stopCh := make(chan struct{})
-				KeyFile:  keyFile,
-				CertFile: certBundleFile,
-				Names:    c.explicitNames,
-			})
-
-			ca, err := caCertFromBundle(certBundleFile)
-			if err != nil {
-				t.Errorf("%q - failed to extract ca cert from SNI cert %d: %v", title, j, err)
-				continue NextTest
-			}
-			caCerts = append(caCerts, ca)
-
-			// store index in namedCertKeys with the signature as the key
-			sig, err := certFileSignature(certBundleFile, keyFile)
-			if err != nil {
-				t.Errorf("%q - failed get SNI cert %d signature: %v", title, j, err)
-				continue NextTest
-			}
-			signatures[sig] = j
-		}
-
-		stopCh := make(chan struct{})
-		func() {
 			defer close(stopCh)
 
 			// launch server
@@ -483,7 +478,7 @@ NextTest:
 			// use a random free port
 			ln, err := net.Listen("tcp", "127.0.0.1:0")
 			if err != nil {
-				t.Errorf("failed to listen on 127.0.0.1:0")
+				t.Fatalf("failed to listen on 127.0.0.1:0")
 			}
 
 			secureOptions.Listener = ln
@@ -491,14 +486,12 @@ NextTest:
 			secureOptions.BindPort = ln.Addr().(*net.TCPAddr).Port
 			config.LoopbackClientConfig = &restclient.Config{}
 			if err := secureOptions.ApplyTo(&config); err != nil {
-				t.Errorf("%q - failed applying the SecureServingOptions: %v", title, err)
+				t.Fatalf("failed applying the SecureServingOptions: %v", err)
-				return
 			}
 
 			s, err := config.Complete(nil).New("test", server.NewEmptyDelegate())
 			if err != nil {
-				t.Errorf("%q - failed creating the server: %v", title, err)
+				t.Fatalf("failed creating the server: %v", err)
-				return
 			}
 
 			// add poststart hook to know when the server is up.
@@ -530,22 +523,20 @@ NextTest:
 				ServerName: test.ServerName, // used for SNI in the client HELLO packet
 			})
 			if err != nil {
-				t.Errorf("%q - failed to connect: %v", title, err)
+				t.Fatalf("failed to connect: %v", err)
-				return
 			}
+			defer conn.Close()
 
 			// check returned server certificate
 			sig := x509CertSignature(conn.ConnectionState().PeerCertificates[0])
 			gotCertIndex, found := signatures[sig]
 			if !found {
-				t.Errorf("%q - unknown signature returned from server: %s", title, sig)
+				t.Errorf("unknown signature returned from server: %s", sig)
 			}
 			if gotCertIndex != test.ExpectedCertIndex {
-				t.Errorf("%q - expected cert index %d, got cert index %d", title, test.ExpectedCertIndex, gotCertIndex)
+				t.Errorf("expected cert index %d, got cert index %d", test.ExpectedCertIndex, gotCertIndex)
 			}
 
-			conn.Close()
-
 			// check that the loopback client can connect
 			host := "127.0.0.1"
 			if len(test.LoopbackClientBindAddressOverride) != 0 {
@@ -554,28 +545,25 @@ NextTest:
 			s.LoopbackClientConfig.Host = net.JoinHostPort(host, strconv.Itoa(secureOptions.BindPort))
 			if test.ExpectLoopbackClientError {
 				if err == nil {
-					t.Errorf("%q - expected error creating loopback client config", title)
+					t.Fatalf("expected error creating loopback client config")
 				}
 				return
 			}
 			if err != nil {
-				t.Errorf("%q - failed creating loopback client config: %v", title, err)
+				t.Fatalf("failed creating loopback client config: %v", err)
-				return
 			}
 			client, err := discovery.NewDiscoveryClientForConfig(s.LoopbackClientConfig)
 			if err != nil {
-				t.Errorf("%q - failed to create loopback client: %v", title, err)
+				t.Fatalf("failed to create loopback client: %v", err)
-				return
 			}
 			got, err := client.ServerVersion()
 			if err != nil {
-				t.Errorf("%q - failed to connect with loopback client: %v", title, err)
+				t.Fatalf("failed to connect with loopback client: %v", err)
-				return
 			}
 			if expected := &v; !reflect.DeepEqual(got, expected) {
-				t.Errorf("%q - loopback client didn't get correct version info: expected=%v got=%v", title, expected, got)
+				t.Errorf("loopback client didn't get correct version info: expected=%v got=%v", expected, got)
 			}
-		}()
+		})
 	}
 }