-
Notifications
You must be signed in to change notification settings - Fork 321
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
Add new lifecycle-sidecar command #176
Conversation
3a1dfbb
to
2b0fe35
Compare
b556749
to
ff006ef
Compare
@@ -6,6 +6,8 @@ require ( | |||
github.com/SAP/go-hdb v0.12.1 // indirect |
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.
Go kept updating these for me.
b1eaa5e
to
c1c2b90
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.
Luke, this looks awesome. I did some basic testing on my cluster, and it works great!
I've left a couple of comments and suggestions.
My only other general feedback is to add some sort of integration test to check that this sidecar is actually added to the pod. Currently, we are checking the patch operations in the handler_test.go
, but it would be nice if we could also check the containers that have been added, not sure if that's possible though.
connect-inject/connect_sidecar.go
Outdated
func (h *Handler) connectSidecar(pod *corev1.Pod) corev1.Container { | ||
command := []string{ | ||
"consul-k8s", | ||
"connect-sidecar", |
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.
Maybe consul-service-register-sidecar
or something like that to err on the side of more descriptive.
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.
The command is generically named "connect-sidecar" (i.e. rather than
re-registerer) to make room for more functionality in the future, e.g.
renewing tokens.
We could change that later I suppose, however folks might have tooling built around the name. I don't mind either way.
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.
Ah that's right! How about consul-agent-sidecar
?
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's a bit misleading as consul-agent might make folks think of an actual consul agent
running in there.
connect-inject/connect_sidecar.go
Outdated
// variable. | ||
{ | ||
Name: "CONSUL_HTTP_ADDR", | ||
Value: "$(HOST_IP):8500", |
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's cool, I didn't know you can interpolate env variables in the env
block.
} | ||
var consulAPICalls []APICall | ||
go func() { | ||
err := http.ListenAndServe(fmt.Sprintf(":%d", randomPort), http.HandlerFunc(func(w http.ResponseWriter, r *http.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.
Any reason we don't want to use the consul test agent in these tests?
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.
It was because I didn't think I could configure the port for the Consul test agent and that I could only use whichever port it gave me. This wouldn't work because I need to know the port beforehand so I can start the command before the agent is actually listening on that port.
Thanks to your comment, I tried setting the port in the config and it actually worked so I've changed this to use the test agent now!
services { | ||
id = "service-id-sidecar-proxy" | ||
name = "service-sidecar-proxy" | ||
port = 2000 | ||
kind = "connect-proxy" |
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.
The indentation is a bit off here, and github doesn't let me edit multipe lines.
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.
It had spaces in there and the rest was tabs! #embarrassing
I don't think we can do this without running a Kube cluster. The
// AdmissionResponse describes an admission response.
type AdmissionResponse struct {
...
// The patch body. Currently we only support "JSONPatch" which implements RFC 6902.
// +optional
Patch []byte `json:"patch,omitempty" protobuf:"bytes,4,opt,name=patch"`
} |
8e6cd87
to
3ec7cab
Compare
This command is expected to run in a sidecar in all Connect injected Pods. The command ensures that the service is registered with the node's Consul client by re-registering the service on a configurable period. This is necessary because if the Consul Client is restarted, e.g. during an upgrade, it loses its registrations. With this sidecar, the service will be re-registered once the new Client starts. The command is generically named "lifecycle-sidecar" (i.e. rather than re-registerer) to make room for more functionality in the future, e.g. renewing tokens. This change also updates the mutating webhook code to add the lifecycle-sidecar as a sidecar to Connect injected Pods. It adds a new required flag to connect-inject: `-consul-k8s-image` that controls the image used for the sidecar.
3ec7cab
to
13c0e8c
Compare
This command is expected to run in a sidecar in all Connect
injected Pods. The command ensures that the service is registered with
the node's Consul client by re-registering the service on a configurable
period. This is necessary because if the Consul Client is restarted,
e.g. during an upgrade, it loses its registrations. With this sidecar,
the service will be re-registered once the new Client starts.
The command is generically named "lifecycle-sidecar" (i.e. rather than
re-registerer) to make room for more functionality in the future, e.g.
renewing tokens.
This change also updates the mutating webhook code to add the
connect-sidecar as a sidecar to Connect injected Pods. It adds a new
required flag to connect-inject:
-consul-k8s-image
that controls theimage used for the sidecar.
Fixes #161, #171
Design decisions that deserve scrutiny:
connect-sidecar
) (update this is now lifecycle-sidecar)consul.hashicorp.com/connect-sync-period
10s
)To run yourself
{{ if .Values.connectInject.imageEnvoy -}} -envoy-image="{{ .Values.connectInject.imageEnvoy }}" \ {{ end -}} + -consul-k8s-image="{{ default .Values.global.imageK8S .Values.connectInject.image }}" \
consul-connect-sidecar
container is running