Repository: envoyproxy/ai-gateway
Pull request #235 (2025-01-30 19:06:35)

controller: refactors flags
**Commit Message**:

This refactors the main function of controller.
Previously it was conflated with the implementation detail
(zap) and that is exposed unnecessarily as controller main
arguments. This resolves it and expose the only necessary
one to have the consistency with extproc.

Notably, this also adds an new controller argument/helm 
value that configures the log level of external processor.
More fine tuned, per-route log level can be added later 
as in #204.

This also increases the test coverage.

**Related Issues/PRs (if applicable)**:

Supersedes #204 


Patch:
diff --git a/cmd/controller/main.go b/cmd/controller/main.go
index bdea057..4162c7e 100644
--- a/cmd/controller/main.go
+++ b/cmd/controller/main.go
@@ -2,10 +2,13 @@ package main
 
 import (
 	"flag"
+	"fmt"
+	"log/slog"
 	"net"
 	"os"
 
 	"github.com/envoyproxy/gateway/proto/extension"
+	"go.uber.org/zap/zapcore"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/health/grpc_health_v1"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -15,42 +18,90 @@ import (
 	"github.com/envoyproxy/ai-gateway/internal/extensionserver"
 )
 
-var setupLog = ctrl.Log.WithName("setup")
+// parseAndValidateFlags parses the command-line arguments provided in args,
+// validates them, and returns the parsed configuration.
+func parseAndValidateFlags(args []string) (
+	extProcLogLevel string,
+	extProcImage string,
+	enableLeaderElection bool,
+	logLevel zapcore.Level,
+	extensionServerPort string,
+	err error,
+) {
+	// Create a new FlagSet, rather than using the global flag.CommandLine.
+	fs := flag.NewFlagSet("my-service-flags", flag.ContinueOnError)
 
-// defaultOptions returns the default values for the program options.
-func defaultOptions() controller.Options {
-	return controller.Options{
-		ExtProcImage:         "ghcr.io/envoyproxy/ai-gateway/extproc:latest",
-		EnableLeaderElection: false,
+	// Define flags within this FlagSet.
+	extProcLogLevelPtr := fs.String(
+		"extProcLogLevel",
+		"info",
+		"The log level for the external processor. One of 'debug', 'info', 'warn', or 'error'.",
+	)
+	extProcImagePtr := fs.String(
+		"extProcImage",
+		"ghcr.io/envoyproxy/ai-gateway/extproc:latest",
+		"The image for the external processor",
+	)
+	enableLeaderElectionPtr := fs.Bool(
+		"enableLeaderElection",
+		true,
+		"Enable leader election for controller manager. Enabling this will ensure there is only one active controller manager.",
+	)
+	logLevelPtr := fs.String(
+		"logLevel",
+		"info",
+		"The log level for the controller manager. One of 'debug', 'info', 'warn', or 'error'.",
+	)
+	extensionServerPortPtr := fs.String(
+		"port",
+		":1063",
+		"gRPC port for the extension server",
+	)
+
+	// Parse the passed-in arguments.
+	if err = fs.Parse(args); err != nil {
+		err = fmt.Errorf("failed to parse flags: %w", err)
+		return
 	}
-}
 
-// getOptions parses the program flags and returns them as Options.
-func getOptions() (opts controller.Options, extensionServerPort *string) {
-	opts = defaultOptions()
-	flag.StringVar(&opts.ExtProcImage, "extprocImage", opts.ExtProcImage, "The image for the external processor")
-	flag.BoolVar(&opts.EnableLeaderElection, "leader-elect", opts.EnableLeaderElection,
-		"Enable leader election for controller manager. "+
-			"Enabling this will ensure there is only one active controller manager.")
-	extensionServerPort = flag.String("port", ":1063", "gRPC port for the extension server")
-	zapOpts := zap.Options{Development: true}
-	zapOpts.BindFlags(flag.CommandLine)
-	opts.ZapOptions = zapOpts
-	flag.Parse()
-	return
+	// Validate log levels by trying to unmarshal them.
+	var slogLevel slog.Level
+	if err = slogLevel.UnmarshalText([]byte(*extProcLogLevelPtr)); err != nil {
+		err = fmt.Errorf("invalid external processor log level: %q", *extProcLogLevelPtr)
+		return
+	}
+
+	var zapLogLevel zapcore.Level
+	if err = zapLogLevel.UnmarshalText([]byte(*logLevelPtr)); err != nil {
+		err = fmt.Errorf("invalid log level: %q", *logLevelPtr)
+		return
+	}
+	return *extProcLogLevelPtr, *extProcImagePtr, *enableLeaderElectionPtr, zapLogLevel, *extensionServerPortPtr, nil
 }
 
 func main() {
-	options, extensionServerPort := getOptions()
-	ctrl.SetLogger(zap.New(zap.UseFlagOptions(&options.ZapOptions)))
+	setupLog := ctrl.Log.WithName("setup")
+
+	flagExtProcLogLevel,
+		flagExtProcImage,
+		flagEnableLeaderElection,
+		zapLogLevel,
+		flagExtensionServerPort,
+		err := parseAndValidateFlags(os.Args[1:])
+	if err != nil {
+		setupLog.Error(err, "failed to parse and validate flags")
+		os.Exit(1)
+	}
+
+	ctrl.SetLogger(zap.New(zap.UseFlagOptions(&zap.Options{Development: true, Level: zapLogLevel})))
 	k8sConfig, err := ctrl.GetConfig()
 	if err != nil {
 		setupLog.Error(err, "failed to get k8s config")
 	}
 
-	lis, err := net.Listen("tcp", *extensionServerPort)
+	lis, err := net.Listen("tcp", flagExtensionServerPort)
 	if err != nil {
-		setupLog.Error(err, "failed to listen", "port", *extensionServerPort)
+		setupLog.Error(err, "failed to listen", "port", flagExtensionServerPort)
 		os.Exit(1)
 	}
 
@@ -72,7 +123,11 @@ func main() {
 	}()
 
 	// Start the controller.
-	if err := controller.StartControllers(ctx, k8sConfig, ctrl.Log.WithName("controller"), options); err != nil {
+	if err := controller.StartControllers(ctx, k8sConfig, ctrl.Log.WithName("controller"), controller.Options{
+		ExtProcImage:         flagExtProcImage,
+		ExtProcLogLevel:      flagExtProcLogLevel,
+		EnableLeaderElection: flagEnableLeaderElection,
+	}); err != nil {
 		setupLog.Error(err, "failed to start controller")
 	}
 }
diff --git a/go.mod b/go.mod
index 29b9e19..05cbc55 100644
--- a/go.mod
+++ b/go.mod
@@ -13,6 +13,7 @@ require (
 	github.com/google/go-cmp v0.6.0
 	github.com/openai/openai-go v0.1.0-alpha.49
 	github.com/stretchr/testify v1.10.0
+	go.uber.org/zap v1.27.0
 	golang.org/x/exp v0.0.0-20250128144449-3edf0e91c1ae
 	google.golang.org/grpc v1.70.0
 	google.golang.org/protobuf v1.36.4
@@ -84,7 +85,6 @@ require (
 	github.com/tidwall/sjson v1.2.5 // indirect
 	github.com/x448/float16 v0.8.4 // indirect
 	go.uber.org/multierr v1.11.0 // indirect
-	go.uber.org/zap v1.27.0 // indirect
 	golang.org/x/net v0.34.0 // indirect
 	golang.org/x/oauth2 v0.25.0 // indirect
 	golang.org/x/sync v0.10.0 // indirect
diff --git a/internal/controller/controller.go b/internal/controller/controller.go
index ecde9b7..dad1bcd 100644
--- a/internal/controller/controller.go
+++ b/internal/controller/controller.go
@@ -16,7 +16,6 @@ import (
 	"k8s.io/client-go/rest"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 	gwapiv1 "sigs.k8s.io/gateway-api/apis/v1"
 	gwapiv1b1 "sigs.k8s.io/gateway-api/apis/v1beta1"
 
@@ -41,9 +40,9 @@ func MustInitializeScheme(scheme *runtime.Scheme) {
 
 // Options defines the program configurable options that may be passed on the command line.
 type Options struct {
+	ExtProcLogLevel      string
 	ExtProcImage         string
 	EnableLeaderElection bool
-	ZapOptions           zap.Options
 }
 
 // StartControllers starts the controllers for the AI Gateway.
@@ -106,7 +105,7 @@ func StartControllers(ctx context.Context, config *rest.Config, logger logr.Logg
 	}
 
 	sink := newConfigSink(c, kubernetes.NewForConfigOrDie(config), logger.
-		WithName("config-sink"), sinkChan, options.ExtProcImage)
+		WithName("config-sink"), sinkChan, options.ExtProcImage, options.ExtProcLogLevel)
 
 	// Before starting the manager, initialize the config sink to sync all AIServiceBackend and AIGatewayRoute objects in the cluster.
 	if err = sink.init(ctx); err != nil {
diff --git a/internal/controller/sink.go b/internal/controller/sink.go
index f0f307d..303b46f 100644
--- a/internal/controller/sink.go
+++ b/internal/controller/sink.go
@@ -53,13 +53,13 @@ type ConfigSinkEventSecretUpdate struct {
 // consolidate the information from both objects to generate the ExtProc ConfigMap
 // and HTTPRoute objects.
 type configSink struct {
-	client                        client.Client
-	kube                          kubernetes.Interface
-	logger                        logr.Logger
-	defaultExtProcImage           string
-	defaultExtProcImagePullPolicy corev1.PullPolicy
-
-	eventChan chan ConfigSinkEvent
+	client                 client.Client
+	kube                   kubernetes.Interface
+	logger                 logr.Logger
+	extProcImage           string
+	extProcImagePullPolicy corev1.PullPolicy
+	extProcLogLevel        string
+	eventChan              chan ConfigSinkEvent
 }
 
 func newConfigSink(
@@ -68,14 +68,16 @@ func newConfigSink(
 	logger logr.Logger,
 	eventChan chan ConfigSinkEvent,
 	extProcImage string,
+	extProcLogLevel string,
 ) *configSink {
 	c := &configSink{
-		client:                        kubeClient,
-		kube:                          kube,
-		logger:                        logger,
-		defaultExtProcImage:           extProcImage,
-		defaultExtProcImagePullPolicy: corev1.PullIfNotPresent,
-		eventChan:                     eventChan,
+		client:                 kubeClient,
+		kube:                   kube,
+		logger:                 logger,
+		extProcImage:           extProcImage,
+		extProcImagePullPolicy: corev1.PullIfNotPresent,
+		extProcLogLevel:        extProcLogLevel,
+		eventChan:              eventChan,
 	}
 	return c
 }
@@ -479,12 +481,12 @@ func (c *configSink) syncExtProcDeployment(ctx context.Context, aiGatewayRoute *
 							Containers: []corev1.Container{
 								{
 									Name:            name,
-									Image:           c.defaultExtProcImage,
-									ImagePullPolicy: c.defaultExtProcImagePullPolicy,
+									Image:           c.extProcImage,
+									ImagePullPolicy: c.extProcImagePullPolicy,
 									Ports:           []corev1.ContainerPort{{Name: "grpc", ContainerPort: 1063}},
 									Args: []string{
 										"-configPath", "/etc/ai-gateway/extproc/" + expProcConfigFileName,
-										"-logLevel", "info",
+										"-logLevel", c.extProcLogLevel,
 									},
 									VolumeMounts: []corev1.VolumeMount{
 										{Name: "config", MountPath: "/etc/ai-gateway/extproc"},
diff --git a/manifests/charts/ai-gateway-helm/templates/deployment.yaml b/manifests/charts/ai-gateway-helm/templates/deployment.yaml
index c0817e5..886e4b5 100644
--- a/manifests/charts/ai-gateway-helm/templates/deployment.yaml
+++ b/manifests/charts/ai-gateway-helm/templates/deployment.yaml
@@ -34,8 +34,9 @@ spec:
             - containerPort: 1063
             - containerPort: 9090
           args:
-            - -zap-log-level={{ .Values.controller.logLevel }}
-            - --extprocImage={{ .Values.extProcImage.repository }}:{{ .Values.extProcImage.tag | default .Chart.AppVersion }}
+            - -logLevel={{ .Values.controller.logLevel }}
+            - --extProcImage={{ .Values.extProc.repository }}:{{ .Values.extProc.tag | default .Chart.AppVersion }}
+            - --extProcLogLevel={{ .Values.extProc.logLevel }}
           livenessProbe:
             grpc:
               port: 1063
diff --git a/manifests/charts/ai-gateway-helm/values.yaml b/manifests/charts/ai-gateway-helm/values.yaml
index d096adb..576eac5 100644
--- a/manifests/charts/ai-gateway-helm/values.yaml
+++ b/manifests/charts/ai-gateway-helm/values.yaml
@@ -1,9 +1,11 @@
 # Default values for ai-gateway-helm.
 
-extProcImage:
+extProc:
   repository: ghcr.io/envoyproxy/ai-gateway/extproc
   # Overrides the image tag whose default is the chart appVersion.
   tag: "latest"
+  # One of "info", "debug", "trace", "warn", "error", "fatal", "panic".
+  logLevel: info
 
 controller:
   logLevel: info


Test patch:
diff --git a/cmd/controller/main_test.go b/cmd/controller/main_test.go
new file mode 100644
index 0000000..df3d42c
--- /dev/null
+++ b/cmd/controller/main_test.go
@@ -0,0 +1,70 @@
+package main
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func Test_parseAndValidateFlags(t *testing.T) {
+	t.Run("no flags", func(t *testing.T) {
+		args := []string{}
+		extProcLogLevel, extProcImage, enableLeaderElection, logLevel, extensionServerPort, err := parseAndValidateFlags(args)
+		require.Equal(t, "info", extProcLogLevel)
+		require.Equal(t, "ghcr.io/envoyproxy/ai-gateway/extproc:latest", extProcImage)
+		require.True(t, enableLeaderElection)
+		require.Equal(t, "info", logLevel.String())
+		require.Equal(t, ":1063", extensionServerPort)
+		require.NoError(t, err)
+	})
+	t.Run("all flags", func(t *testing.T) {
+		for _, tc := range []struct {
+			name string
+			dash string
+		}{
+			{"single dash", "-"},
+			{"double dash", "--"},
+		} {
+			t.Run(tc.name, func(t *testing.T) {
+				args := []string{
+					tc.dash + "extProcLogLevel=debug",
+					tc.dash + "extProcImage=example.com/extproc:latest",
+					tc.dash + "enableLeaderElection=false",
+					tc.dash + "logLevel=debug",
+					tc.dash + "port=:8080",
+				}
+				extProcLogLevel, extProcImage, enableLeaderElection, logLevel, extensionServerPort, err := parseAndValidateFlags(args)
+				require.Equal(t, "debug", extProcLogLevel)
+				require.Equal(t, "example.com/extproc:latest", extProcImage)
+				require.False(t, enableLeaderElection)
+				require.Equal(t, "debug", logLevel.String())
+				require.Equal(t, ":8080", extensionServerPort)
+				require.NoError(t, err)
+			})
+		}
+	})
+
+	t.Run("invalid flags", func(t *testing.T) {
+		for _, tc := range []struct {
+			name   string
+			flags  []string
+			expErr string
+		}{
+			{
+				name:   "invalid extProcLogLevel",
+				flags:  []string{"--extProcLogLevel=invalid"},
+				expErr: "invalid external processor log level: \"invalid\"",
+			},
+			{
+				name:   "invalid logLevel",
+				flags:  []string{"--logLevel=invalid"},
+				expErr: "invalid log level: \"invalid\"",
+			},
+		} {
+			t.Run(tc.name, func(t *testing.T) {
+				_, _, _, _, _, err := parseAndValidateFlags(tc.flags)
+				require.ErrorContains(t, err, tc.expErr)
+			})
+		}
+	})
+}
diff --git a/internal/controller/sink_test.go b/internal/controller/sink_test.go
index 5eb4da0..9d319b0 100644
--- a/internal/controller/sink_test.go
+++ b/internal/controller/sink_test.go
@@ -33,7 +33,7 @@ func TestConfigSink_init(t *testing.T) {
 	kube := fake2.NewClientset()
 
 	eventChan := make(chan ConfigSinkEvent)
-	s := newConfigSink(fakeClient, kube, logr.Discard(), eventChan, "defaultExtProcImage")
+	s := newConfigSink(fakeClient, kube, logr.Discard(), eventChan, "defaultExtProcImage", "debug")
 	require.NotNil(t, s)
 }
 
@@ -42,7 +42,7 @@ func TestConfigSink_syncAIGatewayRoute(t *testing.T) {
 	kube := fake2.NewClientset()
 
 	eventChan := make(chan ConfigSinkEvent, 10)
-	s := newConfigSink(fakeClient, kube, logr.FromSlogHandler(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{})), eventChan, "defaultExtProcImage")
+	s := newConfigSink(fakeClient, kube, logr.FromSlogHandler(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{})), eventChan, "defaultExtProcImage", "debug")
 	require.NotNil(t, s)
 
 	for _, backend := range []*aigv1a1.AIServiceBackend{
@@ -110,7 +110,7 @@ func TestConfigSink_syncAIGatewayRoute(t *testing.T) {
 func TestConfigSink_syncAIServiceBackend(t *testing.T) {
 	eventChan := make(chan ConfigSinkEvent)
 	fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build()
-	s := newConfigSink(fakeClient, nil, logr.Discard(), eventChan, "defaultExtProcImage")
+	s := newConfigSink(fakeClient, nil, logr.Discard(), eventChan, "defaultExtProcImage", "debug")
 	s.syncAIServiceBackend(&aigv1a1.AIServiceBackend{ObjectMeta: metav1.ObjectMeta{Name: "apple", Namespace: "ns1"}})
 }
 
@@ -127,14 +127,14 @@ func TestConfigSink_syncBackendSecurityPolicy(t *testing.T) {
 	}
 	require.NoError(t, fakeClient.Create(context.Background(), &backend, &client.CreateOptions{}))
 
-	s := newConfigSink(fakeClient, nil, logr.Discard(), eventChan, "defaultExtProcImage")
+	s := newConfigSink(fakeClient, nil, logr.Discard(), eventChan, "defaultExtProcImage", "debug")
 	s.syncBackendSecurityPolicy(&aigv1a1.BackendSecurityPolicy{ObjectMeta: metav1.ObjectMeta{Name: "apple", Namespace: "ns"}})
 }
 
 func Test_newHTTPRoute(t *testing.T) {
 	eventChan := make(chan ConfigSinkEvent)
 	fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build()
-	s := newConfigSink(fakeClient, nil, logr.Discard(), eventChan, "defaultExtProcImage")
+	s := newConfigSink(fakeClient, nil, logr.Discard(), eventChan, "defaultExtProcImage", "debug")
 	httpRoute := &gwapiv1.HTTPRoute{
 		ObjectMeta: metav1.ObjectMeta{Name: "route1", Namespace: "ns1"},
 		Spec:       gwapiv1.HTTPRouteSpec{},
@@ -243,7 +243,7 @@ func Test_updateExtProcConfigMap(t *testing.T) {
 	kube := fake2.NewClientset()
 
 	eventChan := make(chan ConfigSinkEvent)
-	s := newConfigSink(fakeClient, kube, logr.Discard(), eventChan, "defaultExtProcImage")
+	s := newConfigSink(fakeClient, kube, logr.Discard(), eventChan, "defaultExtProcImage", "debug")
 	require.NoError(t, fakeClient.Create(context.Background(), &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "some-secret-policy"}}))
 	require.NoError(t, fakeClient.Create(context.Background(), &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "some-secret-policy-2"}}))
 
@@ -439,7 +439,7 @@ func TestConfigSink_SyncExtprocDeployment(t *testing.T) {
 	kube := fake2.NewClientset()
 
 	eventChan := make(chan ConfigSinkEvent)
-	s := newConfigSink(fakeClient, kube, logr.Discard(), eventChan, "envoyproxy/ai-gateway-extproc:foo")
+	s := newConfigSink(fakeClient, kube, logr.Discard(), eventChan, "envoyproxy/ai-gateway-extproc:foo", "debug")
 	err := fakeClient.Create(context.Background(), &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "some-secret-policy"}})
 	require.NoError(t, err)
 
@@ -603,7 +603,7 @@ func TestConfigSink_MountBackendSecurityPolicySecrets(t *testing.T) {
 	kube := fake2.NewClientset()
 
 	eventChan := make(chan ConfigSinkEvent)
-	s := newConfigSink(fakeClient, kube, logr.Discard(), eventChan, "defaultExtProcImage")
+	s := newConfigSink(fakeClient, kube, logr.Discard(), eventChan, "defaultExtProcImage", "debug")
 	err := s.init(context.Background())
 	require.NoError(t, err)
 	require.NoError(t, fakeClient.Create(context.Background(), &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "some-secret-policy"}}))
@@ -790,7 +790,7 @@ func Test_annotateExtProcPods(t *testing.T) {
 	kube := fake2.NewClientset()
 
 	eventChan := make(chan ConfigSinkEvent)
-	s := newConfigSink(fakeClient, kube, logr.Discard(), eventChan, "defaultExtProcImage")
+	s := newConfigSink(fakeClient, kube, logr.Discard(), eventChan, "defaultExtProcImage", "debug")
 
 	aiGatewayRoute := &aigv1a1.AIGatewayRoute{
 		ObjectMeta: metav1.ObjectMeta{Name: "myroute", Namespace: "foons"},