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

unitctl export subcommand #1321

Merged
merged 1 commit into from
Jun 19, 2024
Merged

unitctl export subcommand #1321

merged 1 commit into from
Jun 19, 2024

Conversation

avahahn
Copy link
Contributor

@avahahn avahahn commented Jun 14, 2024

Currently implemented is a unitctl subcommand that saves Unitd configuration to a tarball. This is the first step in addressing #1317.

Yet to be implemented is support for saving NJS modules or certificates. The code enclosed will be trivial to extend to support that later. Until then, #1317 should remain open.

@lcrilly
Copy link
Contributor

lcrilly commented Jun 17, 2024

If unitctl import is how configuration gets applied then shouldn't unitctl export be how configuration is retrieved?

Or perhaps that's already planned, and this is a deliberately different thing that is designed for the entire configuration? It's not clear to me what the overall goal of #1317 is.

It would make it much easier to understand the intent of the PR if the commit message/comment had a sample shell snippet with an example unitctl interaction.

@avahahn
Copy link
Contributor Author

avahahn commented Jun 17, 2024

If unitctl import is how configuration gets applied then shouldn't unitctl export be how configuration is retrieved?

Sure, I am happy to rename the subcommand.

It would make it much easier to understand the intent of the PR if the commit message/comment had a sample shell snippet with an example unitctl interaction.

off the top of my head:

# backup configuration
unitctl export -f backup.tar

# restore backed up configuration
tar -xvf backup.tar
unitctl import backup

I am happy to record a small screencap of the above.

@avahahn
Copy link
Contributor Author

avahahn commented Jun 17, 2024

Subcommand is now renamed to export

@avahahn avahahn changed the title unitctl save subcommand unitctl export subcommand Jun 17, 2024
@lcrilly
Copy link
Contributor

lcrilly commented Jun 17, 2024

@avahahn this won't work:

# backup configuration
unitctl export -f backup.tar

# restore backed up configuration
tar -xvf backup.tar
unitctl import backup

The /certificates endpoint cannot be imported. Unit won't expose the secrets.

So this might have an impact on the overall design (and value) of this feature.

@avahahn
Copy link
Contributor Author

avahahn commented Jun 17, 2024

The /certificates endpoint cannot be imported. Unit won't expose the secrets.

Sounds like you mean it can't be exported. That is unfortunate but I still believe this is worth implementing at least for configuration and js_modules. I will add a note to the Readme that certificates must be replicated separately.

@avahahn avahahn requested review from ac000 and lcrilly June 17, 2024 17:18
@lcrilly
Copy link
Contributor

lcrilly commented Jun 17, 2024

What do you think should happen if an exported configuration contains listeners with tls objects, and that configuration is sent to the import subcommand?

Does this align with your vision for this feature?

# backup configuration
unitctl export -f backup.tar

# take a look
tar tfv backup.tar
drwx------ <uid> <gid>       0 17 Jun  2024 unitd-backup-20240617/
-rw------- <uid> <gid>     502 17 Jun  2024 unitd-backup-20240617/certificates-info-only.json
-rw------- <uid> <gid>    1774 17 Jun  2024 unitd-backup-20240617/config.json
-rw------- <uid> <gid>     256 17 Jun  2024 unitd-backup-20240617/js_modules.json
-rw------- <uid> <gid>     315 17 Jun  2024 unitd-backup-20240617/status.json

import will always destroy and reload the running configuration?
importing from a .tar file assumes that the source is an export? Therefore fatal error if listeners have tls objects. Filenames have special meaning, e.g. ignores status.json instead of treating it as more config.
importing from a directory might contain .pem files - could we try to match with the certificates JSON to get them loaded?

@avahahn
Copy link
Contributor Author

avahahn commented Jun 17, 2024

What do you think should happen if an exported configuration contains listeners with tls objects, and that configuration is sent to the import subcommand?

Currently import already handles pem files (and js_modules). This MR only saves the configuration JSON.
Import will not directly read from a tar file. It is up to the user to extract the tarball and their certificates before importing. This provides a start that addresses at least the second two user stories from the linked ticket.

Copy link
Collaborator

@callahad callahad left a comment

Choose a reason for hiding this comment

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

The functional code is good enough that I wouldn't block merging, but if possible I'd really appreciate resolutions to the open conversations above.

tools/unitctl/README.md Outdated Show resolved Hide resolved
@avahahn avahahn force-pushed the unitctl-save branch 2 times, most recently from 798f392 to e44cece Compare June 19, 2024 06:02
* new subcommand for "export" in CLI
* new cmd submodule for exporting config tarballs
* logic to also output to stdout
* README additions
* limitations documented

Signed-off-by: Ava Hahn <[email protected]>
@avahahn avahahn merged commit 57a0f94 into nginx:master Jun 19, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants