-
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
Implement cleanup controller #433
Conversation
e381c77
to
573e9d7
Compare
573e9d7
to
ab421a2
Compare
ab421a2
to
92b9fca
Compare
70b6d4e
to
f5f0878
Compare
5c99f1d
to
9731e4f
Compare
lock sync.Mutex | ||
} | ||
|
||
// Run starts the long-running Reconcile loop that runs on a timer. |
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 is a slight change to the Run
function from the health check resource (https://github.com/hashicorp/consul-k8s/blob/master/connect-inject/health_check_resource.go#L54-L76).
- I refactored the loop so we don't have
h.Reconcile()
called twice. I believe they're logically equivalent but please double check. reconcile()
here doesn't return an error. I did this because we're already logging all the places there are errors insidereconcile()
with a lot of good context as to why the error is occurring and so logging it again here just results in duplicate error logs.- I've also passed in
ConsulScheme
andConsulPort
as separate fields into the resource struct up above rather thanConsulURL
. I did this because we are only using the scheme and port from the URL so it felt weird to pass in the full URL when we're discarding the hostname.
I plan to contribute these changes back to HealthCheckResource
in a separate PR.
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.
Great idea on point 3!
9731e4f
to
291f359
Compare
@@ -493,31 +494,32 @@ namespace_prefix "prefix-" { | |||
} | |||
} | |||
|
|||
// There are three true/false settings so 8 permutations to test. |
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.
Sorry in advance. I implemented these tests via a truth table that covers all permutations.
* Runs by default * Watches for pod delete events and ensures there are no stale Consul service instances * Runs a Reconcile loop on a timer that checks all Consul services * Adds new connect-inject flags: `-enable-cleanup-controller` (default true), `-cleanup-controller-reconcile-period`. * Adds new server-acl-init flag: `-enable-cleanup-controller` (default true) and modifies the ACL templates.
291f359
to
069ce8b
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.
I tried hard to look for something to pick on, but the best i could find was deregister
to de-register
😫
Great work @lkysow
lol I went back and forth on that because my editor was complaining. https://www.dictionary.com/browse/deregister?s=t says it's allowed and it's one less character which is why I went with |
pff..good chance i would have suggested
❤️ |
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.
Looks really good!!! I really appreciate all comments and notes you left along the way - felt like you read my mind in a lot of places!
I've added some comments, but they are non-blocking.
a94f3a3
to
612ca96
Compare
612ca96
to
d9b1268
Compare
* Implement cleanup controller * Runs by default * Watches for pod delete events and ensures there are no stale Consul service instances * Runs a Reconcile loop on a timer that checks all Consul services * Adds new connect-inject flags: `-enable-cleanup-controller` (default true), `-cleanup-controller-reconcile-period`. * Adds new server-acl-init flag: `-enable-cleanup-controller` (default true) and modifies the ACL templates.
service instances
-enable-cleanup-controller
,-cleanup-controller-reconcile-period
.k8s-namespace
meta key to connect service instances-enable-cleanup-controller
(defaulttrue) and modifies the ACL templates.
How I've tested this PR:
svc.json
:curl -X PUT -v --data @svc.json localhost:8500/v1/agent/service/register
How I expect reviewers to test this PR:
ghcr.io/lkysow/consul-k8s-dev:feb09
and see if you can break itChecklist: