-
Notifications
You must be signed in to change notification settings - Fork 400
feat: CON-1639 HTTPS outcalls pay-as-you-go and dark launch budget trackers #10519
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: master
Are you sure you want to change the base?
Changes from 21 commits
4701dfd
7d7cd45
0ec21dc
fe96484
cf87d44
4923d68
abe2b56
77e6485
937a9bc
e265c2f
dd28e15
a26a9c3
3631e42
37abaff
2930059
ed19c45
cc6be29
51d64d4
2e7b712
d8ec86a
1d5d749
5ed23a8
5132054
35c202a
b8df347
a91a45b
591ecec
4773807
6d32da5
22c5465
ebd2a9b
608b767
81c0f81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,15 +15,17 @@ use ic_logger::{ReplicaLogger, info, warn}; | |
| use ic_management_canister_types_private::{CanisterHttpResponsePayload, TransformArgs}; | ||
| use ic_metrics::MetricsRegistry; | ||
| use ic_types::{ | ||
| CanisterId, NumBytes, NumInstructions, | ||
| CanisterId, CountBytes, NumBytes, NumInstructions, | ||
| canister_http::{ | ||
| CanisterHttpHeader, CanisterHttpMethod, CanisterHttpPaymentReceipt, CanisterHttpReject, | ||
| CanisterHttpRequest, CanisterHttpRequestContext, CanisterHttpResponse, | ||
| CanisterHttpResponseContent, Transform, validate_http_headers_and_body, | ||
| CanisterHttpResponseContent, MAXIMUM_ALLOWED_ERROR_MESSAGE_BYTES, Transform, | ||
| validate_http_headers_and_body, | ||
| }, | ||
| ingress::WasmResult, | ||
| messages::{Query, QuerySource, Request}, | ||
| }; | ||
| use ic_utils::str::StrEllipsize; | ||
| use std::{ | ||
| sync::{Arc, atomic::AtomicU64}, | ||
| time::{Duration, Instant}, | ||
|
|
@@ -65,6 +67,7 @@ pub struct CanisterHttpAdapterClientImpl { | |
| rx: Receiver<(CanisterHttpResponse, CanisterHttpPaymentReceipt)>, | ||
| query_service: TransformExecutionService, | ||
| metrics: Metrics, | ||
| pricing_factory: PricingFactory, | ||
| log: ReplicaLogger, | ||
| } | ||
|
|
||
|
|
@@ -79,13 +82,15 @@ impl CanisterHttpAdapterClientImpl { | |
| ) -> Self { | ||
| let (tx, rx) = channel(inflight_requests); | ||
| let metrics = Metrics::new(&metrics_registry); | ||
| let pricing_factory = PricingFactory::new(&metrics_registry, log.clone()); | ||
| Self { | ||
| rt_handle, | ||
| grpc_channel, | ||
| tx, | ||
| rx, | ||
| query_service, | ||
| metrics, | ||
| pricing_factory, | ||
| log, | ||
| } | ||
| } | ||
|
|
@@ -121,6 +126,7 @@ impl NonBlockingChannel<CanisterHttpRequest> for CanisterHttpAdapterClientImpl { | |
| let mut http_adapter_client = HttpsOutcallsServiceClient::new(self.grpc_channel.clone()); | ||
| let query_handler = self.query_service.clone(); | ||
| let metrics = self.metrics.clone(); | ||
| let pricing_factory = self.pricing_factory.clone(); | ||
| let log = self.log.clone(); | ||
|
|
||
| // Spawn an async task that sends the canister http request to the adapter and awaits the response. | ||
|
|
@@ -131,9 +137,12 @@ impl NonBlockingChannel<CanisterHttpRequest> for CanisterHttpAdapterClientImpl { | |
| id: request_id, | ||
| context: request_context, | ||
| socks_proxy_addrs, | ||
| cost_schedule, | ||
| subnet_size, | ||
| } = canister_http_request; | ||
|
|
||
| let mut budget = PricingFactory::new_tracker(&request_context); | ||
| let mut budget = | ||
| pricing_factory.new_tracker(&request_context, subnet_size, cost_schedule); | ||
| let request_size = request_context.variable_parts_size(); | ||
|
|
||
| let CanisterHttpRequestContext { | ||
|
|
@@ -177,7 +186,7 @@ impl NonBlockingChannel<CanisterHttpRequest> for CanisterHttpAdapterClientImpl { | |
| return; | ||
| } | ||
|
|
||
| let payload = async { | ||
| let mut payload = async { | ||
| // Execute the HTTP request and get the adapter response. | ||
| let (adapter_response, downloaded_bytes, elapsed) = execute_http_request( | ||
| &mut http_adapter_client, | ||
|
|
@@ -262,6 +271,38 @@ impl NonBlockingChannel<CanisterHttpRequest> for CanisterHttpAdapterClientImpl { | |
| } | ||
| .await; | ||
|
|
||
| // Truncate an oversized reject message before pricing and gossiping | ||
| // it, so the gossip cost reflects what is actually gossiped. | ||
| if let Err(reject) = &mut payload | ||
| && reject.message.len() > MAXIMUM_ALLOWED_ERROR_MESSAGE_BYTES | ||
| { | ||
| warn!( | ||
| log, | ||
| "Pruning oversized reject message for request {}: \ | ||
| original size {}, new size {}", | ||
| request_id, | ||
| reject.message.len(), | ||
| MAXIMUM_ALLOWED_ERROR_MESSAGE_BYTES, | ||
| ); | ||
| reject.message = reject | ||
| .message | ||
| .ellipsize(MAXIMUM_ALLOWED_ERROR_MESSAGE_BYTES, 90); | ||
| } | ||
|
|
||
| // Account for the cost of gossiping the final (post-transform) | ||
| // response to peers before creating the receipt. | ||
| let response_size = match &payload { | ||
| Ok(response) => response.len(), | ||
| Err(reject) => reject.count_bytes(), | ||
| }; | ||
| let payload = budget | ||
| .subtract_gossip_usage(NumBytes::from(response_size as u64)) | ||
| .map_err(|PricingError::InsufficientCycles| CanisterHttpReject { | ||
| reject_code: RejectCode::SysFatal, | ||
|
mraszyk marked this conversation as resolved.
Outdated
|
||
| message: "Insufficient cycles".to_string(), | ||
| }) | ||
| .and(payload); | ||
|
|
||
| // Create the payment receipt after all processing is complete. | ||
| let receipt = budget.create_payment_receipt(); | ||
|
|
||
|
|
@@ -553,6 +594,7 @@ where | |
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use ic_https_outcalls_pricing::CanisterCyclesCostSchedule; | ||
| use ic_https_outcalls_service::{ | ||
| HttpsOutcallRequest, HttpsOutcallResponse, HttpsOutcallResult, | ||
| https_outcalls_service_server::{HttpsOutcallsService, HttpsOutcallsServiceServer}, | ||
|
|
@@ -561,7 +603,7 @@ mod tests { | |
| use ic_logger::replica_logger::no_op_logger; | ||
| use ic_test_utilities_types::messages::RequestBuilder; | ||
| use ic_types::{ | ||
| RegistryVersion, | ||
| NumberOfNodes, RegistryVersion, | ||
| canister_http::{ | ||
| MAX_CANISTER_HTTP_RESPONSE_BYTES, PricingVersion, RefundStatus, Replication, Transform, | ||
| }, | ||
|
|
@@ -660,6 +702,8 @@ mod tests { | |
| registry_version: RegistryVersion::from(1), | ||
| }, | ||
| socks_proxy_addrs: vec![], | ||
| cost_schedule: CanisterCyclesCostSchedule::Normal, | ||
| subnet_size: NumberOfNodes::from(13), | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1128,6 +1172,71 @@ mod tests { | |
| assert_eq!(client.try_receive(), Err(TryReceiveError::Empty)); | ||
| } | ||
|
|
||
| // Test that an oversized reject message is truncated (char-boundary-safe) | ||
| // before being returned, so that what is priced and gossiped is bounded. | ||
| #[tokio::test] | ||
| async fn test_oversized_reject_message_is_truncated() { | ||
| // Adapter mock setup. Not relevant; the transform produces the reject. | ||
| let response = HttpsOutcallResponse { | ||
| status: 200, | ||
| headers: Vec::new(), | ||
| content: Vec::new(), | ||
| }; | ||
| let mock_grpc_channel = setup_adapter_mock(Ok(create_result_from_response(response))).await; | ||
| let (svc, mut handle) = setup_system_query_mock(); | ||
|
|
||
| // A multi-byte message whose 1200 bytes exceed the 1024-byte limit, with | ||
| // emoji straddling the truncation boundary to exercise char safety. | ||
| let oversized_message = "😀".repeat(300); | ||
|
Contributor
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. I don't think this crosses the boundary of 1024 chars.
Contributor
Author
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. Why not? There is an assert just below saying that it is
Contributor
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. What I mean is that the emoji is encoded using 4 bytes and the byte limit of 1024 bytes is an integer multiple of that so we don't exercise the case of the emoji crossing the byte limit (in this case truncating to exactly 1024 bytes would cut the emoji encoding and the blob won't be a valid string anymore). |
||
| let oversized_len = oversized_message.len(); | ||
| assert!(oversized_len > MAXIMUM_ALLOWED_ERROR_MESSAGE_BYTES); | ||
| // The client must apply exactly this truncation before pricing the response. | ||
| let expected_message = oversized_message.ellipsize(MAXIMUM_ALLOWED_ERROR_MESSAGE_BYTES, 90); | ||
| tokio::spawn(async move { | ||
| let (_, rsp) = handle.next_request().await.unwrap(); | ||
| rsp.send_response(Ok(( | ||
| Ok(WasmResult::Reject(oversized_message)), | ||
| current_time(), | ||
| ))); | ||
| }); | ||
|
|
||
| let mut client = CanisterHttpAdapterClientImpl::new( | ||
| tokio::runtime::Handle::current(), | ||
| mock_grpc_channel, | ||
| svc, | ||
| 100, | ||
| MetricsRegistry::default(), | ||
| no_op_logger(), | ||
| ); | ||
|
|
||
| assert_eq!( | ||
| client.send(build_mock_canister_http_request( | ||
| 420, | ||
| Some("transform".to_string()) | ||
| )), | ||
| Ok(()) | ||
| ); | ||
| loop { | ||
| match client.try_receive() { | ||
| Err(_) => tokio::time::sleep(Duration::from_millis(10)).await, | ||
| Ok((r, _payment_receipt)) => { | ||
| let CanisterHttpResponseContent::Reject(reject) = r.content else { | ||
| panic!("expected a reject response"); | ||
| }; | ||
| // Ellipsized exactly as the limit dictates: this pins the size, | ||
| // the ellipsize parameters, and char-boundary safety in one check. | ||
| assert_eq!( | ||
| reject.message, expected_message, | ||
| "reject message should be ellipsized to the allowed size" | ||
| ); | ||
| assert!(reject.message.len() <= MAXIMUM_ALLOWED_ERROR_MESSAGE_BYTES); | ||
| assert!(reject.message.len() < oversized_len); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Test client capacity. The capicity of the client is specified by the channel size. | ||
| #[tokio::test] | ||
| async fn test_client_at_capacity() { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.