fix: clean up in-memory connector after delete.

[loafoe]

Overview

Fixes stale references to Connector when re-adding under same id

What this PR does / why we need it

I've investigated and fixed the issue where connectors provisioned via the gRPC API would retain old configurations after being deleted and recreated.

The root cause was that Dex's Server maintains an in-memory cache of opened connectors. When a connector is retrieved, Dex compares its ResourceVersion in storage with the one in the cache. Since the gRPC CreateConnector call hardcodes the ResourceVersion to "1", deleting and recreating a connector with the same ID resulted in matching versions, causing the server to continue using the stale, cached connector object.

I implemented the following changes to resolve this:

1. Added CloseConnector(id string) to Server: This new method safely removes a connector from the server's in-memory cache.
2. Updated gRPC Handlers: I modified the CreateConnector and DeleteConnector handlers in server/api.go to call CloseConnector. This ensures the cache is invalidated whenever a connector is created or removed, forcing the server to re-initialize it with the latest configuration on its next use.

All existing server tests passed.

Special notes for your reviewer

Fixes #4528

[loafoe]

@nabokihms added a test that demonstrates the issue. The test proves that without the CloseConnector calls in CreateConnector and DeleteConnector, the server would reuse a stale in-memory instance of the connector if the ResourceVersion happened to match. Also rebased to latest master

[nabokihms]

@loafoe I tested the test

1. With no changes it fails

➜  dex git:(bugfix/connector-crud) ✗ git diff | cat
diff --git a/server/api.go b/server/api.go
index 62782b44..724c4807 100644
--- a/server/api.go
+++ b/server/api.go
@@ -470,10 +470,6 @@ func (d dexAPI) CreateConnector(ctx context.Context, req *api.CreateConnectorReq
                return nil, fmt.Errorf("create connector: %v", err)
        }
 
-       if d.server != nil {
-               d.server.CloseConnector(req.Connector.Id)
-       }
-
        return &api.CreateConnectorResp{}, nil
 }
 
@@ -542,11 +538,6 @@ func (d dexAPI) DeleteConnector(ctx context.Context, req *api.DeleteConnectorReq
                d.logger.Error("api: failed to delete connector", "err", err)
                return nil, fmt.Errorf("delete connector: %v", err)
        }
-
-       if d.server != nil {
-               d.server.CloseConnector(req.Id)
-       }
-
        return &api.DeleteConnectorResp{}, nil
 }
 
➜  dex git:(bugfix/connector-crud) ✗ go test ./server/...
--- FAIL: TestConnectorCacheInvalidation (0.00s)
    api_cache_test.go:98: failed to login with second password, cache might still be stale
    api_cache_test.go:103: unexpectedly logged in with first password, cache is definitely stale
FAIL
FAIL    github.com/dexidp/dex/server    1.101s
?       github.com/dexidp/dex/server/internal   [no test files]
ok      github.com/dexidp/dex/server/signer     (cached)
FAIL

2. With closing only on deleting it passes

➜  dex git:(bugfix/connector-crud) ✗ git diff | cat
diff --git a/server/api.go b/server/api.go
index 62782b44..eafff924 100644
--- a/server/api.go
+++ b/server/api.go
@@ -470,10 +470,6 @@ func (d dexAPI) CreateConnector(ctx context.Context, req *api.CreateConnectorReq
                return nil, fmt.Errorf("create connector: %v", err)
        }
 
-       if d.server != nil {
-               d.server.CloseConnector(req.Connector.Id)
-       }
-
        return &api.CreateConnectorResp{}, nil
 }
 
➜  dex git:(bugfix/connector-crud) ✗ go test ./server/...
ok      github.com/dexidp/dex/server    1.145s
?       github.com/dexidp/dex/server/internal   [no test files]
ok      github.com/dexidp/dex/server/signer     (cached)

3. With closing only on creating it also passes (I understand why, because it was not closed properly on deleting, we need to close it on creating)

➜  dex git:(bugfix/connector-crud) ✗ git diff | cat      
diff --git a/server/api.go b/server/api.go
index 62782b44..72750811 100644
--- a/server/api.go
+++ b/server/api.go
@@ -543,10 +543,6 @@ func (d dexAPI) DeleteConnector(ctx context.Context, req *api.DeleteConnectorReq
                return nil, fmt.Errorf("delete connector: %v", err)
        }
 
-       if d.server != nil {
-               d.server.CloseConnector(req.Id)
-       }
-
        return &api.DeleteConnectorResp{}, nil
 }

➜  dex git:(bugfix/connector-crud) ✗ go test ./server/...
ok      github.com/dexidp/dex/server    1.143s
?       github.com/dexidp/dex/server/internal   [no test files]
ok      github.com/dexidp/dex/server/signer     (cached)

We need to close on creating only if someone deletes the connector from the database manually (not through the API), e.g., by deleting a row from Postgres or a resource from Kubernetes. This is ok, we can leave closing on creating, but I just want a proper comment in the code for this specific case.

[loafoe]

We need to close on creating only if someone deletes the connector from the database manually (not through the API), e.g., by deleting a row from Postgres or a resource from Kubernetes. This is ok, we can leave closing on creating, but I just want a proper comment in the code for this specific case.

thanks @nabokihms for going through the test code. I've removed the close on delete and left it in the create path with a comment. All tests still pass.