Skip to content

fix(controller): return error instead of panicking on unknown auth mode #1791

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SarthakB11 wants to merge 2 commits into
base: main
Choose a base branch
from
+35 −13
Open

fix(controller): return error instead of panicking on unknown auth mode #1791

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
375d115
fix(controller): return error instead of panicking on unknown auth mode
SarthakB11 May 1, 2026
4431347
test(controller): assert auth-mode error message names invalid mode a…
SarthakB11 May 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions go/core/cmd/controller/auth_mode_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,10 @@ func TestGetAuthenticator(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
authenticator := getAuthenticator(tt.authCfg)
authenticator, err := getAuthenticator(tt.authCfg)
if err != nil {
t.Fatalf("getAuthenticator() unexpected error: %v", err)
}
gotType := getTypeName(authenticator)
if gotType != tt.wantType {
t.Errorf("getAuthenticator() = %s, want %s", gotType, tt.wantType)
Expand All @@ -41,14 +44,14 @@ func TestGetAuthenticator(t *testing.T) {
}
}

func TestGetAuthenticatorPanicsOnUnknownMode(t *testing.T) {
defer func() {
r := recover()
if r == nil {
t.Fatal("expected panic for unknown auth mode, got none")
}
}()
getAuthenticator(struct{ Mode, UserIDClaim string }{"proxy", ""})
func TestGetAuthenticatorErrorsOnUnknownMode(t *testing.T) {
authenticator, err := getAuthenticator(struct{ Mode, UserIDClaim string }{"proxy", ""})
if err == nil {
t.Fatal("expected error for unknown auth mode, got nil")
Copy link
Copy Markdown
Author

@SarthakB11 SarthakB11 May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. pushed 4431347f which asserts the error message includes both the invalid mode name and the list of supported modes, so a regression that drops either signal would fail the test now.

}
if authenticator != nil {
t.Errorf("expected nil authenticator on error, got %T", authenticator)
}
}

func getTypeName(v auth.AuthProvider) string {
Expand Down
15 changes: 10 additions & 5 deletions go/core/cmd/controller/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ limitations under the License.
package main

import (
"fmt"

"github.com/kagent-dev/kagent/go/core/internal/httpserver/auth"
"github.com/kagent-dev/kagent/go/core/pkg/app"
pkgauth "github.com/kagent-dev/kagent/go/core/pkg/auth"
Expand All @@ -31,7 +33,10 @@ import (
func main() {
authorizer := &auth.NoopAuthorizer{}
app.Start(func(bootstrap app.BootstrapConfig) (*app.ExtensionConfig, error) {
authenticator := getAuthenticator(bootstrap.Config.Auth)
authenticator, err := getAuthenticator(bootstrap.Config.Auth)
if err != nil {
return nil, err
Copy link
Copy Markdown
Author

@SarthakB11 SarthakB11 May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, the defer-skip is real. app.Start calls os.Exit(1) at app.go:496 when the extension-config callback returns err, skipping the tracing-shutdown defer registered at app.go:320 that the previous panic would have unwound through.

practical flush impact at this specific call site is small (very few spans are buffered between tracing init and getAuthenticator), but the defer-skip applies to every config-failure path in startup, not just this one. the underlying issue is app.Start using os.Exit rather than returning errors.

the suggested panic(err) keeps getAuthenticator returning a typed error (so the test assertions still hold) and panics at the call site so defers run. happy to apply it. otherwise i can refactor app.Start to thread errors back to main as a separate PR. which would you prefer?

}
return &app.ExtensionConfig{
Authenticator: authenticator,
Authorizer: authorizer,
Expand All @@ -41,13 +46,13 @@ func main() {
}, nil)
}

func getAuthenticator(authCfg struct{ Mode, UserIDClaim string }) pkgauth.AuthProvider {
func getAuthenticator(authCfg struct{ Mode, UserIDClaim string }) (pkgauth.AuthProvider, error) {
switch authCfg.Mode {
case "trusted-proxy":
return auth.NewProxyAuthenticator(authCfg.UserIDClaim)
return auth.NewProxyAuthenticator(authCfg.UserIDClaim), nil
case "unsecure":
return &auth.UnsecureAuthenticator{}
return &auth.UnsecureAuthenticator{}, nil
default:
panic("unknown auth mode: " + authCfg.Mode + " (valid modes: unsecure, trusted-proxy)")
return nil, fmt.Errorf("unknown auth mode %q (valid modes: unsecure, trusted-proxy)", authCfg.Mode)
}
}
Loading