RSDK-1960, RSDK-6945, RSDK-539 Fix CloudMetadata + Test config flow (v…

…iamrobotics#3686)

config/reader.go

@@ -131,33 +131,32 @@ func readCertificateDataFromCloudGRPC(ctx context.Context,
 	signalingInsecure bool,
 	cloudConfigFromDisk *Cloud,
 	logger logging.Logger,
-) (*Cloud, error) {
+) (tlsConfig, error) {
 	conn, err := CreateNewGRPCClient(ctx, cloudConfigFromDisk, logger)
 	if err != nil {
-		return nil, err
+		return tlsConfig{}, err
 	}
 	defer utils.UncheckedErrorFunc(conn.Close)
 
 	service := apppb.NewRobotServiceClient(conn)
 	res, err := service.Certificate(ctx, &apppb.CertificateRequest{Id: cloudConfigFromDisk.ID})
 	if err != nil {
 		// Check cache?
-		return nil, err
+		return tlsConfig{}, err
 	}
 
 	if !signalingInsecure {
 		if res.TlsCertificate == "" {
-			return nil, errors.New("no TLS certificate yet from cloud; try again later")
+			return tlsConfig{}, errors.New("no TLS certificate yet from cloud; try again later")
 		}
 		if res.TlsPrivateKey == "" {
-			return nil, errors.New("no TLS private key yet from cloud; try again later")
+			return tlsConfig{}, errors.New("no TLS private key yet from cloud; try again later")
 		}
 	}
 
-	// TODO(RSDK-539): we might want to use an internal type here. The gRPC api will not return a Cloud json struct.
-	return &Cloud{
-		TLSCertificate: res.TlsCertificate,
-		TLSPrivateKey:  res.TlsPrivateKey,
+	return tlsConfig{
+		certificate: res.TlsCertificate,
+		privateKey:  res.TlsPrivateKey,
 	}, nil
 }
 
@@ -261,16 +260,15 @@ func readFromCloud(
 		certData, err := readCertificateDataFromCloudGRPC(ctxWithTimeout, cfg.Cloud.SignalingInsecure, cloudCfg, logger)
 		if err != nil {
 			cancel()
-			if !errors.Is(err, context.DeadlineExceeded) {
+			if !errors.As(err, &context.DeadlineExceeded) {
 				return nil, err
 			}
 			if tls.certificate == "" || tls.privateKey == "" {
 				return nil, errors.Wrap(err, "error getting certificate data from cloud; try again later")
 			}
 			logger.Warnw("failed to refresh certificate data; using cached for now", "error", err)
 		} else {
-			tls.certificate = certData.TLSCertificate
-			tls.privateKey = certData.TLSPrivateKey
+			tls = certData
 			cancel()
 		}
 	}
@@ -282,6 +280,8 @@ func readFromCloud(
 	managedBy := cfg.Cloud.ManagedBy
 	locationSecret := cfg.Cloud.LocationSecret
 	locationSecrets := cfg.Cloud.LocationSecrets
+	primaryOrgID := cfg.Cloud.PrimaryOrgID
+	locationID := cfg.Cloud.LocationID
 
 	mergeCloudConfig := func(to *Config) {
 		*to.Cloud = *cloudCfg
@@ -294,10 +294,11 @@ func readFromCloud(
 		to.Cloud.LocationSecrets = locationSecrets
 		to.Cloud.TLSCertificate = tls.certificate
 		to.Cloud.TLSPrivateKey = tls.privateKey
+		to.Cloud.PrimaryOrgID = primaryOrgID
+		to.Cloud.LocationID = locationID
 	}
 
 	mergeCloudConfig(cfg)
-	// TODO(RSDK-1960): add more tests around config caching
 	unprocessedConfig.Cloud.TLSCertificate = tls.certificate
 	unprocessedConfig.Cloud.TLSPrivateKey = tls.privateKey
 

config/reader_test.go

@@ -2,16 +2,137 @@ package config
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/google/uuid"
+	pb "go.viam.com/api/app/v1"
 	"go.viam.com/test"
 
+	"go.viam.com/rdk/config/testutils"
 	"go.viam.com/rdk/logging"
 )
 
+func TestFromReader(t *testing.T) {
+	const (
+		robotPartID = "forCachingTest"
+		secret      = testutils.FakeCredentialPayLoad
+	)
+	var (
+		logger = logging.NewTestLogger(t)
+		ctx    = context.Background()
+	)
+
+	// clear cache
+	setupClearCache := func(t *testing.T) {
+		t.Helper()
+		clearCache(robotPartID)
+		_, err := readFromCache(robotPartID)
+		test.That(t, os.IsNotExist(err), test.ShouldBeTrue)
+	}
+
+	setupFakeServer := func(t *testing.T) (*testutils.FakeCloudServer, func()) {
+		t.Helper()
+
+		logger := logging.NewTestLogger(t)
+
+		fakeServer, err := testutils.NewFakeCloudServer(context.Background(), logger)
+		test.That(t, err, test.ShouldBeNil)
+		cleanup := func() {
+			test.That(t, fakeServer.Shutdown(), test.ShouldBeNil)
+		}
+
+		return fakeServer, cleanup
+	}
+
+	t.Run("online", func(t *testing.T) {
+		setupClearCache(t)
+
+		fakeServer, cleanup := setupFakeServer(t)
+		defer cleanup()
+
+		cloudResponse := &Cloud{
+			ManagedBy:        "acme",
+			SignalingAddress: "abc",
+			ID:               robotPartID,
+			Secret:           secret,
+			FQDN:             "fqdn",
+			LocalFQDN:        "localFqdn",
+			LocationSecrets:  []LocationSecret{},
+			LocationID:       "the-location",
+			PrimaryOrgID:     "the-primary-org",
+		}
+		certProto := &pb.CertificateResponse{
+			TlsCertificate: "cert",
+			TlsPrivateKey:  "key",
+		}
+
+		cloudConfProto, err := CloudConfigToProto(cloudResponse)
+		test.That(t, err, test.ShouldBeNil)
+		protoConfig := &pb.RobotConfig{Cloud: cloudConfProto}
+		fakeServer.StoreDeviceConfig(robotPartID, protoConfig, certProto)
+
+		appAddress := fmt.Sprintf("http://%s", fakeServer.Addr().String())
+		cfgText := fmt.Sprintf(`{"cloud":{"id":%q,"app_address":%q,"secret":%q}}`, robotPartID, appAddress, secret)
+		gotCfg, err := FromReader(ctx, "", strings.NewReader(cfgText), logger)
+		defer clearCache(robotPartID)
+		test.That(t, err, test.ShouldBeNil)
+
+		expectedCloud := *cloudResponse
+		expectedCloud.AppAddress = appAddress
+		expectedCloud.TLSCertificate = certProto.TlsCertificate
+		expectedCloud.TLSPrivateKey = certProto.TlsPrivateKey
+		expectedCloud.RefreshInterval = time.Duration(10000000000)
+		test.That(t, gotCfg.Cloud, test.ShouldResemble, &expectedCloud)
+
+		cachedCfg, err := readFromCache(robotPartID)
+		test.That(t, err, test.ShouldBeNil)
+		expectedCloud.AppAddress = ""
+		test.That(t, cachedCfg.Cloud, test.ShouldResemble, &expectedCloud)
+	})
+
+	t.Run("offline with cached config", func(t *testing.T) {
+		setupClearCache(t)
+
+		cachedCloud := &Cloud{
+			ManagedBy:        "acme",
+			SignalingAddress: "abc",
+			ID:               robotPartID,
+			Secret:           secret,
+			FQDN:             "fqdn",
+			LocalFQDN:        "localFqdn",
+			TLSCertificate:   "cert",
+			TLSPrivateKey:    "key",
+			LocationID:       "the-location",
+			PrimaryOrgID:     "the-primary-org",
+		}
+		cachedConf := &Config{Cloud: cachedCloud}
+		err := storeToCache(robotPartID, cachedConf)
+		test.That(t, err, test.ShouldBeNil)
+		defer clearCache(robotPartID)
+
+		fakeServer, cleanup := setupFakeServer(t)
+		defer cleanup()
+		fakeServer.FailOnConfigAndCertsWith(context.DeadlineExceeded)
+		fakeServer.StoreDeviceConfig(robotPartID, nil, nil)
+
+		appAddress := fmt.Sprintf("http://%s", fakeServer.Addr().String())
+		cfgText := fmt.Sprintf(`{"cloud":{"id":%q,"app_address":%q,"secret":%q}}`, robotPartID, appAddress, secret)
+		gotCfg, err := FromReader(ctx, "", strings.NewReader(cfgText), logger)
+		test.That(t, err, test.ShouldBeNil)
+
+		expectedCloud := *cachedCloud
+		expectedCloud.AppAddress = appAddress
+		expectedCloud.TLSCertificate = "cert"
+		expectedCloud.TLSPrivateKey = "key"
+		expectedCloud.RefreshInterval = time.Duration(10000000000)
+		test.That(t, gotCfg.Cloud, test.ShouldResemble, &expectedCloud)
+	})
+}
+
 func TestStoreToCache(t *testing.T) {
 	logger := logging.NewTestLogger(t)
 	ctx := context.Background()

config/testutils/fake_cloud.go

@@ -31,7 +31,7 @@ type FakeCloudServer struct {
 
 	deviceConfigs map[string]*configAndCerts
 
-	failOnConfigAndCerts bool
+	errConfigAndCerts error
 
 	mu sync.Mutex
 }
@@ -92,9 +92,20 @@ func NewFakeCloudServer(ctx context.Context, logger logging.Logger) (*FakeCloudS
 // FailOnConfigAndCerts if `failure` is true the server will return an Internal error on
 // all certficate and config requests.
 func (s *FakeCloudServer) FailOnConfigAndCerts(failure bool) {
+	if failure {
+		s.FailOnConfigAndCertsWith(status.Error(codes.Internal, "oops failure"))
+	} else {
+		s.FailOnConfigAndCertsWith(nil)
+	}
+}
+
+// FailOnConfigAndCertsWith will cause the server to return a given `error` on all
+// certficate and config requests. If `error == nil` then certficate and config
+// requests will succeed.
+func (s *FakeCloudServer) FailOnConfigAndCertsWith(err error) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	s.failOnConfigAndCerts = failure
+	s.errConfigAndCerts = err
 }
 
 // Addr returns the listeners address.
@@ -138,8 +149,8 @@ func (s *FakeCloudServer) Config(ctx context.Context, req *pb.ConfigRequest) (*p
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
-	if s.failOnConfigAndCerts {
-		return nil, status.Error(codes.Internal, "oops failure")
+	if s.errConfigAndCerts != nil {
+		return nil, s.errConfigAndCerts
 	}
 
 	d, ok := s.deviceConfigs[req.Id]
@@ -155,8 +166,8 @@ func (s *FakeCloudServer) Certificate(ctx context.Context, req *pb.CertificateRe
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
-	if s.failOnConfigAndCerts {
-		return nil, status.Error(codes.Internal, "oops failure")
+	if s.errConfigAndCerts != nil {
+		return nil, s.errConfigAndCerts
 	}
 
 	d, ok := s.deviceConfigs[req.Id]

robot/client/client.go

@@ -933,8 +933,8 @@ func (rc *RobotClient) Log(ctx context.Context, log zapcore.Entry, fields []zap.
 	return err
 }
 
-// GetCloudMetadata returns app-related information about the robot.
-func (rc *RobotClient) GetCloudMetadata(ctx context.Context) (cloud.Metadata, error) {
+// CloudMetadata returns app-related information about the robot.
+func (rc *RobotClient) CloudMetadata(ctx context.Context) (cloud.Metadata, error) {
 	cloudMD := cloud.Metadata{}
 	req := &pb.GetCloudMetadataRequest{}
 	resp, err := rc.client.GetCloudMetadata(ctx, req)

robot/client/client_test.go

@@ -2012,7 +2012,7 @@ func TestLoggingInterceptor(t *testing.T) {
 	test.That(t, err, test.ShouldBeNil)
 }
 
-func TestGetCloudMetadata(t *testing.T) {
+func TestCloudMetadata(t *testing.T) {
 	logger := logging.NewTestLogger(t)
 	listener, err := net.Listen("tcp", "localhost:0")
 	test.That(t, err, test.ShouldBeNil)
@@ -2026,7 +2026,7 @@ func TestGetCloudMetadata(t *testing.T) {
 	injectRobot := &inject.Robot{
 		ResourceNamesFunc:   func() []resource.Name { return nil },
 		ResourceRPCAPIsFunc: func() []resource.RPCAPI { return nil },
-		GetCloudMetadataFunc: func(ctx context.Context) (cloud.Metadata, error) {
+		CloudMetadataFunc: func(ctx context.Context) (cloud.Metadata, error) {
 			return injectCloudMD, nil
 		},
 	}
@@ -2045,7 +2045,7 @@ func TestGetCloudMetadata(t *testing.T) {
 		test.That(t, client.Close(context.Background()), test.ShouldBeNil)
 	}()
 
-	md, err := client.GetCloudMetadata(context.Background())
+	md, err := client.CloudMetadata(context.Background())
 	test.That(t, err, test.ShouldBeNil)
 	test.That(t, md, test.ShouldResemble, injectCloudMD)
 }

robot/impl/local_robot.go

@@ -1173,8 +1173,8 @@ func (r *localRobot) checkMaxInstance(api resource.API, max int) error {
 	return nil
 }
 
-// GetCloudMetadata returns app-related information about the robot.
-func (r *localRobot) GetCloudMetadata(ctx context.Context) (cloud.Metadata, error) {
+// CloudMetadata returns app-related information about the robot.
+func (r *localRobot) CloudMetadata(ctx context.Context) (cloud.Metadata, error) {
 	md := cloud.Metadata{}
 	cfg := r.Config()
 	if cfg == nil {

robot/impl/local_robot_test.go

@@ -3233,7 +3233,7 @@ func TestCloudMetadata(t *testing.T) {
 		cfg := &config.Config{}
 		robot, shutdown := initTestRobot(t, ctx, cfg, logger)
 		defer shutdown()
-		_, err := robot.GetCloudMetadata(ctx)
+		_, err := robot.CloudMetadata(ctx)
 		test.That(t, err, test.ShouldBeError, errors.New("cloud metadata not available"))
 	})
 	t.Run("with cloud data", func(t *testing.T) {
@@ -3246,7 +3246,7 @@ func TestCloudMetadata(t *testing.T) {
 		}
 		robot, shutdown := initTestRobot(t, ctx, cfg, logger)
 		defer shutdown()
-		md, err := robot.GetCloudMetadata(ctx)
+		md, err := robot.CloudMetadata(ctx)
 		test.That(t, err, test.ShouldBeNil)
 		test.That(t, md, test.ShouldResemble, cloud.Metadata{
 			RobotPartID:  "the-robot-part",

robot/impl/resource_manager_test.go

@@ -1843,8 +1843,8 @@ func (rr *dummyRobot) Logger() logging.Logger {
 	return rr.robot.Logger()
 }
 
-func (rr *dummyRobot) GetCloudMetadata(ctx context.Context) (cloud.Metadata, error) {
-	return rr.robot.GetCloudMetadata(ctx)
+func (rr *dummyRobot) CloudMetadata(ctx context.Context) (cloud.Metadata, error) {
+	return rr.robot.CloudMetadata(ctx)
 }
 
 func (rr *dummyRobot) Close(ctx context.Context) error {

robot/robot.go

@@ -81,8 +81,8 @@ type Robot interface {
 	// Status takes a list of resource names and returns their corresponding statuses. If no names are passed in, return all statuses.
 	Status(ctx context.Context, resourceNames []resource.Name) ([]Status, error)
 
-	// GetCloudMetadata returns app-related information about the robot.
-	GetCloudMetadata(ctx context.Context) (cloud.Metadata, error)
+	// CloudMetadata returns app-related information about the robot.
+	CloudMetadata(ctx context.Context) (cloud.Metadata, error)
 
 	// Close attempts to cleanly close down all constituent parts of the robot.
 	Close(ctx context.Context) error

robot/server/server.go

@@ -461,7 +461,7 @@ func (s *Server) Log(ctx context.Context, req *pb.LogRequest) (*pb.LogResponse,
 
 // GetCloudMetadata returns app-related information about the robot.
 func (s *Server) GetCloudMetadata(ctx context.Context, _ *pb.GetCloudMetadataRequest) (*pb.GetCloudMetadataResponse, error) {
-	md, err := s.robot.GetCloudMetadata(ctx)
+	md, err := s.robot.CloudMetadata(ctx)
 	if err != nil {
 		return nil, err
 	}

robot/server/server_test.go

@@ -78,7 +78,7 @@ func TestServer(t *testing.T) {
 		injectRobot := &inject.Robot{}
 		server := server.New(injectRobot)
 		req := pb.GetCloudMetadataRequest{}
-		injectRobot.GetCloudMetadataFunc = func(ctx context.Context) (cloud.Metadata, error) {
+		injectRobot.CloudMetadataFunc = func(ctx context.Context) (cloud.Metadata, error) {
 			return cloud.Metadata{
 				RobotPartID:  "the-robot-part",
 				PrimaryOrgID: "the-primary-org",