Skip to content

RSDK-5140 Don't override a local config BindAddress with a default one from Cloud #4985

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aldenh-viam
Copy link
Contributor

@aldenh-viam aldenh-viam commented May 13, 2025

In reader.go:fromReader: the initial read of both local and cloud configs.

In entrypoint.go:configWatcher: runs on detected config file changes.

Bind Address order of precedence is now:

  1. Non-default coming from Cloud
  2. Non-default from Local
  3. Default from Cloud/Local

Possible alternative:
Update all Cloud read functions with a parameter to include/exclude returning the default BindAddress. Note that the default BindAddress is not really coming from Cloud, but rather patched with a constant defined in rdk.

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label May 13, 2025
@aldenh-viam aldenh-viam changed the title Don't override a custom local config BindAddress with a default one f… RSDK-5140 Don't override a local config BindAddress with a default one from Cloud May 13, 2025
@aldenh-viam aldenh-viam requested a review from a team May 13, 2025 19:02
@cheukt
Copy link
Member

cheukt commented May 13, 2025

I'd like to see a log of some kind for each of these cases:

  • Non-default coming from Cloud that overrides non-default from local - something like "cloud config specifies port XXX, overriding port from local config XXX. starting server on port XXX"
  • Non-default coming from Cloud that overrides default from local - something similar to the above log
  • non-default from local overriding default from cloud - something like "local config specifies port XXX, overriding default port from cloud XXX. starting server on port XXX"
  • default port from cloud - "starting server on default port from cloud network config XXX"

basically we should be able to tell that ports were defined and explicitly overridden through the logs

@aldenh-viam
Copy link
Contributor Author

aldenh-viam commented May 13, 2025

I'd like to see a log of some kind for each of these cases:

* Non-default coming from Cloud that overrides non-default from local - something like "cloud config specifies port XXX, overriding port from local config XXX. starting server on port XXX"

* Non-default coming from Cloud that overrides default from local - something similar to the above log

* non-default from local overriding default from cloud - something like "local config specifies port XXX, overriding default port from cloud XXX. starting server on port XXX"

* default port from cloud - "starting server on default port from cloud network config XXX"

basically we should be able to tell that ports were defined and explicitly overridden through the logs

Isn't the current behavior already everything except the third (non-default local overrides default from cloud)? I'll add that one, but is it necessary to log all the others? I think users shouldn't really be expecting to be able to override cloud options with their local config unless we want to explicitly add that feature. Alternatively, I can always log the BindAddress and the source it retrieved it from. EDIT: discussed offline, we'll log 1 and 3, as they seem to be the two cases that have confused users in the past.

… either does or doesn't get overridden with Cloud's
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels May 14, 2025
Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

// Note: DefaultBindAddress "from Cloud" is actually set with a constant in rdk.
if err == nil && !cfgFromDisk.Network.BindAddressDefaultSet {
if cfg.Network.BindAddressDefaultSet {
logger.Infof("Using cloud config, but BindAddress is specified in local config (%v) "+
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer using logger.CInfof, even if it wouldn't matter in this case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants