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

Revert unintentional change for auth key generation API endpoint #109

Merged
merged 3 commits into from
Nov 23, 2022

Conversation

josecelano
Copy link
Member

@josecelano josecelano commented Nov 22, 2022

Tasks:

  • Revert change.
  • E2E test for the endpoint.

@josecelano
Copy link
Member Author

josecelano commented Nov 22, 2022

hi @WarmBeer, I'm trying to add a test for the API endpoint. I'm doing the same as we did for the UDP tracker. I want to run the API and use an HTTP client to make a request to that endpoint.

For some reason I can't connect to the API:

running 1 test
2022-11-22T19:12:06.110254618+00:00 [torrust_tracker::logging][INFO] logging initialized.
2022-11-22T19:12:06.110283885+00:00 [torrust_tracker::jobs::tracker_api][INFO] Starting Torrust API server on: 127.0.0.1:55790
thread 'tracker_api::should_generate_a_new_auth_key' panicked at 'called `Result::unwrap()` on an `Err` value: reqwest::Error { kind: Request, url: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Ipv4(127.0.0.1)), port: Some(55790), path: "/api/key/60", query: Some("token=MyAccessToken"), fragment: None }, source: hyper::Error(Connect, ConnectError("tcp connect error", Os { code: 111, kind: ConnectionRefused, message: "Connection refused" })) }', tests/api.rs:103:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test tracker_api::should_generate_a_new_auth_key ... FAILED

The client configuration is correct because it works when I make a request to a manually executed tracker in the standard 1212 port.

I think the problem could be the Tokyo library is not running a new thread for the API and when the client makes the request, the server is not responding. But I do not know if that makes sense. I suppose the server should be executed again when the client is waiting for the response, even is Tokio is using only one thread.

The other problem could be the setup. But It worked for the UDP tracker, and I do not see any error.

Do you have any idea why it's not working or how can I find out the issue? I've run out of ideas.

@josecelano josecelano force-pushed the issue-108-revert-endpoint-change branch from 7f3c258 to 5ab6851 Compare November 22, 2022 19:31
@mickvandijke
Copy link
Member

hi @WarmBeer, I'm trying to add a test for the API endpoint. I'm doing the same as we did for the UDP tracker. I want to run the API and use an HTTP client to make a request to that endpoint.

For some reason I can't connect to the API:

running 1 test
2022-11-22T19:12:06.110254618+00:00 [torrust_tracker::logging][INFO] logging initialized.
2022-11-22T19:12:06.110283885+00:00 [torrust_tracker::jobs::tracker_api][INFO] Starting Torrust API server on: 127.0.0.1:55790
thread 'tracker_api::should_generate_a_new_auth_key' panicked at 'called `Result::unwrap()` on an `Err` value: reqwest::Error { kind: Request, url: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Ipv4(127.0.0.1)), port: Some(55790), path: "/api/key/60", query: Some("token=MyAccessToken"), fragment: None }, source: hyper::Error(Connect, ConnectError("tcp connect error", Os { code: 111, kind: ConnectionRefused, message: "Connection refused" })) }', tests/api.rs:103:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test tracker_api::should_generate_a_new_auth_key ... FAILED

The client configuration is correct because it works when I make a request to a manually executed tracker in the standard 1212 port.

I think the problem could be the Tokyo library is not running a new thread for the API and when the client makes the request, the server is not responding. But I do not know if that makes sense. I suppose the server should be executed again when the client is waiting for the response, even is Tokio is using only one thread.

The other problem could be the setup. But It worked for the UDP tracker, and I do not see any error.

Do you have any idea why it's not working or how can I find out the issue? I've run out of ideas.

I will have a look.

@mickvandijke
Copy link
Member

mickvandijke commented Nov 23, 2022

hi @WarmBeer, I'm trying to add a test for the API endpoint. I'm doing the same as we did for the UDP tracker. I want to run the API and use an HTTP client to make a request to that endpoint.

For some reason I can't connect to the API:

running 1 test
2022-11-22T19:12:06.110254618+00:00 [torrust_tracker::logging][INFO] logging initialized.
2022-11-22T19:12:06.110283885+00:00 [torrust_tracker::jobs::tracker_api][INFO] Starting Torrust API server on: 127.0.0.1:55790
thread 'tracker_api::should_generate_a_new_auth_key' panicked at 'called `Result::unwrap()` on an `Err` value: reqwest::Error { kind: Request, url: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Ipv4(127.0.0.1)), port: Some(55790), path: "/api/key/60", query: Some("token=MyAccessToken"), fragment: None }, source: hyper::Error(Connect, ConnectError("tcp connect error", Os { code: 111, kind: ConnectionRefused, message: "Connection refused" })) }', tests/api.rs:103:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test tracker_api::should_generate_a_new_auth_key ... FAILED

The client configuration is correct because it works when I make a request to a manually executed tracker in the standard 1212 port.

I think the problem could be the Tokyo library is not running a new thread for the API and when the client makes the request, the server is not responding. But I do not know if that makes sense. I suppose the server should be executed again when the client is waiting for the response, even is Tokio is using only one thread.

The other problem could be the setup. But It worked for the UDP tracker, and I do not see any error.

Do you have any idea why it's not working or how can I find out the issue? I've run out of ideas.

Hey Jose,

Somehow the API server is not reachable during the test. I've tried connecting to the test API server from Postman, but I get the same error as Reqwest: "connection refused".

When running the API manually, it does work like you said. Strange..

I have to focus on an issue for the index frontend right now, but I will try again tomorrow if you haven't found a fix by then.

@josecelano
Copy link
Member Author

hi @WarmBeer, I'm trying to add a test for the API endpoint. I'm doing the same as we did for the UDP tracker. I want to run the API and use an HTTP client to make a request to that endpoint.
For some reason I can't connect to the API:

running 1 test
2022-11-22T19:12:06.110254618+00:00 [torrust_tracker::logging][INFO] logging initialized.
2022-11-22T19:12:06.110283885+00:00 [torrust_tracker::jobs::tracker_api][INFO] Starting Torrust API server on: 127.0.0.1:55790
thread 'tracker_api::should_generate_a_new_auth_key' panicked at 'called `Result::unwrap()` on an `Err` value: reqwest::Error { kind: Request, url: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Ipv4(127.0.0.1)), port: Some(55790), path: "/api/key/60", query: Some("token=MyAccessToken"), fragment: None }, source: hyper::Error(Connect, ConnectError("tcp connect error", Os { code: 111, kind: ConnectionRefused, message: "Connection refused" })) }', tests/api.rs:103:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test tracker_api::should_generate_a_new_auth_key ... FAILED

The client configuration is correct because it works when I make a request to a manually executed tracker in the standard 1212 port.
I think the problem could be the Tokyo library is not running a new thread for the API and when the client makes the request, the server is not responding. But I do not know if that makes sense. I suppose the server should be executed again when the client is waiting for the response, even is Tokio is using only one thread.
The other problem could be the setup. But It worked for the UDP tracker, and I do not see any error.
Do you have any idea why it's not working or how can I find out the issue? I've run out of ideas.

Hey Jose,

Somehow the API server is not reachable during the test. I've tried connecting to the test API server from Postman, but I get the same error as Reqwest: "connection refused".

When running the API manually, it does work like you said. Strange..

I have to focus on an issue for the index frontend right now, but I will try again tomorrow if you haven't found a fix by then.

Thank you very much @WarmBeer! I've just fixed it here. It seems we only needed to add a delay to give time to the server to be ready to accept requests. I saw @zupzup added a delay in this example here. Thanks @zupzup for sharing that repo. It's been very helpful for me.

Anyway, I do not like this solution very much because we wait for a random duration, and we make tests slower. Running the test should be a kind of callback executed when the server is ready.

@josecelano josecelano force-pushed the issue-108-revert-endpoint-change branch 3 times, most recently from 746302d to 92c50e6 Compare November 23, 2022 13:35
@josecelano josecelano marked this pull request as ready for review November 23, 2022 13:57
@josecelano
Copy link
Member Author

hi @WarmBeer @da2ce7, This is ready for review. I've added a simple e2e test. I was considering using warp test features:

https://docs.rs/warp/0.1.20/warp/test/index.html

but I do not know if it makes sense. It is similar to what other frameworks have. It allows you to test the "controller" without running the web server. In our case, we have a lot of bootstrapping, we have to setup the tracker anyway. I do not know if it is worth it since we only avoid creating the socket (and maybe the setup for the other endpoints).

On the other hand, the e2e test is less coupled with the implementation. I think @WarmBeer wanted to change the web framework. If we change it, we do not need to rewrite the tests.

@josecelano josecelano linked an issue Nov 23, 2022 that may be closed by this pull request
The response for the enpoint POST /api/key/:seconds_valid should be:

```json
{
  "key": "IaWDneuFNZi8IB4MPA3qW1CD0M30EZSM",
  "valid_until": 1674804892
}
```

instead of:

```json
{
  "key": "IaWDneuFNZi8IB4MPA3qW1CD0M30EZSM",
  "valid_until":
    {
      "secs": 1674804892,
      "nanos": 423855037
    }
}
```

It was propagated to the API after changing the internal struct `AuthKey` from:

```rust
pub struct AuthKey {
    pub key: String,
    pub valid_until: Option<u64>,
}
```

to:

```rust
pub struct AuthKey {
    pub key: String,
    pub valid_until: Option<DurationSinceUnixEpoch>,
}
```
Added for API end to end tests.
@josecelano josecelano force-pushed the issue-108-revert-endpoint-change branch from 92c50e6 to 409f82a Compare November 23, 2022 15:29
Copy link
Contributor

@da2ce7 da2ce7 left a comment

Choose a reason for hiding this comment

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

ACK 92c50e6

@josecelano
Copy link
Member Author

ACK 92c50e6

@da2ce7 I've rebased to include the latest changes in PR-111.

Copy link
Contributor

@da2ce7 da2ce7 left a comment

Choose a reason for hiding this comment

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

ACK 409f82a

@da2ce7 da2ce7 merged commit 7a2182c into develop Nov 23, 2022
@josecelano josecelano deleted the issue-108-revert-endpoint-change branch November 23, 2022 16:17
@josecelano
Copy link
Member Author

hi @WarmBeer @da2ce7, I've written an article about the problems I had testing the API endpoint:

https://github.com/Nautilus-Cyberneering/testing-in-rust/blob/main/docs/testing-apis-in-rust.md

While writing the article, I tried different options to avoid the nondeterministic "sleep". I managed to make it work with messages instead of using "sleep".

josecelano added a commit to torrust/torrust-index that referenced this pull request Nov 30, 2022
It was added because the tracker API was changed, but that change was
reverted here:

torrust/torrust-tracker#109

and it is not used anymore.
josecelano added a commit to torrust/torrust-index that referenced this pull request Nov 30, 2022
728fe8a fix: [#78] remove unused struct NewTrackerKey (Jose Celano)

Pull request description:

  It was added because the tracker API was changed, but that change was reverted here:

  torrust/torrust-tracker#109

  and it is not used anymore.

ACKs for top commit:
  da2ce7:
    ACK 728fe8a

Tree-SHA512: 3bad8b9d0f38c0567e52d163c37fdc4dfde776d03666486416a833e290a29a7853e841853f7e5000ddeca9712ecebebecc200f86e97c647095dc6e36ceb13246
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

API endpoint for keys was accidentally changed
3 participants