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(rust): allow passing the API URL through the CLI #1495

Merged
merged 36 commits into from
Oct 16, 2024
Merged

feat(rust): allow passing the API URL through the CLI #1495

merged 36 commits into from
Oct 16, 2024

Conversation

mchf
Copy link
Contributor

@mchf mchf commented Jul 24, 2024

Problem

We want to be able to use different remote installation servers

Solution

  • Introduced global option for user to provide remote api url
  • Used newly introduced BaseHTTPClient to handle urls.

Already done:

  • auth, network, localization, users, software, product, storage, questions

Testing

  • Tested manually

rust/agama-cli/src/main.rs Outdated Show resolved Hide resolved
@mvidner
Copy link
Contributor

mvidner commented Jul 24, 2024

I have tried this out, with this testing setup:

  • Run agama in a container, with /testing_using_container.sh; this builds the Rust code in rust/target/debug, and exposes port 443 to 10443 on the host
  • (Now I wanted to simply run the agama binary on the host, but this fails for me because 15.4 is too old and misses the new glibc and openssl that the TW container is using)
  • Compile agama in release mode on the host, cargo build -r which puts the binaries to rust/target/release

Now I can try rust/target/release/agama --api https://localhost:10443/api auth login

But it does not work yet:

  1. there seems to be a funny bug: BaseHTTPClient always wants an authentication token from a file, or else it fails (NotAuthenticated is printed). I needed to mkdir -p $HOME/.local/agama; touch $HOME/.local/agama/token
  2. the --api option seems to be ignored:
$ ./target/release/agama --uri https://ohmu:10443/api auth login
> Please enter the root password: ********
Anyhow(error sending request for url (http://localhost/api/auth): error trying to connect: tcp connect error: Connection refused (os error 111)

@coveralls
Copy link

Coverage Status

coverage: 70.979%. remained the same
when pulling 1d7d84c on api-option
into ce0c770 on master.

mvidner added a commit that referenced this pull request Aug 1, 2024
to enable
- connecting to alternative servers, either production or test mocks
- unauthenticated calls, either for the initial auth or test mocks

This is needed for both #1438 and #1495

In the end, the API changes only by adding a `Default` trait
implementation which does no authentication.
Connecting to a mock server is achieved by assigning to the public
`base_url` field.
@mchf mchf force-pushed the api-option branch 2 times, most recently from 0980b33 to 43307ea Compare August 8, 2024 08:59
@mvidner
Copy link
Contributor

mvidner commented Aug 9, 2024

With #1438 being merged, this PR could be extended to work with agama config load, because this CLI command will ignore the parts of the API that are left unspecified in the JSON, and we can test only on the user, root and localization parts, that already have HTTP clients

@mvidner
Copy link
Contributor

mvidner commented Aug 9, 2024

Also note that in #1438 I needed to connect to a mock HTTP server and made client APIs for that, which should be pretty useful for your use case too :)

@mvidner
Copy link
Contributor

mvidner commented Aug 9, 2024

The current code looks good but the auth endpoint is special and IMHO we need to see how this works with a regular one too.

rust/agama-cli/src/main.rs Outdated Show resolved Hide resolved
@mvidner
Copy link
Contributor

mvidner commented Oct 1, 2024

Yay! It gets somewhere... and then it fails on a self-signed certificate:

$ ./rust/target/release/agama --api https://localhost:10443/api/ config show
Anyhow(Failed to communicate with the HTTP backend 'error sending request for url (https://localhost:10443/api/network/connections)'

Caused by:
    0: error sending request for url (https://localhost:10443/api/network/connections)
    1: client error (Connect)
    2: error:0A000086:SSL routines:tls_post_process_server_certificate:certificate verify failed:ssl/statem/statem_clnt.c:1890: (self-signed certificate)
    3: error:0A000086:SSL routines:tls_post_process_server_certificate:certificate verify failed:ssl/statem/statem_clnt.c:1890:)

@mchf
Copy link
Contributor Author

mchf commented Oct 2, 2024

I was having some troubles with dbus in storage but for now at least local calls seem to be working.
agama_config_show

.github/workflows/ci-rust.yml Outdated Show resolved Hide resolved
rust/agama-lib/src/network/client.rs Outdated Show resolved Hide resolved
@mchf mchf requested a review from mvidner October 7, 2024 10:17
rust/agama-cli/src/main.rs Outdated Show resolved Hide resolved
rust/agama-cli/src/main.rs Outdated Show resolved Hide resolved
@mchf mchf force-pushed the api-option branch 2 times, most recently from c02d644 to eca3c77 Compare October 16, 2024 08:23
@mchf mchf force-pushed the api-option branch 2 times, most recently from fa43349 to d634766 Compare October 16, 2024 08:32
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

It is looking better now. Thanks!

rust/agama-cli/src/auth.rs Outdated Show resolved Hide resolved
rust/agama-cli/src/auth.rs Outdated Show resolved Hide resolved
rust/agama-cli/src/lib.rs Outdated Show resolved Hide resolved
rust/package/agama.changes Outdated Show resolved Hide resolved
@imobachgs imobachgs changed the title Agama cli to be able to use different api urls feat(rust): allow passing the API URL through the CLI Oct 16, 2024
@mchf mchf merged commit 4cefd9b into master Oct 16, 2024
2 checks passed
@mchf mchf deleted the api-option branch October 16, 2024 11:32
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.

5 participants