Skip to content

Commit e117cf0

Browse files
arussellf5arussellf5
committed
Client's UpdateHTTPServers and UpdateStreamServers methods now attempt 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.
1 parent b24ebb1 commit e117cf0

File tree

2 files changed

+222
-20
lines changed

2 files changed

+222
-20
lines changed

‎client/nginx.go‎

Lines changed: 42 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -808,6 +808,7 @@ func (client *NginxClient) DeleteHTTPServer(ctx context.Context, upstream string
808808
// Servers that are in the slice, but don't exist in NGINX will be added to NGINX.
809809
// Servers that aren't in the slice, but exist in NGINX, will be removed from NGINX.
810810
// Servers that are in the slice and exist in NGINX, but have different parameters, will be updated.
811+
// The client will attempt to update all servers, returning all the errors that occurred.
811812
func (client *NginxClient) UpdateHTTPServers(ctx context.Context, upstream string, servers []UpstreamServer) (added []UpstreamServer, deleted []UpstreamServer, updated []UpstreamServer, err error) {
812813
serversInNginx, err := client.GetHTTPServers(ctx, upstream)
813814
if err != nil {
@@ -824,27 +825,37 @@ func (client *NginxClient) UpdateHTTPServers(ctx context.Context, upstream strin
824825
toAdd, toDelete, toUpdate := determineUpdates(formattedServers, serversInNginx)
825826

826827
for _, server := range toAdd {
827-
err := client.AddHTTPServer(ctx, upstream, server)
828-
if err != nil {
829-
return nil, nil, nil, fmt.Errorf("failed to update servers of %v upstream: %w", upstream, err)
828+
addErr := client.AddHTTPServer(ctx, upstream, server)
829+
if addErr != nil {
830+
err = errors.Join(err, addErr)
831+
continue
830832
}
833+
added = append(added, server)
831834
}
832835

833836
for _, server := range toDelete {
834-
err := client.DeleteHTTPServer(ctx, upstream, server.Server)
835-
if err != nil {
836-
return nil, nil, nil, fmt.Errorf("failed to update servers of %v upstream: %w", upstream, err)
837+
deleteErr := client.DeleteHTTPServer(ctx, upstream, server.Server)
838+
if deleteErr != nil {
839+
err = errors.Join(err, deleteErr)
840+
continue
837841
}
842+
deleted = append(deleted, server)
838843
}
839844

840845
for _, server := range toUpdate {
841-
err := client.UpdateHTTPServer(ctx, upstream, server)
842-
if err != nil {
843-
return nil, nil, nil, fmt.Errorf("failed to update servers of %v upstream: %w", upstream, err)
846+
updateErr := client.UpdateHTTPServer(ctx, upstream, server)
847+
if updateErr != nil {
848+
err = errors.Join(err, updateErr)
849+
continue
844850
}
851+
updated = append(updated, server)
852+
}
853+
854+
if err != nil {
855+
err = fmt.Errorf("failed to update servers of %s upstream: %w", upstream, err)
845856
}
846857

847-
return toAdd, toDelete, toUpdate, nil
858+
return added, deleted, updated, err
848859
}
849860

850861
// 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
11081119
// Servers that are in the slice, but don't exist in NGINX will be added to NGINX.
11091120
// Servers that aren't in the slice, but exist in NGINX, will be removed from NGINX.
11101121
// Servers that are in the slice and exist in NGINX, but have different parameters, will be updated.
1122+
// The client will attempt to update all servers, returning all the errors that occurred.
11111123
func (client *NginxClient) UpdateStreamServers(ctx context.Context, upstream string, servers []StreamUpstreamServer) (added []StreamUpstreamServer, deleted []StreamUpstreamServer, updated []StreamUpstreamServer, err error) {
11121124
serversInNginx, err := client.GetStreamServers(ctx, upstream)
11131125
if err != nil {
@@ -1123,27 +1135,37 @@ func (client *NginxClient) UpdateStreamServers(ctx context.Context, upstream str
11231135
toAdd, toDelete, toUpdate := determineStreamUpdates(formattedServers, serversInNginx)
11241136

11251137
for _, server := range toAdd {
1126-
err := client.AddStreamServer(ctx, upstream, server)
1127-
if err != nil {
1128-
return nil, nil, nil, fmt.Errorf("failed to update stream servers of %v upstream: %w", upstream, err)
1138+
addErr := client.AddStreamServer(ctx, upstream, server)
1139+
if addErr != nil {
1140+
err = errors.Join(err, addErr)
1141+
continue
11291142
}
1143+
added = append(added, server)
11301144
}
11311145

11321146
for _, server := range toDelete {
1133-
err := client.DeleteStreamServer(ctx, upstream, server.Server)
1134-
if err != nil {
1135-
return nil, nil, nil, fmt.Errorf("failed to update stream servers of %v upstream: %w", upstream, err)
1147+
deleteErr := client.DeleteStreamServer(ctx, upstream, server.Server)
1148+
if deleteErr != nil {
1149+
err = errors.Join(err, deleteErr)
1150+
continue
11361151
}
1152+
deleted = append(deleted, server)
11371153
}
11381154

11391155
for _, server := range toUpdate {
1140-
err := client.UpdateStreamServer(ctx, upstream, server)
1141-
if err != nil {
1142-
return nil, nil, nil, fmt.Errorf("failed to update stream servers of %v upstream: %w", upstream, err)
1156+
updateErr := client.UpdateStreamServer(ctx, upstream, server)
1157+
if updateErr != nil {
1158+
err = errors.Join(err, updateErr)
1159+
continue
11431160
}
1161+
updated = append(updated, server)
1162+
}
1163+
1164+
if err != nil {
1165+
err = fmt.Errorf("failed to update stream servers of %s upstream: %w", upstream, err)
11441166
}
11451167

1146-
return toAdd, toDelete, toUpdate, nil
1168+
return added, deleted, updated, err
11471169
}
11481170

11491171
func (client *NginxClient) getIDOfStreamServer(ctx context.Context, upstream string, name string) (int, error) {

‎client/nginx_test.go‎

Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package client
22

33
import (
44
"context"
5+
"encoding/json"
56
"net/http"
67
"net/http/httptest"
78
"reflect"
@@ -980,3 +981,182 @@ func TestExtractPlusVersionNegativeCase(t *testing.T) {
980981
})
981982
}
982983
}
984+
985+
func TestClientHTTPUpdateServers(t *testing.T) {
986+
t.Parallel()
987+
988+
responses := []response{
989+
// response for first serversInNginx GET servers
990+
{
991+
statusCode: http.StatusOK,
992+
servers: []UpstreamServer{},
993+
},
994+
// response for AddHTTPServer GET servers for http server
995+
{
996+
statusCode: http.StatusOK,
997+
servers: []UpstreamServer{},
998+
},
999+
// response for AddHTTPServer POST server for http server
1000+
{
1001+
statusCode: http.StatusInternalServerError,
1002+
servers: []UpstreamServer{},
1003+
},
1004+
// response for AddHTTPServer GET servers for https server
1005+
{
1006+
statusCode: http.StatusOK,
1007+
servers: []UpstreamServer{},
1008+
},
1009+
// response for AddHTTPServer POST server for https server
1010+
{
1011+
statusCode: http.StatusCreated,
1012+
servers: []UpstreamServer{},
1013+
},
1014+
}
1015+
1016+
handler := &fakeHandler{
1017+
func(w http.ResponseWriter, _ *http.Request) {
1018+
if len(responses) == 0 {
1019+
t.Fatal("ran out of responses")
1020+
}
1021+
1022+
re := responses[0]
1023+
responses = responses[1:]
1024+
1025+
w.WriteHeader(re.statusCode)
1026+
1027+
resp, err := json.Marshal(re.servers)
1028+
if err != nil {
1029+
t.Fatal(err)
1030+
}
1031+
_, err = w.Write(resp)
1032+
if err != nil {
1033+
t.Fatal(err)
1034+
}
1035+
},
1036+
}
1037+
1038+
server := httptest.NewServer(handler)
1039+
defer server.Close()
1040+
1041+
client, err := NewNginxClient(server.URL, WithHTTPClient(&http.Client{}))
1042+
if err != nil {
1043+
t.Fatal(err)
1044+
}
1045+
1046+
httpServer := UpstreamServer{Server: "127.0.0.1:80"}
1047+
httpsServer := UpstreamServer{Server: "127.0.0.1:443"}
1048+
1049+
// we expect that we will get an error for the 500 error encountered when putting the http server
1050+
// but we also expect that we have the https server added
1051+
added, _, _, err := client.UpdateHTTPServers(context.TODO(), "fakeUpstream", []UpstreamServer{
1052+
httpServer,
1053+
httpsServer,
1054+
})
1055+
if err == nil {
1056+
t.Fatal("expected to receive an error for 500 response when adding first server")
1057+
}
1058+
1059+
if len(added) != 1 {
1060+
t.Fatalf("expected to get one added server, instead got %d", len(added))
1061+
}
1062+
1063+
if !reflect.DeepEqual(httpsServer, added[0]) {
1064+
t.Errorf("expected: %v got: %v", httpsServer, added[0])
1065+
}
1066+
}
1067+
1068+
func TestClientStreamUpdateServers(t *testing.T) {
1069+
t.Parallel()
1070+
1071+
responses := []response{
1072+
// response for first serversInNginx GET servers
1073+
{
1074+
statusCode: http.StatusOK,
1075+
servers: []UpstreamServer{},
1076+
},
1077+
// response for AddStreamServer GET servers for streamServer1
1078+
{
1079+
statusCode: http.StatusOK,
1080+
servers: []UpstreamServer{},
1081+
},
1082+
// response for AddStreamServer POST server for streamServer1
1083+
{
1084+
statusCode: http.StatusInternalServerError,
1085+
servers: []UpstreamServer{},
1086+
},
1087+
// response for AddStreamServer GET servers for streamServer2
1088+
{
1089+
statusCode: http.StatusOK,
1090+
servers: []UpstreamServer{},
1091+
},
1092+
// response for AddStreamServer POST server for streamServer2
1093+
{
1094+
statusCode: http.StatusCreated,
1095+
servers: []UpstreamServer{},
1096+
},
1097+
}
1098+
1099+
handler := &fakeHandler{
1100+
func(w http.ResponseWriter, _ *http.Request) {
1101+
if len(responses) == 0 {
1102+
t.Fatal("ran out of responses")
1103+
}
1104+
1105+
re := responses[0]
1106+
responses = responses[1:]
1107+
1108+
w.WriteHeader(re.statusCode)
1109+
1110+
resp, err := json.Marshal(re.servers)
1111+
if err != nil {
1112+
t.Fatal(err)
1113+
}
1114+
_, err = w.Write(resp)
1115+
if err != nil {
1116+
t.Fatal(err)
1117+
}
1118+
},
1119+
}
1120+
1121+
server := httptest.NewServer(handler)
1122+
defer server.Close()
1123+
1124+
client, err := NewNginxClient(server.URL, WithHTTPClient(&http.Client{}))
1125+
if err != nil {
1126+
t.Fatal(err)
1127+
}
1128+
1129+
streamServer1 := StreamUpstreamServer{Server: "127.0.0.1:2000"}
1130+
streamServer2 := StreamUpstreamServer{Server: "127.0.0.1:3000"}
1131+
1132+
// we expect that we will get an error for the 500 error encountered when putting server1
1133+
// but we also expect that we get the second server added
1134+
added, _, _, err := client.UpdateStreamServers(context.TODO(), "fakeUpstream", []StreamUpstreamServer{
1135+
streamServer1,
1136+
streamServer2,
1137+
})
1138+
if err == nil {
1139+
t.Fatal("expected to receive an error for 500 response when adding first server")
1140+
}
1141+
1142+
if len(added) != 1 {
1143+
t.Fatalf("expected to get one added server, instead got %d", len(added))
1144+
}
1145+
1146+
if !reflect.DeepEqual(streamServer2, added[0]) {
1147+
t.Errorf("expected: %v got: %v", streamServer2, added[0])
1148+
}
1149+
}
1150+
1151+
type response struct {
1152+
servers []UpstreamServer
1153+
statusCode int
1154+
}
1155+
1156+
type fakeHandler struct {
1157+
handler func(w http.ResponseWriter, r *http.Request)
1158+
}
1159+
1160+
func (h *fakeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
1161+
h.handler(w, r)
1162+
}

0 commit comments

Comments
 (0)