Update deployment sidecar when flags change

[rubenvp8510]

Still need to update tests.

[codecov]

Codecov Report

Merging #961 into master will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #961      +/-   ##
==========================================
+ Coverage   64.44%   64.56%   +0.11%     
==========================================
  Files          82       82              
  Lines        6526     6542      +16     
==========================================
+ Hits         4206     4224      +18     
+ Misses       2178     2177       -1     
+ Partials      142      141       -1     

Impacted Files	Coverage Δ
pkg/inject/sidecar.go	96.64% <100.00%> (+1.55%)	⬆️

---

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 7f45096...def0fa1. Read the comment docs.

[jpkrohling]

Could we try a different approach? Instead of checking whether the container needs updating, you could update the container every time. Kubernetes describes the desired state and will act to make this true, so, updating the container should not yield a new deployment (nor an update) if the container spec hasn't changed.

[rubenvp8510]

Could we try a different approach? Instead of checking whether the container needs updating, you could update the container every time. Kubernetes describes the desired state and will act to make this true, so, updating the container should not yield a new deployment (nor an update) if the container spec hasn't changed.

Yes, this was my first approach, I just need to remove 1 commit.

[rubenvp8510]

I tried the approach of updating the sidecar on each reconciliation process.

It works, but I saw a couple of errors on logs. regarding of deployment update: "the object has been modified; please apply your changes to the latest version and try again".

This happens even if the client.Reader is used. (without cache).

I think this is due the fact that the deployment status (ready, number of replicas etc..) is modified while we are injecting (or updating) the deployment.

@jpkrohling

[jpkrohling]

This still needs tests:

1. ensuring that deployments with explicit sidecars aren't affected by this change
2. ensuring that sidecars are actually being updated
3. extra test: add an assertion to the current TestSidecarNeeded, ensuring that there are only two containers in the deployments (to make sure our update is indeed removing the container).

You might also consider simply replacing the container instead of deleting + adding the container.

[jpkrohling]

LGTM. Perhaps @objectiser or @pavolloffay would also like to take a look before merging?

[objectiser]

LGTM - one minor suggestion.

[objectiser]

nit: Can you move this above dep3, and then before dep3 line have a comment about // Compare with no agent or something like that - at the moment the different criteria being tested are being interleaved.

[rubenvp8510]

There is one thing with this PR. I still observe this:

"the object has been modified; please apply your changes to the latest version and try again".

But the sidecar was injected, I have the theory that this happens due the changes in the deployment status. Not sure how to avoid this.

@jpkrohling @pavolloffay @objectiser

[objectiser]

@rubenvp8510 If you could create a separate issue for the "the object has been modified; please apply your changes to the latest version and try again" message with steps to reproduce? along with any thoughts you have.

Thanks!