This is an automated email from the ASF dual-hosted git repository.
xuetaoli pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/dubbo-go-pixiu.git
The following commit(s) were added to refs/heads/develop by this push:
new 4d9a4229 feat(controllers): improve gateway controller configuration
and code quality (#848)
4d9a4229 is described below
commit 4d9a42295d4133e6c551aadaaceec8a88a4c51c9
Author: Tsukikage <[email protected]>
AuthorDate: Sun Dec 28 22:34:06 2025 +0800
feat(controllers): improve gateway controller configuration and code
quality (#848)
- Make Image and ImagePullPolicy configurable instead of hardcoded
- Add GatewayConfig struct with default values
- Default image: mfordjody/pixiugateway:debug
- Default pull policy: Always
- Add warning log for unsupported filter types in mergeFilterConfig
- Refactor duplicate policy loading loops into shared
loadClusterPolicyByName
- Add unit tests for config and utils_policy packages
Resolves PR #827 and #839 review comments
---
controllers/internal/controller/config/config.go | 45 ++++
.../internal/controller/config/config_test.go | 232 +++++++++++++++++++++
.../internal/controller/gateway_controller.go | 5 +-
controllers/internal/controller/policy_loader.go | 29 +--
.../internal/controller/utils_policy_test.go | 109 ++++++++++
controllers/internal/converter/policy_applier.go | 4 +-
6 files changed, 402 insertions(+), 22 deletions(-)
diff --git a/controllers/internal/controller/config/config.go
b/controllers/internal/controller/config/config.go
index 310f098b..42d2f1b7 100644
--- a/controllers/internal/controller/config/config.go
+++ b/controllers/internal/controller/config/config.go
@@ -33,8 +33,17 @@ const (
DefaultLogLevel = "info"
DefaultMetricsAddr = ":8080"
DefaultProbeAddr = ":8081"
+ DefaultGatewayImage = "mfordjody/pixiugateway:debug"
+ DefaultImagePullPolicy = "Always"
)
+// ValidImagePullPolicies contains all valid Kubernetes ImagePullPolicy values
+var ValidImagePullPolicies = map[string]bool{
+ "Always": true,
+ "IfNotPresent": true,
+ "Never": true,
+}
+
type Config struct {
LogLevel string `json:"log_level" yaml:"log_level"`
ControllerName string `json:"controller_name"
yaml:"controller_name"`
@@ -44,6 +53,15 @@ type Config struct {
ProbeAddr string `json:"probe_addr" yaml:"probe_addr"`
SecureMetrics bool `json:"secure_metrics"
yaml:"secure_metrics"`
LeaderElection *LeaderElection `json:"leader_election"
yaml:"leader_election"`
+ Gateway *GatewayConfig `json:"gateway" yaml:"gateway"`
+}
+
+// GatewayConfig contains configuration for the Pixiu Gateway data plane
+type GatewayConfig struct {
+ // Image is the container image for the Pixiu Gateway
+ Image string `json:"image" yaml:"image"`
+ // ImagePullPolicy defines when to pull the container image
+ ImagePullPolicy string `json:"image_pull_policy"
yaml:"image_pull_policy"`
}
type LeaderElection struct {
@@ -65,9 +83,36 @@ func NewDefaultConfig() *Config {
ProbeAddr: DefaultProbeAddr,
MetricsAddr: DefaultMetricsAddr,
LeaderElection: NewLeaderElection(),
+ Gateway: NewDefaultGatewayConfig(),
+ }
+}
+
+// NewDefaultGatewayConfig returns default gateway configuration
+func NewDefaultGatewayConfig() *GatewayConfig {
+ return &GatewayConfig{
+ Image: DefaultGatewayImage,
+ ImagePullPolicy: DefaultImagePullPolicy,
}
}
+// ValidateImagePullPolicy checks if the given policy is a valid Kubernetes
ImagePullPolicy.
+// Returns the validated policy or the default if invalid.
+func ValidateImagePullPolicy(policy string) string {
+ if policy == "" {
+ return DefaultImagePullPolicy
+ }
+ if ValidImagePullPolicies[policy] {
+ return policy
+ }
+ // Invalid policy, return default
+ return DefaultImagePullPolicy
+}
+
+// IsValidImagePullPolicy checks if the given policy is valid
+func IsValidImagePullPolicy(policy string) bool {
+ return ValidImagePullPolicies[policy]
+}
+
func NewLeaderElection() *LeaderElection {
return &LeaderElection{
LeaseDuration: TimeDuration{Duration: 30 * time.Second},
diff --git a/controllers/internal/controller/config/config_test.go
b/controllers/internal/controller/config/config_test.go
new file mode 100644
index 00000000..aff1bd23
--- /dev/null
+++ b/controllers/internal/controller/config/config_test.go
@@ -0,0 +1,232 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package config
+
+import (
+ "encoding/json"
+ "testing"
+ "time"
+)
+
+func TestNewDefaultConfig(t *testing.T) {
+ cfg := NewDefaultConfig()
+
+ if cfg.LogLevel != DefaultLogLevel {
+ t.Errorf("expected LogLevel %s, got %s", DefaultLogLevel,
cfg.LogLevel)
+ }
+
+ if cfg.ControllerName != DefaultControllerName {
+ t.Errorf("expected ControllerName %s, got %s",
DefaultControllerName, cfg.ControllerName)
+ }
+
+ if cfg.LeaderElectionID != DefaultLeaderElectionID {
+ t.Errorf("expected LeaderElectionID %s, got %s",
DefaultLeaderElectionID, cfg.LeaderElectionID)
+ }
+
+ if cfg.Gateway == nil {
+ t.Fatal("expected Gateway config to be non-nil")
+ }
+
+ if cfg.Gateway.Image != DefaultGatewayImage {
+ t.Errorf("expected Gateway.Image %s, got %s",
DefaultGatewayImage, cfg.Gateway.Image)
+ }
+
+ if cfg.Gateway.ImagePullPolicy != DefaultImagePullPolicy {
+ t.Errorf("expected Gateway.ImagePullPolicy %s, got %s",
DefaultImagePullPolicy, cfg.Gateway.ImagePullPolicy)
+ }
+}
+
+func TestNewDefaultGatewayConfig(t *testing.T) {
+ gw := NewDefaultGatewayConfig()
+
+ if gw.Image != DefaultGatewayImage {
+ t.Errorf("expected Image %s, got %s", DefaultGatewayImage,
gw.Image)
+ }
+
+ if gw.ImagePullPolicy != DefaultImagePullPolicy {
+ t.Errorf("expected ImagePullPolicy %s, got %s",
DefaultImagePullPolicy, gw.ImagePullPolicy)
+ }
+}
+
+func TestGatewayConfigJSON(t *testing.T) {
+ gw := &GatewayConfig{
+ Image: "test/image:v1",
+ ImagePullPolicy: "Always",
+ }
+
+ data, err := json.Marshal(gw)
+ if err != nil {
+ t.Fatalf("failed to marshal GatewayConfig: %v", err)
+ }
+
+ var parsed GatewayConfig
+ if err := json.Unmarshal(data, &parsed); err != nil {
+ t.Fatalf("failed to unmarshal GatewayConfig: %v", err)
+ }
+
+ if parsed.Image != gw.Image {
+ t.Errorf("expected Image %s, got %s", gw.Image, parsed.Image)
+ }
+
+ if parsed.ImagePullPolicy != gw.ImagePullPolicy {
+ t.Errorf("expected ImagePullPolicy %s, got %s",
gw.ImagePullPolicy, parsed.ImagePullPolicy)
+ }
+}
+
+func TestSetControllerConfig(t *testing.T) {
+ // Note: This test modifies global state, so it cannot run in parallel
+ // Save and restore to minimize impact on other tests
+ original := ControllerConfig
+ defer SetControllerConfig(original)
+
+ newCfg := &Config{
+ LogLevel: "debug",
+ ControllerName: "test-controller",
+ Gateway: &GatewayConfig{
+ Image: "custom/image:test",
+ ImagePullPolicy: "Never",
+ },
+ }
+
+ SetControllerConfig(newCfg)
+
+ if ControllerConfig.LogLevel != "debug" {
+ t.Errorf("expected LogLevel debug, got %s",
ControllerConfig.LogLevel)
+ }
+
+ if ControllerConfig.Gateway.Image != "custom/image:test" {
+ t.Errorf("expected Gateway.Image custom/image:test, got %s",
ControllerConfig.Gateway.Image)
+ }
+}
+
+func TestTimeDurationJSON(t *testing.T) {
+ tests := []struct {
+ name string
+ duration time.Duration
+ expected string
+ }{
+ {"seconds", 30 * time.Second, `"30s"`},
+ {"minutes", 5 * time.Minute, `"5m0s"`},
+ {"hours", 2 * time.Hour, `"2h0m0s"`},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ td := TimeDuration{Duration: tt.duration}
+ data, err := json.Marshal(&td)
+ if err != nil {
+ t.Fatalf("failed to marshal: %v", err)
+ }
+
+ if string(data) != tt.expected {
+ t.Errorf("expected %s, got %s", tt.expected,
string(data))
+ }
+
+ var parsed TimeDuration
+ if err := json.Unmarshal(data, &parsed); err != nil {
+ t.Fatalf("failed to unmarshal: %v", err)
+ }
+
+ if parsed.Duration != tt.duration {
+ t.Errorf("expected duration %v, got %v",
tt.duration, parsed.Duration)
+ }
+ })
+ }
+}
+
+func TestTimeDurationUnmarshalNumeric(t *testing.T) {
+ // Test unmarshaling numeric value (nanoseconds)
+ data := []byte(`1000000000`)
+ var td TimeDuration
+ if err := json.Unmarshal(data, &td); err != nil {
+ t.Fatalf("failed to unmarshal numeric: %v", err)
+ }
+
+ if td.Duration != time.Second {
+ t.Errorf("expected 1s, got %v", td.Duration)
+ }
+}
+
+func TestNewLeaderElection(t *testing.T) {
+ le := NewLeaderElection()
+
+ if le.LeaseDuration.Duration != 30*time.Second {
+ t.Errorf("expected LeaseDuration 30s, got %v",
le.LeaseDuration.Duration)
+ }
+
+ if le.RenewDeadline.Duration != 20*time.Second {
+ t.Errorf("expected RenewDeadline 20s, got %v",
le.RenewDeadline.Duration)
+ }
+
+ if le.RetryPeriod.Duration != 2*time.Second {
+ t.Errorf("expected RetryPeriod 2s, got %v",
le.RetryPeriod.Duration)
+ }
+
+ if le.Disable != false {
+ t.Error("expected Disable to be false")
+ }
+}
+
+func TestValidateImagePullPolicy(t *testing.T) {
+ tests := []struct {
+ name string
+ input string
+ expected string
+ }{
+ {"valid Always", "Always", "Always"},
+ {"valid IfNotPresent", "IfNotPresent", "IfNotPresent"},
+ {"valid Never", "Never", "Never"},
+ {"empty string returns default", "", DefaultImagePullPolicy},
+ {"invalid policy returns default", "InvalidPolicy",
DefaultImagePullPolicy},
+ {"lowercase invalid", "always", DefaultImagePullPolicy},
+ {"mixed case invalid", "ALWAYS", DefaultImagePullPolicy},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ result := ValidateImagePullPolicy(tt.input)
+ if result != tt.expected {
+ t.Errorf("ValidateImagePullPolicy(%q) = %q,
want %q", tt.input, result, tt.expected)
+ }
+ })
+ }
+}
+
+func TestIsValidImagePullPolicy(t *testing.T) {
+ tests := []struct {
+ name string
+ input string
+ expected bool
+ }{
+ {"valid Always", "Always", true},
+ {"valid IfNotPresent", "IfNotPresent", true},
+ {"valid Never", "Never", true},
+ {"empty string", "", false},
+ {"invalid policy", "InvalidPolicy", false},
+ {"lowercase always", "always", false},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ result := IsValidImagePullPolicy(tt.input)
+ if result != tt.expected {
+ t.Errorf("IsValidImagePullPolicy(%q) = %v, want
%v", tt.input, result, tt.expected)
+ }
+ })
+ }
+}
diff --git a/controllers/internal/controller/gateway_controller.go
b/controllers/internal/controller/gateway_controller.go
index c4714c99..d630ea07 100644
--- a/controllers/internal/controller/gateway_controller.go
+++ b/controllers/internal/controller/gateway_controller.go
@@ -31,6 +31,7 @@ import (
import (
"controllers/api/v1alpha1"
+ "controllers/internal/controller/config"
"controllers/internal/controller/status"
"controllers/internal/converter"
@@ -669,8 +670,8 @@ func (r *GatewayReconciler) ensureDataPlane(ctx
context.Context, gateway *gatewa
Containers: []corev1.Container{
{
Name:
"pixiu",
- Image:
"mfordjody/pixiugateway:debug",
- ImagePullPolicy:
"Always",
+ Image:
config.ControllerConfig.Gateway.Image,
+ ImagePullPolicy:
corev1.PullPolicy(config.ValidateImagePullPolicy(config.ControllerConfig.Gateway.ImagePullPolicy)),
Args: []string{
"gateway",
"start",
diff --git a/controllers/internal/controller/policy_loader.go
b/controllers/internal/controller/policy_loader.go
index aff68def..2ca15981 100644
--- a/controllers/internal/controller/policy_loader.go
+++ b/controllers/internal/controller/policy_loader.go
@@ -93,9 +93,8 @@ func (pl *PolicyLoader) LoadFilterPolicies(ctx
context.Context, httpRoute *gatew
return policies, nil
}
-// LoadClusterPolicy loads PixiuClusterPolicy for a Service
-// This is kept for backward compatibility but may not be used with the new
serviceRef format
-func (pl *PolicyLoader) LoadClusterPolicy(ctx context.Context, namespace,
serviceName string) (*v1alpha1.PixiuClusterPolicy, error) {
+// loadClusterPolicyByName is a shared implementation for loading cluster
policies by service/cluster name
+func (pl *PolicyLoader) loadClusterPolicyByName(ctx context.Context,
namespace, name string) (*v1alpha1.PixiuClusterPolicy, error) {
var policyList v1alpha1.PixiuClusterPolicyList
if err := pl.client.List(ctx, &policyList,
client.InNamespace(namespace)); err != nil {
return nil, fmt.Errorf("failed to list cluster policies: %w",
err)
@@ -104,7 +103,7 @@ func (pl *PolicyLoader) LoadClusterPolicy(ctx
context.Context, namespace, servic
// Search through all policies and their serviceRef entries
for _, policy := range policyList.Items {
for _, serviceConfig := range policy.Spec.ServiceRef {
- if serviceConfig.Name == serviceName {
+ if serviceConfig.Name == name {
return &policy, nil
}
}
@@ -113,24 +112,16 @@ func (pl *PolicyLoader) LoadClusterPolicy(ctx
context.Context, namespace, servic
return nil, nil
}
+// LoadClusterPolicy loads PixiuClusterPolicy for a Service
+// This is kept for backward compatibility but may not be used with the new
serviceRef format
+func (pl *PolicyLoader) LoadClusterPolicy(ctx context.Context, namespace,
serviceName string) (*v1alpha1.PixiuClusterPolicy, error) {
+ return pl.loadClusterPolicyByName(ctx, namespace, serviceName)
+}
+
// LoadClusterPolicyByClusterName loads PixiuClusterPolicy by cluster name
// This searches through serviceRef entries to find matching service name
func (pl *PolicyLoader) LoadClusterPolicyByClusterName(ctx context.Context,
namespace, clusterName string) (*v1alpha1.PixiuClusterPolicy, error) {
- var policyList v1alpha1.PixiuClusterPolicyList
- if err := pl.client.List(ctx, &policyList,
client.InNamespace(namespace)); err != nil {
- return nil, fmt.Errorf("failed to list cluster policies: %w",
err)
- }
-
- // Search through all policies and their serviceRef entries
- for _, policy := range policyList.Items {
- for _, serviceConfig := range policy.Spec.ServiceRef {
- if serviceConfig.Name == clusterName {
- return &policy, nil
- }
- }
- }
-
- return nil, nil
+ return pl.loadClusterPolicyByName(ctx, namespace, clusterName)
}
// LoadAllClusterPolicies loads all PixiuClusterPolicy in a namespace
diff --git a/controllers/internal/controller/utils_policy_test.go
b/controllers/internal/controller/utils_policy_test.go
new file mode 100644
index 00000000..bead490f
--- /dev/null
+++ b/controllers/internal/controller/utils_policy_test.go
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package controller
+
+import (
+ "testing"
+)
+
+func TestSplitClusterName(t *testing.T) {
+ tests := []struct {
+ name string
+ clusterName string
+ wantNamespace string
+ wantService string
+ wantOk bool
+ }{
+ {
+ name: "valid cluster name",
+ clusterName: "default-myservice",
+ wantNamespace: "default",
+ wantService: "myservice",
+ wantOk: true,
+ },
+ {
+ // Note: Current implementation uses SplitN with n=2,
so only first hyphen is used as delimiter
+ // This means namespace cannot contain hyphens -
"kube-system-my-service" splits to ("kube", "system-my-service")
+ name: "hyphen splits at first occurrence only",
+ clusterName: "kube-system-my-service",
+ wantNamespace: "kube",
+ wantService: "system-my-service",
+ wantOk: true,
+ },
+ {
+ name: "no hyphen",
+ clusterName: "invalidname",
+ wantNamespace: "",
+ wantService: "",
+ wantOk: false,
+ },
+ {
+ name: "empty string",
+ clusterName: "",
+ wantNamespace: "",
+ wantService: "",
+ wantOk: false,
+ },
+ {
+ name: "only hyphen",
+ clusterName: "-",
+ wantNamespace: "",
+ wantService: "",
+ wantOk: false,
+ },
+ {
+ name: "empty namespace",
+ clusterName: "-service",
+ wantNamespace: "",
+ wantService: "",
+ wantOk: false,
+ },
+ {
+ name: "empty service",
+ clusterName: "namespace-",
+ wantNamespace: "",
+ wantService: "",
+ wantOk: false,
+ },
+ {
+ name: "multiple hyphens",
+ clusterName: "ns-svc-name-v1",
+ wantNamespace: "ns",
+ wantService: "svc-name-v1",
+ wantOk: true,
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ namespace, service, ok :=
splitClusterName(tt.clusterName)
+
+ if ok != tt.wantOk {
+ t.Errorf("splitClusterName(%q) ok = %v, want
%v", tt.clusterName, ok, tt.wantOk)
+ }
+
+ if namespace != tt.wantNamespace {
+ t.Errorf("splitClusterName(%q) namespace = %q,
want %q", tt.clusterName, namespace, tt.wantNamespace)
+ }
+
+ if service != tt.wantService {
+ t.Errorf("splitClusterName(%q) service = %q,
want %q", tt.clusterName, service, tt.wantService)
+ }
+ })
+ }
+}
diff --git a/controllers/internal/converter/policy_applier.go
b/controllers/internal/converter/policy_applier.go
index 438c30c0..44c22712 100644
--- a/controllers/internal/converter/policy_applier.go
+++ b/controllers/internal/converter/policy_applier.go
@@ -21,6 +21,7 @@ import (
"context"
"encoding/json"
"fmt"
+ "log"
"strings"
)
@@ -647,7 +648,8 @@ func mergeFilterConfig(filter *NetworkFilter, configMap
map[string]interface{},
filter.Config = configMap
}
default:
- // For other filter types, replace config
+ // For other filter types, log a warning and replace config
+ log.Printf("[WARN] mergeFilterConfig: unsupported filter type
%q, replacing config instead of merging", filterType)
filter.Config = configMap
}
return nil
