From 3cea97347199cee1942a3db5ad9ac364173da983 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Burzy=C5=84ski?= Date: Fri, 14 Jul 2023 17:34:32 +0200 Subject: [PATCH] feat: make NewKongClientForWorkspace first verify status + tests (#4357) Makes NewKongClientForWorkspace verify the status before calling other endpoints. Returns a new KongClientNotReadyError when the client is not ready yet. It gives callers a context of whether it makes sense to retry while awaiting for readiness (will be used in the next PRs for #3499). --- internal/adminapi/client_test.go | 38 ++++++++ internal/adminapi/kong.go | 34 +++++-- internal/adminapi/kong_test.go | 91 ++++++++++++++++--- test/envtest/adminapimock.go | 53 +---------- test/mocks/admin_api_handler.go | 149 +++++++++++++++++++++++++++++++ 5 files changed, 296 insertions(+), 69 deletions(-) create mode 100644 internal/adminapi/client_test.go create mode 100644 test/mocks/admin_api_handler.go diff --git a/internal/adminapi/client_test.go b/internal/adminapi/client_test.go new file mode 100644 index 0000000000..aabb973c3a --- /dev/null +++ b/internal/adminapi/client_test.go @@ -0,0 +1,38 @@ +package adminapi_test + +import ( + "context" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" + k8stypes "k8s.io/apimachinery/pkg/types" + + "github.com/kong/kubernetes-ingress-controller/v2/internal/adminapi" + "github.com/kong/kubernetes-ingress-controller/v2/test/mocks" +) + +func TestClientFactory_CreateAdminAPIClientAttachesPodReference(t *testing.T) { + factory := adminapi.NewClientFactoryForWorkspace("workspace", adminapi.HTTPClientOpts{}, "") + + adminAPIHandler := mocks.NewAdminAPIHandler(t) + adminAPIServer := httptest.NewServer(adminAPIHandler) + t.Cleanup(func() { adminAPIServer.Close() }) + + client, err := factory.CreateAdminAPIClient(context.Background(), adminapi.DiscoveredAdminAPI{ + Address: adminAPIServer.URL, + PodRef: k8stypes.NamespacedName{ + Namespace: "namespace", + Name: "name", + }, + }) + require.NoError(t, err) + require.NotNil(t, client) + + ref, ok := client.PodReference() + require.True(t, ok, "expected pod reference to be attached to the client") + require.Equal(t, k8stypes.NamespacedName{ + Namespace: "namespace", + Name: "name", + }, ref) +} diff --git a/internal/adminapi/kong.go b/internal/adminapi/kong.go index 7498877f78..3a3fd707f3 100644 --- a/internal/adminapi/kong.go +++ b/internal/adminapi/kong.go @@ -16,12 +16,27 @@ import ( tlsutil "github.com/kong/kubernetes-ingress-controller/v2/internal/util/tls" ) +// KongClientNotReadyError is returned when the Kong client is not ready to be used yet. +// This can happen if the Kong Admin API is not reachable, or if it's reachable but `GET /status` does not return 200. +type KongClientNotReadyError struct { + Err error +} + +func (e KongClientNotReadyError) Error() string { + return fmt.Sprintf("client not ready: %s", e.Err) +} + +func (e KongClientNotReadyError) Unwrap() error { + return e.Err +} + // NewKongClientForWorkspace returns a Kong API client for a given root API URL and workspace. +// It ensures that the client is ready to be used by performing a status check, returns KongClientNotReadyError if not. // If the workspace does not already exist, NewKongClientForWorkspace will create it. func NewKongClientForWorkspace(ctx context.Context, adminURL string, wsName string, httpclient *http.Client, ) (*Client, error) { - // create the base client, and if no workspace was provided then return that. + // Create the base client, and if no workspace was provided then return that. client, err := kong.NewClient(kong.String(adminURL), httpclient) if err != nil { return nil, fmt.Errorf("creating Kong client: %w", err) @@ -30,13 +45,18 @@ func NewKongClientForWorkspace(ctx context.Context, adminURL string, wsName stri return NewClient(client), nil } - // if a workspace was provided, verify whether or not it exists. + // Ensure that the client is ready to be used by performing a status check. + if _, err = client.Status(ctx); err != nil { + return nil, KongClientNotReadyError{Err: err} + } + + // If a workspace was provided, verify whether or not it exists. exists, err := client.Workspaces.ExistsByName(ctx, kong.String(wsName)) if err != nil { return nil, fmt.Errorf("looking up workspace: %w", err) } - // if the provided workspace does not exist, for convenience we create it. + // If the provided workspace does not exist, for convenience we create it. if !exists { workspace := kong.Workspace{ Name: kong.String(wsName), @@ -47,7 +67,7 @@ func NewKongClientForWorkspace(ctx context.Context, adminURL string, wsName stri } } - // ensure that we set the workspace appropriately + // Ensure that we set the workspace appropriately. client.SetWorkspace(wsName) return NewClient(client), nil @@ -70,7 +90,7 @@ type HTTPClientOpts struct { } const ( - headerNameAdminToken = "Kong-Admin-Token" + HeaderNameAdminToken = "Kong-Admin-Token" ) // MakeHTTPClient returns an HTTP client with the specified mTLS/headers configuration. @@ -133,11 +153,11 @@ func MakeHTTPClient(opts *HTTPClientOpts, kongAdminToken string) (*http.Client, func prepareHeaders(headers []string, kongAdminToken string) []string { if kongAdminToken != "" { contains := lo.ContainsBy(headers, func(header string) bool { - return strings.HasPrefix(header, headerNameAdminToken+":") + return strings.HasPrefix(header, HeaderNameAdminToken+":") }) if !contains { - headers = append(headers, headerNameAdminToken+":"+kongAdminToken) + headers = append(headers, HeaderNameAdminToken+":"+kongAdminToken) } } return headers diff --git a/internal/adminapi/kong_test.go b/internal/adminapi/kong_test.go index 7c907e6ce1..059669c4cd 100644 --- a/internal/adminapi/kong_test.go +++ b/internal/adminapi/kong_test.go @@ -1,7 +1,8 @@ -package adminapi +package adminapi_test import ( "bytes" + "context" "crypto/rand" "crypto/rsa" "crypto/tls" @@ -20,6 +21,9 @@ import ( "time" "github.com/stretchr/testify/require" + + "github.com/kong/kubernetes-ingress-controller/v2/internal/adminapi" + "github.com/kong/kubernetes-ingress-controller/v2/test/mocks" ) func TestMakeHTTPClientWithTLSOpts(t *testing.T) { @@ -31,27 +35,27 @@ func TestMakeHTTPClientWithTLSOpts(t *testing.T) { caPEM, certPEM, certPrivateKeyPEM, err = buildTLS(t) require.NoError(t, err, "Fail to build TLS certificates") - opts := HTTPClientOpts{ + opts := adminapi.HTTPClientOpts{ TLSSkipVerify: true, TLSServerName: "", CACertPath: "", CACert: caPEM.String(), Headers: nil, - TLSClient: TLSClientConfig{ + TLSClient: adminapi.TLSClientConfig{ Cert: certPEM.String(), Key: certPrivateKeyPEM.String(), }, } t.Run("without kong admin token", func(t *testing.T) { - httpclient, err := MakeHTTPClient(&opts, "") + httpclient, err := adminapi.MakeHTTPClient(&opts, "") require.NoError(t, err) require.NotNil(t, httpclient) require.NoError(t, validate(t, httpclient, caPEM, certPEM, certPrivateKeyPEM, "")) }) t.Run("with kong admin token", func(t *testing.T) { - httpclient, err := MakeHTTPClient(&opts, "my-token") + httpclient, err := adminapi.MakeHTTPClient(&opts, "my-token") require.NoError(t, err) require.NotNil(t, httpclient) require.NoError(t, validate(t, httpclient, caPEM, certPEM, certPrivateKeyPEM, "my-token")) @@ -88,33 +92,94 @@ func TestMakeHTTPClientWithTLSOptsAndFilePaths(t *testing.T) { require.Equal(t, certPrivateKeyPEM.Len(), writtenBytes) defer os.Remove(caFile.Name()) - opts := HTTPClientOpts{ + opts := adminapi.HTTPClientOpts{ TLSSkipVerify: true, TLSServerName: "", CACertPath: caFile.Name(), CACert: "", Headers: nil, - TLSClient: TLSClientConfig{ + TLSClient: adminapi.TLSClientConfig{ CertFile: certFile.Name(), KeyFile: certPrivateKeyFile.Name(), }, } t.Run("without kong admin token", func(t *testing.T) { - httpclient, err := MakeHTTPClient(&opts, "") + httpclient, err := adminapi.MakeHTTPClient(&opts, "") require.NoError(t, err) require.NotNil(t, httpclient) require.NoError(t, validate(t, httpclient, caPEM, certPEM, certPrivateKeyPEM, "")) }) t.Run("with kong admin token", func(t *testing.T) { - httpclient, err := MakeHTTPClient(&opts, "my-token") + httpclient, err := adminapi.MakeHTTPClient(&opts, "my-token") require.NoError(t, err) require.NotNil(t, httpclient) require.NoError(t, validate(t, httpclient, caPEM, certPEM, certPrivateKeyPEM, "my-token")) }) } +func TestNewKongClientForWorkspace(t *testing.T) { + const testWorkspace = "workspace" + + testCases := []struct { + name string + adminAPIReady bool + workspaceExists bool + expectError error + }{ + { + name: "admin api is ready and workspace exists", + adminAPIReady: true, + workspaceExists: true, + }, + { + name: "admin api is ready and workspace doesn't exist", + adminAPIReady: true, + workspaceExists: false, + }, + { + name: "admin api is not ready", + adminAPIReady: false, + expectError: adminapi.KongClientNotReadyError{}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + adminAPIHandler := mocks.NewAdminAPIHandler( + t, + mocks.WithWorkspaceExists(tc.workspaceExists), + mocks.WithReady(tc.adminAPIReady), + ) + adminAPIServer := httptest.NewServer(adminAPIHandler) + t.Cleanup(func() { adminAPIServer.Close() }) + + client, err := adminapi.NewKongClientForWorkspace( + context.Background(), + adminAPIServer.URL, + testWorkspace, + adminAPIServer.Client(), + ) + + if tc.expectError != nil { + require.IsType(t, err, tc.expectError) + return + } + require.NoError(t, err) + require.NotNil(t, client) + + if !tc.workspaceExists { + require.True(t, adminAPIHandler.WasWorkspaceCreated(), "expected workspace to be created") + } + + require.Equal(t, client.AdminAPIClient().Workspace(), testWorkspace) + _, ok := client.PodReference() + require.False(t, ok, "expected no pod reference to be attached to the client") + }) + } +} + func buildTLS(t *testing.T) (caPEM *bytes.Buffer, certPEM *bytes.Buffer, certPrivateKeyPEM *bytes.Buffer, err error) { const rsaKeySize = 2048 @@ -251,19 +316,19 @@ func validate(t *testing.T, successMessage := "connection successful" server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if kongAdminToken != "" { - v, ok := r.Header[http.CanonicalHeaderKey(headerNameAdminToken)] + v, ok := r.Header[http.CanonicalHeaderKey(adminapi.HeaderNameAdminToken)] if !ok { - fmt.Fprintf(w, "%s header not found", headerNameAdminToken) + fmt.Fprintf(w, "%s header not found", adminapi.HeaderNameAdminToken) return } if len(v) != 1 { fmt.Fprintf(w, "%s header expected to contain %s but found %v", - headerNameAdminToken, kongAdminToken, v) + adminapi.HeaderNameAdminToken, kongAdminToken, v) return } if v[0] != kongAdminToken { fmt.Fprintf(w, "%s header expected to contain %s but found %s", - headerNameAdminToken, kongAdminToken, v[0]) + adminapi.HeaderNameAdminToken, kongAdminToken, v[0]) return } } diff --git a/test/envtest/adminapimock.go b/test/envtest/adminapimock.go index ecc9563f32..3fa3d3a3b8 100644 --- a/test/envtest/adminapimock.go +++ b/test/envtest/adminapimock.go @@ -1,64 +1,19 @@ package envtest import ( - "io" - "net/http" "net/http/httptest" "testing" -) -const dblessConfig = `{ - "version": "3.3.0", - "configuration": { - "database": "off", - "router_flavor": "traditional", - "role": "traditional", - "proxy_listeners": [ - { - "backlog=%d+": false, - "ipv6only=on": false, - "ipv6only=off": false, - "ssl": false, - "so_keepalive=off": false, - "so_keepalive=%w*:%w*:%d*": false, - "listener": "0.0.0.0:8000", - "bind": false, - "port": 8000, - "deferred": false, - "so_keepalive=on": false, - "http2": false, - "proxy_protocol": false, - "ip": "0.0.0.0", - "reuseport": false - } - ] - } -}` + "github.com/kong/kubernetes-ingress-controller/v2/test/mocks" +) // StartAdminAPIServerMock starts a mock Kong Admin API server. // Server's .Close() method will be called during test's cleanup. func StartAdminAPIServerMock(t *testing.T) *httptest.Server { t.Helper() - mux := http.NewServeMux() - mux.HandleFunc("/config", func(w http.ResponseWriter, r *http.Request) { - body, _ := io.ReadAll(r.Body) - t.Logf("Admin API config: %s %s %s", r.Method, r.URL, string(body)) - }) - mux.HandleFunc("/status", func(w http.ResponseWriter, r *http.Request) { - if _, err := w.Write([]byte(dblessConfig)); err != nil { - w.WriteHeader(500) - return - } - }) - mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { - if _, err := w.Write([]byte(dblessConfig)); err != nil { - w.WriteHeader(500) - return - } - }) - - s := httptest.NewServer(mux) + handler := mocks.NewAdminAPIHandler(t) + s := httptest.NewServer(handler) t.Cleanup(func() { s.Close() }) diff --git a/test/mocks/admin_api_handler.go b/test/mocks/admin_api_handler.go new file mode 100644 index 0000000000..05dfefefe6 --- /dev/null +++ b/test/mocks/admin_api_handler.go @@ -0,0 +1,149 @@ +package mocks + +import ( + "net/http" + "sync/atomic" + "testing" +) + +const defaultDBLessStatusResponse = `{ + "version": "3.3.0", + "configuration": { + "database": "off", + "router_flavor": "traditional", + "role": "traditional", + "proxy_listeners": [ + { + "backlog=%d+": false, + "ipv6only=on": false, + "ipv6only=off": false, + "ssl": false, + "so_keepalive=off": false, + "so_keepalive=%w*:%w*:%d*": false, + "listener": "0.0.0.0:8000", + "bind": false, + "port": 8000, + "deferred": false, + "so_keepalive=on": false, + "http2": false, + "proxy_protocol": false, + "ip": "0.0.0.0", + "reuseport": false + } + ] + } +}` + +// AdminAPIHandler is a mock implementation of the Admin API. It only implements the endpoints that are +// required for the tests. +type AdminAPIHandler struct { + mux *http.ServeMux + t *testing.T + + // ready is a flag that indicates whether the server should return a 200 OK or a 503 Service Unavailable. + // It's set to true by default. + ready bool + + // workspaceExists makes `/workspace/workspaces/:id` return 200 when true, or 404 otherwise. + workspaceExists bool + + // workspaceWasCreated is set to true when a workspace `POST /workspaces` was called. + workspaceWasCreated atomic.Bool +} + +type AdminAPIHandlerOpt func(h *AdminAPIHandler) + +func WithWorkspaceExists(exists bool) AdminAPIHandlerOpt { + return func(h *AdminAPIHandler) { + h.workspaceExists = exists + } +} + +func WithReady(ready bool) AdminAPIHandlerOpt { + return func(h *AdminAPIHandler) { + h.ready = ready + } +} + +func NewAdminAPIHandler(t *testing.T, opts ...AdminAPIHandlerOpt) *AdminAPIHandler { + h := &AdminAPIHandler{ + t: t, + ready: true, + } + + for _, opt := range opts { + opt(h) + } + + mux := http.NewServeMux() + + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(defaultDBLessStatusResponse)) + return + } + + t.Errorf("unexpected request: %s %s", r.Method, r.URL) + }) + mux.HandleFunc("/status", func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + if !h.ready { + w.WriteHeader(http.StatusServiceUnavailable) + } else { + _, _ = w.Write([]byte(defaultDBLessStatusResponse)) + } + return + } + + t.Errorf("unexpected request: %s %s", r.Method, r.URL) + }) + mux.HandleFunc("/workspace/workspaces/", func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + if h.workspaceExists { + w.WriteHeader(http.StatusOK) + } else { + w.WriteHeader(http.StatusNotFound) + } + return + } + + t.Errorf("unexpected request: %s %s", r.Method, r.URL) + }) + mux.HandleFunc("/workspaces", func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + if !h.workspaceExists { + h.workspaceWasCreated.Store(true) + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id": "workspace"}`)) + } else { + t.Errorf("unexpected workspace creation") + } + return + } + + t.Errorf("unexpected request: %s %s", r.Method, r.URL) + }) + mux.HandleFunc("/config", func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`{"version": "3.3.0"}`)) + return + } + if r.Method == http.MethodPost { + w.WriteHeader(http.StatusNoContent) + return + } + + t.Errorf("unexpected request: %s %s", r.Method, r.URL) + }) + h.mux = mux + return h +} + +func (m *AdminAPIHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + m.t.Logf("AdminAPIHandler received request: %s %s", r.Method, r.URL) + m.mux.ServeHTTP(w, r) +} + +func (m *AdminAPIHandler) WasWorkspaceCreated() bool { + return m.workspaceWasCreated.Load() +}