-
Notifications
You must be signed in to change notification settings - Fork 345
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
Sidecar not removed #1508
Sidecar not removed #1508
Conversation
384439a
to
fab19c1
Compare
…false Signed-off-by: Marco Freyre <marco.fz85@gmail.com>
… is set to false, added method for remove sidecar when is not needed in deploy controller Signed-off-by: Marco Freyre <marco.fz85@gmail.com>
fab19c1
to
24ca36f
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.
Thank you for the PR! I'll ask @rubenvp8510 to review it as well, but I believe there are a couple of changes necessary. In particular, we should account for the fact that annotations might be at the namespace level.
pkg/inject/sidecar.go
Outdated
_, nsExist := ns.Annotations[Annotation] | ||
|
||
if depExist { | ||
if depExist && !strings.EqualFold(annotationValue, "false") { |
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.
What if the annotation is at the namespace level? Isn't this logic already present in the inject.Desired
?
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.
inject.Desired just test if the annotation is present, but if the annotation exists with a "false" value it still returns "true".
Should i validate the same condition for the namespace?
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'm confident we have a logic somewhere that determines whether a deployment should have a sidecar based on annotations from both the deployment itself and the namespace.
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's present in "inject.select", but doesn't it make more sense for this to be validated in "inject.desired" ?, it is a bit confusing that desired returns "true" when it is not really desired.
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.
True. Would you be able to create an issue to refactor that? Also, it would be great to have an e2e test to assert the behavior of this PR here. It doesn't have to be done as part of this PR, though.
I'd still like to get @rubenvp8510's review before merging, but looks good to me!
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.
True. Would you be able to create an issue to refactor that? Also, it would be great to have an e2e test to assert the behavior of this PR here. It doesn't have to be done as part of this PR, though.
count on it
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 covers both cases, because inject.Needed
is used when we modify the namespace to determine if the deployments that belongs to namespace needs an injection. Same case for individual deployments.
Except that we already have the cleaning logic on namespace but not on individual deployments, this is what this PR is adding.,
I think this modification to the desired behaviour makes sense , but it is a little bit confusing the difference between needed and desired, Indeed inject.Needed
calls inject.Desired.
so I think is enough to expose Needed
and use it on the controllers. Desired could be renamed to desired (lowercase so don't expose outside package) and use only Needed on the contgrollers.
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.
BTW, I think we shouldn't use inject.Select
for validating these cases, inject.Select is just when we are make sure we need to inject something but we need to find which instance we need to inject (and if it exists)
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 refactor is done, can you review @rubenvp8510 ?
Signed-off-by: Marco Freyre <marco.fz85@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #1508 +/- ##
==========================================
- Coverage 87.97% 87.68% -0.29%
==========================================
Files 93 93
Lines 5811 5830 +19
==========================================
Hits 5112 5112
- Misses 533 552 +19
Partials 166 166
Continue to review full report at Codecov.
|
@@ -116,7 +116,15 @@ func (r *ReconcileDeployment) Reconcile(request reconcile.Request) (reconcile.Re | |||
} | |||
|
|||
if !inject.Desired(dep, ns) { |
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 would add this cleaning logic on the else statement of the if inject.Needed(dep, ns) {
@@ -170,6 +178,23 @@ func (r *ReconcileDeployment) Reconcile(request reconcile.Request) (reconcile.Re | |||
return reconcile.Result{}, nil | |||
} | |||
|
|||
func (r *ReconcileDeployment) removeSidecar(ctx context.Context, dep *appsv1.Deployment) { |
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.
Is this the same logic that we used for the namespace case?
Is there a way to do a single function and reuse it for both case? Not need to be part of this PR, but we can have another PR with a re-factoring of this logic.
A couple of comments and one change, I would try to use This is the logic I think both controllers should have (this is pseudo-code):
We might want to have this logic in one place and reuse it for both controllers, but that could be part of a refactor PR. |
Signed-off-by: Marco Freyre <marco.fz85@gmail.com>
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!
Which problem is this PR solving?
Short description of the changes