Initial connection MDS implementation #802

howardjohn · 2024-02-07T19:33:28Z

Store each connection in a shared map
Capture special IP address, redirect to MDS handler
MDS handler takes 4 tuple as input.
Response is JSON containing identity

Also has an example Golang library implementation to add HTTP middleware that extracts the identity.

Istio tests: istio/istio#44536 (blocked by this PR, of course), which also show how to use it

Original PR in #504 got force deleted so cannot re-open

src/proxy/metadata.rs

ilrudie

I didn't envision connection manager being used in this way so we'll probably need to make more changes to ensure all outbound connections register and release correctly

src/proxy/outbound.rs

hzxuzhonghu

Can you add desc of the usage

hzxuzhonghu · 2024-02-08T02:42:18Z

src/proxy/connection_manager.rs

+#[derive(Clone, Debug, Serialize)]
+pub struct ConnectionMetadata {
+    #[serde(default)]
+    identity: Option<Identity>,


I am confused about a connection's identity. Usually, we say the src identity or dest identity

In the implementation we've got it will be the source identity. I suppose keeping the type general would allow us to represent peer identity generically with the same type in the future though.

I documented a bit more now

go-metadata/istio.go

ilrudie

This is looking good. I have a few comments but nothing I suspect to be a show stopper for this direction.

ilrudie · 2024-02-28T14:09:27Z

src/proxy/connection_manager.rs

+#[derive(Clone, Debug, Serialize)]
+pub struct ConnectionMetadata {
+    #[serde(default)]
+    identity: Option<Identity>,


In the implementation we've got it will be the source identity. I suppose keeping the type general would allow us to represent peer identity generically with the same type in the future though.

ilrudie · 2024-02-28T14:23:34Z

src/proxy/metadata.rs

+                        serve_request(&ct, remote_addr, req).await.or_else(|e| {
+                            Ok(crate::hyper_util::plaintext_response(
+                                StatusCode::UNPROCESSABLE_ENTITY,
+                                e.to_string(),


Do we need to be concerned about e leaking something we shouldn't to the mds client?

I think its ok? I documented that we expose errors which could help a bit

We can iterate if it becomes a problem later.

src/proxy/metadata.rs

ilrudie · 2024-02-28T14:31:20Z

src/proxy/connection_manager.rs

@@ -118,6 +133,19 @@ impl ConnectionManager {
        // potentially large copy under read lock, could require optomization
        self.drains.read().await.keys().cloned().collect()
    }
+
+    /// fetch looks up a tuple and returns the connection metadata
+    // TODO: we should key on tuple so we can more efficiently lookup


Maybe another index instead of re-keying? I suppose we could see about keying on tuple and storing the full ProxyRbacContext in the ConnectionDrain struct. Otherwise we'll need to do work to re-construct the full context when we need to assert.

Yeah I would probably do the index if we do it

ilrudie

LGTM

istio-testing · 2024-03-15T22:03:45Z

PR needs rebase.

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.

istio-testing · 2024-05-15T20:51:32Z

@howardjohn: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
test_ztunnel	`c8c232c`	link	unknown	`/test test`

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-sigs/prow repository. I understand the commands that are listed here.

stevenctl · 2024-05-15T21:07:28Z

Do we still want to merge this anytime soon? Sorry I missed it.

ilrudie · 2024-05-15T21:31:17Z

IMO it's worth doing. also, I did not realize that my review would get retroactively upgraded... 😬

months old, will take another look

howardjohn added the release-notes-none Indicates a PR that does not require release notes. label Feb 7, 2024

howardjohn requested a review from a team as a code owner February 7, 2024 19:33

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 7, 2024

howardjohn marked this pull request as draft February 7, 2024 19:33

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Feb 7, 2024

bleggett reviewed Feb 7, 2024

View reviewed changes

src/proxy/metadata.rs Outdated Show resolved Hide resolved

bleggett reviewed Feb 7, 2024

View reviewed changes

src/proxy/metadata.rs Show resolved Hide resolved

ilrudie reviewed Feb 7, 2024

View reviewed changes

src/proxy/outbound.rs Outdated Show resolved Hide resolved

hzxuzhonghu reviewed Feb 8, 2024

View reviewed changes

ilrudie mentioned this pull request Feb 8, 2024

initial improvements for connection_manager #804

Merged

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 27, 2024

howardjohn force-pushed the mds/initial branch from dd717d1 to e54eb0b Compare February 27, 2024 22:45

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 27, 2024

howardjohn marked this pull request as ready for review February 27, 2024 22:57

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Feb 27, 2024

ilrudie reviewed Feb 28, 2024

View reviewed changes

istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 28, 2024

howardjohn added 3 commits February 28, 2024 16:01

Initial connection MDS implementation

501e706

cleanup

7edf705

fmt

c8c232c

howardjohn force-pushed the mds/initial branch from 5f49bc1 to c8c232c Compare February 29, 2024 00:03

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 29, 2024

ilrudie previously approved these changes Feb 29, 2024

View reviewed changes

howardjohn assigned stevenctl Feb 29, 2024

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial connection MDS implementation #802

Initial connection MDS implementation #802

howardjohn commented Feb 7, 2024 •

edited

ilrudie left a comment •

edited

hzxuzhonghu left a comment

hzxuzhonghu Feb 8, 2024

ilrudie Feb 28, 2024

howardjohn Feb 29, 2024

ilrudie left a comment

ilrudie Feb 28, 2024

ilrudie Feb 28, 2024

howardjohn Feb 29, 2024

ilrudie Feb 29, 2024

ilrudie Feb 28, 2024

howardjohn Feb 29, 2024

ilrudie left a comment

istio-testing commented Mar 15, 2024

istio-testing commented May 15, 2024

stevenctl commented May 15, 2024

ilrudie commented May 15, 2024

Initial connection MDS implementation #802

Are you sure you want to change the base?

Initial connection MDS implementation #802

Conversation

howardjohn commented Feb 7, 2024 • edited

ilrudie left a comment • edited

Choose a reason for hiding this comment

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilrudie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilrudie left a comment

Choose a reason for hiding this comment

istio-testing commented Mar 15, 2024

istio-testing commented May 15, 2024

stevenctl commented May 15, 2024

ilrudie commented May 15, 2024

howardjohn commented Feb 7, 2024 •

edited

ilrudie left a comment •

edited