fix(registry): Answer 400 when the endpoint URL cannot be reached (#923)

Signed-off-by: Prasanth Baskar <prasanth@8gears.com>

Author: bupd

src/controller/registry/controller.go

@@ -16,7 +16,9 @@ package registry
 
 import (
 	"context"
+	stderrors "errors"
 	"math/rand"
+	neturl "net/url"
 	"strings"
 	"time"
 
@@ -92,7 +94,9 @@ func (c *controller) validate(ctx context.Context, registry *model.Registry) err
 	if len(registry.Name) > 64 {
 		return errors.New(nil).WithCode(errors.BadRequestCode).WithMessage("the max length of name is 64")
 	}
-	url, err := lib.ValidateURL(registry.URL)
+	// a registry endpoint is only ever spoken to over HTTP; the default scheme
+	// set of ValidateURL also admits ftp/s3/sftp, which net/http cannot dial
+	url, err := lib.ValidateURL(registry.URL, "http", "https")
 	if err != nil {
 		return err
 	}
@@ -170,17 +174,44 @@ func (c *controller) Delete(ctx context.Context, id int64) error {
 }
 
 func (c *controller) IsHealthy(ctx context.Context, registry *model.Registry) (bool, error) {
+	// the Harbor adapter probes /api/version while the adapter is built, so an
+	// unreachable endpoint surfaces from CreateAdapter rather than HealthCheck
 	adapter, err := c.regMgr.CreateAdapter(ctx, registry)
 	if err != nil {
-		return false, err
+		return false, unreachableEndpointError(registry.URL, err)
 	}
 	status, err := adapter.HealthCheck()
 	if err != nil {
-		return false, err
+		return false, unreachableEndpointError(registry.URL, err)
 	}
 	return status == model.Healthy, nil
 }
 
+// unreachableEndpointError maps a transport failure onto a bad request.
+//
+// net/http returns *url.Error for every failure that stops an HTTP exchange
+// from completing, and each one describes the endpoint the caller supplied
+// rather than a fault inside Harbor:
+//
+//   - the host does not resolve, or the connection is refused or times out
+//   - the scheme is one net/http cannot dial
+//   - the TLS handshake fails, including an untrusted or mismatched
+//     certificate, which the caller fixes with insecure or ca_certificate
+//
+// All of them are a 400 deliberately: the caller has to change what it sent.
+// The message keeps the cause verbatim so the specific reason is visible even
+// though the prefix is shared. Every other error is passed through untouched,
+// so a genuine internal failure keeps its own code, as does a registry that
+// answers over HTTP with a status Harbor did not expect.
+func unreachableEndpointError(url string, err error) error {
+	var urlErr *neturl.Error
+	if !stderrors.As(err, &urlErr) {
+		return err
+	}
+	return errors.New(nil).WithCode(errors.BadRequestCode).
+		WithMessagef("failed to reach the registry endpoint %s: %v", url, err)
+}
+
 func (c *controller) GetInfo(ctx context.Context, id int64) (*model.RegistryInfo, error) {
 	var (
 		registry *model.Registry

src/controller/registry/controller_test.go

@@ -16,13 +16,18 @@ package registry
 
 import (
 	"context"
+	stderrors "errors"
+	"net"
+	neturl "net/url"
 	"testing"
 
 	"github.com/stretchr/testify/suite"
 
 	"github.com/goharbor/harbor/src/common"
 	"github.com/goharbor/harbor/src/lib/config"
+	"github.com/goharbor/harbor/src/lib/errors"
 	_ "github.com/goharbor/harbor/src/pkg/config/inmemory"
+	"github.com/goharbor/harbor/src/pkg/reg"
 	"github.com/goharbor/harbor/src/pkg/reg/model"
 	"github.com/goharbor/harbor/src/testing/mock"
 	testingproject "github.com/goharbor/harbor/src/testing/pkg/project"
@@ -68,15 +73,18 @@ func (r *registryTestSuite) TestValidate() {
 	err = r.ctl.validate(nil, registry)
 	r.NotNil(err)
 
-	// URL with FTP scheme
+	// URL with FTP scheme: rejected before an adapter is ever built, because
+	// net/http cannot dial it and the transport error used to surface as a 500
 	registry = &model.Registry{
 		Name: "endpoint01",
 		URL:  "ftp://example.com",
 	}
-	mock.OnAnything(r.regMgr, "CreateAdapter").Return(r.adapter, nil)
-	mock.OnAnything(r.adapter, "HealthCheck").Return(model.Healthy, nil)
 	err = r.ctl.validate(nil, registry)
-	r.Nil(err)
+	r.NotNil(err)
+	r.True(errors.IsErr(err, errors.BadRequestCode), "want a bad request error, got %v", err)
+	r.regMgr.AssertNotCalled(r.T(), "CreateAdapter", mock.Anything, mock.Anything)
+
+	r.SetupTest()
 
 	// URL without scheme
 	registry = &model.Registry{
@@ -151,6 +159,84 @@ func (r *registryTestSuite) TestValidate() {
 	r.adapter.AssertExpectations(r.T())
 }
 
+// An endpoint that cannot be reached is a bad request, not a 500. This drives
+// the real registry manager and the real Harbor adapter factory rather than
+// mocks: the probe that fails is the GET /api/version issued while the adapter
+// is being built, so it never reaches the HealthCheck call a mocked manager
+// would exercise.
+func (r *registryTestSuite) TestValidateUnreachableEndpoint() {
+	config.InitWithSettings(map[string]any{
+		common.CoreURL: "http://core:8080",
+	})
+
+	// Hold a loopback listener open and hang up on every connection. Keeping
+	// the port bound is what makes this deterministic: releasing it first
+	// would let another process take it between the bind and the request.
+	listener, err := net.Listen("tcp", "127.0.0.1:0")
+	r.Require().NoError(err)
+	defer listener.Close()
+	address := listener.Addr().String()
+	go func() {
+		for {
+			conn, err := listener.Accept()
+			if err != nil {
+				return
+			}
+			conn.Close()
+		}
+	}()
+
+	ctl := &controller{
+		regMgr: reg.Mgr,
+		repMgr: r.repMgr,
+		proMgr: r.proMgr,
+	}
+
+	err = ctl.validate(context.Background(), &model.Registry{
+		Name: "endpoint01",
+		Type: model.RegistryTypeHarbor,
+		URL:  "http://" + address,
+	})
+
+	r.Require().NotNil(err)
+	r.True(errors.IsErr(err, errors.BadRequestCode), "want a bad request error, got %v", err)
+	r.Contains(err.Error(), "failed to reach the registry endpoint")
+}
+
+// The other branch: an adapter that builds but whose health check cannot reach
+// the endpoint. Only reachable with a mocked adapter, since the adapters that
+// probe lazily are the ones that need credentials.
+func (r *registryTestSuite) TestValidateHealthCheckTransportError() {
+	transportErr := &neturl.Error{
+		Op:  "Get",
+		URL: "http://example.com/v2/",
+		Err: stderrors.New("dial tcp: connection refused"),
+	}
+	mock.OnAnything(r.regMgr, "CreateAdapter").Return(r.adapter, nil)
+	mock.OnAnything(r.adapter, "HealthCheck").Return("", transportErr)
+
+	err := r.ctl.validate(context.Background(), &model.Registry{
+		Name: "endpoint01",
+		URL:  "http://example.com",
+	})
+
+	r.Require().NotNil(err)
+	r.True(errors.IsErr(err, errors.BadRequestCode), "want a bad request error, got %v", err)
+	r.Contains(err.Error(), "failed to reach the registry endpoint")
+	r.regMgr.AssertExpectations(r.T())
+	r.adapter.AssertExpectations(r.T())
+}
+
+// Errors that are not transport failures keep their own code, so a genuine
+// internal fault is not reported to the caller as a bad request.
+func (r *registryTestSuite) TestUnreachableEndpointErrorPassesOtherErrorsThrough() {
+	internal := stderrors.New("boom")
+	r.Equal(internal, unreachableEndpointError("http://example.com", internal))
+
+	notFound := errors.New(nil).WithCode(errors.NotFoundCode).WithMessage("gone")
+	r.Equal(notFound, unreachableEndpointError("http://example.com", notFound))
+}
+
 func (r *registryTestSuite) TestDelete() {
 	// referenced by replication policy
 	mock.OnAnything(r.repMgr, "Count").Return(int64(1), nil)

src/server/v2.0/handler/registry.go

@@ -174,7 +174,7 @@ func (r *registryAPI) UpdateRegistry(ctx context.Context, params operation.Updat
 			registry.Credential.AccessSecret = *params.Registry.AccessSecret
 		}
 		if registry.URL != storedURL && params.Registry.AccessSecret == nil {
-			normalizedURL, err := lib.ValidateURL(registry.URL)
+			normalizedURL, err := lib.ValidateURL(registry.URL, "http", "https")
 			if err != nil {
 				return r.SendError(ctx, err)
 			}
@@ -256,7 +256,7 @@ func (r *registryAPI) PingRegistry(ctx context.Context, params operation.PingReg
 		// authoritative; ignore url/insecure/ca overrides so the ping (and the saved
 		// credentials it sends) can't be redirected to or MITM'd via an untrusted endpoint
 		if params.Registry.URL != nil && params.Registry.ID == nil {
-			url, err := lib.ValidateURL(*params.Registry.URL)
+			url, err := lib.ValidateURL(*params.Registry.URL, "http", "https")
 			if err != nil {
 				return r.SendError(ctx, err)
 			}

src/server/v2.0/handler/registry_test.go

@@ -190,10 +190,13 @@ func (suite *RegistryTestSuite) TestUpdateRegistryInvalidURLReturnsError() {
 	}
 }
 
-// TestUpdateRegistryStorageSchemeURLAccepted documents that schema-aware validation
-// accepts storage-backed registry URLs (sftp/s3) on update, and that changing to one
-// still clears the stored AccessSecret like any other URL change.
-func (suite *RegistryTestSuite) TestUpdateRegistryStorageSchemeURLAccepted() {
+// TestUpdateRegistryStorageSchemeURLRejected pins the update path to http/https,
+// the only schemes a replication adapter can speak. #742 originally documented
+// sftp:// and s3:// as accepted here, but no adapter under src/pkg/reg/ handles
+// either, so such an endpoint was accepted and then failed its health check with
+// a 500 -- the bug this lane fixes. The controller refuses them too; this keeps
+// the handler and the controller saying the same thing.
+func (suite *RegistryTestSuite) TestUpdateRegistryStorageSchemeURLRejected() {
 	for _, newURL := range []string{
 		"sftp://storage.example.com",
 		"s3://bucket.example.com",
@@ -210,18 +213,12 @@ func (suite *RegistryTestSuite) TestUpdateRegistryStorageSchemeURLAccepted() {
 		}
 		mock.OnAnything(suite.regCtl, "Get").Return(saved, nil).Once()
 
-		var updated *model.Registry
-		suite.regCtl.On("Update", mock.Anything, mock.Anything).Return(nil).Once().
-			Run(func(args testifymock.Arguments) { updated = args.Get(1).(*model.Registry) })
-
 		res, err := suite.PutJSON("/registries/1", &models.RegistryUpdate{
 			URL: suite.ptrStr(newURL),
 		})
 		suite.NoError(err)
-		suite.Equal(200, res.StatusCode, "URL %q must be accepted", newURL)
-		suite.Require().NotNil(updated)
-		suite.Equal(newURL, updated.URL)
-		suite.Empty(updated.Credential.AccessSecret)
+		suite.Equal(400, res.StatusCode, "URL %q must be rejected", newURL)
+		suite.regCtl.AssertNotCalled(suite.T(), "Update", testifymock.Anything, testifymock.Anything)
 	}
 }