-
Notifications
You must be signed in to change notification settings - Fork 0
[sample #10736] eBPF: Hybrid conntrack cleaner #199
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: coc-sample-10736-base
Are you sure you want to change the base?
Changes from all commits
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 | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -24,229 +24,66 @@ struct ct_iter_ctx { | |||||||||
| __u64 now; | ||||||||||
| __u64 end_time; | ||||||||||
|
|
||||||||||
| __u64 num_kvs_seen_normal; | ||||||||||
| __u64 num_kvs_seen_nat_fwd; | ||||||||||
| __u64 num_kvs_seen_nat_rev; | ||||||||||
| __u64 num_kvs_deleted_normal; | ||||||||||
| __u64 num_kvs_deleted_nat_fwd; | ||||||||||
| __u64 num_kvs_deleted_nat_rev; | ||||||||||
| __u64 num_cleaned; | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| // sub_age calculates now-then assuming that the difference is less than | ||||||||||
| // 1<<63. Values larger than that are assumed to have wrapped (then>now) and | ||||||||||
| // 0 is returned in that case. | ||||||||||
| static __u64 sub_age(__u64 now, __u64 then) | ||||||||||
| { | ||||||||||
| __u64 age = now - then; | ||||||||||
| if (age > (1ull<<63)) { | ||||||||||
| // Wrapped, assume that means then > now. | ||||||||||
| return 0; | ||||||||||
| } | ||||||||||
| return age; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| #define CT_VALUE_FINS_SEEN_DSR(value) (value->a_to_b.fin_seen || value->b_to_a.fin_seen) | ||||||||||
| #define CT_VALUE_FINS_SEEN_NON_DSR(value) (value->a_to_b.fin_seen && value->b_to_a.fin_seen) | ||||||||||
| #define CT_VALUE_ESTABLISHED(value) (value->a_to_b.syn_seen && value->a_to_b.ack_seen && value->b_to_a.syn_seen && value->b_to_a.ack_seen) | ||||||||||
| #define CT_VALUE_DSR(value) (value->flags & CALI_CT_FLAG_DSR_FWD) | ||||||||||
|
|
||||||||||
| // max_age returns the maximum age for the given conntrack "tracking" entry. | ||||||||||
| static __u64 calculate_max_age(const struct calico_ct_key *key, const struct calico_ct_value *value) | ||||||||||
| { | ||||||||||
| __u64 max_age; | ||||||||||
| switch (key->protocol) { | ||||||||||
| case IPPROTO_TCP: | ||||||||||
| if (value->a_to_b.rst_seen || value->b_to_a.rst_seen) { | ||||||||||
| max_age = __globals.tcp_reset_seen; | ||||||||||
| } else if ( | ||||||||||
| (CT_VALUE_DSR(value) && CT_VALUE_FINS_SEEN_DSR(value)) || | ||||||||||
| CT_VALUE_FINS_SEEN_NON_DSR(value) | ||||||||||
| ) { | ||||||||||
| max_age = __globals.tcp_fins_seen; | ||||||||||
| } else if (CT_VALUE_ESTABLISHED(value) || CT_VALUE_DSR(value)) { | ||||||||||
| if (value->rst_seen) { | ||||||||||
| /* We have seen RST in the past, but we have seen traffic | ||||||||||
| * since then so we want to be cautious and not tear down | ||||||||||
| * the conntrack too soon in case the RST was spurious, | ||||||||||
| * but we are also not sure if the connection is still | ||||||||||
| * established. | ||||||||||
| */ | ||||||||||
| max_age = __globals.tcp_fins_seen; | ||||||||||
| } else { | ||||||||||
| max_age = __globals.tcp_established; | ||||||||||
| } | ||||||||||
| } else { | ||||||||||
| max_age = __globals.tcp_syn_sent; | ||||||||||
| } | ||||||||||
| break; | ||||||||||
| case IPPROTO_UDP: | ||||||||||
| max_age = __globals.udp_timeout; | ||||||||||
| break; | ||||||||||
| case IPPROTO_ICMP_46: | ||||||||||
| max_age = __globals.icmp_timeout; | ||||||||||
| break; | ||||||||||
| default: | ||||||||||
| max_age = __globals.generic_timeout; | ||||||||||
| break; | ||||||||||
| } | ||||||||||
| CALI_DEBUG("max_age %d", max_age / 1000000000); | ||||||||||
| return max_age; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| static bool entry_expired(const struct calico_ct_key *key, const struct calico_ct_value *value, struct ct_iter_ctx *ctx) | ||||||||||
| { | ||||||||||
| __u64 age = sub_age(ctx->now, value->last_seen); | ||||||||||
| __u64 max_age = calculate_max_age(key, value); | ||||||||||
| return age > max_age; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // process_ct_entry callback function for the conntrack map iteration. Checks | ||||||||||
| // the entry for expiry. Expired normal entries are deleted inline. Expired | ||||||||||
| // NAT entries are queued for deletion in the cali_ccq map. | ||||||||||
| static long process_ct_entry(void *map, const void *key, void *value, void *ctx) | ||||||||||
| // process_ccq_entry processes an entry in the "cleanup queue" map. The map | ||||||||||
| // is keyed on the reverse NAT entry's key, with the value storing the forward | ||||||||||
| // entry's key (or a zero value if the forward entry is missing). | ||||||||||
|
Comment on lines
+30
to
+32
|
||||||||||
| static long process_ccq_entry(void *map, struct calico_ct_key *key, struct cali_ccq_value *value, void *ctx) | ||||||||||
|
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. 🔎 Correctness — process_ccq_entry’s signature was changed to use concrete types and remove const from key:
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. 🧪 Maintainability & Tests — The callback signature to bpf_for_each_map_elem is conventionally (void *map, const void key, void value, void ctx). Here the function uses strongly typed arguments (struct calico_ct_key, struct cali_ccq_value). While clang will accept it, it obscures that the helper calls with void. Suggest using the standard signature and doing explicit casts at the top for readability and to match kernel idioms:
|
||||||||||
| { | ||||||||||
| const struct calico_ct_key *ct_key = key; | ||||||||||
| struct calico_ct_value *ct_value = value; | ||||||||||
| struct calico_ct_value *rev_value; | ||||||||||
| bool isNatForward = false; | ||||||||||
|
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. 🧪 Maintainability & Tests — Variable naming mixes styles (isNatForward) in a codebase that otherwise uses snake_case in C. For consistency and readability, prefer snake_case (e.g., is_nat_forward) in the C BPF code. |
||||||||||
| struct calico_ct_key *lookup_key = key; | ||||||||||
| struct ct_iter_ctx *ictx = ctx; | ||||||||||
|
|
||||||||||
| __u64 age = sub_age(ictx->now, ct_value->last_seen); | ||||||||||
| __u64 age_s = age/1000000000ull; | ||||||||||
|
|
||||||||||
| CALI_DEBUG("Checking: proto=%d " IP_FMT ":%d", ct_key->protocol, debug_ip(ct_key->addr_a), ct_key->port_a); | ||||||||||
| CALI_DEBUG(" <-> " IP_FMT ":%d age=%ds", debug_ip(ct_key->addr_b), ct_key->port_b, age_s); | ||||||||||
|
|
||||||||||
| __u64 max_age, max_age_s; | ||||||||||
|
|
||||||||||
| switch (ct_value->type) { | ||||||||||
| case CALI_CT_TYPE_NORMAL: | ||||||||||
| // Non-NAT entry, we only need to look at this entry to determine if it | ||||||||||
| // has expired. | ||||||||||
| ictx->num_kvs_seen_normal++; | ||||||||||
| max_age = calculate_max_age(ct_key, ct_value); | ||||||||||
| max_age_s = max_age/1000000000ull; | ||||||||||
| if (age > max_age) { | ||||||||||
| CALI_DEBUG(" EXPIRED: normal entry (max_age=%d).", max_age_s); | ||||||||||
| if (cali_ct_delete_elem(ct_key) == 0) { | ||||||||||
| ictx->num_kvs_deleted_normal++; | ||||||||||
| } | ||||||||||
| } | ||||||||||
| break; | ||||||||||
| case CALI_CT_TYPE_NAT_FWD: | ||||||||||
| // One half of a NAT entry. The "forward" NAT entry is just a pointer | ||||||||||
| // to the "reverse" one, where we do the book-keeping. In particular, | ||||||||||
| // the last-seen timestamp on the "reverse" entry is updated when we see | ||||||||||
| // traffic in either direction. | ||||||||||
| ictx->num_kvs_seen_nat_fwd++; | ||||||||||
| rev_value = cali_ct_lookup_elem(&ct_value->nat_rev_key); | ||||||||||
| if (!rev_value) { | ||||||||||
| // No reverse value found, see if this is a new entry. | ||||||||||
| __u64 age = sub_age(ictx->now, ct_value->last_seen); | ||||||||||
| if (age < __globals.creation_grace) { | ||||||||||
| // New entry, assume we're racing with creation. | ||||||||||
| CALI_DEBUG(" INVALID: Forward NAT entry with no reverse entry, ignoring due to creation grace period."); | ||||||||||
| break; | ||||||||||
| } | ||||||||||
| // If NAT fwd entry, do a lookup of the rev_key. | ||||||||||
|
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. 🧪 Maintainability & Tests — The check for “NAT forward” is value->rev_key.protocol != 0. This sentinel is subtle and non-obvious. Please add a comment here explaining this contract (protocol==0 indicates normal/reverse entries, non-zero indicates a queued forward entry provided with its reverse key), or better, use an explicit flag in the value if available. |
||||||||||
| if (value->rev_key.protocol != 0) { | ||||||||||
| isNatForward = true; | ||||||||||
| lookup_key = &value->rev_key; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Entry is not fresh so it looks invalid. Clean it up. | ||||||||||
| CALI_DEBUG(" INVALID: Forward NAT entry with no reverse entry, cleaning up."); | ||||||||||
| if (cali_ct_delete_elem(ct_key) == 0) { | ||||||||||
| ictx->num_kvs_deleted_nat_fwd++; | ||||||||||
| const struct calico_ct_value *actual_ct_value = cali_ct_lookup_elem(lookup_key); | ||||||||||
| if (actual_ct_value) { | ||||||||||
| if (isNatForward) { | ||||||||||
| // If the reverse key has expired, go to delete. | ||||||||||
| if (actual_ct_value->last_seen == value->rev_last_seen) { | ||||||||||
|
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. 🧪 Maintainability & Tests — The decision to delete when actual_ct_value->last_seen == value->rev_last_seen encodes an important invariant: equality means the entry hasn’t changed since it was queued. Please add a short comment clarifying the race you’re guarding against and why equality (not <=) is the right choice with monotonic ktime, and note units (ns). |
||||||||||
| goto delete; | ||||||||||
| } | ||||||||||
| break; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Got a reverse entry, which has the overall "last_seen" for the | ||||||||||
| // connection. Check if this entry has expired. | ||||||||||
| age = sub_age(ictx->now, rev_value->last_seen); | ||||||||||
| age_s = age/1000000000ull; | ||||||||||
|
|
||||||||||
| CALI_DEBUG(" Reverse NAT: proto=%d " IP_FMT ":%d", ct_key->protocol, debug_ip(ct_value->nat_rev_key.addr_a), ct_value->nat_rev_key.port_a); | ||||||||||
| CALI_DEBUG(" <->" IP_FMT ":%d age=%ds", debug_ip(ct_value->nat_rev_key.addr_b), ct_value->nat_rev_key.port_b, age_s); | ||||||||||
|
|
||||||||||
| max_age = calculate_max_age(&ct_value->nat_rev_key, rev_value); | ||||||||||
| max_age_s = max_age/1000000000ull; | ||||||||||
| if (age > max_age) { | ||||||||||
| // Expired, mark the entries for cleanup. We can't just delete | ||||||||||
| // them now because it's not safe to delete _other_ entries from | ||||||||||
| // the map while iterating. | ||||||||||
| CALI_DEBUG(" EXPIRED: forward/reverse NAT entries (max_age=%d), queuing for deletion.", max_age_s); | ||||||||||
| if (cali_ccq_update_elem(&ct_value->nat_rev_key, ct_key, BPF_ANY)) { | ||||||||||
| CALI_DEBUG(" Failed to queue entry, queue full?"); | ||||||||||
| return 0; | ||||||||||
| } else { | ||||||||||
| // This is normal or NAT reverse entry. | ||||||||||
| if (actual_ct_value->last_seen == value->last_seen) { | ||||||||||
| goto delete; | ||||||||||
| } | ||||||||||
| return 0; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| break; | ||||||||||
| case CALI_CT_TYPE_NAT_REV: | ||||||||||
| // One half of a NAT entry. The "reverse" entry is updated when we see | ||||||||||
| // traffic in either direction. | ||||||||||
| ictx->num_kvs_seen_nat_rev++; | ||||||||||
| if (entry_expired(ct_key, ct_value, ictx)) { | ||||||||||
| // Reverse entry has expired, but the reverse entry doesn't have a | ||||||||||
| // link to the forward entry so we write a dummy key and use | ||||||||||
| // BPF_NOEXIST to avoid overwriting a real key. | ||||||||||
| struct calico_ct_key dummy_key = {}; | ||||||||||
| long rc = cali_ccq_update_elem(ct_key, &dummy_key, BPF_NOEXIST); | ||||||||||
| if (rc) { | ||||||||||
| CALI_DEBUG(" EXPIRED: Reverse NAT entry, already in queue or queue full (rc=%d).", rc); | ||||||||||
| } else { | ||||||||||
| CALI_DEBUG(" EXPIRED: Reverse NAT entry, queued for deletion. (Forward NAT entry not yet seen.)"); | ||||||||||
| } | ||||||||||
| } else if (isNatForward) { | ||||||||||
|
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. 🧪 Maintainability & Tests — Comment/code mismatch: the comment says “Set the lookup_key to the forward entry and do a lookup of the fwd entry,” but the code sets lookup_key = key (which is the rev key as passed by the iteration). This is confusing for future readers. Either fix the comment to reflect the actual key being looked up or adjust the code to match the intent. |
||||||||||
| // Its a NAT forward entry but the reverse entry is not present in the CT table. | ||||||||||
| // Set the lookup_key to the forward entry and do a lookup of the fwd entry. | ||||||||||
| // If the timestamp's match, delete it. | ||||||||||
| lookup_key = key; | ||||||||||
| actual_ct_value = cali_ct_lookup_elem(lookup_key); | ||||||||||
| // Reverse key is present, but it is already deleted from the CT table. | ||||||||||
| if (actual_ct_value && actual_ct_value->last_seen == value->last_seen) { | ||||||||||
| goto delete; | ||||||||||
| } | ||||||||||
| break; | ||||||||||
| return 0; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return 0; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // process_ccq_entry processes an entry in the "cleanup queue" map. The map | ||||||||||
| // is keyed on the reverse NAT entry's key, with the value storing the forward | ||||||||||
| // entry's key (or a zero value if the forward entry is missing). | ||||||||||
| static long process_ccq_entry(void *map, const void *key, void *value, void *ctx) | ||||||||||
| { | ||||||||||
| // Map stores mapping from reverse key to forward key (if known). | ||||||||||
| const struct calico_ct_key *rev_key = key; | ||||||||||
| const struct calico_ct_key *fwd_key = value; | ||||||||||
| struct ct_iter_ctx *ictx = ctx; | ||||||||||
|
|
||||||||||
| // It might be a few ms since we queued this entry, recheck it to make sure | ||||||||||
| // it is still expired. | ||||||||||
| const struct calico_ct_value *rev_value = cali_ct_lookup_elem(rev_key); | ||||||||||
| if (rev_value) { | ||||||||||
| __u64 age = sub_age(ictx->now, rev_value->last_seen); | ||||||||||
| __u64 age_s = age/1000000000ull; | ||||||||||
| #ifdef IPVER6 | ||||||||||
| CALI_DEBUG("Re-checking: proto=%d [%pI6]:%d", rev_key->protocol, &rev_key->addr_a, rev_key->port_a); | ||||||||||
| CALI_DEBUG(" <->[%pI6]:%d age=%ds", &rev_key->addr_b, rev_key->port_b, age_s); | ||||||||||
| #else | ||||||||||
| CALI_DEBUG("Re-checking: proto=%d %pI4:%d", rev_key->protocol, &rev_key->addr_a, rev_key->port_a); | ||||||||||
| CALI_DEBUG(" <->%pI4:%d age=%ds", &rev_key->addr_b, rev_key->port_b, age_s); | ||||||||||
| #endif | ||||||||||
| __u64 max_age = calculate_max_age(rev_key, rev_value); | ||||||||||
| __u64 max_age_s = max_age/1000000000ull; | ||||||||||
| if (age < max_age) { | ||||||||||
| // Race with a packet, CT entry now live again. | ||||||||||
| CALI_DEBUG(" RESURRECTED: entry no longer expired (max_age=%d).", max_age_s); | ||||||||||
| goto out; | ||||||||||
| delete: | ||||||||||
|
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. 🧪 Maintainability & Tests — Minor typos in the deletion block comments (“togethere”, “Its”) make this delicate logic harder to parse. Please correct and tighten the wording since this section explains a crucial two-entry deletion sequence. |
||||||||||
| // If the entry is Normal or Reverse, lookup_key points to the entry. | ||||||||||
| // Delete the entry and if successful, increment the counter. | ||||||||||
| // If the entry is forward, we have 2 cases, | ||||||||||
| // Rev_entry present - lookup_key points to the reverse entry and key points to the | ||||||||||
| // forward entry. Delete both the entries togethere. | ||||||||||
|
|
||||||||||
| // Rev_entry not present - lookup_key points to the forward entry. | ||||||||||
| // Delete it. | ||||||||||
| if(!cali_ct_delete_elem(lookup_key)) { | ||||||||||
|
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. 🧪 Maintainability & Tests — The CCQ map entry is only removed if cali_ct_delete_elem(lookup_key) succeeds. Previously, the queue entry was always removed. This behavior change can leave stale entries in the queue on transient failures. If the intention is to retry, please add a comment explaining the retry path and how unbounded growth is avoided (e.g., later scans reprocess and eventually purge). Otherwise, consider always removing the queue entry to avoid leaks and rely on future scanners to re-queue if needed. |
||||||||||
| ictx->num_cleaned++; | ||||||||||
| if (isNatForward && !cali_ct_delete_elem(key)) { | ||||||||||
| ictx->num_cleaned++; | ||||||||||
| } | ||||||||||
| CALI_DEBUG(" EXPIRED: cleaning up forward/reverse entries (max_age=%d).", max_age_s); | ||||||||||
| } else { | ||||||||||
| CALI_DEBUG(" MISSING: lookup failed."); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Still expired, delete both entries. The forward key might be a dummy | ||||||||||
| // all-zeros key but we know that key won't exist so we just let | ||||||||||
| // cali_ct_delete_elem handle that. | ||||||||||
| if (cali_ct_delete_elem(fwd_key) == 0) { | ||||||||||
| ictx->num_kvs_deleted_nat_fwd++; | ||||||||||
| } | ||||||||||
| if (cali_ct_delete_elem(rev_key) == 0) { | ||||||||||
| ictx->num_kvs_deleted_nat_rev++; | ||||||||||
| cali_ccq_delete_elem(key); | ||||||||||
|
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. 🔎 Correctness — The cleanup queue (CCQ) entry is only deleted from the CCQ map when the CT deletion succeeds; otherwise, it remains in the CCQ map. There are also early returns above (e.g., at lines 52, 58, and 70) that exit without removing the CCQ entry.
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.
One option (mirroring the previous out: path) would be to unconditionally delete the CCQ entry at the end of the handler:
Suggested change
(or, if we want to keep “retry until timestamps match”, maybe delete on “resurrected” too.) |
||||||||||
| } | ||||||||||
|
|
||||||||||
| out: | ||||||||||
| // Always remove the entry in our scratch map. | ||||||||||
| cali_ccq_delete_elem(key); | ||||||||||
| return 0; | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -264,21 +101,10 @@ __attribute__((section("tc"))) int conntrack_cleanup(struct __sk_buff *skb) | |||||||||
| // time. | ||||||||||
| ictx.now = bpf_ktime_get_ns(); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| CALI_DEBUG("Scanning conntrack map for expired non-NAT entries..."); | ||||||||||
| bpf_for_each_map_elem(&CT_MAP_V, process_ct_entry, &ictx, 0); | ||||||||||
| CALI_DEBUG("First pass complete, expired %d KVs so far of %d total.", | ||||||||||
| ictx.num_kvs_deleted_normal + ictx.num_kvs_deleted_nat_fwd + ictx.num_kvs_deleted_nat_rev, | ||||||||||
| ictx.num_kvs_seen_normal + ictx.num_kvs_seen_nat_fwd + ictx.num_kvs_seen_nat_rev); | ||||||||||
| CALI_DEBUG("Processing NAT entries..."); | ||||||||||
| CALI_DEBUG("Scanning conntrack cleanup map for entries to be cleaned up..."); | ||||||||||
| bpf_for_each_map_elem(&CCQ_MAP_V, process_ccq_entry, &ictx, 0); | ||||||||||
| CALI_DEBUG("Conntrack cleanup complete: expired %d KVs of %d total.", | ||||||||||
| ictx.num_kvs_deleted_normal + ictx.num_kvs_deleted_nat_fwd + ictx.num_kvs_deleted_nat_rev, | ||||||||||
| ictx.num_kvs_seen_normal + ictx.num_kvs_seen_nat_fwd + ictx.num_kvs_seen_nat_rev); | ||||||||||
|
|
||||||||||
| // Give detailed stats back to userspace. | ||||||||||
| ictx.end_time = bpf_ktime_get_ns(); | ||||||||||
| bpf_skb_store_bytes(skb, 0, &ictx, sizeof(ictx), 0); | ||||||||||
|
|
||||||||||
| return 0; | ||||||||||
| } | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,24 +13,30 @@ | |
|
|
||
| #ifdef IPVER6 | ||
| #define CCQ_MAP cali_v6_ccq | ||
| #define CCQ_MAP_V cali_v6_ccq1 | ||
| #define CCQ_MAP_V cali_v6_ccq2 | ||
|
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. 🧪 Maintainability & Tests — CCQ_MAP_V changed from cali_v[46]_ccq1 to cali_v[46]_ccq2. This is a versioned name change; please ensure there’s a clear migration path (e.g., old map cleanup and pin removal) and add a brief comment indicating that this is a new on-disk version to help future maintainers during upgrades/downgrades. |
||
| #define CT_MAP_V cali_v6_ct4 | ||
| #else | ||
| #define CCQ_MAP cali_v4_ccq | ||
| #define CCQ_MAP_V cali_v4_ccq1 | ||
| #define CCQ_MAP_V cali_v4_ccq2 | ||
| #define CT_MAP_V cali_v4_ct4 | ||
| #endif | ||
|
|
||
| struct cali_ccq_value { | ||
|
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. 🧪 Maintainability & Tests — The new cali_ccq_value struct needs documentation. Please add a comment describing:
|
||
| struct calico_ct_key rev_key; | ||
| __u64 last_seen; | ||
| __u64 rev_last_seen; | ||
| }; | ||
|
|
||
| // The cali_ccq map is our "cleanup queue". NAT records in the conntrack map | ||
| // require two entries in the map, a forward entry and a reverse entry. When | ||
| // deleting a NAT entry pair, we want to delete both entries together with | ||
| // as little time between as possible in order to avoid racing with the | ||
| // dataplane. To do that, we copy the keys to this map temporarily and then | ||
| // iterate over this map, deleting the pair together. | ||
| CALI_MAP_NAMED(CCQ_MAP, cali_ccq, 1, | ||
| CALI_MAP_NAMED(CCQ_MAP, cali_ccq, 2, | ||
|
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. 🧪 Maintainability & Tests — Map definition bumps version to 2 and sets UpdatedByBPF=false. Please add a short comment explaining why UpdatedByBPF is false now (userspace writes queue entries), and note any compatibility expectations with previously pinned v1 instances. |
||
| BPF_MAP_TYPE_HASH, | ||
| struct calico_ct_key, // key = NAT rev key | ||
| struct calico_ct_key, // value = NAT fwd key | ||
| struct cali_ccq_value, // value = NAT fwd key | ||
|
Comment on lines
38
to
+39
|
||
| 100000, | ||
| BPF_F_NO_PREALLOC | ||
| ); | ||
|
|
||
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.
Can we update the comment to reflect the new key/value and timestamp semantics?