feat: add core AI MITM proxy daemon #21296
Conversation
11a58c6 to
f095630
Compare
f095630 to
a6abd82
Compare
| Flag: "aiproxy-listen-addr", | ||
| Env: "CODER_AIPROXY_LISTEN_ADDR", | ||
| Value: &c.AI.ProxyConfig.ListenAddr, | ||
| Default: ":8888", |
There was a problem hiding this comment.
Not sure if there is a better standard for a proxy port 🤔
There was a problem hiding this comment.
8080is kinda common because HTTP is 80, but I don't think it matters.
I think we listen on 8080 already for our web frontend in dev mode; might make things complicated.
| Value: &c.AI.ProxyConfig.KeyFile, | ||
| Default: "", | ||
| Group: &deploymentGroupAIProxy, | ||
| YAML: "key_file", |
There was a problem hiding this comment.
Just to confirm, do we need to set this as a secret? Something like: Annotations: serpent.Annotations{}.Mark(annotationSecretKey, "true"),
This is the file path, so the content is the actual secret 🤔
| Value: &c.AI.ProxyConfig.ListenAddr, | ||
| Default: ":8888", | ||
| Group: &deploymentGroupAIProxy, | ||
| YAML: "listen_addr", |
There was a problem hiding this comment.
Not sure if we have a convention for yaml between snake_case and camelCase, as I see some options of aibridge with both, for instance, rateLimit and inject_coder_mcp_tools
There was a problem hiding this comment.
Good catch, it should be underscore.
Would you mind raising an issue to address these inconsistencies with rateLimit?
dannykopping
left a comment
There was a problem hiding this comment.
Looking good 👍
Needs some basic test coverage please, and the renaming that we discussed offline.
| Flag: "aiproxy-listen-addr", | ||
| Env: "CODER_AIPROXY_LISTEN_ADDR", | ||
| Value: &c.AI.ProxyConfig.ListenAddr, | ||
| Default: ":8888", |
There was a problem hiding this comment.
8080is kinda common because HTTP is 80, but I don't think it matters.
I think we listen on 8080 already for our web frontend in dev mode; might make things complicated.
| Value: &c.AI.ProxyConfig.ListenAddr, | ||
| Default: ":8888", | ||
| Group: &deploymentGroupAIProxy, | ||
| YAML: "listen_addr", |
There was a problem hiding this comment.
Good catch, it should be underscore.
Would you mind raising an issue to address these inconsistencies with rateLimit?
| // Decrypt all HTTPS requests via MITM. Requests are forwarded to | ||
| // the original destination without modification for now. | ||
| // TODO(ssncferreira): Route requests to aibridged | ||
| // See https://github.com/coder/internal/issues/1181 |
There was a problem hiding this comment.
In stacked PRs, we sometimes refer to "upstack" and "downstack".
In this comment you could mention that this will be "implemented upstack", to distinguish it from something we're not gonna do now but at some nebulous point in the future.
| return certFile, keyFile | ||
| } | ||
|
|
||
| func TestNew(t *testing.T) { |
There was a problem hiding this comment.
This test is good but it's not validating the actual behaviour.
Can you add a test which exercises the proxy? i.e. that it actually tunnels to some mock handler?
It can be simple since we'll be doing a lot more upstack.
There was a problem hiding this comment.
I was planning to add more detailed tests on the following upstack PRs, but that is a good callout.
Addressed in feca199
No description provided.