Webhook refactor #454
Merged
Webhook refactor #454
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thisisnotashwin
changed the base branch from
feature-tproxy
to
endpoints-controller
ishustava
reviewed
Mar 15, 2021
|
The failing tests seem related to the refactor with consul-init. It was failing on the branch and i didnt spend too much time digging into it. |
thisisnotashwin
force-pushed
the
webhook-refactor
branch
from
ad0d781
to
fd1b1c3
Compare
thisisnotashwin
changed the base branch from
endpoints-controller
to
basic-endpoints-controller-and-tests
thisisnotashwin
force-pushed
the
webhook-refactor
branch
5 times, most recently
from
11a22f7
to
5afa987
Compare
ishustava
reviewed
Mar 18, 2021
There was a problem hiding this comment.
Looks great!!! I ❤️ how you cleaned up the handler and that we don't need cert watcher anymore and to run our own webhook server!
I left some minor comments. The only blocking question on my side is whether we should keep the -listen flag backward compatible.
thisisnotashwin
force-pushed
the
webhook-refactor
branch
2 times, most recently
from
6f5f8e7
to
2d1c75b
Compare
kschoche
reviewed
Mar 19, 2021
kschoche
reviewed
Mar 19, 2021
kschoche
reviewed
Mar 19, 2021
kschoche
reviewed
Mar 19, 2021
kschoche
approved these changes
Mar 19, 2021
ndhanushkodi
force-pushed
the
basic-endpoints-controller-and-tests
branch
from
752eca4
to
715854a
Compare
thisisnotashwin
force-pushed
the
webhook-refactor
branch
2 times, most recently
from
9b36f01
to
c2c32fb
Compare
thisisnotashwin
force-pushed
the
webhook-refactor
branch
from
9504d0d
to
2d73c52
Compare
Base automatically changed from
basic-endpoints-controller-and-tests
to
feature-tproxy
March 24, 2021 22:01
* supports using serviceName annotation (different k8s service name) * supports using servicePort annotation * supports creating endpoints (with serviceName annotation) * supports updating endpoints (adding and removing addresses) * supports deleting endpoints (with different k8s service name) todo: * tags, meta, and upstreams from annotations + tests * cleaning up and refactoring code to run the endpoints controller with manager * deleting old service registration * testing with new connect-init command * testing end to end Co-authored-by: Iryna Shustava <iryna@hashicorp.com> Co-authored-by: Kyle Schochenmaier <kyle.schochenmaier@hashicorp.com>
- Replace pointer references with values refernces in methods used by connect-inject for Pods.
- This watcher watches for Consul Agent pods to be in a running phase and the condition ready to be true and then reconcile all endpoints that have a ready/not-ready address that share a node name with that of the consul agent pod.
thisisnotashwin
force-pushed
the
webhook-refactor
branch
from
2d73c52
to
72f21d6
Compare
thisisnotashwin
added a commit
that referenced
this pull request
Mar 26, 2021
* Refactor handler webhook to be of type admission.Webhook - Replace pointer references with values refernces in methods used by connect-inject for Pods. * Add watcher for agent pods to endpoints controller - This watcher watches for Consul Agent pods to be in a running phase and the condition ready to be true and then reconcile all endpoints that have a ready/not-ready address that share a node name with that of the consul agent pod.
thisisnotashwin
added a commit
that referenced
this pull request
Mar 26, 2021
* Refactor handler webhook to be of type admission.Webhook - Replace pointer references with values refernces in methods used by connect-inject for Pods. * Add watcher for agent pods to endpoints controller - This watcher watches for Consul Agent pods to be in a running phase and the condition ready to be true and then reconcile all endpoints that have a ready/not-ready address that share a node name with that of the consul agent pod.
ishustava
pushed a commit
that referenced
this pull request
Apr 14, 2021
* Refactor handler webhook to be of type admission.Webhook - Replace pointer references with values refernces in methods used by connect-inject for Pods. * Add watcher for agent pods to endpoints controller - This watcher watches for Consul Agent pods to be in a running phase and the condition ready to be true and then reconcile all endpoints that have a ready/not-ready address that share a node name with that of the consul agent pod.
ndhanushkodi
pushed a commit
to ndhanushkodi/consul-k8s
that referenced
this pull request
Jul 9, 2021
…nit-role Restrict permissions for the server-acl-init job
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor the connect-inject webhook to use the
admission.Webhooklibrary for it's behavior. The behavior of the webhook should remain unchanged. The difference now is that the webhook does not manage its own certificates. Rather it relies on the webhook cert manager to provision certificates for it as a mounted secret.This allows us to remove our dependency from DiskCerts and also remove the code involved with cert generation on the connect webhook. This is hopefully allow for the a degree of HA with the deployment as the webhook certificates will be consistent across the pods and webhook configuration.
I have also updated some of the methods within the connect package that previously has method signatures with (*Pod) -> (Pod) as they were not performing modifications on the Pod object.
How to test this PR: Code Review
There will be a companion Helm PR where these changes shall be acceptance tested and will try and ensure behavior is still the same.