-
Notifications
You must be signed in to change notification settings - Fork 313
Add support for event hub transport layer proxy #3051
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
base: main
Are you sure you want to change the base?
Conversation
Thank you for your contribution @k-shlomi! We will review the pull request and get back to you soon. |
@microsoft-github-policy-service agree company="Sweet Security" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few notes, but @LarryOsterman should definitely have a look. Thanks for the contribution!
sdk/core/azure_core_amqp/Cargo.toml
Outdated
tokio-socks = "0.5" | ||
tokio-native-tls = "0.3" | ||
native-tls = "0.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be defined as part of an optional feature for anyone to enable who wants to use the protocol. Most customers likely won't need it and the increased build time required.
@LarryOsterman we'd also want to consider whether these should be declared directly herein or in the workspace Cargo.toml
. The only net new dependency is tokio-socks
, but I suspect that - at least for now - AMQP is the only protocol that would support it. The others are already in the Cargo.lock
file and should just be in the workspace if they even need to be declared explicitly. [email protected]
already has a dependency on [email protected]
and [email protected]
so they probably don't need to be in here at all.
// Copyright (c) Microsoft Corporation. All Rights reserved | ||
// Licensed under the MIT license. | ||
|
||
//! SOCKS5 proxy support for Azure AMQP connections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great module documentation, but given its not pub
when imported in src/lib/rs
, no one is going to see it. Necessary information might be better served in the crate README.md
.
sdk/core/azure_core_amqp/README.md
Outdated
let mut options = AmqpConnectionOptions::default(); | ||
options.custom_endpoint = Some(Url::parse("socks5h://proxy.corp.com:8080")?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally prefer the construction pattern:
let options = AmqpConnectionOptions {
custom_endpoint = Some("socks5h://proxy.contoso.com:8080".parse()?),
..Default::default()
};
And use contoso.com or example.com.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also means azure_core::http::Url
doesn't need to be imported.
azure_core::Error::new(azure_core::error::ErrorKind::Other, Box::new(e)).with_context( | ||
format!( | ||
"SOCKS5 connection failed: proxy={}, target={}", | ||
Self::mask_credentials(proxy_url), | ||
target_url | ||
), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Error::with_error
:
azure_core::Error::new(azure_core::error::ErrorKind::Other, Box::new(e)).with_context( | |
format!( | |
"SOCKS5 connection failed: proxy={}, target={}", | |
Self::mask_credentials(proxy_url), | |
target_url | |
), | |
) | |
azure_core::Error::with_error(azure_core::error::ErrorKind::Other, e, | |
format!( | |
"SOCKS5 connection failed: proxy={}, target={}", | |
Self::mask_credentials(proxy_url), | |
target_url | |
), | |
) |
(Something like that)
/azp run rust - eventhubs - weekly |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I'm singularly impressed. I have one significant concern, which is that I believe that these new tests will fail in our weekly test runs because we run our tests with --all-features
and the CI pipeline doesn't have a SOCKS5 proxy available to work.
Also I'd like to have someone from the EventHubs service team take a look to confirm that they want SOCKS5 proxy support as a feature (it's complicated).
/// This function is automatically used in all logging statements within | ||
/// the SOCKS5 implementation. Manual usage is typically not required | ||
/// unless implementing custom logging or error handling. | ||
pub(crate) fn mask_credentials(url: &Url) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to consider using the azure_core extension method sanitize
here - it implements our standardized sanitization logic. You would still need to redact the username and password since azure URLs are prohibited from using the URL username and password fields, but this adds an additional level of sanity checks on the URL.
Basically change the last line to masked.sanitize(&[])
/// | ||
/// This function replaces username and password in the proxy URL with asterisks | ||
/// to prevent credential exposure in log files. | ||
fn mask_proxy_credentials(url: &str) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about using the sanitize()
extension for the rest of the URL.
// Copyright (c) Microsoft Corporation. All Rights reserved | ||
// Licensed under the MIT license. | ||
|
||
#![cfg(feature = "socks5")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our tests are built with --all-features
- how do you ensure that these tests don't fail in our live tests CI pipeline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these tests should move to examples...which you already have, so if the examples are sufficient, I say we remove these integration tests if we don't have any easy way to test them on devs' machines (we don't control nor dictate how they run tests) or hosted agents.
ed8db60
to
2ece6c7
Compare
Hi @heaths and @LarryOsterman |
@weshaggard @hallipr what's going on with the vcpkg install on Windows? From the logs,
We're using the same setup that other repos are using, so did something break recently? |
@k-shlomi please address my comments as well. See our root |
@heaths I believe I did fix your comments. can you point me out to comments I missed? |
@danieljurek is investigating the vcpkg issue. |
Notes from investigation so far:
|
There was a breaking change to binary caching for vcpkg in this version. I have a fix working locally... PR coming soon |
Fix is in. Rebase on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't take direct dependencies in a crate. There are also some sanitization concerns.
tokio-native-tls = { version = "0.3", optional = true } | ||
tracing.workspace = true | ||
typespec = { workspace = true, features = ["amqp"] } | ||
typespec_client_core = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything in here is in azure_core
. azure_*
crates outside of core should not reference typespec
crates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@heaths I referenced it in sdk/eventhubs/azure_messaging_eventhubs/Cargo.toml in order to use the sanitize. any suggestion about that? remove sanitize? leave it in the file? what is the best course of action here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm saying that the sanitization function should be exported in azure_core
as well, in the same module (hopefully, unless we missed something). For any azure_*
crates we're trying to "hide" the typespec crates as an implementation detail. Everything they export is imported and re-exported by azure_core
in the same module path. A couple types we "override", but internally make convertible to the types they "override" in typespec.
sdk/core/azure_core_amqp/Cargo.toml
Outdated
tokio-socks = { version = "0.5", optional = true } | ||
native-tls = { version = "0.2", optional = true } | ||
tokio-native-tls = { version = "0.3", optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 of these don't need to be referenced directly as we already have dependencies. The other has to be defined in the workspace /Cargo.toml
. We don't allow direct dependencies unless @LarryOsterman thinks is worth exempting a version dependency in our appropriate place. This is to avoid version mismatches throughout the workspace - especially import as the number of crates grows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
"serde_amqp", | ||
"serde_bytes", | ||
] | ||
socks5 = ["dep:tokio-socks", "dep:native-tls", "dep:tokio-native-tls"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need to add this to the docs.rs metadata table below or it won't be documented. We intentionally don't document the others as they are meant more for internal testing / development.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
727fc0d
to
93fe99f
Compare
@heaths one question for you in regards to your last comments. |
Adds SOCKS5 proxy support for Rust EventHubs client, enabling connections through corporate proxies and restricted network environments.
Changes
Core Implementation
Dependencies
Documentation & Examples
Usage
Resolves enterprise customer requirements for proxy support in restricted network environments.