diff --git a/auth.go b/auth.go index 94e9231..3c1e97f 100644 --- a/auth.go +++ b/auth.go @@ -130,6 +130,9 @@ func newAuthStore(cfg *Config) (*authStore, error) { // goes through resolveSecret before digesting, so env:/file: references // work the same as for provider keys. func (a *authStore) buildEntry(uc *UserConfig) (*authEntry, error) { + if uc == nil { + return nil, fmt.Errorf("llmgateway: user config entry must not be nil") + } grp, ok := a.groups[uc.Group] if !ok { return nil, fmt.Errorf("llmgateway: user %q references unknown group %q", uc.Name, uc.Group) @@ -154,7 +157,16 @@ func (a *authStore) buildEntry(uc *UserConfig) (*authEntry, error) { // (Task 4) to pick up changes to Users.File without restarting the plugin. // On error the store is left exactly as it was before the call — the new // set is validated and built in full before it replaces the old one. +// +// mu is held for the whole rebuild, not just the final swap. That serializes +// concurrent callers (a reload timer must never race itself), so one call's +// result can never be partially overwritten or interleaved with another's. +// identify's reads stay fast (RLock) and reloads are rare, so the extra hold +// time is a good trade. func (a *authStore) replaceFileUsers(us []*UserConfig) error { + a.mu.Lock() + defer a.mu.Unlock() + next := make(map[[32]byte]*authEntry, len(a.inline)+len(us)) for digest, entry := range a.inline { next[digest] = entry @@ -170,9 +182,7 @@ func (a *authStore) replaceFileUsers(us []*UserConfig) error { next[entry.digest] = entry } - a.mu.Lock() a.byDigest = next - a.mu.Unlock() return nil } diff --git a/auth_test.go b/auth_test.go index 6f29a7d..c53d799 100644 --- a/auth_test.go +++ b/auth_test.go @@ -1,7 +1,9 @@ package traefikllmgateway import ( + "fmt" "net/http/httptest" + "sync" "testing" ) @@ -280,3 +282,95 @@ func TestAuthStore_ReplaceFileUsers_UnknownGroup_ReturnsError(t *testing.T) { t.Fatal("want error for file user referencing unknown group") } } + +// TestAuthStore_ReplaceFileUsers_NilElement_ReturnsErrorNotPanic is the +// regression for a nil *UserConfig slice element reaching buildEntry. This +// path is reachable at runtime: json.Unmarshal of a `null` array element +// into []*UserConfig yields a nil entry, and the config-reload path (Task 4) +// that calls replaceFileUsers is NOT wrapped by recoverPanic — that guard +// only covers ServeHTTP. An unguarded nil dereference here would take down +// the whole process instead of failing one reload. +func TestAuthStore_ReplaceFileUsers_NilElement_ReturnsErrorNotPanic(t *testing.T) { + a, err := newAuthStore(testAuthCfg()) + if err != nil { + t.Fatal(err) + } + if err := a.replaceFileUsers([]*UserConfig{nil}); err == nil { + t.Fatal("want error for a nil user config element, got nil") + } + + // The store must still be usable — the rejected replace must not have + // partially mutated it. + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.Header.Set("Authorization", "Bearer sk-secret") + if _, _, ok := a.identify(r); !ok { + t.Fatal("want inline user still resolvable after a rejected nil-element replace") + } +} + +// TestNewAuthStore_NilInlineElement_ReturnsErrorNotPanic covers the same +// nil-element hazard reachable from newAuthStore's inline-user loop, which +// runs at plugin construction time — also outside recoverPanic's coverage. +func TestNewAuthStore_NilInlineElement_ReturnsErrorNotPanic(t *testing.T) { + cfg := &Config{ + Providers: map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}}, + Groups: map[string]*GroupConfig{"eng": {}}, + Users: &UsersConfig{Inline: []*UserConfig{nil}}, + } + if _, err := newAuthStore(cfg); err == nil { + t.Fatal("want error for a nil inline user config element, got nil") + } +} + +// TestAuthStore_ReplaceFileUsers_ConcurrentWritersAndReaders exercises +// replaceFileUsers and identify from many goroutines simultaneously under +// -race. Each writer swaps in a single, uniquely-keyed file user, so +// whichever writer's rebuild is applied last, the store must always hold +// exactly the inline users plus one file user's worth of entries — never a +// partial mix from two overlapping rebuilds, and never a state where the +// always-present inline user briefly disappears. +func TestAuthStore_ReplaceFileUsers_ConcurrentWritersAndReaders(t *testing.T) { + a, err := newAuthStore(testAuthCfg()) // inline user "a" / sk-secret, group eng + if err != nil { + t.Fatal(err) + } + + const writers = 20 + const readers = 10 + const readsPerReader = 50 + + var wg sync.WaitGroup + wg.Add(writers + readers) + + for i := 0; i < writers; i++ { + go func(i int) { + defer wg.Done() + uc := &UserConfig{Name: fmt.Sprintf("f%d", i), Group: "eng", APIKey: fmt.Sprintf("sk-file-%d", i)} + if err := a.replaceFileUsers([]*UserConfig{uc}); err != nil { + t.Errorf("replaceFileUsers(%d): %v", i, err) + } + }(i) + } + for i := 0; i < readers; i++ { + go func() { + defer wg.Done() + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.Header.Set("Authorization", "Bearer sk-secret") + for j := 0; j < readsPerReader; j++ { + if _, _, ok := a.identify(r); !ok { + t.Error("inline user must remain identifiable during concurrent file-user swaps") + return + } + } + }() + } + wg.Wait() + + a.mu.RLock() + got := len(a.byDigest) + a.mu.RUnlock() + want := len(a.inline) + 1 // inline users plus exactly one winning file-user batch + if got != want { + t.Fatalf("want exactly one file-user batch to survive concurrent swaps (size %d), got size %d — a rebuild was lost or merged with another", want, got) + } +}