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

Refactor the c8y remote access plugin to use the c8y auth proxy #2597

Conversation

jarhodes314
Copy link
Contributor

Proposed changes

This PR adds support for websockets to the auth proxy, and modifies the c8y remote access plugin to connect via the auth proxy, rather than making a direct connection to c8y.

There are still a few TODOs in the code, but the broad shape of the changes are in place and the happy path works for me testing locally.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

Copy link
Contributor

github-actions bot commented Jan 17, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
381 0 3 381 100 51m55.441s

@jarhodes314 jarhodes314 force-pushed the 2591-refactor-the-c8y-remote-access-plugin-to-use-the-c8y-auth-proxy branch from fea8836 to 78f95c1 Compare January 18, 2024 18:09
Copy link

codecov bot commented Jan 18, 2024

Codecov Report

Attention: 76 lines in your changes are missing coverage. Please review.

Comparison is base (fb82765) 75.8% compared to head (845480e) 75.9%.
Report is 4 commits behind head on main.

Additional details and impacted files
Files Coverage Δ
crates/extensions/c8y_auth_proxy/src/url.rs 80.8% <ø> (+1.1%) ⬆️
plugins/c8y_remote_access_plugin/src/proxy.rs 34.6% <0.0%> (+0.4%) ⬆️
crates/extensions/c8y_auth_proxy/src/actor.rs 0.0% <0.0%> (ø)
plugins/c8y_remote_access_plugin/src/lib.rs 20.8% <0.0%> (-1.5%) ⬇️
crates/extensions/c8y_auth_proxy/src/server.rs 89.5% <89.2%> (+0.6%) ⬆️

... and 4 files with indirect coverage changes

Copy link
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

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

I'm no expert of either the axum or the tungstenite crates. But the connection/conversion logic looked sensible. Just spotted one minor bug in the conversion task error handler.

plugins/c8y_remote_access_plugin/src/proxy.rs Show resolved Hide resolved
crates/extensions/c8y_auth_proxy/src/server.rs Outdated Show resolved Hide resolved
error!("Websocket error proxying messages from Cumulocity to the client: {e}");
}
c8y_to_client.abort();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Query: Since this whole function doesn't return anything even while detecting one of the above failures, how would the wrapping server react? I'm assuming it would continue functioning without the proxying capability. Wouldn't it be better to propagate these errors so that the server can choose to shutdown/panic, with the hope of restarting without that issue?

This might be out of scope for this PR specifically, as it's related to our general strategy on how we handle partial failures of a single component, in a multi-component process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure exactly what axum does internally, but my belief is that this is a separate task spawned when we call ws.on_upgrade. In any case, it only affects the current connection, nothing else, and I believe this is the most desirable behaviour. If we were talking over HTTP, this is a point at which I would give up and respond with 500 Internal Server Error to the current request, but I wouldn't want the server to stop running, particularly as the issue at this point is quite likely one with Cumulocity.

Comment on lines +448 to +645
let (server, port) = axum_server();
tokio::spawn(server.serve(test_app.into_make_service()));
let proxy_port = start_server_port(port, vec!["outdated token", "correct token"]);
let (mut ws, _) = connect_to_websocket_port(proxy_port).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a block of code repeated in the previous test as well, that can be moved to a test setup function that takes the Router definition as an argument.

crates/extensions/c8y_auth_proxy/src/actor.rs Outdated Show resolved Hide resolved
Signed-off-by: James Rhodes <jarhodes314@gmail.com>
@jarhodes314 jarhodes314 force-pushed the 2591-refactor-the-c8y-remote-access-plugin-to-use-the-c8y-auth-proxy branch from 5217054 to 845480e Compare January 26, 2024 10:46
Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Approved. Working as expected and nicely done.

@jarhodes314 jarhodes314 added this pull request to the merge queue Jan 26, 2024
Merged via the queue into thin-edge:main with commit 9c4bad9 Jan 26, 2024
20 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.

None yet

4 participants