From 0b289e521f4d57ead06c4d97c653006f97006b52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Ma=C5=82ek?= Date: Mon, 18 Nov 2024 16:59:47 +0100 Subject: [PATCH] fix: fix Kong client's status check --- CHANGELOG.md | 3 ++ internal/adminapi/kong.go | 40 +++++++------- internal/adminapi/kong_test.go | 52 +++++++++++++++++-- .../dataplane/configfetcher/config_fetcher.go | 3 ++ .../configfetcher/config_fetcher_test.go | 28 ++++++++-- 5 files changed, 100 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d7b19eb55d7..296fd77d699 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -131,6 +131,9 @@ Adding a new version? You'll need three changes: [#6585](https://github.com/Kong/kubernetes-ingress-controller/pull/6585) - Fix panic when handling `KongConsumer` without `username` specified. [#6665](https://github.com/Kong/kubernetes-ingress-controller/pull/6665) +- Fixed Kong client status check causing unnecessary `config update failed` errors + and `KongConfigurationApplyFailed` events being generated. + [#6689](https://github.com/Kong/kubernetes-ingress-controller/pull/6689) ### Added diff --git a/internal/adminapi/kong.go b/internal/adminapi/kong.go index 0eeec4f6f84..144cfb1ebe3 100644 --- a/internal/adminapi/kong.go +++ b/internal/adminapi/kong.go @@ -64,33 +64,37 @@ func NewKongClientForWorkspace( if err != nil { return nil, fmt.Errorf("creating Kong client: %w", err) } - if wsName == "" { - return NewClient(client), nil - } - // 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 wsName != "" { + // 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 !exists { - workspace := kong.Workspace{ - Name: kong.String(wsName), + // If the provided workspace does not exist, for convenience we create it. + if !exists { + workspace := kong.Workspace{ + Name: kong.String(wsName), + } + if _, err := client.Workspaces.Create(ctx, &workspace); err != nil { + return nil, fmt.Errorf("creating workspace: %w", err) + } } - if _, err := client.Workspaces.Create(ctx, &workspace); err != nil { - return nil, fmt.Errorf("creating workspace: %w", err) + // Ensure that we set the workspace appropriately. + client.SetWorkspace(wsName) + + // Now that we have set the workspace, ensure that the client is ready + // to be used with said workspace. + if _, err := client.Status(ctx); err != nil { + return nil, KongClientNotReadyError{Err: err} } } - // Ensure that we set the workspace appropriately. - client.SetWorkspace(wsName) cl := NewClient(client) fetchedKongVersion, err := cl.GetKongVersion(ctx) @@ -99,7 +103,7 @@ func NewKongClientForWorkspace( } kongVersion, err := kong.NewVersion(fetchedKongVersion) if err != nil { - return nil, KongGatewayUnsupportedVersionError{msg: fmt.Sprintf("getting Kong version: %v", err)} + return nil, KongGatewayUnsupportedVersionError{msg: fmt.Sprintf("invalid Kong version: %v", err)} } kongSemVersion := semver.Version{Major: kongVersion.Major(), Minor: kongVersion.Minor(), Patch: kongVersion.Patch()} if kongSemVersion.LT(versions.KICv3VersionCutoff) { diff --git a/internal/adminapi/kong_test.go b/internal/adminapi/kong_test.go index 06b28f58f18..0660ff3dd61 100644 --- a/internal/adminapi/kong_test.go +++ b/internal/adminapi/kong_test.go @@ -105,56 +105,100 @@ func TestMakeHTTPClientWithTLSOptsAndFilePaths(t *testing.T) { } func TestNewKongClientForWorkspace(t *testing.T) { - const testWorkspace = "workspace" + const workspace = "workspace" testCases := []struct { name string adminAPIReady bool adminAPIVersion string + workspace string workspaceExists bool expectError error }{ + { + name: "admin api is ready for default workspace", + adminAPIReady: true, + workspace: "", + }, { name: "admin api is ready and workspace exists", adminAPIReady: true, + workspace: workspace, workspaceExists: true, }, { name: "admin api is ready and workspace doesn't exist", adminAPIReady: true, + workspace: workspace, workspaceExists: false, }, { name: "admin api is not ready", adminAPIReady: false, + workspace: "", expectError: adminapi.KongClientNotReadyError{}, }, + { + name: "admin api is not ready for an existing workspace", + adminAPIReady: false, + workspace: workspace, + workspaceExists: true, + expectError: adminapi.KongClientNotReadyError{}, + }, { name: "admin api is in too old version", adminAPIReady: true, adminAPIVersion: "3.4.0", + workspace: "", expectError: adminapi.KongGatewayUnsupportedVersionError{}, }, { name: "admin api is in supported OSS version", adminAPIReady: true, + workspace: "", adminAPIVersion: versions.KICv3VersionCutoff.String(), }, { name: "admin api has malformed version", adminAPIReady: true, adminAPIVersion: "3-malformed-version", + workspace: "", + expectError: adminapi.KongGatewayUnsupportedVersionError{}, + }, + { + name: "admin api has malformed version for workspace", + adminAPIReady: true, + adminAPIVersion: "3-malformed-version", + workspace: workspace, + workspaceExists: true, expectError: adminapi.KongGatewayUnsupportedVersionError{}, }, { name: "admin api has enterprise version", adminAPIReady: true, + workspace: "", + adminAPIVersion: "3.4.1.2", + }, + { + name: "admin api has enterprise version for workspace", + adminAPIReady: true, + workspace: workspace, + workspaceExists: true, adminAPIVersion: "3.4.1.2", }, { name: "admin api has too old enterprise version", adminAPIReady: true, adminAPIVersion: "3.4.0.2", + workspace: "", + expectError: adminapi.KongGatewayUnsupportedVersionError{}, + }, + { + name: "admin api has too old enterprise version for workspace", + adminAPIReady: true, + adminAPIVersion: "3.4.0.2", + workspace: workspace, + workspaceExists: true, expectError: adminapi.KongGatewayUnsupportedVersionError{}, }, } @@ -173,7 +217,7 @@ func TestNewKongClientForWorkspace(t *testing.T) { client, err := adminapi.NewKongClientForWorkspace( context.Background(), adminAPIServer.URL, - testWorkspace, + tc.workspace, adminAPIServer.Client(), ) @@ -184,11 +228,11 @@ func TestNewKongClientForWorkspace(t *testing.T) { require.NoError(t, err) require.NotNil(t, client) - if !tc.workspaceExists { + if tc.workspace != "" && !tc.workspaceExists { require.True(t, adminAPIHandler.WasWorkspaceCreated(), "expected workspace to be created") } - require.Equal(t, client.AdminAPIClient().Workspace(), testWorkspace) + require.Equal(t, client.AdminAPIClient().Workspace(), tc.workspace) _, ok := client.PodReference() require.False(t, ok, "expected no pod reference to be attached to the client") }) diff --git a/internal/dataplane/configfetcher/config_fetcher.go b/internal/dataplane/configfetcher/config_fetcher.go index a5e86d6fe62..86b8865a1fe 100644 --- a/internal/dataplane/configfetcher/config_fetcher.go +++ b/internal/dataplane/configfetcher/config_fetcher.go @@ -152,6 +152,9 @@ func (cf *DefaultKongLastGoodConfigFetcher) TryFetchingValidConfigFromGateways( } cf.lastValidState = goodKongState logger.V(logging.DebugLevel).Info("Last good configuration fetched from Kong node", "url", clientUsed.BaseRootURL()) + // If we've found at least one good configuration, we can return early without an error. + // We got what we wanted, which is the last good configuration. + return nil } return errs } diff --git a/internal/dataplane/configfetcher/config_fetcher_test.go b/internal/dataplane/configfetcher/config_fetcher_test.go index 4e141a74f4d..a950bb2f03a 100644 --- a/internal/dataplane/configfetcher/config_fetcher_test.go +++ b/internal/dataplane/configfetcher/config_fetcher_test.go @@ -25,15 +25,15 @@ func TestTryFetchingValidConfigFromGateways(t *testing.T) { adminAPIServer := httptest.NewServer(adminAPIHandler) t.Cleanup(func() { adminAPIServer.Close() }) - client, err := adminapi.NewKongClientForWorkspace( - ctx, + // NOTE: We use here adminapi.NewKongAPIClient() as that doesn't check + // the status of the Kong Gateway but just returns the client. + client, err := adminapi.NewKongAPIClient( adminAPIServer.URL, - "", // no workspace adminAPIServer.Client(), ) require.NoError(t, err) require.NotNil(t, client) - return client + return adminapi.NewClient(client) } testCases := []struct { @@ -71,6 +71,26 @@ func TestTryFetchingValidConfigFromGateways(t *testing.T) { }, expectError: true, }, + { + name: "one out of 2 is ready", + adminAPIClients: func(t *testing.T, ctx context.Context) []*adminapi.Client { + return []*adminapi.Client{ + startAdminAPI(t, ctx, mocks.WithReady(true), mocks.WithConfigurationHash(configHash)), + startAdminAPI(t, ctx, mocks.WithReady(false)), + } + }, + expectedLastValidStatus: true, + }, + { + name: "one out of 2 is ready with zero config hash", + adminAPIClients: func(t *testing.T, ctx context.Context) []*adminapi.Client { + return []*adminapi.Client{ + startAdminAPI(t, ctx, mocks.WithReady(true), mocks.WithConfigurationHash(zeroConfigHash)), + startAdminAPI(t, ctx, mocks.WithReady(false)), + } + }, + expectError: true, + }, } for _, tc := range testCases {