feat(boundary): rate-limit canister improvements (#3291)

Improvements to `rate-limit` canister:
- combined numerous metrics variables into one single struct
`CanisterMetrics`
- added candid compatibility test, which compares interface `.did`
against the actual interface in Rust
- added canister logging, used e.g. to log errors when fetching API
nodes from the registry canister
- use safe `checked_add` to error in case of overflows
- removed some unused deps

---------

Co-authored-by: IDX GitLab Automation <idx@dfinity.org>
Co-authored-by: IDX GitHub Automation <infra+github-automation@dfinity.org>

rs/boundary_node/ic_boundary/src/rate_limiting/fetcher.rs

@@ -130,13 +130,13 @@ impl FetchesRules for CanisterFetcher {
                 let Some(raw) = x.rule_raw else {
                     return Err(anyhow!(
                         "rule with id {} ({:?}) is None",
-                        x.id,
+                        x.rule_id,
                         x.description
                     ));
                 };
 
                 let rule = RateLimitRule::from_bytes_yaml(&raw)
-                    .context(format!("unable to decode raw rule with id {}", x.id))?;
+                    .context(format!("unable to decode raw rule with id {}", x.rule_id))?;
 
                 Ok(rule)
             })
@@ -172,7 +172,7 @@ mod test {
                     is_redacted: false,
                     rules: vec![
                         OutputRule {
-                            id: "foobar".into(),
+                            rule_id: "foobar".into(),
                             incident_id: "barfoo".into(),
                             rule_raw: Some(indoc! {"
                                 canister_id: aaaaa-aa
@@ -183,7 +183,7 @@ mod test {
                             description: None
                         },
                         OutputRule {
-                            id: "foobaz".into(),
+                            rule_id: "foobaz".into(),
                             incident_id: "barfoo".into(),
                             rule_raw: Some(indoc! {"
                                 canister_id: 5s2ji-faaaa-aaaaa-qaaaq-cai
@@ -194,7 +194,7 @@ mod test {
                             description: None
                         },
                         OutputRule {
-                            id: "deadbeef".into(),
+                            rule_id: "deadbeef".into(),
                             incident_id: "barfoo".into(),
                             rule_raw: Some(indoc! {"
                                 canister_id: aaaaa-aa
@@ -242,7 +242,7 @@ mod test {
                     schema_version: SCHEMA_VERSION,
                     is_redacted: false,
                     rules: vec![OutputRule {
-                        id: "foobar".into(),
+                        rule_id: "foobar".into(),
                         incident_id: "barfoo".into(),
                         rule_raw: None,
                         description: None,

rs/boundary_node/rate_limits/BUILD.bazel

@@ -7,14 +7,13 @@ DEPENDENCIES = [
     # Keep sorted.
     "//rs/boundary_node/rate_limits/api:rate_limits_api",
     "//rs/nns/constants",
+    "//rs/rust_canisters/canister_log",
     "//rs/rust_canisters/http_types",
     "@crate_index//:anyhow",
     "@crate_index//:candid",
     "@crate_index//:getrandom",
-    "@crate_index//:hex",
     "@crate_index//:ic-cdk",
     "@crate_index//:ic-cdk-timers",
-    "@crate_index//:ic-metrics-encoder",
     "@crate_index//:ic-stable-structures",
     "@crate_index//:mockall",
     "@crate_index//:prometheus",
@@ -47,6 +46,7 @@ rust_test(
     srcs = glob(["canister/**/*.rs"]),
     crate_name = "rate_limit_canister",
     crate_root = "canister/lib.rs",
+    data = ["canister/interface.did"],
     proc_macro_deps = MACRO_DEPENDENCIES,
-    deps = DEPENDENCIES,
+    deps = DEPENDENCIES + ["@crate_index//:candid_parser"],
 )

rs/boundary_node/rate_limits/Cargo.toml

@@ -10,8 +10,8 @@ documentation.workspace = true
 anyhow = { workspace = true }
 candid = { workspace = true }
 getrandom = { version = "0.2", features = ["custom"] }
-hex = { workspace = true }
 ic-canisters-http-types = { path = "../../rust_canisters/http_types" }
+ic-canister-log = { path = "../../rust_canisters/canister_log" }
 ic-cdk = { workspace = true }
 ic-cdk-macros = { workspace = true }
 ic-cdk-timers = { workspace = true }
@@ -29,6 +29,7 @@ thiserror = { workspace = true }
 uuid = { workspace = true, features = ['serde', 'v4'] }
 
 [dev-dependencies]
+candid_parser = { workspace = true }
 rate-limit-canister-integration-tests = { path = "./integration_tests" }
 
 [lib]

rs/boundary_node/rate_limits/api/src/lib.rs

@@ -108,15 +108,15 @@ pub struct InputRule {
 
 #[derive(CandidType, Deserialize, Debug, PartialEq)]
 pub struct OutputRule {
-    pub id: RuleId,
+    pub rule_id: RuleId,
     pub incident_id: IncidentId,
     pub rule_raw: Option<Vec<u8>>,
     pub description: Option<String>,
 }
 
 #[derive(CandidType, Deserialize, Debug, PartialEq)]
 pub struct OutputRuleMetadata {
-    pub id: RuleId,
+    pub rule_id: RuleId,
     pub incident_id: IncidentId,
     pub rule_raw: Option<Vec<u8>>,
     pub description: Option<String>,
@@ -158,7 +158,7 @@ impl std::fmt::Display for OutputConfig {
         writeln!(f, "{INDENT}Is redacted: {}", self.is_redacted)?;
         for (i, rule) in self.rules.iter().enumerate() {
             writeln!(f, "{DOUBLE_INDENT}Rule {}:", i + 1)?;
-            writeln!(f, "{DOUBLE_INDENT}ID: {}", rule.id)?;
+            writeln!(f, "{DOUBLE_INDENT}ID: {}", rule.rule_id)?;
             writeln!(f, "{DOUBLE_INDENT}Incident ID: {}", rule.incident_id)?;
             if let Some(ref description) = rule.description {
                 writeln!(f, "{DOUBLE_INDENT}Description: {description}")?;
@@ -175,7 +175,7 @@ impl std::fmt::Display for OutputConfig {
 impl std::fmt::Display for OutputRuleMetadata {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         writeln!(f, "\nOutputRuleMetadata")?;
-        writeln!(f, "{INDENT}ID: {}", self.id)?;
+        writeln!(f, "{INDENT}ID: {}", self.rule_id)?;
         writeln!(
             f,
             "{INDENT}Disclosed at: {}",

rs/boundary_node/rate_limits/canister/add_config.rs

@@ -90,7 +90,11 @@ impl<A: CanisterApi> AddsConfig for ConfigAdder<A> {
             AddConfigError::Internal(anyhow!("No config for version={current_version} found"))
         })?;
 
-        let next_version = current_version + 1;
+        let next_version = current_version.checked_add(1).ok_or_else(|| {
+            AddConfigError::Internal(anyhow!(
+                "Overflow occurred while incrementing the current version {current_version}"
+            ))
+        })?;
 
         // Ordered IDs of all rules in the submitted config
         let mut rule_ids = Vec::<RuleId>::new();

rs/boundary_node/rate_limits/canister/canister.rs

@@ -5,12 +5,13 @@ use crate::confidentiality_formatting::{
 };
 use crate::disclose::{DisclosesRules, RulesDiscloser};
 use crate::getter::{ConfigGetter, EntityGetter, IncidentGetter, RuleGetter};
+use crate::logs::{self, Log, LogEntry, Priority, P0};
 use crate::metrics::{
-    export_metrics_as_http_response, with_metrics_registry, WithMetrics, LAST_CANISTER_CHANGE_TIME,
-    LAST_SUCCESSFUL_REGISTRY_POLL_TIME, REGISTRY_POLL_CALLS_COUNTER,
+    export_metrics_as_http_response, with_metrics_registry, WithMetrics, METRICS,
 };
 use crate::state::{init_version_and_config, with_canister_state, CanisterApi};
 use candid::Principal;
+use ic_canister_log::{export as export_logs, log};
 use ic_canisters_http_types::{HttpRequest, HttpResponse, HttpResponseBuilder};
 use ic_cdk::api::call::call;
 use ic_cdk_macros::{init, inspect_message, post_upgrade, query, update};
@@ -20,7 +21,7 @@ use rate_limits_api::{
     GetApiBoundaryNodeIdsRequest, GetConfigResponse, GetRuleByIdResponse,
     GetRulesByIncidentIdResponse, IncidentId, InitArg, InputConfig, RuleId, Version,
 };
-use std::{sync::Arc, time::Duration};
+use std::{borrow::BorrowMut, str::FromStr, sync::Arc, time::Duration};
 
 const REGISTRY_CANISTER_METHOD: &str = "get_api_boundary_node_ids";
 const UPDATE_METHODS: [&str; 2] = ["add_config", "disclose_rules"];
@@ -84,8 +85,11 @@ fn init(init_arg: InitArg) {
         );
     });
     // Update metric.
-    LAST_CANISTER_CHANGE_TIME.with(|cell| {
-        cell.borrow_mut().set(current_time as i64);
+    METRICS.with(|cell| {
+        let mut cell = cell.borrow_mut();
+        cell.last_canister_change_time
+            .borrow_mut()
+            .set(current_time as i64);
     });
 }
 
@@ -146,14 +150,14 @@ fn get_rules_by_incident_id(incident_id: IncidentId) -> GetRulesByIncidentIdResp
 fn add_config(config: InputConfig) -> AddConfigResponse {
     let caller_id = ic_cdk::api::caller();
     let current_time = ic_cdk::api::time();
-    let result = with_canister_state(|state| {
+    with_canister_state(|state| {
         let access_resolver = AccessLevelResolver::new(caller_id, state.clone());
         let adder = ConfigAdder::new(state);
         let adder = WithAuthorization::new(adder, access_resolver);
         let adder = WithMetrics::new(adder);
         adder.add_config(config, current_time)
     })?;
-    Ok(result)
+    Ok(())
 }
 
 /// Makes specified rules publicly accessible for viewing
@@ -164,14 +168,14 @@ fn add_config(config: InputConfig) -> AddConfigResponse {
 fn disclose_rules(args: DiscloseRulesArg) -> DiscloseRulesResponse {
     let caller_id = ic_cdk::api::caller();
     let disclose_time = ic_cdk::api::time();
-    let result = with_canister_state(|state| {
+    with_canister_state(|state| {
         let access_resolver = AccessLevelResolver::new(caller_id, state.clone());
         let discloser = RulesDiscloser::new(state);
         let discloser = WithAuthorization::new(discloser, access_resolver);
         let discloser = WithMetrics::new(discloser);
         discloser.disclose_rules(args, disclose_time)
     })?;
-    Ok(result)
+    Ok(())
 }
 
 #[query(decoding_quota = 10000)]
@@ -180,6 +184,50 @@ fn http_request(request: HttpRequest) -> HttpResponse {
         "/metrics" => with_canister_state(|state| {
             with_metrics_registry(|registry| export_metrics_as_http_response(registry, state))
         }),
+        "/logs" => {
+            use serde_json;
+
+            let max_skip_timestamp = match request.raw_query_param("time") {
+                Some(arg) => match u64::from_str(arg) {
+                    Ok(value) => value,
+                    Err(_) => {
+                        return HttpResponseBuilder::bad_request()
+                            .with_body_and_content_length("failed to parse the 'time' parameter")
+                            .build()
+                    }
+                },
+                None => 0,
+            };
+
+            let mut entries: Log = Default::default();
+            for entry in export_logs(&logs::P0) {
+                entries.entries.push(LogEntry {
+                    timestamp: entry.timestamp,
+                    counter: entry.counter,
+                    priority: Priority::P0,
+                    file: entry.file.to_string(),
+                    line: entry.line,
+                    message: entry.message,
+                });
+            }
+            for entry in export_logs(&logs::P1) {
+                entries.entries.push(LogEntry {
+                    timestamp: entry.timestamp,
+                    counter: entry.counter,
+                    priority: Priority::P1,
+                    file: entry.file.to_string(),
+                    line: entry.line,
+                    message: entry.message,
+                });
+            }
+            entries
+                .entries
+                .retain(|entry| entry.timestamp >= max_skip_timestamp);
+            HttpResponseBuilder::ok()
+                .header("Content-Type", "application/json; charset=utf-8")
+                .with_body_and_content_length(serde_json::to_string(&entries).unwrap_or_default())
+                .build()
+        }
         _ => HttpResponseBuilder::not_found().build(),
     }
 }
@@ -205,22 +253,98 @@ fn periodically_poll_api_boundary_nodes(interval: u64, canister_api: Arc<dyn Can
                         canister_api.set_api_boundary_nodes_principals(
                             api_bn_records.into_iter().filter_map(|n| n.id).collect(),
                         );
-                        // Update metrics.
+                        // Update metric.
                         let current_time = ic_cdk::api::time() as i64;
-                        LAST_SUCCESSFUL_REGISTRY_POLL_TIME.with(|cell| {
-                            cell.borrow_mut().set(current_time);
+                        METRICS.with(|cell| {
+                            let mut cell = cell.borrow_mut();
+                            cell.last_successful_registry_poll_time
+                                .borrow_mut()
+                                .set(current_time);
                         });
                         ("success", "")
                     }
-                    Ok((Err(_),)) => ("failure", "calling_canister_method_failed"),
-                    Err(_) => ("failure", "canister_call_rejected"),
+                    Ok((Err(err),)) => {
+                        log!(
+                        P0,
+                        "[poll_api_boundary_nodes]: failed to fetch nodes from registry {err:?}",
+                    );
+                        ("failure", "calling_canister_method_failed")
+                    }
+                    Err(err) => {
+                        log!(
+                        P0,
+                        "[poll_api_boundary_nodes]: failed to fetch nodes from registry {err:?}",
+                    );
+                        ("failure", "canister_call_rejected")
+                    }
                 };
 
             // Update metric.
-            REGISTRY_POLL_CALLS_COUNTER.with(|cell| {
-                let metric = cell.borrow_mut();
-                metric.with_label_values(&[call_status, message]).inc();
+            METRICS.with(|cell| {
+                let mut cell = cell.borrow_mut();
+                cell.registry_poll_calls
+                    .borrow_mut()
+                    .with_label_values(&[call_status, message])
+                    .inc();
             });
         });
     });
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn check_candid_interface_compatibility() {
+        use candid_parser::utils::{service_equal, CandidSource};
+
+        fn source_to_str(source: &CandidSource) -> String {
+            match source {
+                CandidSource::File(f) => {
+                    std::fs::read_to_string(f).unwrap_or_else(|_| "".to_string())
+                }
+                CandidSource::Text(t) => t.to_string(),
+            }
+        }
+
+        fn check_service_equal(
+            new_name: &str,
+            new: CandidSource,
+            old_name: &str,
+            old: CandidSource,
+        ) {
+            let new_str = source_to_str(&new);
+            let old_str = source_to_str(&old);
+            match service_equal(new, old) {
+                Ok(_) => {}
+                Err(e) => {
+                    eprintln!(
+                        "{} is not compatible with {}!\n\n\
+                {}:\n\
+                {}\n\n\
+                {}:\n\
+                {}\n",
+                        new_name, old_name, new_name, new_str, old_name, old_str
+                    );
+                    panic!("{:?}", e);
+                }
+            }
+        }
+
+        candid::export_service!();
+
+        let new_interface = __export_service();
+
+        // check the public interface against the actual one
+        let old_interface = std::path::PathBuf::from(std::env::var("CARGO_MANIFEST_DIR").unwrap())
+            .join("canister/interface.did");
+
+        check_service_equal(
+            "actual rate-limit candid interface",
+            candid_parser::utils::CandidSource::Text(&new_interface),
+            "declared candid interface in interface.did file",
+            candid_parser::utils::CandidSource::File(old_interface.as_path()),
+        );
+    }
+}