MIR-1619: Choose cluster routes when connecting

cli/commands/cluster.go

@@ -101,6 +101,8 @@ func ClusterList(ctx *Context, opts struct {
 			address := ccfg.Hostname
 			if ccfg.ViaCloud {
 				address = lipgloss.NewStyle().Foreground(theme.Muted).Render("via miren cloud")
+			} else if address == "" && ccfg.XID != "" && ccfg.Identity != "" {
+				address = lipgloss.NewStyle().Foreground(theme.Muted).Render("direct when reachable, otherwise via cloud")
 			} else if host, port, err := net.SplitHostPort(address); err == nil {
 				// Color the port portion gray for table display
 				grayPort := lipgloss.NewStyle().Foreground(theme.Muted).Render(":" + port)

cli/commands/cluster_add.go

@@ -327,6 +327,7 @@ func addCluster(ctx *Context, opts addClusterOptions) (*addedCluster, error) {
 	var clusterCert *clusterCertificate
 	var allAddresses []string
 	var clusterXID string
+	var selectedCluster *ClusterResponse
 
 	// What the cluster is called in cloud, and whose it is. Empty in manual
 	// mode, where cloud is never asked and neither is knowable.
@@ -351,8 +352,7 @@ func addCluster(ctx *Context, opts addClusterOptions) (*addedCluster, error) {
 		}
 
 		var (
-			selectedCluster *ClusterResponse
-			localName       string
+			localName string
 			// Which of the undialable clusters cloud can still reach. Skipped
 			// when the caller already said --via-cloud, since the answer would
 			// change nothing.
@@ -421,9 +421,9 @@ func addCluster(ctx *Context, opts addClusterOptions) (*addedCluster, error) {
 			ctx.Info("%s advertises no address this machine can dial, and cloud can reach it.", selectedCluster.Name)
 		}
 
+		allAddresses = selectedCluster.APIAddresses
 		if opts.viaCloud {
-			// No address and no certificate: both describe dialing the cluster,
-			// which is the thing this entry exists to avoid.
+			// Keep advertised addresses for automatic routing on later connections.
 			//
 			// The route itself is checked when the caller asked for it by name,
 			// rather than when it was chosen above (which already probed). The
@@ -440,9 +440,6 @@ func addCluster(ctx *Context, opts addClusterOptions) (*addedCluster, error) {
 
 			announceClusterAdd(ctx, selectedCluster.Name, localName, "routed through Miren Cloud")
 		} else {
-			// Store all available addresses
-			allAddresses = selectedCluster.APIAddresses
-
 			// Try to connect to the cluster
 			workingAddress, cert, err := tryConnectToCluster(ctx, selectedCluster, true)
 			if err != nil {
@@ -456,11 +453,7 @@ func addCluster(ctx *Context, opts addClusterOptions) (*addedCluster, error) {
 
 				ctx.Info("Could not reach %s directly, but cloud has a link to it.", selectedCluster.Name)
 
-				// The address and certificate belong to a route that does not
-				// work from here. Keeping either would write an entry that
-				// looks dialable and is not.
 				opts.viaCloud = true
-				allAddresses = nil
 
 				announceClusterAdd(ctx, selectedCluster.Name, localName, "routed through Miren Cloud")
 			} else {
@@ -489,7 +482,10 @@ func addCluster(ctx *Context, opts addClusterOptions) (*addedCluster, error) {
 		AllAddresses: allAddresses,
 		Identity:     identityName,
 		XID:          clusterXID,
-		ViaCloud:     opts.viaCloud,
+		ViaCloud:     explicitViaCloud,
+	}
+	if selectedCluster != nil {
+		clusterConfig.CACertFingerprint = selectedCluster.CACertFingerprint
 	}
 
 	// A cloud reached over plain HTTP is a development one, and routing through
@@ -498,7 +494,7 @@ func addCluster(ctx *Context, opts addClusterOptions) (*addedCluster, error) {
 	// that cannot work, or one that quietly ships a credential in the clear
 	// without recording that anywhere, are both worse than saying it out loud
 	// here and putting the admission in the file.
-	if opts.viaCloud && identity != nil && strings.HasPrefix(identity.Issuer, "http://") {
+	if clusterXID != "" && identity != nil && strings.HasPrefix(identity.Issuer, "http://") {
 		clusterConfig.Insecure = true
 		ctx.Warn("%s is not encrypted, so commands to this cluster will send your credentials in the clear.", identity.Issuer)
 		ctx.Warn("Recorded as insecure: true on the cluster. Use an https cloud for anything but local development.")
@@ -573,9 +569,12 @@ func addCluster(ctx *Context, opts addClusterOptions) (*addedCluster, error) {
 		return nil, codedErrorf(codeConfigError, "failed to save cluster configuration: %w", err)
 	}
 
-	if opts.viaCloud {
+	if explicitViaCloud {
 		ctx.Completed("Successfully added cluster %q with identity %q, routed through Miren Cloud", clusterName, identityName)
 		ctx.Info("Commands reach it over the connection it holds open to cloud, so it needs no address here.")
+	} else if opts.viaCloud {
+		ctx.Completed("Successfully added cluster %q with identity %q; reached through Miren Cloud while adding", clusterName, identityName)
+		ctx.Info("Future connections use direct when reachable, otherwise via cloud.")
 	} else if identityName != "" {
 		ctx.Completed("Successfully added cluster %q with identity %q at %s", clusterName, identityName, address)
 	} else {
@@ -597,6 +596,7 @@ func addCluster(ctx *Context, opts addClusterOptions) (*addedCluster, error) {
 		XID:        clusterXID,
 		Address:    address,
 		ViaCloud:   opts.viaCloud,
+		ForceCloud: clusterConfig.ViaCloud,
 		Identity:   identityName,
 		Insecure:   clusterConfig.Insecure,
 		Active:     mainConfig.ActiveCluster() == clusterName,

cli/commands/cluster_add_byname_test.go

@@ -71,6 +71,37 @@ func TestAddClusterByNameRoutedThroughCloud(t *testing.T) {
 	r.Error(err, "the cloud name is not also written")
 }
 
+func TestAddClusterFallbackPreservesAutomaticRouting(t *testing.T) {
+	addresses := []string{"invalid::::"}
+	srv := fakeCloud(t, []ClusterResponse{
+		{Name: "prod", XID: "cluster-prod", APIAddresses: addresses, CACertFingerprint: "trusted-pin"},
+	}, nil)
+	configWithIdentity(t, srv.URL)
+	withCloudRouteProbe(t, func(*Context, *clientconfig.Config, string, *clientconfig.IdentityConfig, *ClusterResponse) bool {
+		return true
+	})
+	ctx := presenceContext(t)
+	added, err := addCluster(ctx, addClusterOptions{clusterName: "prod"})
+	require.NoError(t, err)
+	require.True(t, added.ViaCloud, "cloud was used while adding")
+	require.False(t, added.ForceCloud, "a successful fallback is not a forced route")
+	require.Contains(t, ctx.Stdout.(*bytes.Buffer).String(), "direct when reachable, otherwise via cloud")
+	require.NotContains(t, ctx.Stdout.(*bytes.Buffer).String(), "needs no address here")
+	cfg, err := clientconfig.LoadConfig()
+	require.NoError(t, err)
+	cluster, err := cfg.GetCluster("prod")
+	require.NoError(t, err)
+	require.False(t, cluster.ViaCloud)
+	require.Equal(t, addresses, cluster.AllAddresses)
+	require.Equal(t, "trusted-pin", cluster.CACertFingerprint)
+	listCtx := presenceContext(t)
+	require.NoError(t, ClusterList(listCtx, struct {
+		FormatOptions
+		ConfigCentric
+	}{}))
+	require.Contains(t, listCtx.Stdout.(*bytes.Buffer).String(), "direct when reachable, otherwise via cloud")
+}
+
 // A cluster nothing can dial and cloud cannot reach is a dead end. The picker
 // greys it out; named, it has to be refused outright — falling through would
 // probe localhost and could pin whatever is listening there under a remote

cli/commands/cluster_add_json.go

@@ -89,7 +89,7 @@ type commandError struct {
 	Message string `json:"message"`
 }
 
-// addedCluster describes what was written to the config.
+// addedCluster describes the added configuration and the route checked at add time.
 type addedCluster struct {
 	// Name is the local name, which is what every other command takes.
 	Name string `json:"name"`
@@ -104,6 +104,9 @@ type addedCluster struct {
 	// no address of its own.
 	Address  string `json:"address,omitempty"`
 	ViaCloud bool   `json:"via_cloud"`
+	// ForceCloud is the persisted override, unlike ViaCloud which reports the
+	// route used while adding and may change on the next connection.
+	ForceCloud bool `json:"force_cloud"`
 
 	Identity string `json:"identity,omitempty"`
 
@@ -159,7 +162,10 @@ const clusterAddJSONDoc = "With `--format json`, the command prints one result d
 	"```\n\n" +
 	"`name` is the local name, which is what every other command takes. `cloud_name` " +
 	"appears alongside it only when `--as` stored the cluster under a different name than " +
-	"it has in Miren Cloud, and `address` is absent for a cluster reached through cloud.\n\n" +
+	"it has in Miren Cloud, and `address` is absent for a cluster reached through cloud. " +
+	"`via_cloud` reports the route selected while adding; `force_cloud` reports whether " +
+	"future connections are forced through cloud. With `force_cloud: false`, cloud-authenticated " +
+	"clusters try direct when reachable, otherwise via cloud.\n\n" +
 	"Messages are written for people and will be reworded. The code is the stable part:\n\n" +
 	"| Code | Meaning |\n" +
 	"|------|---------|\n" +

cli/commands/cluster_add_json_test.go

@@ -81,13 +81,34 @@ func TestClusterAddJSONSuccess(t *testing.T) {
 	r.Equal("cluster-prod", added.XID)
 	r.Equal("Acme", added.Organization)
 	r.True(added.ViaCloud)
+	r.True(added.ForceCloud)
 	r.Empty(added.Address, "a routed cluster is dialed at no address of its own")
 	r.Equal("cloud", added.Identity)
 	r.True(added.Insecure, "the test cloud is http, and the entry says so")
 	r.True(added.Active, "the first cluster added becomes the active one")
 	r.Contains(added.ConfigFile, "acme-prod.yaml")
 }
 
+func TestClusterAddJSONAutomaticFallback(t *testing.T) {
+	srv := fakeCloud(t, []ClusterResponse{{Name: "prod", XID: "cluster-prod"}}, map[string]bool{"cluster-prod": true})
+	configWithIdentity(t, srv.URL)
+	withCloudRouteProbe(t, func(*Context, *clientconfig.Config, string, *clientconfig.IdentityConfig, *ClusterResponse) bool {
+		return true
+	})
+	result, printed, exitCode := runClusterAdd(t, jsonAddOpts("prod"))
+	require.Zero(t, exitCode)
+	require.True(t, result.OK)
+	require.True(t, result.Cluster.ViaCloud)
+	require.False(t, result.Cluster.ForceCloud)
+	require.Contains(t, printed, `"force_cloud": false`)
+	require.Empty(t, result.Cluster.Address)
+	cfg, err := clientconfig.LoadConfig()
+	require.NoError(t, err)
+	cluster, err := cfg.GetCluster("prod")
+	require.NoError(t, err)
+	require.False(t, cluster.ViaCloud)
+}
+
 // A failure has to be legible twice over: as a document a caller can branch on,
 // and as a non-zero exit status for one that only checks that.
 func TestClusterAddJSONReportsFailures(t *testing.T) {

cli/commands/cluster_add_viacloud_test.go

@@ -184,51 +184,3 @@ func TestOnlineButUnansweringClusterIsNotRouted(t *testing.T) {
 		&ClusterResponse{Name: "old-runtime", XID: "cluster-old"}),
 		"presence in cloud is not evidence the cluster will answer over the relay")
 }
-
-// What the fallback has to write, and the RFD's claim that a routed entry needs
-// no address and no CA.
-//
-// A cluster that could not be dialed still advertised addresses and may have
-// presented a certificate, and keeping either would write an entry that looks
-// dialable and is not. The next command would then try the dead address instead
-// of the route that works.
-func TestRoutedEntryKeepsNoDialableFields(t *testing.T) {
-	r := require.New(t)
-
-	// The shape addCluster builds once the fallback has fired: viaCloud set,
-	// and the address, addresses and certificate deliberately left empty.
-	cfg := &clientconfig.ClusterConfig{
-		Hostname:     "",
-		AllAddresses: nil,
-		Identity:     "cloud",
-		XID:          "cluster-abc",
-		ViaCloud:     true,
-	}
-
-	r.True(cfg.ViaCloud)
-	r.Empty(cfg.Hostname, "a routed entry is never dialed, so an address would be a trap")
-	r.Empty(cfg.AllAddresses)
-	r.Empty(cfg.CACert, "the certificate on the wire belongs to cloud, not the cluster")
-	r.NotEmpty(cfg.XID, "routing needs the cluster's id in cloud")
-	r.NotEmpty(cfg.Identity, "and an identity to authenticate with")
-
-	// And it resolves to a relay endpoint rather than a direct dial.
-	endpoint, err := cfg.CloudEndpoint(viaCloudTestConfig(t, cfg))
-	r.NoError(err)
-	r.NotEmpty(endpoint)
-}
-
-// viaCloudTestConfig wraps a cluster entry in a config holding the identity it
-// names, which is what CloudEndpoint resolves the cloud through.
-func viaCloudTestConfig(t *testing.T, cluster *clientconfig.ClusterConfig) *clientconfig.Config {
-	t.Helper()
-
-	cfg := clientconfig.NewConfig()
-	cfg.SetIdentity("cloud", &clientconfig.IdentityConfig{
-		Type:   clientconfig.IdentityToken,
-		Issuer: "https://api.miren.cloud",
-		Token:  freshLoginJWT(t),
-	})
-	cfg.SetCluster("prod", cluster)
-	return cfg
-}

cli/commands/rpc_errors.go

@@ -21,6 +21,30 @@ import (
 // user actually typed — "miren route list" is a far better thing to put in an
 // error than the capability "entities" it happens to resolve.
 func (c *Context) wrapRPCError(err error) error {
+	if routes, ok := errors.AsType[*clientconfig.ClusterRouteError](err); ok {
+		var d *ui.Diagnostic
+		if direct, ok := errors.AsType[*rpc.ResolveError](routes.Direct); ok {
+			d = c.diagnoseResolve(direct)
+		}
+		if d == nil {
+			d = &ui.Diagnostic{
+				Detail: "Neither the direct addresses nor the cloud relay could establish a connection.",
+				Actions: []ui.Action{
+					{Command: "miren doctor", Note: "check what's reachable"},
+					{Command: "miren cluster list", Note: "confirm the addresses"},
+				},
+			}
+		}
+		d.Summary = fmt.Sprintf("couldn't reach %s directly or through Miren Cloud", clusterLabel(c.ClusterName))
+		d.Facts = append(d.Facts,
+			ui.Fact{Label: "Direct", Value: routes.Direct.Error()},
+			ui.Fact{Label: "Miren Cloud", Value: routes.Cloud.Error()},
+		)
+		d.Cause = routes
+		d.ShowCause = c.Verbose()
+		return d
+	}
+
 	re, ok := errors.AsType[*rpc.ResolveError](err)
 	if !ok {
 		return err