Skip to content
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

feat: add zetaclientd-supervisor #2113

Merged
merged 11 commits into from
May 10, 2024
Merged

feat: add zetaclientd-supervisor #2113

merged 11 commits into from
May 10, 2024

Conversation

gartnera
Copy link
Member

@gartnera gartnera commented May 2, 2024

Description

Add a zetaclientd-supervisor process which will watch the version of zetacored and automatically restart zetaclient if the version changes. When the version has changed, cosmovisor will have already downloaded and symlinked in the new zetaclientd binary so all we need to do is restart to pick up the new change.

We are careful to do a few things:

  • forward signals from zetaclientd-supervisor to zetaclientd
  • serialize the stdout so that log messages from the zetaclientd subprocess are not interleaved with the zetaclientd-supervisor logs messages
  • cache the hotkey and tss password so that it only needs to be input once even if zetaclientd restarts/crashes

If we decide not to rely on cosmovisor autodownload for delivery of the zetaclientd binary, we would have to add autodownload/upgrade functionality to this process as well. That logic isn't terribly complex but great care must be taken to ensure the download is performed correctly. We would retrieve the binaries by grabbing the current upgrade plan from zetacore via rpc.

Automatic download and symlinking like cosmovisor has now been added. The new zetaclient binary will automatically be downloaded and loaded when the zetacored version changes.

This is functional in the localnet docker images and upgrade tests. Example with v16 -> v17 -> v18 (dummy release delivered via upgrade info binary urls):

➜  ~ docker logs zetaclient0 2>&1 | grep zetaclientd-supervisor | grep -v 'connection error'
2024-05-08T22:25:10Z WRN got new upgrade plan (v17) module=zetaclientdSupervisor process=zetaclientd-supervisor
2024-05-08T22:25:10Z ERR downloadZetaclientd failed error="upgrade info empty" module=zetaclientdSupervisor process=zetaclientd-supervisor
2024-05-08T22:27:02Z WRN core version change (v12.3.0-rc-96-g181ba7ff -> v12.3.0-rc-124-gcb66f60c) module=zetaclientdSupervisor process=zetaclientd-supervisor
2024-05-08T22:27:02Z ERR error while waiting error="exit status 2" process=zetaclientd-supervisor
2024-05-08T22:30:43Z WRN got new upgrade plan (v18) module=zetaclientdSupervisor process=zetaclientd-supervisor
2024-05-08T22:30:43Z INF downloading zetaclientd module=zetaclientdSupervisor process=zetaclientd-supervisor
2024-05-08T22:31:35Z WRN core version change (v12.3.0-rc-124-gcb66f60c -> v12.3.0-rc-113-gb4605569) module=zetaclientdSupervisor process=zetaclientd-supervisor
2024-05-08T22:31:35Z ERR error while waiting error="exit status 2" process=zetaclientd-supervisor
zetaclient0:~/.zetaclientd/upgrades# find .
.
./genesis
./genesis/zetaclientd
./v18
./v18/zetaclientd
./current
./v17
./v17/zetaclientd
zetaclient0:~/.zetaclientd/upgrades# ls -lth
total 12K
lrwxrwxrwx    1 root     root          31 May  8 22:31 current -> /root/.zetaclientd/upgrades/v18
drwxr-x---    2 root     root        4.0K May  8 22:30 v18
drwxr-xr-x    2 root     root        4.0K May  8 22:22 v17
drwxr-xr-x    2 root     root        4.0K May  8 22:22 genesis

Relates to DEVOP-642

Degraded without #2135

How Has This Been Tested?

  • Tested CCTX in localnet
  • Tested in development environment
  • Go unit tests
  • Go integration tests
  • Tested via GitHub Actions

Checklist:

  • I have added unit tests that prove my fix feature works

@gartnera gartnera marked this pull request as ready for review May 3, 2024 17:38
@gartnera gartnera force-pushed the zetaclientd-supervisor branch from 4299c39 to f167c6a Compare May 8, 2024 19:50
@gartnera gartnera force-pushed the zetaclientd-supervisor branch from 35f7a86 to d8ad7df Compare May 9, 2024 14:43
Copy link

github-actions bot commented May 9, 2024

!!!WARNING!!!
nosec detected in the following files: cmd/zetaclientd-supervisor/main.go

Be very careful about using #nosec in code. It can be a quick way to suppress security warnings and move forward with development, it should be employed with caution. Suppressing warnings with #nosec can hide potentially serious vulnerabilities. Only use #nosec when you're absolutely certain that the security issue is either a false positive or has been mitigated in another way.

Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203
Broad #nosec annotations should be avoided, as they can hide other vulnerabilities. The CI will block you from merging this PR until you remove #nosec annotations that do not target specific rules.

Pay extra attention to the way #nosec is being used in the files listed above.

@github-actions github-actions bot added the nosec label May 9, 2024
@gartnera gartnera force-pushed the zetaclientd-supervisor branch from d8ad7df to ead518f Compare May 9, 2024 14:53
Copy link
Contributor

@kingpinXD kingpinXD left a comment

Choose a reason for hiding this comment

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

LGTM ,
NIT : comments before every function might be useful, not required but definitely good to have

@gartnera gartnera requested a review from a team May 9, 2024 17:25
Copy link
Member

@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

Looks good to me and will be very useful.

Regarding

cache the hotkey and tss password so that it only needs to be input once even if zetaclientd restarts/crashes

Caching here means it is kept as a local variable in the function and reused? Doesn't it bring some security concerns if someone can dump the memory?
ccing @CharlieMc0

@gartnera gartnera force-pushed the zetaclientd-supervisor branch from 9c8e14f to 88aa40a Compare May 10, 2024 15:14
@gartnera
Copy link
Member Author

cache the hotkey and tss password so that it only needs to be input once even if zetaclientd restarts/crashes

Caching here means it is kept as a local variable in the function and reused? Doesn't it bring some security concerns if someone can dump the memory? ccing @CharlieMc0

Today, you could just dump the zetaclient process memory. We make no efforts to zero the memory/variables we use to store the password in zetaclient.

@gartnera gartnera requested a review from lumtis May 10, 2024 15:18
@CharlieMc0
Copy link
Member

cache the hotkey and tss password so that it only needs to be input once even if zetaclientd restarts/crashes

Caching here means it is kept as a local variable in the function and reused? Doesn't it bring some security concerns if someone can dump the memory? ccing @CharlieMc0

Today, you could just dump the zetaclient process memory. We make no efforts to zero the memory/variables we use to store the password in zetaclient.

Yeah the risk of someone dumping memory already exists because zetaclient has to hold the key in memory unencrypted. It's already cached in memory while zetaclient is running. The only difference here is the supervisor also caches it between restarts.

@gartnera gartnera merged commit 9014044 into develop May 10, 2024
21 checks passed
@gartnera gartnera deleted the zetaclientd-supervisor branch May 10, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants