add spec.observer (auto|none) and wire to router#2348
add spec.observer (auto|none) and wire to router#2348AryanP123 merged 4 commits intoskupperproject:mainfrom
Conversation
ce0ee75 to
d8023c1
Compare
d8023c1 to
afdd14f
Compare
afdd14f to
e0419b3
Compare
nluaces
left a comment
There was a problem hiding this comment.
LGTM!
Checked that the listener has the expected observer type in the router config:
$ skmanage query --type tcpListener
[
{
"address": "database",
"host": "",
"backlog": 0,
"port": "1024",
"siteId": "f77ac22c-0e6b-49d1-b9b9-c3fe1ca031fb",
"connectionsOpened": 0,
"connectionsClosed": 0,
"bytesIn": 0,
"bytesOut": 0,
"authenticatePeer": false,
"operStatus": "down",
"connectionMsg": "",
"observer": "auto",
"name": "database",
"identity": "tcpListener/:1024:database",
"type": "io.skupper.router.tcpListener"
}
]
I propose to wait for @c-kruse approval before merging this PR.
c-kruse
left a comment
There was a problem hiding this comment.
Router configuration looks solid to me! Now on to the API and docs.
| type: object | ||
| properties: | ||
| observer: | ||
| description: Control protocol observation for this listener. Default 'auto'. Set 'none' to disable. |
There was a problem hiding this comment.
We should review how we plan to expose this option to users as a larger group.
To me an optional field with an enum of auto/none strongly indicates a boolean flag like disableObserver. That said, that does not help users pick a specific observer like http1 or http2 (or any future implementations.)
The description also needs to explain what the observer is and what the implications of setting it are. I would suggest we lean on @pwright and @ted-ross for help in this department.
There was a problem hiding this comment.
My attempt at required docs (one piece for user guide, another for migration):
https://gist.github.com/pwright/d0916a452e5eb170f410d3ad8428765b
| type: object | ||
| properties: | ||
| observer: | ||
| description: Control protocol observation for this listener. Default 'auto'. Use 'none' to disable, or 'http1'/'http2' to force a specific observer. |
There was a problem hiding this comment.
| description: Control protocol observation for this listener. Default 'auto'. Use 'none' to disable, or 'http1'/'http2' to force a specific observer. | |
| description: |- | |
| The listener observer controls how the listener inspects network traffic for application protocol information. When unset or set to `auto` the listener inspects traffic for known application protocols and produces telemetry events for that application traffic. Set to a specific protocol, `http1` or `http2`, to only inspect traffic for that particular protocol. Set to `none` to disable the observer and reduce overhead from traffic inspection and application level telemetry. |
Probably needs some formatting and a 👍 from @pwright
There was a problem hiding this comment.
submitted my suggestion, mostly C's text
|
Proposed Release Note: Protocol inspectionYou can now control application-level protocol inspection on listener traffic using the new |
Co-authored-by: Paul Wright <5154224+pwright@users.noreply.github.com>
Fixes #2342
Testing:
$ kubectl -n skupper patch listener test-tcp --type=merge -p '{"spec":{"observer":"none"}}' listener.skupper.io/test-tcp patched (no change)$ kubectl -n skupper patch listener test-tcp --type=merge -p '{"spec":{"observer":"non"}}' The Listener "test-tcp" is invalid: spec.observer: Unsupported value: "non": supported values: "auto", "none"$ kubectl -n skupper get cm -o json \ | jq -r '.items[]|select(.data)|.data[]|select(test("tcpListener"))' \ | grep -A12 '"name": "test-tcp"' "name": "test-tcp", "port": "1024", "address": "backend", "siteId": "7e9ba960-e605-4f9a-8b1b-0cc5f5d0b6da", "observer": "none" } ] ]