Skip to content

Commit e6f7db2

Browse files
maximpertsov10zingpd
maximpertsov
authored and
10zingpd
committed
RSDK-1960, RSDK-6945, RSDK-539 Fix CloudMetadata + Test config flow (#3686)
1 parent b3b067c commit e6f7db2

12 files changed

Lines changed: 173 additions & 40 deletions

File tree

‎config/reader.go‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -131,33 +131,32 @@ func readCertificateDataFromCloudGRPC(ctx context.Context,
131131
signalingInsecure bool,
132132
cloudConfigFromDisk *Cloud,
133133
logger logging.Logger,
134-
) (*Cloud, error) {
134+
) (tlsConfig, error) {
135135
conn, err := CreateNewGRPCClient(ctx, cloudConfigFromDisk, logger)
136136
if err != nil {
137-
return nil, err
137+
return tlsConfig{}, err
138138
}
139139
defer utils.UncheckedErrorFunc(conn.Close)
140140

141141
service := apppb.NewRobotServiceClient(conn)
142142
res, err := service.Certificate(ctx, &apppb.CertificateRequest{Id: cloudConfigFromDisk.ID})
143143
if err != nil {
144144
// Check cache?
145-
return nil, err
145+
return tlsConfig{}, err
146146
}
147147

148148
if !signalingInsecure {
149149
if res.TlsCertificate == "" {
150-
return nil, errors.New("no TLS certificate yet from cloud; try again later")
150+
return tlsConfig{}, errors.New("no TLS certificate yet from cloud; try again later")
151151
}
152152
if res.TlsPrivateKey == "" {
153-
return nil, errors.New("no TLS private key yet from cloud; try again later")
153+
return tlsConfig{}, errors.New("no TLS private key yet from cloud; try again later")
154154
}
155155
}
156156

157-
// TODO(RSDK-539): we might want to use an internal type here. The gRPC api will not return a Cloud json struct.
158-
return &Cloud{
159-
TLSCertificate: res.TlsCertificate,
160-
TLSPrivateKey: res.TlsPrivateKey,
157+
return tlsConfig{
158+
certificate: res.TlsCertificate,
159+
privateKey: res.TlsPrivateKey,
161160
}, nil
162161
}
163162

@@ -261,16 +260,15 @@ func readFromCloud(
261260
certData, err := readCertificateDataFromCloudGRPC(ctxWithTimeout, cfg.Cloud.SignalingInsecure, cloudCfg, logger)
262261
if err != nil {
263262
cancel()
264-
if !errors.Is(err, context.DeadlineExceeded) {
263+
if !errors.As(err, &context.DeadlineExceeded) {
265264
return nil, err
266265
}
267266
if tls.certificate == "" || tls.privateKey == "" {
268267
return nil, errors.Wrap(err, "error getting certificate data from cloud; try again later")
269268
}
270269
logger.Warnw("failed to refresh certificate data; using cached for now", "error", err)
271270
} else {
272-
tls.certificate = certData.TLSCertificate
273-
tls.privateKey = certData.TLSPrivateKey
271+
tls = certData
274272
cancel()
275273
}
276274
}
@@ -282,6 +280,8 @@ func readFromCloud(
282280
managedBy := cfg.Cloud.ManagedBy
283281
locationSecret := cfg.Cloud.LocationSecret
284282
locationSecrets := cfg.Cloud.LocationSecrets
283+
primaryOrgID := cfg.Cloud.PrimaryOrgID
284+
locationID := cfg.Cloud.LocationID
285285

286286
mergeCloudConfig := func(to *Config) {
287287
*to.Cloud = *cloudCfg
@@ -294,10 +294,11 @@ func readFromCloud(
294294
to.Cloud.LocationSecrets = locationSecrets
295295
to.Cloud.TLSCertificate = tls.certificate
296296
to.Cloud.TLSPrivateKey = tls.privateKey
297+
to.Cloud.PrimaryOrgID = primaryOrgID
298+
to.Cloud.LocationID = locationID
297299
}
298300

299301
mergeCloudConfig(cfg)
300-
// TODO(RSDK-1960): add more tests around config caching
301302
unprocessedConfig.Cloud.TLSCertificate = tls.certificate
302303
unprocessedConfig.Cloud.TLSPrivateKey = tls.privateKey
303304

‎config/reader_test.go‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,16 +2,137 @@ package config
22

33
import (
44
"context"
5+
"fmt"
56
"os"
67
"strings"
78
"testing"
9+
"time"
810

911
"github.com/google/uuid"
12+
pb "go.viam.com/api/app/v1"
1013
"go.viam.com/test"
1114

15+
"go.viam.com/rdk/config/testutils"
1216
"go.viam.com/rdk/logging"
1317
)
1418

19+
func TestFromReader(t *testing.T) {
20+
const (
21+
robotPartID = "forCachingTest"
22+
secret = testutils.FakeCredentialPayLoad
23+
)
24+
var (
25+
logger = logging.NewTestLogger(t)
26+
ctx = context.Background()
27+
)
28+
29+
// clear cache
30+
setupClearCache := func(t *testing.T) {
31+
t.Helper()
32+
clearCache(robotPartID)
33+
_, err := readFromCache(robotPartID)
34+
test.That(t, os.IsNotExist(err), test.ShouldBeTrue)
35+
}
36+
37+
setupFakeServer := func(t *testing.T) (*testutils.FakeCloudServer, func()) {
38+
t.Helper()
39+
40+
logger := logging.NewTestLogger(t)
41+
42+
fakeServer, err := testutils.NewFakeCloudServer(context.Background(), logger)
43+
test.That(t, err, test.ShouldBeNil)
44+
cleanup := func() {
45+
test.That(t, fakeServer.Shutdown(), test.ShouldBeNil)
46+
}
47+
48+
return fakeServer, cleanup
49+
}
50+
51+
t.Run("online", func(t *testing.T) {
52+
setupClearCache(t)
53+
54+
fakeServer, cleanup := setupFakeServer(t)
55+
defer cleanup()
56+
57+
cloudResponse := &Cloud{
58+
ManagedBy: "acme",
59+
SignalingAddress: "abc",
60+
ID: robotPartID,
61+
Secret: secret,
62+
FQDN: "fqdn",
63+
LocalFQDN: "localFqdn",
64+
LocationSecrets: []LocationSecret{},
65+
LocationID: "the-location",
66+
PrimaryOrgID: "the-primary-org",
67+
}
68+
certProto := &pb.CertificateResponse{
69+
TlsCertificate: "cert",
70+
TlsPrivateKey: "key",
71+
}
72+
73+
cloudConfProto, err := CloudConfigToProto(cloudResponse)
74+
test.That(t, err, test.ShouldBeNil)
75+
protoConfig := &pb.RobotConfig{Cloud: cloudConfProto}
76+
fakeServer.StoreDeviceConfig(robotPartID, protoConfig, certProto)
77+
78+
appAddress := fmt.Sprintf("http://%s", fakeServer.Addr().String())
79+
cfgText := fmt.Sprintf(`{"cloud":{"id":%q,"app_address":%q,"secret":%q}}`, robotPartID, appAddress, secret)
80+
gotCfg, err := FromReader(ctx, "", strings.NewReader(cfgText), logger)
81+
defer clearCache(robotPartID)
82+
test.That(t, err, test.ShouldBeNil)
83+
84+
expectedCloud := *cloudResponse
85+
expectedCloud.AppAddress = appAddress
86+
expectedCloud.TLSCertificate = certProto.TlsCertificate
87+
expectedCloud.TLSPrivateKey = certProto.TlsPrivateKey
88+
expectedCloud.RefreshInterval = time.Duration(10000000000)
89+
test.That(t, gotCfg.Cloud, test.ShouldResemble, &expectedCloud)
90+
91+
cachedCfg, err := readFromCache(robotPartID)
92+
test.That(t, err, test.ShouldBeNil)
93+
expectedCloud.AppAddress = ""
94+
test.That(t, cachedCfg.Cloud, test.ShouldResemble, &expectedCloud)
95+
})
96+
97+
t.Run("offline with cached config", func(t *testing.T) {
98+
setupClearCache(t)
99+
100+
cachedCloud := &Cloud{
101+
ManagedBy: "acme",
102+
SignalingAddress: "abc",
103+
ID: robotPartID,
104+
Secret: secret,
105+
FQDN: "fqdn",
106+
LocalFQDN: "localFqdn",
107+
TLSCertificate: "cert",
108+
TLSPrivateKey: "key",
109+
LocationID: "the-location",
110+
PrimaryOrgID: "the-primary-org",
111+
}
112+
cachedConf := &Config{Cloud: cachedCloud}
113+
err := storeToCache(robotPartID, cachedConf)
114+
test.That(t, err, test.ShouldBeNil)
115+
defer clearCache(robotPartID)
116+
117+
fakeServer, cleanup := setupFakeServer(t)
118+
defer cleanup()
119+
fakeServer.FailOnConfigAndCertsWith(context.DeadlineExceeded)
120+
fakeServer.StoreDeviceConfig(robotPartID, nil, nil)
121+
122+
appAddress := fmt.Sprintf("http://%s", fakeServer.Addr().String())
123+
cfgText := fmt.Sprintf(`{"cloud":{"id":%q,"app_address":%q,"secret":%q}}`, robotPartID, appAddress, secret)
124+
gotCfg, err := FromReader(ctx, "", strings.NewReader(cfgText), logger)
125+
test.That(t, err, test.ShouldBeNil)
126+
127+
expectedCloud := *cachedCloud
128+
expectedCloud.AppAddress = appAddress
129+
expectedCloud.TLSCertificate = "cert"
130+
expectedCloud.TLSPrivateKey = "key"
131+
expectedCloud.RefreshInterval = time.Duration(10000000000)
132+
test.That(t, gotCfg.Cloud, test.ShouldResemble, &expectedCloud)
133+
})
134+
}
135+
15136
func TestStoreToCache(t *testing.T) {
16137
logger := logging.NewTestLogger(t)
17138
ctx := context.Background()

‎config/testutils/fake_cloud.go‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ type FakeCloudServer struct {
3131

3232
deviceConfigs map[string]*configAndCerts
3333

34-
failOnConfigAndCerts bool
34+
errConfigAndCerts error
3535

3636
mu sync.Mutex
3737
}
@@ -92,9 +92,20 @@ func NewFakeCloudServer(ctx context.Context, logger logging.Logger) (*FakeCloudS
9292
// FailOnConfigAndCerts if `failure` is true the server will return an Internal error on
9393
// all certficate and config requests.
9494
func (s *FakeCloudServer) FailOnConfigAndCerts(failure bool) {
95+
if failure {
96+
s.FailOnConfigAndCertsWith(status.Error(codes.Internal, "oops failure"))
97+
} else {
98+
s.FailOnConfigAndCertsWith(nil)
99+
}
100+
}
101+
102+
// FailOnConfigAndCertsWith will cause the server to return a given `error` on all
103+
// certficate and config requests. If `error == nil` then certficate and config
104+
// requests will succeed.
105+
func (s *FakeCloudServer) FailOnConfigAndCertsWith(err error) {
95106
s.mu.Lock()
96107
defer s.mu.Unlock()
97-
s.failOnConfigAndCerts = failure
108+
s.errConfigAndCerts = err
98109
}
99110

100111
// Addr returns the listeners address.
@@ -138,8 +149,8 @@ func (s *FakeCloudServer) Config(ctx context.Context, req *pb.ConfigRequest) (*p
138149
s.mu.Lock()
139150
defer s.mu.Unlock()
140151

141-
if s.failOnConfigAndCerts {
142-
return nil, status.Error(codes.Internal, "oops failure")
152+
if s.errConfigAndCerts != nil {
153+
return nil, s.errConfigAndCerts
143154
}
144155

145156
d, ok := s.deviceConfigs[req.Id]
@@ -155,8 +166,8 @@ func (s *FakeCloudServer) Certificate(ctx context.Context, req *pb.CertificateRe
155166
s.mu.Lock()
156167
defer s.mu.Unlock()
157168

158-
if s.failOnConfigAndCerts {
159-
return nil, status.Error(codes.Internal, "oops failure")
169+
if s.errConfigAndCerts != nil {
170+
return nil, s.errConfigAndCerts
160171
}
161172

162173
d, ok := s.deviceConfigs[req.Id]

‎robot/client/client.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -933,8 +933,8 @@ func (rc *RobotClient) Log(ctx context.Context, log zapcore.Entry, fields []zap.
933933
return err
934934
}
935935

936-
// GetCloudMetadata returns app-related information about the robot.
937-
func (rc *RobotClient) GetCloudMetadata(ctx context.Context) (cloud.Metadata, error) {
936+
// CloudMetadata returns app-related information about the robot.
937+
func (rc *RobotClient) CloudMetadata(ctx context.Context) (cloud.Metadata, error) {
938938
cloudMD := cloud.Metadata{}
939939
req := &pb.GetCloudMetadataRequest{}
940940
resp, err := rc.client.GetCloudMetadata(ctx, req)

‎robot/client/client_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2012,7 +2012,7 @@ func TestLoggingInterceptor(t *testing.T) {
20122012
test.That(t, err, test.ShouldBeNil)
20132013
}
20142014

2015-
func TestGetCloudMetadata(t *testing.T) {
2015+
func TestCloudMetadata(t *testing.T) {
20162016
logger := logging.NewTestLogger(t)
20172017
listener, err := net.Listen("tcp", "localhost:0")
20182018
test.That(t, err, test.ShouldBeNil)
@@ -2026,7 +2026,7 @@ func TestGetCloudMetadata(t *testing.T) {
20262026
injectRobot := &inject.Robot{
20272027
ResourceNamesFunc: func() []resource.Name { return nil },
20282028
ResourceRPCAPIsFunc: func() []resource.RPCAPI { return nil },
2029-
GetCloudMetadataFunc: func(ctx context.Context) (cloud.Metadata, error) {
2029+
CloudMetadataFunc: func(ctx context.Context) (cloud.Metadata, error) {
20302030
return injectCloudMD, nil
20312031
},
20322032
}
@@ -2045,7 +2045,7 @@ func TestGetCloudMetadata(t *testing.T) {
20452045
test.That(t, client.Close(context.Background()), test.ShouldBeNil)
20462046
}()
20472047

2048-
md, err := client.GetCloudMetadata(context.Background())
2048+
md, err := client.CloudMetadata(context.Background())
20492049
test.That(t, err, test.ShouldBeNil)
20502050
test.That(t, md, test.ShouldResemble, injectCloudMD)
20512051
}

‎robot/impl/local_robot.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1173,8 +1173,8 @@ func (r *localRobot) checkMaxInstance(api resource.API, max int) error {
11731173
return nil
11741174
}
11751175

1176-
// GetCloudMetadata returns app-related information about the robot.
1177-
func (r *localRobot) GetCloudMetadata(ctx context.Context) (cloud.Metadata, error) {
1176+
// CloudMetadata returns app-related information about the robot.
1177+
func (r *localRobot) CloudMetadata(ctx context.Context) (cloud.Metadata, error) {
11781178
md := cloud.Metadata{}
11791179
cfg := r.Config()
11801180
if cfg == nil {

‎robot/impl/local_robot_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3233,7 +3233,7 @@ func TestCloudMetadata(t *testing.T) {
32333233
cfg := &config.Config{}
32343234
robot, shutdown := initTestRobot(t, ctx, cfg, logger)
32353235
defer shutdown()
3236-
_, err := robot.GetCloudMetadata(ctx)
3236+
_, err := robot.CloudMetadata(ctx)
32373237
test.That(t, err, test.ShouldBeError, errors.New("cloud metadata not available"))
32383238
})
32393239
t.Run("with cloud data", func(t *testing.T) {
@@ -3246,7 +3246,7 @@ func TestCloudMetadata(t *testing.T) {
32463246
}
32473247
robot, shutdown := initTestRobot(t, ctx, cfg, logger)
32483248
defer shutdown()
3249-
md, err := robot.GetCloudMetadata(ctx)
3249+
md, err := robot.CloudMetadata(ctx)
32503250
test.That(t, err, test.ShouldBeNil)
32513251
test.That(t, md, test.ShouldResemble, cloud.Metadata{
32523252
RobotPartID: "the-robot-part",

‎robot/impl/resource_manager_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1839,8 +1839,8 @@ func (rr *dummyRobot) Logger() logging.Logger {
18391839
return rr.robot.Logger()
18401840
}
18411841

1842-
func (rr *dummyRobot) GetCloudMetadata(ctx context.Context) (cloud.Metadata, error) {
1843-
return rr.robot.GetCloudMetadata(ctx)
1842+
func (rr *dummyRobot) CloudMetadata(ctx context.Context) (cloud.Metadata, error) {
1843+
return rr.robot.CloudMetadata(ctx)
18441844
}
18451845

18461846
func (rr *dummyRobot) Close(ctx context.Context) error {

‎robot/robot.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,8 @@ type Robot interface {
8181
// Status takes a list of resource names and returns their corresponding statuses. If no names are passed in, return all statuses.
8282
Status(ctx context.Context, resourceNames []resource.Name) ([]Status, error)
8383

84-
// GetCloudMetadata returns app-related information about the robot.
85-
GetCloudMetadata(ctx context.Context) (cloud.Metadata, error)
84+
// CloudMetadata returns app-related information about the robot.
85+
CloudMetadata(ctx context.Context) (cloud.Metadata, error)
8686

8787
// Close attempts to cleanly close down all constituent parts of the robot.
8888
Close(ctx context.Context) error

‎robot/server/server.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -461,7 +461,7 @@ func (s *Server) Log(ctx context.Context, req *pb.LogRequest) (*pb.LogResponse,
461461

462462
// GetCloudMetadata returns app-related information about the robot.
463463
func (s *Server) GetCloudMetadata(ctx context.Context, _ *pb.GetCloudMetadataRequest) (*pb.GetCloudMetadataResponse, error) {
464-
md, err := s.robot.GetCloudMetadata(ctx)
464+
md, err := s.robot.CloudMetadata(ctx)
465465
if err != nil {
466466
return nil, err
467467
}

0 commit comments

Comments
 (0)