-
Notifications
You must be signed in to change notification settings - Fork 87
add spec.observer (auto|none) and wire to router #2348
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
Conversation
ce0ee75 to
d8023c1
Compare
d8023c1 to
afdd14f
Compare
afdd14f to
e0419b3
Compare
nluaces
left a comment
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
submitted my suggestion, mostly C's text
pwright
left a comment
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.
consider updating desc
|
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" } ] ]