From c629c2d3fc963777c719d0af3d6aa216929a482d Mon Sep 17 00:00:00 2001 From: Maxim Pertsov Date: Fri, 15 Mar 2024 09:59:52 -0400 Subject: [PATCH] RSDK-1960, RSDK-6945, RSDK-539 Fix CloudMetadata + Test config flow (#3686) --- config/reader.go | 27 ++++--- config/reader_test.go | 121 ++++++++++++++++++++++++++++ config/testutils/fake_cloud.go | 23 ++++-- robot/client/client.go | 4 +- robot/client/client_test.go | 6 +- robot/impl/local_robot.go | 4 +- robot/impl/local_robot_test.go | 4 +- robot/impl/resource_manager_test.go | 4 +- robot/robot.go | 4 +- robot/server/server.go | 2 +- robot/server/server_test.go | 2 +- testutils/inject/robot.go | 12 +-- 12 files changed, 173 insertions(+), 40 deletions(-) diff --git a/config/reader.go b/config/reader.go index dc3918a3336..a9b2a910817 100644 --- a/config/reader.go +++ b/config/reader.go @@ -131,10 +131,10 @@ 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) @@ -142,22 +142,21 @@ func readCertificateDataFromCloudGRPC(ctx context.Context, 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,7 +260,7 @@ 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 == "" { @@ -269,8 +268,7 @@ func readFromCloud( } 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 diff --git a/config/reader_test.go b/config/reader_test.go index ac75e431243..bb76abb956b 100644 --- a/config/reader_test.go +++ b/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() diff --git a/config/testutils/fake_cloud.go b/config/testutils/fake_cloud.go index e321c924695..08d220771e6 100644 --- a/config/testutils/fake_cloud.go +++ b/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] diff --git a/robot/client/client.go b/robot/client/client.go index d33847c60dd..c43fb3a9107 100644 --- a/robot/client/client.go +++ b/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) diff --git a/robot/client/client_test.go b/robot/client/client_test.go index 6266bdfd915..a253605153e 100644 --- a/robot/client/client_test.go +++ b/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) } diff --git a/robot/impl/local_robot.go b/robot/impl/local_robot.go index 7e6667a6884..4ad5e082453 100644 --- a/robot/impl/local_robot.go +++ b/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 { diff --git a/robot/impl/local_robot_test.go b/robot/impl/local_robot_test.go index 173f64f2cb0..500d6a70084 100644 --- a/robot/impl/local_robot_test.go +++ b/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", diff --git a/robot/impl/resource_manager_test.go b/robot/impl/resource_manager_test.go index be45d55d1ef..34e64c5cb05 100644 --- a/robot/impl/resource_manager_test.go +++ b/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 { diff --git a/robot/robot.go b/robot/robot.go index aa6cee62c1b..4d581fae046 100644 --- a/robot/robot.go +++ b/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 diff --git a/robot/server/server.go b/robot/server/server.go index ebf54588074..1b15de492d9 100644 --- a/robot/server/server.go +++ b/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 } diff --git a/robot/server/server_test.go b/robot/server/server_test.go index 07cb7c81cc5..8b83a5feadd 100644 --- a/robot/server/server_test.go +++ b/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", diff --git a/testutils/inject/robot.go b/testutils/inject/robot.go index 5c84f182758..62120380d51 100644 --- a/testutils/inject/robot.go +++ b/testutils/inject/robot.go @@ -48,7 +48,7 @@ type Robot struct { TransformPointCloudFunc func(ctx context.Context, srcpc pointcloud.PointCloud, srcName, dstName string) (pointcloud.PointCloud, error) StatusFunc func(ctx context.Context, resourceNames []resource.Name) ([]robot.Status, error) ModuleAddressFunc func() (string, error) - GetCloudMetadataFunc func(ctx context.Context) (cloud.Metadata, error) + CloudMetadataFunc func(ctx context.Context) (cloud.Metadata, error) ops *operation.Manager SessMgr session.Manager @@ -284,14 +284,14 @@ func (r *Robot) ModuleAddress() (string, error) { return r.ModuleAddressFunc() } -// GetCloudMetadata calls the injected GetCloudMetadata or the real one. -func (r *Robot) GetCloudMetadata(ctx context.Context) (cloud.Metadata, error) { +// CloudMetadata calls the injected CloudMetadata or the real one. +func (r *Robot) CloudMetadata(ctx context.Context) (cloud.Metadata, error) { r.Mu.RLock() defer r.Mu.RUnlock() - if r.GetCloudMetadataFunc == nil { - return r.GetCloudMetadata(ctx) + if r.CloudMetadataFunc == nil { + return r.CloudMetadata(ctx) } - return r.GetCloudMetadataFunc(ctx) + return r.CloudMetadataFunc(ctx) } type noopSessionManager struct{}