Client's UpdateHTTPServers and UpdateStreamServers methods now attemp…

…t to update all servers and return a combined error Previously these methods would return as soon as an error was encountered. Now they attempt to apply all the server updates and collect all errors encountered along the way. The reasoning behind this is that we want to apply as many of the desired changes as we possibly can and don't want any transitory errors that may affect a single server update to cause all the rest of the updates to be skipped.
nginxinc · Dec 4, 2024 · 37314eb · 37314eb
1 parent b24ebb1
commit 37314eb
Show file tree

Hide file tree

Showing 2 changed files with 165 additions and 20 deletions.
diff --git a/client/nginx.go b/client/nginx.go
@@ -808,6 +808,7 @@ func (client *NginxClient) DeleteHTTPServer(ctx context.Context, upstream string
 // Servers that are in the slice, but don't exist in NGINX will be added to NGINX.
 // Servers that aren't in the slice, but exist in NGINX, will be removed from NGINX.
 // Servers that are in the slice and exist in NGINX, but have different parameters, will be updated.
+// The client will attempt to update all servers, returning all the errors that occurred.
 func (client *NginxClient) UpdateHTTPServers(ctx context.Context, upstream string, servers []UpstreamServer) (added []UpstreamServer, deleted []UpstreamServer, updated []UpstreamServer, err error) {
 	serversInNginx, err := client.GetHTTPServers(ctx, upstream)
 	if err != nil {
@@ -824,27 +825,37 @@ func (client *NginxClient) UpdateHTTPServers(ctx context.Context, upstream strin
 	toAdd, toDelete, toUpdate := determineUpdates(formattedServers, serversInNginx)
 
 	for _, server := range toAdd {
-		err := client.AddHTTPServer(ctx, upstream, server)
-		if err != nil {
-			return nil, nil, nil, fmt.Errorf("failed to update servers of %v upstream: %w", upstream, err)
+		addErr := client.AddHTTPServer(ctx, upstream, server)
+		if addErr != nil {
+			err = errors.Join(err, addErr)
+			continue
 		}
+		added = append(added, server)
 	}
 
 	for _, server := range toDelete {
-		err := client.DeleteHTTPServer(ctx, upstream, server.Server)
-		if err != nil {
-			return nil, nil, nil, fmt.Errorf("failed to update servers of %v upstream: %w", upstream, err)
+		deleteErr := client.DeleteHTTPServer(ctx, upstream, server.Server)
+		if deleteErr != nil {
+			err = errors.Join(err, deleteErr)
+			continue
 		}
+		deleted = append(deleted, server)
 	}
 
 	for _, server := range toUpdate {
-		err := client.UpdateHTTPServer(ctx, upstream, server)
-		if err != nil {
-			return nil, nil, nil, fmt.Errorf("failed to update servers of %v upstream: %w", upstream, err)
+		updateErr := client.UpdateHTTPServer(ctx, upstream, server)
+		if updateErr != nil {
+			err = errors.Join(err, updateErr)
+			continue
 		}
+		updated = append(updated, server)
+	}
+
+	if err != nil {
+		err = fmt.Errorf("failed to update servers of %s upstream: %w", upstream, err)
 	}
 
-	return toAdd, toDelete, toUpdate, nil
+	return added, deleted, updated, err
 }
 
 // haveSameParameters checks if a given server has the same parameters as a server already present in NGINX. Order matters.
@@ -1108,6 +1119,7 @@ func (client *NginxClient) DeleteStreamServer(ctx context.Context, upstream stri
 // Servers that are in the slice, but don't exist in NGINX will be added to NGINX.
 // Servers that aren't in the slice, but exist in NGINX, will be removed from NGINX.
 // Servers that are in the slice and exist in NGINX, but have different parameters, will be updated.
+// The client will attempt to update all servers, returning all the errors that occurred.
 func (client *NginxClient) UpdateStreamServers(ctx context.Context, upstream string, servers []StreamUpstreamServer) (added []StreamUpstreamServer, deleted []StreamUpstreamServer, updated []StreamUpstreamServer, err error) {
 	serversInNginx, err := client.GetStreamServers(ctx, upstream)
 	if err != nil {
@@ -1123,27 +1135,37 @@ func (client *NginxClient) UpdateStreamServers(ctx context.Context, upstream str
 	toAdd, toDelete, toUpdate := determineStreamUpdates(formattedServers, serversInNginx)
 
 	for _, server := range toAdd {
-		err := client.AddStreamServer(ctx, upstream, server)
-		if err != nil {
-			return nil, nil, nil, fmt.Errorf("failed to update stream servers of %v upstream: %w", upstream, err)
+		addErr := client.AddStreamServer(ctx, upstream, server)
+		if addErr != nil {
+			err = errors.Join(err, addErr)
+			continue
 		}
+		added = append(added, server)
 	}
 
 	for _, server := range toDelete {
-		err := client.DeleteStreamServer(ctx, upstream, server.Server)
-		if err != nil {
-			return nil, nil, nil, fmt.Errorf("failed to update stream servers of %v upstream: %w", upstream, err)
+		deleteErr := client.DeleteStreamServer(ctx, upstream, server.Server)
+		if deleteErr != nil {
+			err = errors.Join(err, deleteErr)
+			continue
 		}
+		deleted = append(deleted, server)
 	}
 
 	for _, server := range toUpdate {
-		err := client.UpdateStreamServer(ctx, upstream, server)
-		if err != nil {
-			return nil, nil, nil, fmt.Errorf("failed to update stream servers of %v upstream: %w", upstream, err)
+		updateErr := client.UpdateStreamServer(ctx, upstream, server)
+		if updateErr != nil {
+			err = errors.Join(err, updateErr)
+			continue
 		}
+		updated = append(updated, server)
+	}
+
+	if err != nil {
+		err = fmt.Errorf("failed to update stream servers of %s upstream: %w", upstream, err)
 	}
 
-	return toAdd, toDelete, toUpdate, nil
+	return added, deleted, updated, err
 }
 
 func (client *NginxClient) getIDOfStreamServer(ctx context.Context, upstream string, name string) (int, error) {

diff --git a/tests/client_test.go b/tests/client_test.go
@@ -304,6 +304,67 @@ func TestStreamUpstreamServer(t *testing.T) {
 	}
 }
 
+func TestUpdateStreamUpstreamServersWithError(t *testing.T) {
+	t.Parallel()
+	c, err := client.NewNginxClient(helpers.GetAPIEndpoint())
+	if err != nil {
+		t.Fatalf("Error connecting to nginx: %v", err)
+	}
+
+	maxFails := 64
+	weight := 10
+	maxConns := 321
+	backup := true
+	down := true
+
+	serversToUpdate := []client.StreamUpstreamServer{
+		{
+			ID:     -1,
+			Server: "127.0.0.1:3000",
+		},
+		{
+			Server:      "127.0.0.1:5000",
+			MaxConns:    &maxConns,
+			MaxFails:    &maxFails,
+			FailTimeout: "21s",
+			SlowStart:   "12s",
+			Weight:      &weight,
+			Backup:      &backup,
+			Down:        &down,
+		},
+	}
+	ctx := context.Background()
+
+	added, _, _, err := c.UpdateStreamServers(ctx, streamUpstream, serversToUpdate)
+	if err == nil {
+		t.Fatalf("Expected the client to return an error for adding a server with ID -1")
+	}
+
+	if len(added) != 1 {
+		t.Fatalf("Expected one server to be added, instead %d were added", len(added))
+	}
+
+	servers, err := c.GetStreamServers(ctx, streamUpstream)
+	if err != nil {
+		t.Fatalf("Error getting stream servers: %v", err)
+	}
+	if len(servers) != 1 {
+		t.Errorf("Too many servers")
+	}
+	// don't compare IDs
+	servers[0].ID = 0
+
+	if !reflect.DeepEqual(serversToUpdate[1], servers[0]) {
+		t.Errorf("Expected: %v Got: %v", serversToUpdate[1], servers[0])
+	}
+
+	// remove stream upstream servers
+	_, _, _, err = c.UpdateStreamServers(ctx, streamUpstream, []client.StreamUpstreamServer{})
+	if err != nil {
+		t.Errorf("Couldn't remove servers: %v", err)
+	}
+}
+
 //nolint:paralleltest
 func TestClient(t *testing.T) {
 	c, err := client.NewNginxClient(helpers.GetAPIEndpoint())
@@ -577,6 +638,68 @@ func TestUpstreamServer(t *testing.T) {
 	}
 }
 
+//nolint:paralleltest
+func TestUpdateUpstreamServersWithError(t *testing.T) {
+	c, err := client.NewNginxClient(helpers.GetAPIEndpoint())
+	if err != nil {
+		t.Fatalf("Error connecting to nginx: %v", err)
+	}
+
+	maxFails := 64
+	weight := 10
+	maxConns := 321
+	backup := true
+	down := true
+
+	serversToUpdate := []client.UpstreamServer{
+		{
+			ID:     -1,
+			Server: "127.0.0.1:3000",
+		},
+		{
+			Server:      "127.0.0.1:2000",
+			MaxConns:    &maxConns,
+			MaxFails:    &maxFails,
+			FailTimeout: "21s",
+			SlowStart:   "12s",
+			Weight:      &weight,
+			Route:       "test",
+			Backup:      &backup,
+			Down:        &down,
+		},
+	}
+
+	ctx := context.Background()
+	added, _, _, err := c.UpdateHTTPServers(ctx, upstream, serversToUpdate)
+	if err == nil {
+		t.Fatal("Expected the client to return an error for adding a server with ID -1")
+	}
+
+	if len(added) != 1 {
+		t.Fatalf("Expected one server to be added, instead %d were added", len(added))
+	}
+
+	servers, err := c.GetHTTPServers(ctx, upstream)
+	if err != nil {
+		t.Fatalf("Error getting HTTPServers: %v", err)
+	}
+	if len(servers) != 1 {
+		t.Errorf("Too many servers")
+	}
+	// don't compare IDs
+	servers[0].ID = 0
+
+	if !reflect.DeepEqual(serversToUpdate[1], servers[0]) {
+		t.Errorf("Expected: %v Got: %v", serversToUpdate[1], servers[0])
+	}
+
+	// remove upstream servers
+	_, _, _, err = c.UpdateHTTPServers(ctx, upstream, []client.UpstreamServer{})
+	if err != nil {
+		t.Errorf("Couldn't remove servers: %v", err)
+	}
+}
+
 //nolint:paralleltest
 func TestStats(t *testing.T) {
 	c, err := client.NewNginxClient(helpers.GetAPIEndpoint())