NETOBSERV-2609 show tls usage in topology view#1451
Open
jpinsonneau wants to merge 4 commits into
Open
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughAdds TLS-version and group visibility end-to-end: backend aggregate keys and Loki test, metric parsing and merging with optional tls metadata, topology model and edge/tag state, SVG lock icons and styles, element-drawer TLS details with quick-filters, display options for cleartext hints, filter encoder updates, tests, and English locale strings. ChangesTLS Visibility Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 2 linked repositories, but your current plan allows 1. Analyzed Linked repositories: Couldn't analyze Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jpinsonneau
added
the
ok-to-test
|
New image: quay.io/netobserv/network-observability-console-plugin:fa4506a
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=fa4506a make set-plugin-image |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1451 +/- ##
==========================================
+ Coverage 52.23% 52.45% +0.22%
==========================================
Files 233 236 +3
Lines 12552 12893 +341
Branches 1575 1659 +84
==========================================
+ Hits 6556 6763 +207
- Misses 5378 5489 +111
- Partials 618 641 +23
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Member
Author
|
@jotak can you please share how to generate deprecated TLS traffic ? I tried some curl using TLS1.2 but the eBPF agents are not capturing it 😞 |
github-actions
Bot
removed
the
ok-to-test
jpinsonneau
added
the
ok-to-test
|
New image: quay.io/netobserv/network-observability-console-plugin:7bdb498
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=7bdb498 make set-plugin-image |
jotak
reviewed
May 11, 2026
jotak
reviewed
May 11, 2026
Comment on lines
+62
to
+70
| if (!metricType) { | ||
| return false; | ||
| } | ||
| return ( | ||
| metricType === 'Bytes' || | ||
| metricType === 'Packets' || | ||
| metricType === 'PktDropBytes' || | ||
| metricType === 'PktDropPackets' | ||
| ); |
Member
There was a problem hiding this comment.
So, I think the way I'm changing the metrics here won't work that way. It would be a new query to run, with TlsFlows as the metric type
Member
Author
There was a problem hiding this comment.
Sure, let me adapt to the new approach
Member
|
In terms of graphical design it looks nice to me! |
github-actions
Bot
removed
the
ok-to-test
Member
Author
|
Refactored based on #1478 Will rebased when merged and cherry picked on PF6 branch |
Member
Author
|
Rebased without changes |
jpinsonneau
added
ok-to-test
|
New image: quay.io/netobserv/network-observability-console-plugin:6c5cd8d
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=6c5cd8d make set-plugin-image |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
web/src/utils/metrics.ts (1)
61-63: ⚡ Quick winIndex TLS rows by edge key before merge.
This nested filter does a full
tlsRowsscan for every volume row. Build a keyed map once (src.id@dst.id) and merge from O(1) lookups.Proposed refactor
export const mergeTlsIntoTopologyMetrics = ( volume: TopologyMetrics[], tlsRows: TopologyMetrics[] ): TopologyMetrics[] => { const mergeTls = (a: GenericMetricTls | undefined, b: GenericMetricTls | undefined): GenericMetricTls | undefined => { @@ return { ...(types.length ? { types } : {}), ...(versions.length ? { versions } : {}) }; }; + const tlsByEdge = new Map<string, GenericMetricTls | undefined>(); + for (const t of tlsRows) { + const k = `${t.source.id}@${t.destination.id}`; + tlsByEdge.set(k, mergeTls(tlsByEdge.get(k), t.tls)); + } + return volume.map(m => { - const matches = tlsRows.filter(t => t.source.id === m.source.id && t.destination.id === m.destination.id); - if (!matches.length) { - return m; - } - let mergedTls = m.tls; - for (const t of matches) { - mergedTls = mergeTls(mergedTls, t.tls); - } + const mergedTls = mergeTls(m.tls, tlsByEdge.get(`${m.source.id}@${m.destination.id}`)); if (!mergedTls) { return m; } return { ...m, tls: mergedTls }; }); };As per coding guidelines, "Optimize frontend performance by avoiding unnecessary re-renders and optimizing query patterns".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/utils/metrics.ts` around lines 61 - 63, The current volume.map callback repeatedly scans tlsRows with tlsRows.filter inside volume.map, causing O(N*M) work; precompute a lookup map keyed by `${source.id}@${destination.id}` (build once from tlsRows) and then in the volume.map use that map.get(key) to retrieve matching TLS row(s) in O(1). Change the logic around the existing volume.map and the local variable matches (used in the map callback) to look up via the new map instead of calling tlsRows.filter each iteration; ensure keys use the same id fields (source.id and destination.id) as in the original comparison.web/src/utils/__tests__/metrics.spec.ts (1)
511-563: ⚡ Quick winAdd regression test for empty-first-row merge bucket.
Please add a case where the first
TLSVersionrow hasvalues: []and a later row has datapoints; merged output should keep later datapoints.Suggested test
describe('mergeTlsVersionUsageMetrics', () => { + it('keeps datapoints when first bucket row is empty', () => { + const empty: [number, number][] = []; + const values: [number, number][] = [[100, 7]]; + const merged = mergeTlsVersionUsageMetrics([ + { name: 'TLS 1.2', values: empty, stats: computeStats(empty), aggregateBy: 'TLSVersion' }, + { name: 'TLS 1.2', values, stats: computeStats(values), aggregateBy: 'TLSVersion' } + ]); + expect(merged).toHaveLength(1); + expect(merged[0].metric.values).toEqual([[100, 7]]); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/utils/__tests__/metrics.spec.ts` around lines 511 - 563, Add a regression test to metrics.spec.ts for mergeTlsVersionUsageMetrics that covers the case where the first TLSVersion row has values: [] and a later row for the same name contains datapoints; create two GenericMetric-like inputs (first with empty values and stats for empty, second with non-empty values and computeStats for them), call mergeTlsVersionUsageMetrics([first, second]) and assert the merged output contains a single bucket with the later datapoints preserved (e.g., merged[0].metric.values equals the non-empty datapoints) and appropriate name/filterValue; place this new it block inside the existing describe('mergeTlsVersionUsageMetrics', ...) alongside the other tests.web/src/utils/__tests__/filter-definitions.spec.ts (1)
7-22: ⚡ Quick winAdd a negated endpoint regression case.
These assertions only cover
=. The new encoder also changes!=/!~, and that's where the src/dst join semantics can drift. Anamespace != ns1expectation here would lock the behavior down.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/utils/__tests__/filter-definitions.spec.ts` around lines 7 - 22, Add a negated-case unit test mirroring the existing positive case to lock the src/dst join semantics for negation: use findFilter (or getFilterDefinitions+findFilter when testing columnsWithoutFilterLink) to get the 'namespace' FilterDefinitionSample, call def.encoder([{ v: 'ns1' }], FilterCompare.notEqual, false) and assert the encoded string equals 'SrcK8S_Namespace!=ns1|DstK8S_Namespace!=ns1' so the encoder's handling of !=/!~ for the namespace filter is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/components/drawer/element/element-panel-content.tsx`:
- Around line 165-171: The icon-only Button (id={buttonId}, icon using
TimesIcon/FilterIcon and onclick toggleQuickFilterValue) lacks an accessible
label and E2E hook; add a localized aria-label using t(...) that reflects the
current state (e.g., t('filters.clear', { value }) when isFiltered true and
t('filters.apply', { value }) when false) and add a stable data-test attribute
(e.g., data-test={`quick-filter-${def}-${value}`}) to the Button component so
screen readers and tests can target it reliably.
- Around line 223-231: The fallback message branch incorrectly checks
tlsVersionLabels; update the conditional that decides whether to render the "No
TLS message type breakdown..." block to test tlsTypeLabels (i.e., render when
tagTlsSecure is true AND tlsTypeLabels is absent/empty) so secure edges with
versions but missing message-type labels show the explanatory Content; locate
the conditional around tagTlsSecure in element-panel-content (the ternary that
currently inspects tlsVersionLabels) and replace that check to use tlsTypeLabels
instead.
In `@web/src/components/drawer/element/element-panel.tsx`:
- Around line 107-119: The activeTab reset logic fails to account for noMetrics
hiding the Metrics tab: update the condition that currently checks (activeTab
=== 'metrics' && _.isEmpty(metrics)) to also consider noMetrics (e.g., treat the
Metrics tab as unavailable when noMetrics is true), so if noMetrics becomes true
while metrics still has data we call setActiveTab('details'); modify the
useEffect conditions (referencing activeTab, metrics, noMetrics, showDroppedTab,
showHealthTab, setActiveTab) accordingly and apply the same fix to the other
similar reset check that references these symbols.
In `@web/src/components/dropdowns/topology-display-options.tsx`:
- Around line 189-200: The new Checkbox control (id
"edges-cleartext-lock-switch") lacks a stable E2E selector; add a data-test
attribute (e.g. data-test="edges-cleartext-lock-switch" or data-test-id) to the
<Checkbox> component so tests can reliably find the TLS toggle, ensuring you
update the JSX props on the Checkbox where id="edges-cleartext-lock-switch" is
defined (preserve existing props like label, isDisabled, isChecked, onChange).
In `@web/src/components/tabs/netflow-topology/2d/topology-content.tsx`:
- Around line 79-80: The component is currently prop-drilling the isTLSTracking
flag; stop passing isTLSTracking through props and instead read it from the
shared NetflowContext at the consumer level. In topology-content.tsx (component
TopologyContent or its exported function), remove isTLSTracking from the
props/interface, import and use the NetflowContext or the provided hook from
web/src/model/netflow-context.ts (e.g., useNetflowContext()) to obtain
isTLSTracking, and update all internal uses to reference the context value; also
update any call sites that passed isTLSTracking (see the blocks around lines
299–305/304) to stop supplying that prop. Ensure type signatures and
tests/consumers no longer expect the prop.
In `@web/src/components/tabs/netflow-topology/netflow-topology.tsx`:
- Line 127: Create a single effective TLS flag (e.g. effectiveIsTLSTracking)
computed from caps.isTLSTracking and props.isTLSTracking, and use that value
everywhere instead of mixing the two flags: replace the fetch condition that
sets fetchTlsGeneric (currently using caps.isTLSTracking &&
showTLSHints(metricType)) to use effectiveIsTLSTracking &&
showTLSHints(metricType), pass effectiveIsTLSTracking into TopologyContent
rendering props, and update any useEffect/useCallback dependency arrays that
currently reference caps.isTLSTracking or props.isTLSTracking to depend on
effectiveIsTLSTracking instead so fetches, rendering, and callbacks remain
consistent.
- Around line 166-205: commitMainMetrics currently calls
setMetrics(currentMetrics) using a stale snapshot captured earlier, which can
clobber concurrent updates (e.g., droppedRate) — change each setMetrics(...)
inside commitMainMetrics to the functional form setMetrics(prev => ({ ...prev,
... })) so you merge updates against the latest state; for each branch
(Bytes/Packets, PktDropBytes/PktDropPackets, DnsLatencyMs, TimeFlowRttNs)
compute the new partial payload (rate/droppedRate/dnsLatency/rtt using
getRateMetricKey/getFunctionMetricKey and Result.success/Result.empty as before)
and then call setMetrics(prev => ({ ...prev, ...partialPayload })) to avoid lost
updates from concurrent requests.
In `@web/src/utils/filter-definitions.ts`:
- Around line 95-102: The endpointEitherSideEncoder currently always joins left
and right clauses with '|' which fails for negated compares; update
endpointEitherSideEncoder to pick the joiner based on the compare: call
simpleFiltersEncoder(srcField) and simpleFiltersEncoder(dstField) as before, but
if the incoming compare is a negated compare (e.g. FilterCompare '!=' /
isNegated), join the left and right strings with '&' so both sides are excluded,
otherwise use '|' for the non-negated case; reference endpointEitherSideEncoder,
srcField, dstField, simpleFiltersEncoder, and compare when making this change.
In `@web/src/utils/metrics.ts`:
- Around line 570-574: The merge currently seeds from group[0].values which
drops all rows when that first row is empty; update the logic in this merge
block to locate the first row in group whose .values is non-empty (e.g., find
index i where group[i].values.length>0), use
stepFromGenericValues(group[i].values) and set values = group[i].values as the
initial seed, then loop over the rest of group (skipping index i) and call
combineValues(values, group[j].values, step, (a,b)=>a+b) for each; if no
non-empty row exists, return/leave values as an empty array to avoid losing
data.
---
Nitpick comments:
In `@web/src/utils/__tests__/filter-definitions.spec.ts`:
- Around line 7-22: Add a negated-case unit test mirroring the existing positive
case to lock the src/dst join semantics for negation: use findFilter (or
getFilterDefinitions+findFilter when testing columnsWithoutFilterLink) to get
the 'namespace' FilterDefinitionSample, call def.encoder([{ v: 'ns1' }],
FilterCompare.notEqual, false) and assert the encoded string equals
'SrcK8S_Namespace!=ns1|DstK8S_Namespace!=ns1' so the encoder's handling of !=/!~
for the namespace filter is validated.
In `@web/src/utils/__tests__/metrics.spec.ts`:
- Around line 511-563: Add a regression test to metrics.spec.ts for
mergeTlsVersionUsageMetrics that covers the case where the first TLSVersion row
has values: [] and a later row for the same name contains datapoints; create two
GenericMetric-like inputs (first with empty values and stats for empty, second
with non-empty values and computeStats for them), call
mergeTlsVersionUsageMetrics([first, second]) and assert the merged output
contains a single bucket with the later datapoints preserved (e.g.,
merged[0].metric.values equals the non-empty datapoints) and appropriate
name/filterValue; place this new it block inside the existing
describe('mergeTlsVersionUsageMetrics', ...) alongside the other tests.
In `@web/src/utils/metrics.ts`:
- Around line 61-63: The current volume.map callback repeatedly scans tlsRows
with tlsRows.filter inside volume.map, causing O(N*M) work; precompute a lookup
map keyed by `${source.id}@${destination.id}` (build once from tlsRows) and then
in the volume.map use that map.get(key) to retrieve matching TLS row(s) in O(1).
Change the logic around the existing volume.map and the local variable matches
(used in the map callback) to look up via the new map instead of calling
tlsRows.filter each iteration; ensure keys use the same id fields (source.id and
destination.id) as in the original comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1bf00d96-3efb-4b92-a67d-db77c669e15e
📒 Files selected for processing (35)
pkg/config/config.gopkg/loki/flow_query.gopkg/loki/topology_query.gopkg/loki/topology_query_test.goweb/locales/en/plugin__netobserv-plugin.jsonweb/src/api/ipfix.tsweb/src/api/loki.tsweb/src/components/drawer/element/__tests__/element-panel.spec.tsxweb/src/components/drawer/element/element-panel-content.tsxweb/src/components/drawer/element/element-panel.cssweb/src/components/drawer/element/element-panel.tsxweb/src/components/drawer/netflow-traffic-drawer.tsxweb/src/components/dropdowns/topology-display-dropdown.tsxweb/src/components/dropdowns/topology-display-options.tsxweb/src/components/icons/index.tsweb/src/components/icons/tls-lock-icons.tsxweb/src/components/tabs/netflow-topology/2d/components/edge.tsxweb/src/components/tabs/netflow-topology/2d/components/topology-connector-tag.tsxweb/src/components/tabs/netflow-topology/2d/styles/styleEdge.tsxweb/src/components/tabs/netflow-topology/2d/topology-content.cssweb/src/components/tabs/netflow-topology/2d/topology-content.tsxweb/src/components/tabs/netflow-topology/netflow-topology.tsxweb/src/components/toolbar/filters/__tests__/filters-toolbar.spec.tsxweb/src/components/toolbar/view-options-toolbar.tsxweb/src/model/__tests__/flow-query.spec.tsweb/src/model/filters.tsweb/src/model/flow-query.tsweb/src/model/netflow-context.tsweb/src/model/topology.tsweb/src/utils/__tests__/filter-definitions.spec.tsweb/src/utils/__tests__/metrics.spec.tsweb/src/utils/__tests__/tls-lock-severity.spec.tsweb/src/utils/filter-definitions.tsweb/src/utils/metrics.tsweb/src/utils/tls-lock-severity.ts
| const fq = caps.flowQuery; | ||
| const features = config.features; | ||
| const { getMetrics } = caps.fetchFunctions; | ||
| const fetchTlsGeneric = caps.isTLSTracking && showTLSHints(metricType); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the file and check its size
wc -l web/src/components/tabs/netflow-topology/netflow-topology.tsxRepository: netobserv/netobserv-web-console
Length of output: 141
🏁 Script executed:
#!/bin/bash
# Read the file to understand the TLS tracking usage
cat -n web/src/components/tabs/netflow-topology/netflow-topology.tsx | head -400Repository: netobserv/netobserv-web-console
Length of output: 17611
Unify TLS-tracking flag between fetch and render.
The TLS query at line 127 only checks caps.isTLSTracking, ignoring the props.isTLSTracking override that affects TopologyContent rendering. This allows TLS UI to be hidden while TLS queries still run, or vice versa. Create one effectiveIsTLSTracking value and use it consistently in the fetch decision, render prop, and callback dependencies.
Suggested fix
+ const effectiveIsTLSTracking = props.isTLSTracking ?? caps.isTLSTracking;
...
- const fetchTlsGeneric = caps.isTLSTracking && showTLSHints(metricType);
+ const fetchTlsGeneric = effectiveIsTLSTracking && showTLSHints(metricType);
...
- caps.isTLSTracking,
+ effectiveIsTLSTracking,
...
- isTLSTracking={props.isTLSTracking ?? caps.isTLSTracking}
+ isTLSTracking={effectiveIsTLSTracking}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/components/tabs/netflow-topology/netflow-topology.tsx` at line 127,
Create a single effective TLS flag (e.g. effectiveIsTLSTracking) computed from
caps.isTLSTracking and props.isTLSTracking, and use that value everywhere
instead of mixing the two flags: replace the fetch condition that sets
fetchTlsGeneric (currently using caps.isTLSTracking && showTLSHints(metricType))
to use effectiveIsTLSTracking && showTLSHints(metricType), pass
effectiveIsTLSTracking into TopologyContent rendering props, and update any
useEffect/useCallback dependency arrays that currently reference
caps.isTLSTracking or props.isTLSTracking to depend on effectiveIsTLSTracking
instead so fetches, rendering, and callbacks remain consistent.
Contributor
There was a problem hiding this comment.
@jotak Yes, it's valid. The fetch path uses caps.isTLSTracking directly while the render prop uses props.isTLSTracking ?? caps.isTLSTracking. If a parent ever passes isTLSTracking={false} explicitly (with caps.isTLSTracking still true), TLS queries would still fire even though the UI shows nothing — wasted network call and potential stale data.
The fix is one line at the top of the component body:
const effectiveIsTLSTracking = props.isTLSTracking ?? caps.isTLSTracking;Then replace the three occurrences:
const fetchTlsGeneric = effectiveIsTLSTracking && showTLSHints(metricType);isTLSTracking={effectiveIsTLSTracking}onTopologyContentcaps.isTLSTracking→effectiveIsTLSTrackingin theuseCallbackdependency array
jotak
reviewed
May 19, 2026
Comment on lines
+232
to
+235
| // appendTLSTypesArrayLabelFromLine adds a TLSTypes label from the raw log line. | ||
| // Loki's |json stage does not extract JSON array fields as labels, so sum by(TLSTypes,...) would | ||
| // otherwise drop the dimension even though flows include "TLSTypes":[...] in JSON. | ||
| func (q *FlowQueryBuilder) appendTLSTypesArrayLabelFromLine(sb *strings.Builder) { |
Member
There was a problem hiding this comment.
I'm not sure to understand why not just using the appendTLSFilter defined above. Did I miss an edge case there?
Member
There was a problem hiding this comment.
Ah, I guess I get it: it's because you're doing an aggregation per TLS type, right?
I think we can simplify and forget about TLS types here; not only because imho it's not a super important thing to show in aggregated data, but also because I omitted it intentionally in prom metrics (for cardinality)
I think if we omit TLS types in aggregations, that simplifies the whole things quite a bit?
Member
There was a problem hiding this comment.
Actually, TLSGroup is much more meaningful to have in aggregateed data. PQC compliance is based on TLSGroup.
jotak
reviewed
May 19, 2026
| }; | ||
|
|
||
| /** When column config has no `filter` key (tests) or no `field`, map endpoint filter id → [src,dst] ipfix fields. */ | ||
| const ENDPOINT_DUAL_FIELD_FALLBACK: Partial<Record<FilterId, [Field, Field]>> = { |
Member
There was a problem hiding this comment.
per conventions, rename to endpointDualFieldFallback
jotak
reviewed
May 19, 2026
| }; | ||
| }; | ||
|
|
||
| /** `[SrcFoo,DstFoo]` on columns that aggregate both endpoints (no single `field`). */ |
Member
There was a problem hiding this comment.
(I'm trying to understand what triggered the need for these changes). Is there some columns / filters that lacked some definition in the config file? If I understand correctly, it's some columns with no assigned field that didn't work as expected?
Member
Author
There was a problem hiding this comment.
Yes I had issues with Namespace filter and took the opportunity to fix it here based on calculated field.
jotak
reviewed
May 19, 2026
| export type AggregateBy = FlowScope | Field; | ||
|
|
||
| /** Suffix for aggregateBy keys that mirror a topology scope plus TLSVersion (see backend GetAggregateKeyLabels). */ | ||
| export const TOPOLOGY_TLS_VERSION_AGGREGATE_SUFFIX = '__TLSVersion'; |
Member
There was a problem hiding this comment.
it should be low camel case per convention
jotak
reviewed
May 19, 2026
| */ | ||
| export type TlsLockSeverity = 'deprecated' | 'legacy' | 'modern' | 'unknown'; | ||
|
|
||
| const SEVERITY_RANK: Record<TlsLockSeverity, number> = { |
Member
There was a problem hiding this comment.
can we add a "PQC" rank? based on version==1.3 + group being one of X25519MLKEM768, Secp256r1MLKEM768 or Secp384r1MLKEM1024
jotak
reviewed
May 19, 2026
| }; | ||
|
|
||
| /** Classify a single TLSVersion label from Loki / flow JSON. */ | ||
| export const classifyTlsVersionString = (raw: string): TlsLockSeverity | null => { |
Member
There was a problem hiding this comment.
this is catching very large possibilities, like it's an open input, but since we control the source we could make it simpler and explicit? idk, just to keep it simpler .. of course this approach would be worse if one day we end up gathering it from more sources.
fwiw currently that's the possible values:
- SSLv3
- TLS 1.0
- TLS 1.1
- TLS 1.2
- TLS 1.3
- potentially prefixed with "~ "
It's up to you, if you prefer to keep as is
Member
Author
There was a problem hiding this comment.
sure let's keep it simple
github-actions
Bot
removed
the
ok-to-test
jpinsonneau
added
the
ok-to-test
|
New image: quay.io/netobserv/network-observability-console-plugin:8ae20ac
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=8ae20ac make set-plugin-image |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/components/tabs/netflow-topology/2d/topology-content.tsx`:
- Around line 474-482: The edge reset path calls e.setData({...e.getData(), tag:
undefined, tagTlsSecure: undefined, tagTlsLockSeverity: undefined,
tagTlsCleartext: undefined, tlsTypeLabels: undefined, tlsVersionLabels:
undefined}) but omits clearing tlsGroupLabels; update the reset to also set
tlsGroupLabels: undefined so stale TLS group labels are removed when resetting
edge data (locate the reset block using e.setData and e.getData in
topology-content.tsx and add tlsGroupLabels to the cleared fields).
In `@web/src/utils/metrics.ts`:
- Around line 557-569: In mergeTlsVersionUsageMetrics, preserve the original,
untrimmed TLS label for the bucket filterValue: keep using row.name.trim() for
displayName but set filterValue to the raw row.name (not trimmed) so the exact
Loki label contract on MergedTlsVersionMetricRow is honored; update the bucket
creation (the variable b and its filterValue) to assign filterValue = row.name
while leaving displayName = display and rows = [].
In `@web/src/utils/tls-lock-severity.ts`:
- Around line 10-16: SEVERITY_RANK currently assigns pqc the highest numeric
value so the "worst-wins" aggregation (the code that selects the max/worst
severity using SEVERITY_RANK) will prefer pqc over clearly insecure labels like
deprecated/legacy; update the SEVERITY_RANK mapping for TlsLockSeverity so that
deprecated and legacy have higher numeric ranks than pqc (i.e., ensure
deprecated > legacy > pqc > modern > unknown) so the existing aggregation logic
that compares SEVERITY_RANK values returns the actual worst/insecure protocol
observed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eb9df904-6c19-4eef-aa57-d9ef4db65376
📒 Files selected for processing (23)
pkg/config/config.gopkg/loki/topology_query_test.goweb/locales/en/plugin__netobserv-plugin.jsonweb/src/api/loki.tsweb/src/components/drawer/element/__tests__/element-panel.spec.tsxweb/src/components/drawer/element/element-panel-content.tsxweb/src/components/drawer/element/element-panel.tsxweb/src/components/dropdowns/topology-display-options.tsxweb/src/components/icons/tls-lock-icons.tsxweb/src/components/tabs/netflow-topology/2d/components/topology-connector-tag.tsxweb/src/components/tabs/netflow-topology/2d/topology-content.cssweb/src/components/tabs/netflow-topology/2d/topology-content.tsxweb/src/components/tabs/netflow-topology/netflow-topology.tsxweb/src/model/__tests__/flow-query.spec.tsweb/src/model/filters.tsweb/src/model/flow-query.tsweb/src/model/topology.tsweb/src/utils/__tests__/filter-definitions.spec.tsweb/src/utils/__tests__/metrics.spec.tsweb/src/utils/__tests__/tls-lock-severity.spec.tsweb/src/utils/filter-definitions.tsweb/src/utils/metrics.tsweb/src/utils/tls-lock-severity.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- web/src/model/filters.ts
- web/src/components/dropdowns/topology-display-options.tsx
- web/src/utils/tests/filter-definitions.spec.ts
- web/src/api/loki.ts
- web/src/utils/tests/metrics.spec.ts
- pkg/config/config.go
- web/src/model/flow-query.ts
- web/src/components/tabs/netflow-topology/2d/topology-content.css
- web/src/utils/filter-definitions.ts
- web/src/components/tabs/netflow-topology/2d/components/topology-connector-tag.tsx
- web/locales/en/plugin__netobserv-plugin.json
- web/src/components/drawer/element/element-panel-content.tsx
- web/src/components/drawer/element/element-panel.tsx
- web/src/components/tabs/netflow-topology/netflow-topology.tsx
- web/src/model/topology.ts
github-actions
Bot
removed
the
ok-to-test
jpinsonneau
added
the
ok-to-test
|
New image: quay.io/netobserv/network-observability-console-plugin:5ebe411
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=5ebe411 make set-plugin-image |
jotak
reviewed
May 19, 2026
| /** TLS breakdown from Loki matrix metric labels when present (topology TLS aggregate). */ | ||
| export type GenericMetricTls = { | ||
| /** Present on flow records; omitted from topology aggregation (cardinality). */ | ||
| types?: string[]; |
Member
Author
There was a problem hiding this comment.
Removed in 3ab6360
I initially kept that in the UI just in case but it seems we are going to a direction where it will never be the case.
We can reintroduce it if needed in the future (maybe if we introduce a side panel for overview metrics ?)
github-actions
Bot
removed
the
ok-to-test
Member
|
/ok-to-test |
openshift-ci
Bot
added
the
ok-to-test
|
New image: quay.io/netobserv/network-observability-console-plugin:43e0514
It will expire in two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=43e0514 make set-plugin-image |
Member
|
/label qe-approved |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Display TLS versions and message types in topology side panel
Also display a lock with a green / yellow / red color showing if the TLS version involved is valid
Add an option to show an open lock when no TLS is involved
Dependencies
Based on #1478 / #1530
Checklist
Summary by CodeRabbit
New Features
Tests