fix: fix Kong client's status check

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
 

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) {

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")
 		})

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
 }

internal/dataplane/configfetcher/config_fetcher_test.go

@@ -20,53 +20,73 @@ func TestTryFetchingValidConfigFromGateways(t *testing.T) {
 		configHash     = "8f1dd2f83bc2627cc6b71c76d1476592"
 	)
 
-	startAdminAPI := func(t *testing.T, ctx context.Context, opts ...mocks.AdminAPIHandlerOpt) *adminapi.Client {
+	startAdminAPI := func(t *testing.T, opts ...mocks.AdminAPIHandlerOpt) *adminapi.Client {
 		adminAPIHandler := mocks.NewAdminAPIHandler(t, opts...)
 		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 {
 		name                    string
 		expectError             bool
 		expectedLastValidStatus bool
-		adminAPIClients         func(t *testing.T, ctx context.Context) []*adminapi.Client
+		adminAPIClients         func(t *testing.T) []*adminapi.Client
 	}{
 		{
 			name: "correct configuration hash",
-			adminAPIClients: func(t *testing.T, ctx context.Context) []*adminapi.Client {
+			adminAPIClients: func(t *testing.T) []*adminapi.Client {
 				return []*adminapi.Client{
-					startAdminAPI(t, ctx, mocks.WithReady(true), mocks.WithConfigurationHash(configHash)),
-					startAdminAPI(t, ctx, mocks.WithReady(true), mocks.WithConfigurationHash(configHash)),
+					startAdminAPI(t, mocks.WithReady(true), mocks.WithConfigurationHash(configHash)),
+					startAdminAPI(t, mocks.WithReady(true), mocks.WithConfigurationHash(configHash)),
 				}
 			},
 			expectedLastValidStatus: true,
 		},
 		{
 			name: "zero configuration hash",
-			adminAPIClients: func(t *testing.T, ctx context.Context) []*adminapi.Client {
+			adminAPIClients: func(t *testing.T) []*adminapi.Client {
 				return []*adminapi.Client{
-					startAdminAPI(t, ctx, mocks.WithReady(true), mocks.WithConfigurationHash(zeroConfigHash)),
-					startAdminAPI(t, ctx, mocks.WithReady(true), mocks.WithConfigurationHash(zeroConfigHash)),
+					startAdminAPI(t, mocks.WithReady(true), mocks.WithConfigurationHash(zeroConfigHash)),
+					startAdminAPI(t, mocks.WithReady(true), mocks.WithConfigurationHash(zeroConfigHash)),
 				}
 			},
 		},
 		{
 			name: "none are ready",
-			adminAPIClients: func(t *testing.T, ctx context.Context) []*adminapi.Client {
+			adminAPIClients: func(t *testing.T) []*adminapi.Client {
 				return []*adminapi.Client{
-					startAdminAPI(t, ctx, mocks.WithReady(false)),
-					startAdminAPI(t, ctx, mocks.WithReady(false)),
+					startAdminAPI(t, mocks.WithReady(false)),
+					startAdminAPI(t, mocks.WithReady(false)),
+				}
+			},
+			expectError: true,
+		},
+		{
+			name: "one out of 2 is ready",
+			adminAPIClients: func(t *testing.T) []*adminapi.Client {
+				return []*adminapi.Client{
+					startAdminAPI(t, mocks.WithReady(true), mocks.WithConfigurationHash(configHash)),
+					startAdminAPI(t, mocks.WithReady(false)),
+				}
+			},
+			expectedLastValidStatus: true,
+		},
+		{
+			name: "one out of 2 is ready with zero config hash",
+			adminAPIClients: func(t *testing.T) []*adminapi.Client {
+				return []*adminapi.Client{
+					startAdminAPI(t, mocks.WithReady(true), mocks.WithConfigurationHash(zeroConfigHash)),
+					startAdminAPI(t, mocks.WithReady(false)),
 				}
 			},
 			expectError: true,
@@ -81,7 +101,9 @@ func TestTryFetchingValidConfigFromGateways(t *testing.T) {
 			require.Nil(t, state)
 
 			ctx := context.Background()
-			err := fetcher.TryFetchingValidConfigFromGateways(ctx, zapr.NewLogger(zap.NewNop()), tc.adminAPIClients(t, ctx), nil)
+			clients := tc.adminAPIClients(t)
+			logger := zapr.NewLogger(zap.NewNop())
+			err := fetcher.TryFetchingValidConfigFromGateways(ctx, logger, clients, nil)
 			if tc.expectError {
 				require.Error(t, err)
 				assert.False(t, ok)