Merge pull request #558 from appgate/503-error-handling

additional check for changed hostname resolution

cmd/appliance/upgrade/complete.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"io"
 	"regexp"
+	"slices"
 	"sort"
 	"strings"
 	"text/template"
@@ -17,6 +18,7 @@ import (
 	"github.com/appgate/sdpctl/pkg/api"
 	appliancepkg "github.com/appgate/sdpctl/pkg/appliance"
 	"github.com/appgate/sdpctl/pkg/appliance/change"
+	"github.com/appgate/sdpctl/pkg/cmdutil"
 	"github.com/appgate/sdpctl/pkg/configuration"
 	"github.com/appgate/sdpctl/pkg/docs"
 	"github.com/appgate/sdpctl/pkg/factory"
@@ -547,14 +549,34 @@ func upgradeCompleteRun(cmd *cobra.Command, args []string, opts *upgradeComplete
 				go t.Watch(appliancepkg.StatReady, []string{appliancepkg.UpgradeStatusFailed})
 			}
 
-			logEntry := log.WithField("appliance", controller.GetName())
+			logEntry := log.WithFields(log.Fields{
+				"appliance": controller.GetName(),
+				"url":       cfg.URL,
+			})
+			ips, err := network.ResolveHostnameIPs(cfg.URL)
+			if err != nil {
+				logEntry.WithError(err).Error("failed to lookup hostname ips")
+			}
 			logEntry.Info("Completing upgrade and switching partition")
 			if err := a.UpgradeComplete(ctx, controller.GetId(), true); err != nil {
 				return err
 			}
 			msg := "Upgrading primary Controller, installing and rebooting..."
 			logEntry.WithField("want", appliancepkg.StatReady).Info(msg)
 			if err := a.UpgradeStatusWorker.WaitForUpgradeStatus(context.WithValue(ctx, appliancepkg.PrimaryUpgrade, true), controller, []string{appliancepkg.UpgradeStatusIdle}, []string{appliancepkg.UpgradeStatusFailed}, t); err != nil {
+				if errors.Is(err, cmdutil.ErrControllerMaintenanceMode) {
+					if ips != nil {
+						postIPs, _ := network.ResolveHostnameIPs(cfg.URL)
+						cmpResult := slices.Equal(ips, postIPs)
+						if !cmpResult {
+							logEntry.WithError(fmt.Errorf("hostname resolves to different ip")).WithFields(log.Fields{
+								"original_resolution": ips,
+								"current_resolution":  postIPs,
+							}).Error("changed hostname resolution detected")
+						}
+					}
+					return fmt.Errorf("possible primary controller redirection detected: %w", err)
+				}
 				return err
 			}
 			if err := a.ApplianceStats.WaitForApplianceState(ctx, controller, appliancepkg.StatReady, t); err != nil {

pkg/appliance/upgrade_status.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"github.com/appgate/sdp-api-client-go/api/v20/openapi"
+	"github.com/appgate/sdpctl/pkg/cmdutil"
 	"github.com/appgate/sdpctl/pkg/tui"
 	"github.com/appgate/sdpctl/pkg/util"
 	"github.com/cenkalti/backoff/v4"
@@ -51,6 +52,7 @@ func (u *UpgradeStatus) upgradeStatus(ctx context.Context, appliance openapi.App
 	hasRebooted := false
 	offlineRegex := regexp.MustCompile(`No response Get`)
 	onlineRegex := regexp.MustCompile(`Bad Gateway`)
+	unavailableRegex := regexp.MustCompile(`Service Temporarily Unavailable`)
 	return func() error {
 		ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
 		defer cancel()
@@ -62,14 +64,23 @@ func (u *UpgradeStatus) upgradeStatus(ctx context.Context, appliance openapi.App
 				// Check if upgrading primary controller and apply logic for offline check
 				if primaryUpgrade, ok := ctx.Value(PrimaryUpgrade).(bool); ok && primaryUpgrade {
 					msg = "switching partition"
-					if offlineRegex.MatchString(err.Error()) {
+					errMsg := err.Error()
+					if offlineRegex.MatchString(errMsg) {
 						hasRebooted = true
-					} else if onlineRegex.MatchString(err.Error()) {
+					} else if onlineRegex.MatchString(errMsg) {
 						if hasRebooted {
 							msg = "initializing"
 						} else {
 							msg = "installing"
 						}
+					} else if unavailableRegex.MatchString(errMsg) {
+						// This error indicates that the controller we are polling is in maintanance mode
+						// This is fatal to the upgrade process since we are polling the wrong controller,
+						// probably because of some load balancer redirecting traffic
+						err = cmdutil.ErrControllerMaintenanceMode
+						tracker.Update(undesiredStatuses[0])
+						tracker.Fail(fmt.Sprintf("failed - %s", err.Error()))
+						return backoff.Permanent(err)
 					} else {
 						msg = err.Error()
 					}

pkg/cmdutil/errors.go

@@ -35,4 +35,6 @@ var (
 	ErrVersionCheckDisabled = errors.New("version check disabled")
 	// ErrUnsupportedOperation is used when the user tries to do an operation that is unsupported by the Appliance, likely due to version
 	ErrUnsupportedOperation = errors.New("Operation not supported on your appliance version")
+	// ErrControllerMaintenanceMode is used when trying to connect to appliance where maintenance mode is enabled
+	ErrControllerMaintenanceMode = errors.New("controller seem to be in maintenance mode")
 )

pkg/network/network.go

@@ -2,8 +2,11 @@ package network
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net"
+	"net/url"
+	"slices"
 	"sort"
 	"strings"
 
@@ -15,6 +18,10 @@ type ResolverError struct {
 	hostname string
 }
 
+var (
+	ErrNameResolution = errors.New("failed to resolve any hostname IPs")
+)
+
 func (r *ResolverError) Error() string {
 	ips := make([]string, 0, len(r.ip))
 	for _, addr := range r.ip {
@@ -61,3 +68,25 @@ func ValidateHostnameUniqueness(addr string) error {
 
 	return nil
 }
+
+func ResolveHostnameIPs(addr string) ([]string, error) {
+	url, err := url.ParseRequestURI(addr)
+	if err != nil {
+		return nil, err
+	}
+
+	ctx := context.Background()
+	ips, err := Resolver.LookupIP(ctx, "ip", url.Hostname())
+	if err != nil {
+		return nil, err
+	}
+	if len(ips) <= 0 {
+		return nil, ErrNameResolution
+	}
+	res := make([]string, 0, len(ips))
+	for _, i := range ips {
+		res = append(res, i.String())
+	}
+	slices.Sort(res)
+	return res, nil
+}

pkg/network/network_test.go

@@ -0,0 +1,100 @@
+package network
+
+import (
+	"reflect"
+	"slices"
+	"testing"
+
+	"github.com/appgate/sdpctl/pkg/dns"
+	"github.com/foxcpp/go-mockdns"
+)
+
+func TestResolveHostnameIPs(t *testing.T) {
+	tests := []struct {
+		name    string
+		addr    string
+		want    []string
+		wantErr error
+	}{
+		{
+			name: "test resolve to 1",
+			want: []string{"9.6.5.7"},
+		},
+		{
+			name:    "no resolution",
+			wantErr: ErrNameResolution,
+		},
+		{
+			name: "resolve multiple",
+			want: []string{"9.4.5.6", "8.5.6.2"},
+		},
+		{
+			name: "resolve with custom port in hostname",
+			addr: "https://appgate.test:8567",
+			want: []string{"9.8.7.6"},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			slices.Sort(tt.want)
+			_, teardown := dns.RunMockDNSServer(map[string]mockdns.Zone{
+				"appgate.test.": {
+					A: tt.want,
+				},
+			})
+			defer teardown()
+			addr := "https://appgate.test"
+			if len(tt.addr) > 0 {
+				addr = tt.addr
+			}
+			got, err := ResolveHostnameIPs(addr)
+			if (err != nil) != (tt.wantErr != nil) {
+				t.Errorf("ResolveHostnameIPs() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("ResolveHostnameIPs() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestValidateHostnameUniqueness(t *testing.T) {
+	type resolution struct {
+		ipv4 []string
+		ipv6 []string
+	}
+	tests := []struct {
+		name       string
+		resolution resolution
+		wantErr    bool
+	}{
+		{
+			name: "resolve no error",
+			resolution: resolution{
+				ipv4: []string{"7.6.8.5"},
+			},
+		},
+		{
+			name: "want error",
+			resolution: resolution{
+				ipv4: []string{"5.6.7.8", "4.5.6.7"},
+			},
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_, teardown := dns.RunMockDNSServer(map[string]mockdns.Zone{
+				"appgate.test.": {
+					A:    tt.resolution.ipv4,
+					AAAA: tt.resolution.ipv6,
+				},
+			})
+			defer teardown()
+			if err := ValidateHostnameUniqueness("appgate.test"); (err != nil) != tt.wantErr {
+				t.Errorf("ValidateHostnameUniqueness() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}