New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
exec auth: support TLS config caching #112017
Conversation
/assign @liggitt |
// - these rest configs are used to create distinct clientsets, then | ||
// | ||
// the underlying TLS config is shared between those clientsets. | ||
func TestExecTLSCache(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran this test against master
and confirmed that it failed as expected.
cc @soltysh |
diff --git a/staging/src/k8s.io/client-go/transport/cache_test.go b/staging/src/k8s.io/client-go/transport/cache_test.go
index e07d77215ff..349ebe34617 100644
--- a/staging/src/k8s.io/client-go/transport/cache_test.go
+++ b/staging/src/k8s.io/client-go/transport/cache_test.go
@@ -19,10 +19,14 @@ package transport
import (
"context"
"crypto/tls"
+ "fmt"
+ "io"
"net"
"net/http"
+ "net/http/httptest"
"net/url"
"reflect"
+ "sync"
"testing"
)
@@ -188,3 +192,119 @@ func TestTLSConfigKey(t *testing.T) {
}
}
}
+
+func TestDifferentDialers(t *testing.T) {
+
+ var counter1 int
+ var mu1 sync.Mutex
+ dialFn1 := func(ctx context.Context, network, address string) (net.Conn, error) {
+ mu1.Lock()
+ counter1++
+ mu1.Unlock()
+ return (&net.Dialer{}).DialContext(ctx, network, address)
+ }
+
+ var counter2 int
+ var mu2 sync.Mutex
+ dialFn2 := func(ctx context.Context, network, address string) (net.Conn, error) {
+ mu2.Lock()
+ counter2++
+ mu2.Unlock()
+ return (&net.Dialer{}).DialContext(ctx, network, address)
+ }
+
+ ts := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ fmt.Fprintf(w, "Hello, %s", r.Proto)
+ }))
+ ts.EnableHTTP2 = true
+ ts.StartTLS()
+ defer ts.Close()
+
+ tr1, err := tlsCache.get(&Config{
+ TLS: TLSConfig{
+ Insecure: true,
+ },
+ Dial: &DialHolder{F: dialFn1}},
+ )
+ if err != nil {
+ t.Fatalf("failed to create transport: %v", err)
+ }
+
+ req, err := http.NewRequest("GET", ts.URL, nil)
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+
+ resp1, err := tr1.RoundTrip(req)
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+
+ data, err := io.ReadAll(resp1.Body)
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ resp1.Body.Close()
+
+ if string(data) != "Hello, HTTP/2.0" {
+ t.Fatalf("unexpected response: %s", data)
+ }
+
+ tr2, err := tlsCache.get(&Config{
+ TLS: TLSConfig{
+ Insecure: true,
+ },
+ Dial: &DialHolder{F: dialFn2}},
+ )
+ if err != nil {
+ t.Fatalf("failed to create transport: %v", err)
+ }
+ resp2, err := tr2.RoundTrip(req)
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ defer resp2.Body.Close()
+ data, err = io.ReadAll(resp2.Body)
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ resp2.Body.Close()
+ if string(data) != "Hello, HTTP/2.0" {
+ t.Fatalf("unexpected response: %s", data)
+ }
+
+ tr3, err := tlsCache.get(&Config{
+ TLS: TLSConfig{
+ Insecure: true,
+ },
+ Dial: &DialHolder{F: dialFn2}},
+ )
+ if err != nil {
+ t.Fatalf("failed to create transport: %v", err)
+ }
+ resp3, err := tr3.RoundTrip(req)
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+
+ data, err = io.ReadAll(resp3.Body)
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ resp3.Body.Close()
+ if string(data) != "Hello, HTTP/2.0" {
+ t.Fatalf("unexpected response: %s", data)
+ }
+
+ mu1.Lock()
+ mu2.Lock()
+ if counter1 != 1 || counter2 != 2 {
+ t.Fatalf("unexpacted dials: Dialer1 %d times Dialer2 %d times", counter1, counter2)
+ }
+ mu1.Unlock()
+ mu2.Unlock()
+
+ if len(tlsCache.transports) != 2 {
+ t.Fatalf("Unexpcted number of transports on the cache, expected 2 got %d", len(tlsCache.transports))
+ }
+} this is a very complex thing, I had to create one test to verify this as is scary how many things can break touching so deep inside client-go, I don't know if I'm doing something wrong, but I have two identical transport with the same dialers but it doesn't cache them
|
@aojea your test is incorrect. If you want caching to work, you need to arrange for the |
@aojea to fix your test, simply change |
@@ -68,7 +68,7 @@ type Config struct { | |||
WrapTransport WrapperFunc | |||
|
|||
// Dial specifies the dial function for creating unencrypted TCP connections. | |||
Dial func(ctx context.Context, network, address string) (net.Conn, error) | |||
Dial *DialHolder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh @enj , now I understand why I have the test wrong, thanks for clarifying ...
indeed, you are right, I've modified it and also verifies that reuses the http2 transport and only calls the dialer once func TestDifferentDialers(t *testing.T) {
var counter1 int
var mu1 sync.Mutex
dialFn1 := &DialHolder{F: func(ctx context.Context, network, address string) (net.Conn, error) {
mu1.Lock()
counter1++
mu1.Unlock()
return (&net.Dialer{}).DialContext(ctx, network, address)
}}
var counter2 int
var mu2 sync.Mutex
dialFn2 := &DialHolder{F: func(ctx context.Context, network, address string) (net.Conn, error) {
mu2.Lock()
counter2++
mu2.Unlock()
return (&net.Dialer{}).DialContext(ctx, network, address)
}}
ts := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
fmt.Fprintf(w, "Hello, %s", r.Proto)
}))
ts.EnableHTTP2 = true
ts.StartTLS()
defer ts.Close()
tr1, err := tlsCache.get(&Config{
TLS: TLSConfig{
Insecure: true,
},
Dial: dialFn1,
})
if err != nil {
t.Fatalf("failed to create transport: %v", err)
}
req, err := http.NewRequest("GET", ts.URL, nil)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
resp1, err := tr1.RoundTrip(req)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
data, err := io.ReadAll(resp1.Body)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
resp1.Body.Close()
if string(data) != "Hello, HTTP/2.0" {
t.Fatalf("unexpected response: %s", data)
}
tr2, err := tlsCache.get(&Config{
TLS: TLSConfig{
Insecure: true,
},
Dial: dialFn2,
})
if err != nil {
t.Fatalf("failed to create transport: %v", err)
}
resp2, err := tr2.RoundTrip(req)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
defer resp2.Body.Close()
data, err = io.ReadAll(resp2.Body)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
resp2.Body.Close()
if string(data) != "Hello, HTTP/2.0" {
t.Fatalf("unexpected response: %s", data)
}
dialFn3 := dialFn2
tr3, err := tlsCache.get(&Config{
TLS: TLSConfig{
Insecure: true,
},
Dial: dialFn3,
})
if err != nil {
t.Fatalf("failed to create transport: %v", err)
}
resp3, err := tr3.RoundTrip(req)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
data, err = io.ReadAll(resp3.Body)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
resp3.Body.Close()
if string(data) != "Hello, HTTP/2.0" {
t.Fatalf("unexpected response: %s", data)
}
mu1.Lock()
mu2.Lock()
// HTTP2 reuses the connection, so only 1 Dial per each Dialer
if counter1 != 1 || counter2 != 1 {
t.Errorf("unexpacted dials: Dialer1 got %d expected 1 times Dialer2 got %d expected 1 times", counter1, counter2)
}
mu1.Unlock()
mu2.Unlock()
if len(tlsCache.transports) != 2 {
t.Fatalf("Unexpcted number of transports on the cache, expected 2 got %d", len(tlsCache.transports))
}
} |
I'm not very familiar with this code, but what if instead of operating on transport.Config
you operate on the returned transport of the config directly, and to the cache locally to this plugin?
|
@aojea what problem are you trying to solve? The reason this code acts on the transport config is because you are allowed to continue changing the transport config (say you want another wrapper to inject some headers) before constructing the final transport. |
not modifying a public API that will cause consumers to change their code on the next release https://pkg.go.dev/k8s.io/client-go/rest#Config.Transport ... EDIT wrong link 😄 https://pkg.go.dev/k8s.io/client-go/transport#Config |
@aojea the set of consumers manually constructing transport configs with custom dialers/cert callbacks is small so the breaking change is acceptable IMO (rest configs are generally used). Other approaches will make the diff larger and harder to backport. The TLS cache / authenticator cache are not going anywhere - too many consumers rely on them. |
@@ -68,7 +68,7 @@ type Config struct { | |||
WrapTransport WrapperFunc | |||
|
|||
// Dial specifies the dial function for creating unencrypted TCP connections. | |||
Dial func(ctx context.Context, network, address string) (net.Conn, error) | |||
Dial *DialHolder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a clever way to make these comparable. I think we can make use of this mechanism in a way that wouldn't break compile compatibility in a backport by adding peer DialHolder / GetCertHolder fields in transport.Config, rather than changing the type of the existing fields, and requiring the address(Dial) == address(DialHolder.Dial) or address(GetCert) == address(GetCertHolder.GetCert) to be cacheable.
I took a stab at that in https://github.com/liggitt/kubernetes/commits/enj/i/exec_tls_cache
We could revisit simplifying that back down to a single field in 1.26
I also wonder if that could be backported back to 1.22, since it doesn't rely on the single client construction work @aojea did in 1.23
39ad816
to
3747dda
Compare
// cannot determine equality for functions | ||
return tlsCacheKey{}, false, nil | ||
} | ||
if c.Dial != nil && (c.DialHolder == nil || c.DialHolder.Dial == nil || reflect.ValueOf(c.DialHolder.Dial).Pointer() != reflect.ValueOf(c.Dial).Pointer()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should verify the address matches in isValidHolders
and treat a mismatch as an error, instead of silently ignoring/making uncacheable. Same thing for GetCert address comparison below
That simplifies this to:
if c.Dial != nil && c.DialHolder == nil { return ... }
3747dda
to
d15dce7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code change lgtm, I wasn't sure about the TLS:true bit on one unit test (might be pre-existing, I don't see anything in this change that looks related to it)
lgtm just the unit test questions |
This change updates the transport.Config .Dial and .TLS.GetCert fields to use a struct wrapper. This indirection via a pointer allows the functions to be compared and thus makes them valid to use as map keys. This change is then leveraged by the existing global exec auth and TLS config caches to return the same authenticator and TLS config even when distinct but identical rest configs were used to create distinct clientsets. Signed-off-by: Monis Khan <mok@microsoft.com>
d15dce7
to
831d95b
Compare
@liggitt @aojea I fixed the one unit test I could change without having test failures per #112017 (comment). |
/lgtm |
I'm curious how cleanly this picks to 1.25 - 1.22 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enj, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
There was a trivial import path conflict in |
…upstream-release-1.25 Automated cherry pick of #112017: exec auth: support TLS config caching
…upstream-release-1.24 Automated cherry pick of #112017: exec auth: support TLS config caching
…upstream-release-1.23 Automated cherry pick of #112017: exec auth: support TLS config caching
…upstream-release-1.22 Automated cherry pick of #112017: exec auth: support TLS config caching
This change updates the transport.Config .Dial and .TLS.GetCert fields
to use a struct wrapper. This indirection via a pointer allows the
functions to be compared and thus makes them valid to use as map keys.
This change is then leveraged by the existing global exec auth and TLS
config caches to return the same authenticator and TLS config even when
distinct but identical rest configs were used to create distinct clientsets.
Signed-off-by: Monis Khan mok@microsoft.com
/kind bug
/sig auth
/milestone v1.26
/priority important-soon
/triage accepted
Fixes #111911