Skip to content
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.

Sign up for GitHub

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

Jump to bottom

replace deployment reconciler with webhook #1828

Merged
merged 6 commits into from
Mar 31, 2022
Merged

replace deployment reconciler with webhook #1828

merged 6 commits into from
Mar 31, 2022

Conversation

frzifus
Copy link
Member

@frzifus frzifus commented Mar 25, 2022 •
edited
Loading

Which problem is this PR solving?

  • this pr prevents containers with valid annotation from being created without a sidecar and getting immediately replaced. This was caused by the reconciler determining that a sidecar was needed after the pod was already launched.
  • Belongs to Use mutating webhook to inject sidecar #1804

Short description of the changes

  • moved deployment reconciler logic into webhook

cc @rubenvp8510

@frzifus frzifus force-pushed the feat/deployment_webhook branch from f50dddd to 20cecf0 Compare March 25, 2022 17:13
@codecov
Copy link

codecov bot commented Mar 25, 2022 •
edited
Loading

Codecov Report

Merging #1828 (2eb817c) into main (2167682) will increase coverage by 0.86%.
The diff coverage is 88.57%.

❗ Current head 2eb817c differs from pull request most recent head f400d1e. Consider uploading reports for the commit f400d1e to get more accurate results

@@            Coverage Diff             @@
##             main    #1828      +/-   ##
==========================================
+ Coverage   88.18%   89.04%   +0.86%     
==========================================
  Files         101      100       -1     
  Lines        6153     6136      -17     
==========================================
+ Hits         5426     5464      +38     
+ Misses        544      495      -49     
+ Partials      183      177       -6
Impacted Files Coverage Δ
controllers/jaegertracing/jaeger_controller.go 41.66% <0.00%> (-3.79%) ⬇️
pkg/inject/sidecar.go 97.47% <ø> (ø)
controllers/appsv1/deployment_webhook.go 88.65% <88.65%> (ø)
pkg/controller/jaeger/jaeger_controller.go 57.96% <90.47%> (+7.42%) ⬆️
pkg/metrics/instances.go 89.36% <0.00%> (+0.79%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2167682...f400d1e. Read the comment docs.

@frzifus frzifus force-pushed the feat/deployment_webhook branch 4 times, most recently from 59af5f0 to 783c306 Compare March 28, 2022 21:38
rubenvp8510
rubenvp8510 reviewed Mar 29, 2022
View reviewed changes
pkg/controller/jaeger/jaeger_controller.go Outdated
@@ -352,3 +356,75 @@ func (r ReconcileJaeger) getSecretsForNamespace(secrets []corev1.Secret, namespa
}
return secretsForNamespace
}

// SyncOnJaegerChanges sync deployments with sidecars when a jaeger CR changes
func (r *ReconcileJaeger) SyncOnJaegerChanges(object client.Object) []reconcile.Request {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand this function is watching Jaeger CRs for changes, and triggers deployment/namespace reconciliations using a modified annotation.
Why do we need this, Could be a part of the Jaeger CRs reconciliation? what is the difference?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure we can move it up. the function exists only because I have tried to change as little as possible in the existing behavior. But it makes sense. especially considering the potential error when the deployment state has changed.

Copy link
Member Author

@frzifus frzifus Mar 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved syncOnJaegerChanges into the reconcile loop. 👍

see: 72abc37

call syncOnJaegerChanges in jaeger reconciler

This change ensures that in the event of an error, e.g. when
updating a deployment or ns, syncOnJaegerChanges is executed again.

Signed-off-by: Benedikt Bongartz <[email protected]>

@frzifus frzifus force-pushed the feat/deployment_webhook branch 2 times, most recently from 72c69d5 to 955e4c5 Compare March 29, 2022 11:50
@frzifus frzifus requested a review from rubenvp8510 March 29, 2022 11:53
rubenvp8510
rubenvp8510 previously approved these changes Mar 29, 2022
View reviewed changes
Copy link
Collaborator

@rubenvp8510 rubenvp8510 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rubenvp8510 rubenvp8510 enabled auto-merge (squash) March 31, 2022 01:59
frzifus added 6 commits March 31, 2022 09:59
@frzifus
move deployment reconciler logic into deployment webhook
1b8a0c7 
Signed-off-by: Benedikt Bongartz <[email protected]>
@frzifus
remove deployment controller
3a9e26d 
Signed-off-by: Benedikt Bongartz <[email protected]>
@frzifus
port reconciler tests to webhook
cef3512 
- extend tests

Signed-off-by: Benedikt Bongartz <[email protected]>
@frzifus
call syncOnJaegerChanges in jaeger reconciler
d4b87fc 
This change ensures that in the event of an error, e.g. when
updating a deployment or ns, syncOnJaegerChanges is executed again.

Signed-off-by: Benedikt Bongartz <[email protected]>
@frzifus
fix flaky unittest of deployment webhook
32eeb2a 
Signed-off-by: Benedikt Bongartz <[email protected]>
@frzifus
regenerate manifests and csv
f400d1e 
Signed-off-by: Benedikt Bongartz <[email protected]>
auto-merge was automatically disabled March 31, 2022 08:00

Head branch was pushed to by a user without write access

@frzifus frzifus force-pushed the feat/deployment_webhook branch from 2f305f4 to f400d1e Compare March 31, 2022 08:00
@frzifus
Copy link
Member Author

frzifus commented Mar 31, 2022 •
edited
Loading

improved flaky unittest 32eeb2a and rebased branch onto main.

@frzifus frzifus requested a review from rubenvp8510 March 31, 2022 08:03
@rubenvp8510 rubenvp8510 enabled auto-merge (squash) March 31, 2022 13:31
@rubenvp8510 rubenvp8510 merged commit 7f8c23f into jaegertracing:main Mar 31, 2022
@frzifus frzifus deleted the feat/deployment_webhook branch March 31, 2022 13:37
This was referenced Mar 31, 2022
Closed
Closed
This was referenced Mar 31, 2022
Merged
Closed
@rubenvp8510 rubenvp8510 mentioned this pull request Apr 6, 2022
Closed
2 tasks
@czomo
Copy link

czomo commented May 31, 2022

Hey, could you tell me how should work provisioning of tls certificate. I see that it is self signed with custom issuer. Beside that the DNS records points to webhook-services svc. My issue is that controller push log http: TLS handshake error from 100.96.2.0:17965: remote error: tls: bad certificate. I think that issue is that certificate is signed by unknown authority. Is there any workaround for this? @rubenvp8510 @frzifus

@Nerkho Nerkho mentioned this pull request Jun 21, 2022
Merged
6 tasks
@frzifus frzifus mentioned this pull request Oct 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rubenvp8510 rubenvp8510 rubenvp8510 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@frzifus @czomo @rubenvp8510