RSDK-7921 - Cleanup processConfig in entrypoint #4450
Conversation
viambot
added
the
safe to test
viambot
added
safe to test
| @@ -303,8 +303,7 @@ func (s *robotServer) serveWeb(ctx context.Context, cfg *config.Config) (err err | |||
| ctx = rpc.ContextWithDialer(ctx, rpcDialer) | |||
|
|
|||
| processConfig := func(in *config.Config) (*config.Config, error) { | |||
| tlsCfg := config.NewTLSConfig(cfg) | |||
There was a problem hiding this comment.
NewTLSConfig seems to only exist here because of the if statement in https://github.com/viamrobotics/rdk/pull/4450/files#diff-fe44f09c4d5977b5f5eaea29170b6a0748819c9d02271746a20d81a5f3efca17L964 and the fact that it's pulling in the original config instead of the config passed in. However, any subsequent/updated config will have the same fields as well. There does not seem to be anything else that is different than just creating a tls.Config inside ProcessConfig. the if statement also does not do anything substantive and can be easily ignored.
| selfCreds = &rpc.Credentials{rutils.CredentialsTypeRobotSecret, in.Cloud.Secret} | ||
| out.Network.TLSConfig = tlsCfg.Config // override |
There was a problem hiding this comment.
this is the only logic change in the PR, if in.Cloud != nil but in.Cloud.TLSCertificate == "", out.Network.TLSConfig will stay nil now instead of being empty. I think it's better for it to stay nil than empty, and other places where we use TLSConfig already check for nil
There was a problem hiding this comment.
Are we aware of a circumstance in which the cfg.Cloud exists, but TLSCertificate is the empty string?
There was a problem hiding this comment.
no, it would imply a app connected robot without a cert, which is not possible in the current setup except maybe a local dev environment? I tested starting a robot offline with the cert deleted from the cache but could still connect to it after changing some options (WithAllowInsecureWithCredentialsDowngrade())
There was a problem hiding this comment.
Sweet, that's what I was hoping to hear. Can we document the in.Cloud.TLSCertificate = "" check with:
We expect a cloud config from app to always contain a non-empty `TLSCertificate` field. We do this empty string check just to cope with unexpected input. Such as cached configs that are hand altered to have their `TLSCertificate` removed.
| } | ||
|
|
||
| // UpdateCert updates the TLS certificate to be returned. | ||
| func (t *TLSConfig) UpdateCert(cfg *Config) error { |
There was a problem hiding this comment.
we only call UpdateCert in one place, right after we create TLSConfig - if we combine the operations we don't need this structure at all
| selfCreds = &rpc.Credentials{rutils.CredentialsTypeRobotSecret, in.Cloud.Secret} | ||
| out.Network.TLSConfig = tlsCfg.Config // override |
There was a problem hiding this comment.
Sweet, that's what I was hoping to hear. Can we document the in.Cloud.TLSCertificate = "" check with:
We expect a cloud config from app to always contain a non-empty `TLSCertificate` field. We do this empty string check just to cope with unexpected input. Such as cached configs that are hand altered to have their `TLSCertificate` removed.
simplifies the logic a lot, annotated the pr but no additional comments as I think it is unnecessary - the previous logic is convoluted and now it's simple.
the original refactor is here for reference https://github.com/viamrobotics/rdk/pull/699/files#diff-780f3d307fb668fed30f86c777b47efbbd6848b5aa3282dc1958bc76b2f07ecb
checked cloud connected case, cloud connected but offline case, both times robot starts up correctly and clients can connect.