Copilot commented on code in PR #892:
URL: https://github.com/apache/dubbo-go-pixiu/pull/892#discussion_r2934586885
##########
pkg/server/router_manager.go:
##########
@@ -46,6 +46,19 @@ func (rm *RouterManager) AddRouterListener(l RouterListener)
{
rm.rls = append(rm.rls, l)
}
+func (rm *RouterManager) RemoveRouterListener(l RouterListener) {
+ for i, listener := range rm.rls {
+ if listener == l {
+ rm.rls = append(rm.rls[:i], rm.rls[i+1:]...)
+ return
+ }
+ }
+}
+
+func (rm *RouterManager) ClearRouterListeners() {
+ rm.rls = nil
+}
Review Comment:
RouterManager listener list (rm.rls) is mutated/cleared with no
synchronization, but it’s also iterated in AddRouter/DeleteRouter/UpdateRoutes.
Calling RemoveRouterListener/ClearRouterListeners concurrently with route
updates can cause data races and potentially panics. Consider adding a mutex
(or copy-on-write snapshot) to protect rm.rls across add/remove/clear and all
iterations.
##########
pkg/hotreload/route_reloader.go:
##########
@@ -56,16 +54,44 @@ func (r *RouteReloader) CheckUpdate(oldConfig, newConfig
*model.Bootstrap) bool
// HotReload applies the new route configuration.
func (r *RouteReloader) HotReload(oldConfig, newConfig *model.Bootstrap) error
{
- oldRoutes := extractRoutes(oldConfig)
- newRoutes := extractRoutes(newConfig)
+ logger.Info("Starting route hot reload")
+
+ srv := server.GetServer()
+ if srv == nil {
+ logger.Error("Server instance is nil")
+ return errors.New("server instance is nil")
+ }
+ logger.Info("Got server instance")
+
+ logger.Info("Reinitializing server components...")
+
+ listenerManager := srv.GetListenerManager()
+ if listenerManager == nil {
+ logger.Error("Listener manager is nil")
+ return errors.New("listener manager is nil")
+ }
+
+ refreshed := 0
+ for _, listener := range newConfig.StaticResources.Listeners {
+ logger.Infof("Refreshing listener: name=%s, protocol=%s",
listener.Name, listener.ProtocolStr)
+
+ srv.GetRouterManager().ClearRouterListeners()
+
+ if err := listenerManager.UpdateListener(listener); err != nil {
+ logger.Errorf("Failed to refresh listener %s: %v",
listener.Name, err)
+ return errors.Wrapf(err, "failed to refresh listener
%s", listener.Name)
+ }
Review Comment:
`ClearRouterListeners()` is called inside the listener refresh loop. This
will drop listeners registered by earlier refreshed listeners, so only the last
refreshed listener’s RouterCoordinator remains subscribed (breaking dynamic
route updates for multiple listeners). If the goal is to avoid duplicate
registrations, clear once before the loop (and then re-register all
coordinators) or remove only the stale listener(s) being replaced.
##########
pkg/hotreload/http_handler.go:
##########
@@ -0,0 +1,212 @@
+/*
+ * 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 hotreload
+
+import (
+ "encoding/json"
+ "fmt"
+ "io"
+ "net/http"
+ "os"
+ "sync"
+ "time"
+)
+
+import (
+ "gopkg.in/yaml.v3"
+)
+
+import (
+ "github.com/apache/dubbo-go-pixiu/pkg/config"
+ "github.com/apache/dubbo-go-pixiu/pkg/logger"
+ "github.com/apache/dubbo-go-pixiu/pkg/model"
+)
+
+var (
+ reloadMutex sync.Mutex
+ configPath string
+)
+
+func SetConfigPath(path string) {
+ configPath = path
+}
+
+// ReloadHandler handles HTTP reload requests
+type ReloadHandler struct{}
+
+// ServeHTTP handles the reload HTTP request
+func (h *ReloadHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+ if r.Method != http.MethodPost {
+ http.Error(w, "Method not allowed, use POST",
http.StatusMethodNotAllowed)
+ return
+ }
+
+ logger.Info("Received reload request via HTTP")
+
+ var err error
+ if r.ContentLength > 0 {
+ err = triggerConfigReloadFromBody(r)
+ } else {
+ err = triggerConfigReload()
+ }
+
+ if err != nil {
+ logger.Errorf("Reload failed: %v", err)
+ http.Error(w, fmt.Sprintf("Reload failed: %v", err),
http.StatusInternalServerError)
+ return
+ }
+
+ w.Header().Set("Content-Type", "application/json")
+ w.WriteHeader(http.StatusOK)
+ writeJSONResponse(w, "success", "Configuration reloaded successfully")
+ logger.Info("Reload completed successfully")
+}
+
+func writeJSONResponse(w http.ResponseWriter, status, message string) {
+ response := map[string]string{
+ "status": status,
+ "message": message,
+ "time": time.Now().Format(time.RFC3339),
+ }
+ if err := json.NewEncoder(w).Encode(response); err != nil {
+ logger.Errorf("Failed to encode response: %v", err)
+ }
+}
+
+// HealthHandler handles health check requests
+type HealthHandler struct{}
+
+// ServeHTTP handles the health check HTTP request
+func (h *HealthHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+ w.Header().Set("Content-Type", "application/json")
+ w.WriteHeader(http.StatusOK)
+ writeJSONResponse(w, "healthy", "")
+}
+
+// triggerConfigReload reloads configuration from file
+func triggerConfigReload() error {
+ reloadMutex.Lock()
+ defer reloadMutex.Unlock()
+
+ if configPath == "" {
+ return fmt.Errorf("config path not set")
+ }
+
+ logger.Infof("Reloading configuration from: %s", configPath)
+
+ content, err := os.ReadFile(configPath)
+ if err != nil {
+ return fmt.Errorf("failed to read config file: %w", err)
+ }
+
+ return reloadFromYAML(content)
+}
+
+// triggerConfigReloadFromBody reloads configuration from HTTP request body
+func triggerConfigReloadFromBody(r *http.Request) error {
+ reloadMutex.Lock()
+ defer reloadMutex.Unlock()
+
+ logger.Info("Reloading configuration from request body")
+
+ content, err := io.ReadAll(r.Body)
+ if err != nil {
+ return fmt.Errorf("failed to read request body: %w", err)
+ }
+ defer func() {
+ if err := r.Body.Close(); err != nil {
+ logger.Errorf("Failed to close request body: %v", err)
+ }
+ }()
+
+ return reloadFromYAML(content)
+}
+
+// reloadFromYAML performs the actual reload from YAML content
+func reloadFromYAML(content []byte) error {
+ newConfig := &model.Bootstrap{}
+ if err := yaml.Unmarshal(content, newConfig); err != nil {
+ return fmt.Errorf("failed to parse config: %w", err)
+ }
+
+ if err := config.Adapter(newConfig); err != nil {
+ return fmt.Errorf("failed to adapt config: %w", err)
+ }
+
+ oldConfig := config.GetBootstrap()
+ if oldConfig == nil {
+ return fmt.Errorf("current config is nil")
+ }
+
+ logger.Infof("Old config has %d listeners, new config has %d listeners",
+ len(oldConfig.StaticResources.Listeners),
len(newConfig.StaticResources.Listeners))
+
+ wg := &sync.WaitGroup{}
+ var reloadErrors []error
+ errorMutex := &sync.Mutex{}
+
+ for _, reloader := range coordinator.reloaders {
+ logger.Infof("Triggering reload for %T", reloader)
+ wg.Add(1)
+ go func(r HotReloader) {
+ defer wg.Done()
+ if err := r.HotReload(oldConfig, newConfig); err != nil
{
+ logger.Errorf("Hot reload failed for %T: %v",
r, err)
+ errorMutex.Lock()
+ reloadErrors = append(reloadErrors, err)
+ errorMutex.Unlock()
+ }
+ }(reloader)
+ }
+
+ wg.Wait()
+
+ if len(reloadErrors) > 0 {
+ return fmt.Errorf("reload completed with %d errors",
len(reloadErrors))
+ }
+
+ config.SetBootstrap(newConfig)
+ logger.Info("Configuration reloaded successfully and global config
updated")
+ return nil
+}
+
+// StartReloadServer starts the HTTP server for reload endpoint
+func StartReloadServer(port int) error {
+ mux := http.NewServeMux()
+
+ mux.Handle("/-/reload", &ReloadHandler{})
+ mux.Handle("/-/health", &HealthHandler{})
+
+ addr := fmt.Sprintf(":%d", port)
+ logger.Infof("Starting reload HTTP server on %s", addr)
+
+ server := &http.Server{
+ Addr: addr,
+ Handler: mux,
+ ReadTimeout: 10 * time.Second,
+ WriteTimeout: 10 * time.Second,
+ }
Review Comment:
`StartReloadServer` binds to `:<port>` and exposes `/-/reload` without any
authentication/authorization. This allows anyone with network access to the pod
to push arbitrary config and trigger reloads. Please restrict the bind address
(e.g., loopback by default), and/or require an auth token / mTLS (and make it
configurable) before enabling this endpoint by default.
##########
controllers/internal/controller/gateway_controller.go:
##########
@@ -729,12 +805,20 @@ func (r *GatewayReconciler) ensureDataPlane(ctx
context.Context, gateway *gatewa
needsUpdate := false
existingHash :=
existingDeployment.Spec.Template.Annotations["pixiu.apache.org/config-hash"]
if existingHash != configHash {
- r.Log.Info("config hash changed, triggering deployment
update",
+ r.Log.Info("config hash changed, triggering hot reload",
"gateway", gateway.GetName(),
"deployment", deploymentName,
"oldHash", existingHash,
"newHash", configHash)
- needsUpdate = true
+
+ if err := r.triggerHotReload(ctx, gateway,
deploymentName); err != nil {
+ r.Log.Error(err, "failed to trigger hot reload,
will update deployment", "gateway", gateway.GetName())
+ needsUpdate = true
+ } else {
+ // Update annotation only after successful hot
reload
+
existingDeployment.Spec.Template.Annotations["pixiu.apache.org/config-hash"] =
configHash
+ needsUpdate = true
+ }
Review Comment:
Even when `triggerHotReload` succeeds, the code still updates
`existingDeployment.Spec.Template.Annotations["pixiu.apache.org/config-hash"]`
and later patches `existingDeployment.Spec` to the new pod template. Any change
to the pod template annotations triggers a Deployment rollout/restart, which
largely negates the benefit of hot reload. If hot reload is meant to avoid pod
restarts, consider keeping the hash outside the pod template (e.g., Deployment
metadata) and skipping spec/template updates on successful reload.
##########
controllers/internal/converter/policy_applier.go:
##########
@@ -154,8 +154,23 @@ func ApplyClusterConfig(cluster *Cluster, clusterConfig
*v1alpha1.ClusterConfig)
cluster.Type = clusterConfig.Type
}
- // Endpoints are already resolved in gateway_controller.go
- // This function is called after endpoints are resolved
+ if len(clusterConfig.Endpoints) > 0 {
+ cluster.Endpoints = []*Endpoint{}
+ for i, ep := range clusterConfig.Endpoints {
+ endpoint := &Endpoint{
+ SocketAddress: SocketAddress{
+ Address: ep.Address,
+ Port: int(ep.Port),
+ },
+ }
+ if ep.ID != nil {
+ endpoint.ID = int(*ep.ID)
+ } else {
+ endpoint.ID = i + 1
+ }
+ cluster.Endpoints = append(cluster.Endpoints, endpoint)
+ }
+ }
Review Comment:
ApplyClusterConfig now overwrites `cluster.Endpoints` from
`clusterConfig.Endpoints`. In ensureGatewayConfigMap, endpoints are resolved
first via resolveClusterEndpoints() (to Pod IPs / ClusterIP), but this call
then replaces the resolved endpoints with the original policy endpoints (often
DNS), effectively undoing the resolution. Consider not mutating endpoints here
(only apply LB/type), or pass/apply the resolved endpoints instead.
##########
pkg/common/router/router.go:
##########
@@ -65,7 +75,23 @@ func CreateRouterCoordinator(routeConfig
*model.RouteConfiguration) *RouterCoord
return rc
}
+func (rm *RouterCoordinator) Close() {
+ if rm.dynamic {
+ routerMgr := server.GetRouterManager()
+ if routerMgr != nil {
+ routerMgr.RemoveRouterListener(rm)
+ }
+ }
+}
+
func (rm *RouterCoordinator) Route(hc *http.HttpContext) (*model.RouteAction,
error) {
+ if rm.needsRegistration && rm.dynamic {
+ routerMgr := server.GetRouterManager()
+ if routerMgr != nil {
+ routerMgr.AddRouterListener(rm)
+ rm.needsRegistration = false
+ }
Review Comment:
The delayed RouterListener registration in Route() mutates needsRegistration
without synchronization. Since Route() can be called concurrently, this
introduces a Go data race and can also register the same coordinator multiple
times. Use sync.Once / atomic CAS (or guard with rm.mu) around the registration
and flag update.
##########
pkg/common/http/manager.go:
##########
@@ -66,6 +66,10 @@ func CreateHttpConnectionManager(hcmc
*model.HttpConnectionManagerConfig) *HttpC
route.Match.Methods = []string{}
}
}
+
+ // Force enable dynamic routing for hot reload
+ hcmc.RouteConfig.Dynamic = true
+
Review Comment:
Forcing `hcmc.RouteConfig.Dynamic = true` changes routing behavior globally
(even when config sets dynamic=false) and can also cause RouterListener
registrations to accumulate across listener refreshes. This should be driven by
configuration/feature-flag (or limited to hot-reload mode) rather than
unconditionally overriding user config.
##########
pkg/cmd/gateway.go:
##########
@@ -116,6 +116,14 @@ func (d *DefaultDeployer) initialize() error {
hotreload.StartHotReload(d.configManger, d.bootstrap)
+ // Set config path for hot reload
+ hotreload.SetConfigPath(configPath)
+
+ // Start HTTP reload endpoint on port 18380
+ if err := hotreload.StartReloadServer(18380); err != nil {
+ logger.Warnf("[startGatewayCmd] failed to start reload server:
%s", err.Error())
+ }
+
Review Comment:
Starting the reload HTTP server unconditionally on a hard-coded port (18380)
makes this feature always-on and not configurable/disable-able, which is risky
operationally (port conflicts in non-container runs, unexpected exposed surface
area). Consider wiring the port/bind address and an enable flag through
CLI/env/config, and defaulting to disabled in production.
##########
pkg/config/config_load.go:
##########
@@ -58,6 +58,11 @@ func GetBootstrap() *model.Bootstrap {
return config
}
+// SetBootstrap set config global
+func SetBootstrap(cfg *model.Bootstrap) {
+ config = cfg
+}
Review Comment:
SetBootstrap updates the global `config` pointer without synchronization.
With hot-reload now calling SetBootstrap at runtime, concurrent reads via
GetBootstrap() can race. Consider storing the bootstrap in an `atomic.Pointer`
or guarding Get/Set with an RWMutex.
##########
controllers/internal/controller/gateway_controller.go:
##########
@@ -618,6 +682,13 @@ func (r *GatewayReconciler) ensureGatewayConfigMap(ctx
context.Context, gateway
return "", fmt.Errorf("failed to update
configmap: %w", err)
}
r.Log.Info("updated gateway configmap", "gateway",
gateway.GetName(), "configmap", configMapName)
+
+ deploymentName := fmt.Sprintf("%s-%s",
gateway.GetName(), string(gateway.GetUID())[:8])
+ if err := r.triggerHotReload(ctx, gateway,
deploymentName); err != nil {
+ r.Log.Error(err, "failed to trigger hot reload
after configmap update", "gateway", gateway.GetName())
+ } else {
+ r.Log.Info("hot reload triggered successfully
after configmap update", "gateway", gateway.GetName())
+ }
Review Comment:
Hot reload is triggered immediately after updating the ConfigMap here, and
then (on the same reconcile) ensureDataPlane() also triggers hot reload when it
sees the config-hash change. This can result in duplicate reload attempts per
change. Consider triggering reload in only one place (or short-circuiting the
later check when reload already succeeded).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
