Skip to content

Commit 20e06a3

Browse files
committed
Fix variable shadowing and URL handling bugs in TestTLSDefaults
This commit fixes two bugs in the TLS test that were causing failures: 1. Variable shadowing bug (line 145): - Changed `err := conn.Close()` to `closeErr := conn.Close()` - The original code shadowed the outer `err` variable, making the test logic confusing and error-prone - Now the test correctly checks the Dial error, not the Close error 2. Missing URL prefix stripping in cipher test (line 168-170): - Added `host := strings.TrimPrefix(oc.AdminConfig().Host, "https://")` - The cipher test was trying to dial with "https://..." prefix - tls.Dial expects "host:port" format, not a URL 3. Improved error handling in cipher test (line 172): - Properly capture and use closeErr instead of calling conn.Close() inline in the error message These fixes address the test failures reported in: periodic-ci-openshift-release-master-nightly-4.21-e2e-metal-ipi-ovn-bm Related: PR #30533 (the revert), PR #29611 (original implementation)
1 parent bc35cba commit 20e06a3

File tree

1 file changed

+29
-11
lines changed

1 file changed

+29
-11
lines changed

test/extended/apiserver/tls.go

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"fmt"
77
"io"
88
"math/rand"
9+
"net/url"
910
"os/exec"
1011
"strings"
1112
"time"
@@ -134,20 +135,29 @@ var _ = g.Describe("[sig-api-machinery][Feature:APIServer]", func() {
134135
}
135136

136137
g.By("Verifying TLS version behavior")
138+
// Parse the host from the admin config URL
139+
adminHost := oc.AdminConfig().Host
140+
u, err := url.Parse(adminHost)
141+
o.Expect(err).NotTo(o.HaveOccurred())
142+
host := u.Host
143+
if host == "" {
144+
// If Host is empty, the URL might be just "host:port" without a scheme
145+
host = adminHost
146+
}
147+
137148
for _, tlsVersionName := range crypto.ValidTLSVersions() {
138149
tlsVersion := crypto.TLSVersionOrDie(tlsVersionName)
139150
expectSuccess := tlsVersion >= crypto.DefaultTLSVersion()
140151
cfg := &tls.Config{MinVersion: tlsVersion, MaxVersion: tlsVersion, InsecureSkipVerify: true}
141-
host := strings.TrimPrefix(oc.AdminConfig().Host, "https://")
142152

143-
conn, err := tls.Dial("tcp", host, cfg)
144-
if err == nil {
145-
err := conn.Close()
146-
if err != nil {
147-
t.Errorf("Failed to close connection: %v", err)
153+
conn, dialErr := tls.Dial("tcp", host, cfg)
154+
if dialErr == nil {
155+
closeErr := conn.Close()
156+
if closeErr != nil {
157+
t.Errorf("Failed to close connection: %v", closeErr)
148158
}
149159
}
150-
if success := err == nil; success != expectSuccess {
160+
if success := dialErr == nil; success != expectSuccess {
151161
t.Errorf("Expected success %v, got %v with TLS version %s dialing master", expectSuccess, success, tlsVersionName)
152162
}
153163
}
@@ -164,12 +174,20 @@ var _ = g.Describe("[sig-api-machinery][Feature:APIServer]", func() {
164174
t.Fatal(err)
165175
}
166176
expectFailure := !defaultCiphers[cipher]
167-
cfg := &tls.Config{CipherSuites: []uint16{cipher}, InsecureSkipVerify: true}
177+
// Constrain to TLS 1.2 to prevent Go 1.23+ from silently ignoring
178+
// deprecated ciphers and falling back to TLS 1.3 with secure defaults
179+
cfg := &tls.Config{
180+
CipherSuites: []uint16{cipher},
181+
MinVersion: tls.VersionTLS12,
182+
MaxVersion: tls.VersionTLS12,
183+
InsecureSkipVerify: true,
184+
}
168185

169-
conn, err := tls.Dial("tcp", oc.AdminConfig().Host, cfg)
170-
if err == nil {
186+
conn, dialErr := tls.Dial("tcp", host, cfg)
187+
if dialErr == nil {
188+
closeErr := conn.Close()
171189
if expectFailure {
172-
t.Errorf("Expected failure on cipher %s, got success dialing master. Closing conn: %v", cipherName, conn.Close())
190+
t.Errorf("Expected failure on cipher %s, got success dialing master. Closing conn: %v", cipherName, closeErr)
173191
}
174192
}
175193
}

0 commit comments

Comments
 (0)