-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Remove Connect Managed Proxies #6220
Conversation
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.
So red! It's great, but I think there are a few more things you can still round up and delete.
e737e10
to
3a4b476
Compare
59ca5d9
to
40702b3
Compare
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.
That was a mammoth sized PR. Overall everything looks in order. I added a few notes thoughout (I will resolve some of them shortly as it will be easier to find them once I complete the review).
CI will catch Envoy integration issues but have you gone through the Connect guide on Learn using the builtin proxy. Thats probably the quickest way to ensure things work as expected.
@@ -248,7 +229,7 @@ func (s *HTTPServer) AgentServices(resp http.ResponseWriter, req *http.Request) | |||
|
|||
// Use empty list instead of nil | |||
for id, s := range services { | |||
agentService := buildAgentService(s, proxies) | |||
agentService := buildAgentService(s) |
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.
Adding this here as a note that we should probably double check the v1/agent/services
output on 1.6.0 beta 3 against this branch when non-managed sidecar proxies are used. I believe all should be well but it would be good to verify that central config still gets factored into the output here.
Given some of my uncertainty around the Are there any unit tests I should be adding here? It seems like some of the CLI flag/env var functionality is untested currently - any suggestions of examples or patterns would be appreciated. |
agent-global config structs and associated tests
e09a737
to
7ece6fb
Compare
There's some future work that may be nice here to consolidate the interface, environment variables and CLI flags common to built-in proxies and Envoy (and any future proxy integrations), but punting on that for now in favor of minimizing optional refactoring in what is a pretty massive PR already. I walked through the Connect tutorial again and the built-in proxy functionality appears to be working just fine, think this should be ready to squash! |
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
Requested changes have been addressed.
"ModifyIndex": 0 | ||
} | ||
], | ||
"passing": [ |
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 you nuked 312->340 by accident.
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.
Opened #6303
Closes #5848
This is feeling big and I definitely need a gut check on if I'm inadvertently removing unrelated things that just happen to reference other proxies.
consul connect envoy
still use the environment variables defined in theproxyprocess
interface?proxyprocess
?ProxyManager
supervisionConnect.Proxy
andConnect.ProxyDefaults
objects be completely gone now? YES, remove from service definition and global config.service.connect.proxy
key in service definitions go away? YES./v1/agent/connect/proxy/:proxy_id
endpoint go away? YES.Agent
still need to be able toverifyProxyToken
andresolveProxyToken
?TestAgentConnectCALeafCert_aclDefaultDeny
) that appear to incidentally configure a managed proxy - how should these tests be refactored?