mirror of
https://github.com/0xJacky/nginx-ui.git
synced 2026-09-03 07:24:52 +08:00
fix(cluster): survive unsupported config names in a directory sync
An end-to-end run against a two node cluster surfaced two problems in the
synchronization added by 9f70d47.
A real Nginx configuration directory holds files the config validator rejects,
such as nginx.conf.bak.1738662518. The collector pushed them anyway and the
receiver aborted the whole batch on the first one, so a single stale backup
stopped every other file from being deployed. Names the receiver would reject
are now skipped while collecting, and the receiver reports a per-file failure
list instead of discarding the batch, keeping the reply an error only when
nothing could be applied at all. The caller turns a partially applied batch
into a failed result so a summary never claims a clean run.
sync_interval_minutes also advertised a minimum of one minute that the
omitempty rule never enforced. Zero is what the model already treats as "use
the default", so the rule now says so and rejects negatives instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -84,7 +84,7 @@ func AddNamespace(c *gin.Context) {
|
||||
"upstream_test_type": "omitempty,oneof=" + model.UpstreamTestLocal + " " + model.UpstreamTestRemote + " " + model.UpstreamTestMirror,
|
||||
"deploy_mode": "omitempty,oneof=" + model.DeployModeLocal + " " + model.DeployModeRemote,
|
||||
"sync_strategy": "omitempty,oneof=" + model.SyncStrategyManual + " " + model.SyncStrategyAuto,
|
||||
"sync_interval_minutes": "omitempty,min=1",
|
||||
"sync_interval_minutes": "omitempty,min=0",
|
||||
}).
|
||||
Create()
|
||||
}
|
||||
@@ -98,7 +98,7 @@ func ModifyNamespace(c *gin.Context) {
|
||||
"upstream_test_type": "omitempty,oneof=" + model.UpstreamTestLocal + " " + model.UpstreamTestRemote + " " + model.UpstreamTestMirror,
|
||||
"deploy_mode": "omitempty,oneof=" + model.DeployModeLocal + " " + model.DeployModeRemote,
|
||||
"sync_strategy": "omitempty,oneof=" + model.SyncStrategyManual + " " + model.SyncStrategyAuto,
|
||||
"sync_interval_minutes": "omitempty,min=1",
|
||||
"sync_interval_minutes": "omitempty,min=0",
|
||||
}).
|
||||
Modify()
|
||||
}
|
||||
|
||||
@@ -26,13 +26,20 @@ func SyncConfigBatch(c *gin.Context) {
|
||||
return
|
||||
}
|
||||
|
||||
// One rejected file must not discard the rest of the batch: a real
|
||||
// configuration directory always holds something the validator dislikes, and
|
||||
// dropping the whole sync over it would make the feature useless.
|
||||
written := 0
|
||||
skipped := 0
|
||||
failures := make([]gin.H, 0)
|
||||
|
||||
for _, file := range json.Files {
|
||||
relativePath := filepath.ToSlash(filepath.Join(file.BaseDir, file.Name))
|
||||
|
||||
path, err := config.ResolveConfPath(helper.UnescapeURL(file.BaseDir), helper.UnescapeURL(file.Name))
|
||||
if err != nil {
|
||||
cosy.ErrHandler(c, err)
|
||||
return
|
||||
failures = append(failures, gin.H{"path": relativePath, "error": err.Error()})
|
||||
continue
|
||||
}
|
||||
|
||||
if !json.Overwrite && helper.FileExists(path) {
|
||||
@@ -41,18 +48,18 @@ func SyncConfigBatch(c *gin.Context) {
|
||||
}
|
||||
|
||||
if err = config.ValidateConfigFile(path, file.Content); err != nil {
|
||||
cosy.ErrHandler(c, err)
|
||||
return
|
||||
failures = append(failures, gin.H{"path": relativePath, "error": err.Error()})
|
||||
continue
|
||||
}
|
||||
|
||||
if err = os.MkdirAll(filepath.Dir(path), 0755); err != nil {
|
||||
cosy.ErrHandler(c, err)
|
||||
return
|
||||
failures = append(failures, gin.H{"path": relativePath, "error": err.Error()})
|
||||
continue
|
||||
}
|
||||
|
||||
if err = os.WriteFile(path, []byte(file.Content), 0644); err != nil {
|
||||
cosy.ErrHandler(c, err)
|
||||
return
|
||||
failures = append(failures, gin.H{"path": relativePath, "error": err.Error()})
|
||||
continue
|
||||
}
|
||||
|
||||
written++
|
||||
@@ -65,10 +72,17 @@ func SyncConfigBatch(c *gin.Context) {
|
||||
}
|
||||
}
|
||||
|
||||
c.JSON(http.StatusOK, gin.H{
|
||||
"message": "ok",
|
||||
"written": written,
|
||||
"skipped": skipped,
|
||||
// Only a batch where nothing could be applied is an error.
|
||||
status := http.StatusOK
|
||||
if written == 0 && skipped == 0 && len(failures) > 0 {
|
||||
status = http.StatusInternalServerError
|
||||
}
|
||||
|
||||
c.JSON(status, gin.H{
|
||||
"message": "ok",
|
||||
"written": written,
|
||||
"skipped": skipped,
|
||||
"failures": failures,
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -68,13 +68,20 @@ func (n nodeRef) post(ctx context.Context, path string, body any) error {
|
||||
// postWithStatus behaves like post but also reports the status code so callers
|
||||
// can detect endpoints that an older node does not implement yet.
|
||||
func (n nodeRef) postWithStatus(ctx context.Context, path string, body any) (int, error) {
|
||||
_, status, err := n.postForBody(ctx, path, body)
|
||||
return status, err
|
||||
}
|
||||
|
||||
// postForBody returns the response body so callers can inspect a partial
|
||||
// success reported inside a 2xx answer.
|
||||
func (n nodeRef) postForBody(ctx context.Context, path string, body any) ([]byte, int, error) {
|
||||
resp, err := n.client.R().SetContext(ctx).SetBody(body).Post(path)
|
||||
if err != nil {
|
||||
return 0, err
|
||||
return nil, 0, err
|
||||
}
|
||||
if resp.StatusCode() < http.StatusOK || resp.StatusCode() >= http.StatusMultipleChoices {
|
||||
return resp.StatusCode(), fmt.Errorf("%s responded %d: %s", path, resp.StatusCode(), resp.String())
|
||||
return resp.Body(), resp.StatusCode(), fmt.Errorf("%s responded %d: %s", path, resp.StatusCode(), resp.String())
|
||||
}
|
||||
|
||||
return resp.StatusCode(), nil
|
||||
return resp.Body(), resp.StatusCode(), nil
|
||||
}
|
||||
|
||||
@@ -7,6 +7,7 @@ import (
|
||||
"strings"
|
||||
"unicode/utf8"
|
||||
|
||||
"github.com/0xJacky/Nginx-UI/internal/config"
|
||||
"github.com/0xJacky/Nginx-UI/internal/helper"
|
||||
"github.com/0xJacky/Nginx-UI/internal/nginx"
|
||||
"github.com/uozi-tech/cosy/logger"
|
||||
@@ -89,6 +90,15 @@ func CollectConfigFiles(root string) ([]ConfigFile, error) {
|
||||
return nil
|
||||
}
|
||||
|
||||
// A real configuration directory accumulates files the receiver will
|
||||
// always reject, such as nginx.conf.bak.1738662518. Leaving them out of
|
||||
// the batch keeps a whole-directory sync from being dragged down by one
|
||||
// name that is not a valid configuration file anywhere.
|
||||
if err := config.ValidateConfigFilename(path); err != nil {
|
||||
logger.Debugf("cluster sync skips unsupported config name %s: %v", path, err)
|
||||
return nil
|
||||
}
|
||||
|
||||
relativeDir, err := filepath.Rel(confPath, filepath.Dir(path))
|
||||
if err != nil {
|
||||
return nil
|
||||
|
||||
@@ -163,3 +163,39 @@ func TestConfigFileRelativePathHandlesRootFiles(t *testing.T) {
|
||||
t.Fatalf("got %q, want %q", got, "conf.d/a.conf")
|
||||
}
|
||||
}
|
||||
|
||||
func TestCollectConfigFilesSkipsNamesTheReceiverRejects(t *testing.T) {
|
||||
// A real configuration directory accumulates backups like these. Including
|
||||
// them used to make the receiver reject the whole batch.
|
||||
confDir := withConfDir(t, map[string]string{
|
||||
"nginx.conf": "events {}\n",
|
||||
"conf.d/a.conf": "# a\n",
|
||||
"nginx.conf.bak.1738662518": "events {}\n",
|
||||
"nginx_bak.conf": "events {}\n",
|
||||
"conf.d/notes.txt": "not a config\n",
|
||||
})
|
||||
|
||||
files, err := CollectConfigFiles(confDir)
|
||||
if err != nil {
|
||||
t.Fatalf("collect: %v", err)
|
||||
}
|
||||
|
||||
got := collectedPaths(files)
|
||||
for _, path := range got {
|
||||
if path == "nginx.conf.bak.1738662518" || path == "conf.d/notes.txt" {
|
||||
t.Fatalf("unsupported config name must be skipped, got %v", got)
|
||||
}
|
||||
}
|
||||
|
||||
// The .conf files are still collected.
|
||||
want := map[string]bool{"conf.d/a.conf": true, "nginx_bak.conf": true}
|
||||
for _, path := range got {
|
||||
if !want[path] {
|
||||
t.Fatalf("unexpected file %s in %v", path, got)
|
||||
}
|
||||
delete(want, path)
|
||||
}
|
||||
if len(want) != 0 {
|
||||
t.Fatalf("missing files %v, got %v", want, got)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2,6 +2,7 @@ package clustersync
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/http"
|
||||
@@ -42,9 +43,9 @@ func configBatchItem(name string, files []ConfigFile, overwrite bool) item {
|
||||
name: name,
|
||||
push: func(ctx context.Context, node nodeRef) error {
|
||||
payload := configBatchPayload{Files: files, Overwrite: overwrite}
|
||||
status, err := node.postWithStatus(ctx, "/api/config_sync_batch", payload)
|
||||
body, status, err := node.postForBody(ctx, "/api/config_sync_batch", payload)
|
||||
if err == nil {
|
||||
return nil
|
||||
return batchFailure(body)
|
||||
}
|
||||
if status != http.StatusNotFound {
|
||||
return err
|
||||
@@ -56,6 +57,34 @@ func configBatchItem(name string, files []ConfigFile, overwrite bool) item {
|
||||
}
|
||||
}
|
||||
|
||||
// batchResponse is the answer of the batch receiver. Files it could not apply
|
||||
// are reported individually instead of failing the whole request.
|
||||
type batchResponse struct {
|
||||
Written int `json:"written"`
|
||||
Skipped int `json:"skipped"`
|
||||
Failures []struct {
|
||||
Path string `json:"path"`
|
||||
Error string `json:"error"`
|
||||
} `json:"failures"`
|
||||
}
|
||||
|
||||
// batchFailure turns a partially applied batch into an error so the summary
|
||||
// does not claim a clean run.
|
||||
func batchFailure(body []byte) error {
|
||||
var response batchResponse
|
||||
if err := json.Unmarshal(body, &response); err != nil || len(response.Failures) == 0 {
|
||||
return nil
|
||||
}
|
||||
|
||||
failures := make([]error, 0, len(response.Failures))
|
||||
for _, failure := range response.Failures {
|
||||
failures = append(failures, fmt.Errorf("%s: %s", failure.Path, failure.Error))
|
||||
}
|
||||
|
||||
return fmt.Errorf("applied %d of %d files: %w",
|
||||
response.Written, response.Written+len(response.Failures), errors.Join(failures...))
|
||||
}
|
||||
|
||||
// pushConfigFilesIndividually keeps mixed-version clusters working by using the
|
||||
// long-standing single file endpoint.
|
||||
func pushConfigFilesIndividually(ctx context.Context, node nodeRef, files []ConfigFile, overwrite bool) error {
|
||||
|
||||
@@ -63,8 +63,8 @@ func (n *Namespace) IsAutoSync() bool {
|
||||
return n != nil && n.SyncStrategy == SyncStrategyAuto
|
||||
}
|
||||
|
||||
// EffectiveSyncInterval returns the auto sync interval, falling back to the
|
||||
// default when the stored value is unusable.
|
||||
// EffectiveSyncInterval returns the auto sync interval. Zero means "use the
|
||||
// default", which is what the API accepts when the field is left unset.
|
||||
func (n *Namespace) EffectiveSyncInterval() int {
|
||||
if n == nil || n.SyncIntervalMinutes <= 0 {
|
||||
return DefaultSyncIntervalMinutes
|
||||
|
||||
Reference in New Issue
Block a user