-
Notifications
You must be signed in to change notification settings - Fork 193
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
Enable stalled stream protection for uploads behind new behavior version #3527
Changes from all commits
d9d3a2f
477d1d5
e72da5e
df57d1e
3f3ac63
6d25692
4dac882
f7731ba
a21b6d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,17 +3,23 @@ | |
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
//! Behavior Major version of the client | ||
//! Behavior version of the client | ||
|
||
/// Behavior major-version of the client | ||
/// Behavior version of the client | ||
/// | ||
/// Over time, new best-practice behaviors are introduced. However, these behaviors might not be | ||
/// backwards compatible. For example, a change which introduces new default timeouts or a new | ||
/// retry-mode for all operations might be the ideal behavior but could break existing applications. | ||
#[derive(Clone)] | ||
#[derive(Copy, Clone, PartialEq)] | ||
pub struct BehaviorVersion { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably add a section to the developer guide on behavior versions, what they are and guidance on how to choose, and what changed in each. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
// currently there is only 1 MV so we don't actually need anything in here. | ||
_private: (), | ||
inner: Inner, | ||
} | ||
|
||
#[derive(Copy, Clone, Debug, Ord, PartialOrd, Eq, PartialEq)] | ||
enum Inner { | ||
// IMPORTANT: Order matters here for the `Ord` derive. Newer versions go to the bottom. | ||
V2023_11_09, | ||
V2024_03_28, | ||
} | ||
|
||
impl BehaviorVersion { | ||
|
@@ -26,23 +32,60 @@ impl BehaviorVersion { | |
/// If, however, you're writing a service that is very latency sensitive, or that has written | ||
/// code to tune Rust SDK behaviors, consider pinning to a specific major version. | ||
/// | ||
/// The latest version is currently [`BehaviorVersion::v2023_11_09`] | ||
/// The latest version is currently [`BehaviorVersion::v2024_03_28`] | ||
pub fn latest() -> Self { | ||
Self::v2023_11_09() | ||
Self::v2024_03_28() | ||
} | ||
|
||
/// This method returns the behavior configuration for November 9th, 2023 | ||
/// Behavior version for March 28th, 2024. | ||
/// | ||
/// This version enables stalled stream protection for uploads (request bodies) by default. | ||
/// | ||
/// When a new behavior major version is released, this method will be deprecated. | ||
pub fn v2024_03_28() -> Self { | ||
Self { | ||
inner: Inner::V2024_03_28, | ||
} | ||
} | ||
|
||
/// Behavior version for November 9th, 2023. | ||
#[deprecated( | ||
since = "1.4.0", | ||
note = "Superceded by v2024_03_28, which enabled stalled stream protection for uploads (request bodies) by default." | ||
)] | ||
pub fn v2023_11_09() -> Self { | ||
Self { _private: () } | ||
Self { | ||
inner: Inner::V2023_11_09, | ||
} | ||
} | ||
|
||
/// True if this version is newer or equal to the given `other` version. | ||
pub fn is_at_least(&self, other: BehaviorVersion) -> bool { | ||
self.inner >= other.inner | ||
} | ||
} | ||
|
||
impl std::fmt::Debug for BehaviorVersion { | ||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
f.debug_struct("BehaviorVersion") | ||
.field("name", &"v2023_11_09") | ||
.finish() | ||
f.debug_tuple("BehaviorVersion").field(&self.inner).finish() | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
|
||
#[test] | ||
#[allow(deprecated)] | ||
fn version_comparison() { | ||
assert!(BehaviorVersion::latest() == BehaviorVersion::latest()); | ||
assert!(BehaviorVersion::v2023_11_09() == BehaviorVersion::v2023_11_09()); | ||
assert!(BehaviorVersion::v2024_03_28() != BehaviorVersion::v2023_11_09()); | ||
assert!(BehaviorVersion::latest().is_at_least(BehaviorVersion::latest())); | ||
assert!(BehaviorVersion::latest().is_at_least(BehaviorVersion::v2023_11_09())); | ||
assert!(BehaviorVersion::latest().is_at_least(BehaviorVersion::v2024_03_28())); | ||
assert!(!BehaviorVersion::v2023_11_09().is_at_least(BehaviorVersion::v2024_03_28())); | ||
assert!(Inner::V2024_03_28 > Inner::V2023_11_09); | ||
assert!(Inner::V2023_11_09 < Inner::V2024_03_28); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -176,10 +176,11 @@ pub fn default_identity_cache_plugin() -> Option<SharedRuntimePlugin> { | |
note = "This function wasn't intended to be public, and didn't take the behavior major version as an argument, so it couldn't be evolved over time." | ||
)] | ||
pub fn default_stalled_stream_protection_config_plugin() -> Option<SharedRuntimePlugin> { | ||
#[allow(deprecated)] | ||
default_stalled_stream_protection_config_plugin_v2(BehaviorVersion::v2023_11_09()) | ||
} | ||
fn default_stalled_stream_protection_config_plugin_v2( | ||
_behavior_version: BehaviorVersion, | ||
behavior_version: BehaviorVersion, | ||
) -> Option<SharedRuntimePlugin> { | ||
Some( | ||
default_plugin( | ||
|
@@ -191,13 +192,13 @@ fn default_stalled_stream_protection_config_plugin_v2( | |
}, | ||
) | ||
.with_config(layer("default_stalled_stream_protection_config", |layer| { | ||
layer.store_put( | ||
StalledStreamProtectionConfig::enabled() | ||
// TODO(https://github.com/smithy-lang/smithy-rs/issues/3510): enable behind new behavior version | ||
.upload_enabled(false) | ||
.grace_period(Duration::from_secs(5)) | ||
.build(), | ||
); | ||
let mut config = | ||
StalledStreamProtectionConfig::enabled().grace_period(Duration::from_secs(5)); | ||
// Before v2024_03_28, upload streams did not have stalled stream protection by default | ||
if !behavior_version.is_at_least(BehaviorVersion::v2024_03_28()) { | ||
config = config.upload_enabled(false); | ||
} | ||
layer.store_put(config.build()); | ||
Comment on lines
194
to
+201
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for debuggability, I wonder if we should alter the name of the plugin. I would probably make two separate plugins that we select based on BMV instead, may be clearer and less likely to accidentally regress. That said, I don't have strong feelings. |
||
})) | ||
.into_shared(), | ||
) | ||
|
@@ -293,3 +294,47 @@ pub fn default_plugins( | |
.flatten() | ||
.collect::<Vec<SharedRuntimePlugin>>() | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
use aws_smithy_runtime_api::client::runtime_plugin::RuntimePlugins; | ||
|
||
fn test_plugin_params(version: BehaviorVersion) -> DefaultPluginParams { | ||
DefaultPluginParams::new() | ||
.with_behavior_version(version) | ||
.with_retry_partition_name("dontcare") | ||
} | ||
fn config_for(plugins: impl IntoIterator<Item = SharedRuntimePlugin>) -> ConfigBag { | ||
let mut config = ConfigBag::base(); | ||
let plugins = RuntimePlugins::new().with_client_plugins(plugins); | ||
plugins.apply_client_configuration(&mut config).unwrap(); | ||
config | ||
} | ||
|
||
#[test] | ||
#[allow(deprecated)] | ||
fn v2024_03_28_stalled_stream_protection_difference() { | ||
let latest = config_for(default_plugins(test_plugin_params( | ||
BehaviorVersion::latest(), | ||
))); | ||
let v2023 = config_for(default_plugins(test_plugin_params( | ||
BehaviorVersion::v2023_11_09(), | ||
))); | ||
|
||
assert!( | ||
latest | ||
.load::<StalledStreamProtectionConfig>() | ||
.unwrap() | ||
.upload_enabled(), | ||
"stalled stream protection on uploads MUST be enabled after v2024_03_28" | ||
); | ||
assert!( | ||
!v2023 | ||
.load::<StalledStreamProtectionConfig>() | ||
.unwrap() | ||
.upload_enabled(), | ||
"stalled stream protection on uploads MUST NOT be enabled before v2024_03_28" | ||
); | ||
} | ||
} |
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.
should we copy one of these tests, use the older BMV, and validate that stalled stream upload protection is not enabled?