-
Notifications
You must be signed in to change notification settings - Fork 115
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
Track outbound connections #939
Conversation
Skipping CI for Draft Pull Request. |
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.
This looks really nice. Hides the gritty details of handling the late rbac checks and makes it less fiddly to use.
Unit tests can probably be added for the new code. Maybe that's a follow up.
mut self, | ||
send: impl Future<Output = Result<(u64, u64), Error>> + Sized, | ||
) -> Result<(u64, u64), Error> { | ||
let watch = self.watch.take().expect("watch cannot be taken twice"); |
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.
comment would be nice, it's a little subtle why this expect is OK I think.
"watch can't be taken twice because this ConnectionGuard is consumed by handle_connection" maybe
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.
Shouldn't we error instead of crashing here? Maybe debug_assert
the err path.
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.
I think because the guard is owned by handle_connection and not returned it shouldn't ever be called twice. It's dropped once this returns.
@@ -226,6 +226,11 @@ impl OutboundConnection { | |||
return; | |||
} | |||
let connection_metrics = Self::conn_metrics_from_request(&req); | |||
// TODO: should we use the original address or the actual address? Both seems nice! | |||
let _conn_guard = |
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.
I think definitely both.
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
/cherrypick release-1.22 |
@howardjohn: new pull request created: #948 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Ultimately this enables istioctl to give us:
Or waypoints: