fix: unsolicited rollout after upgrade from v0.10->v1.0 when pod was using service account

[jessesuen]

Partially fixes #1356

Signed-off-by: Jesse Suen jesse_suen@intuit.com

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
3.7% Duplication

[codecov]

Codecov Report

Merging #1367 (e4e39e9) into master (0c559a0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1367   +/-   ##
=======================================
  Coverage   81.27%   81.27%           
=======================================
  Files         108      108           
  Lines       10029    10031    +2     
=======================================
+ Hits         8151     8153    +2     
  Misses       1318     1318           
  Partials      560      560           

Impacted Files	Coverage Δ
utils/replicaset/replicaset.go	89.47% <100.00%> (+0.07%)	⬆️

---

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 0c559a0...e4e39e9. Read the comment docs.

[huikang]

Hi, @jessesuen , I am just wondering what if the existing pod has both serviceAccountName and serviceAccount in the template. Will this PR handle the case properly? Thanks.

[khhirani]

LGTM

[jessesuen]

Hi, @jessesuen , I am just wondering what if the existing pod has both serviceAccountName and serviceAccount in the template. Will this PR handle the case properly? Thanks.

First, serviceAccountName and serviceAccount should always be the same value. I just verified that if you try to create a Deployment with different values, serviceAccount will get overridden with value from serviceAccountName.

Given that, serviceAccountName is the only thing we should care about. Users should be able to modify this field (add/remove/update), and the rollout should properly detect this as a change and perform an update.

However, the serviceAccount field in the pod is safe to ignore completely. It is a deprecated kubernetes field which has special treatment in k8s codebase. AFIK, there is not any other field in the pod spec which has this special treatment. The serviceAccount field is auto-populated in a special way (not by pod spec defaulting) because it derives its value from serviceAccountName. We incorrectly detect the absence of serviceAccount in the desired spec as a change in the pod template, which causes the unsolicited update.

NOTE for this to happen, ComputeHash would have had to have changed meaning from underneath us. Something which I will address in a v1.1 specific PR.

[huikang]

Hi, @jessesuen , I am just wondering what if the existing pod has both serviceAccountName and serviceAccount in the template. Will this PR handle the case properly? Thanks.

First, serviceAccountName and serviceAccount should always be the same value. I just verified that if you try to create a Deployment with different values, serviceAccount will get overridden with value from serviceAccountName.

Given that, serviceAccountName is the only thing we should care about. Users should be able to modify this field (add/remove/update), and the rollout should properly detect this as a change and perform an update.

However, the serviceAccount field in the pod is safe to ignore completely. It is a deprecated kubernetes field which has special treatment in k8s codebase. AFIK, there is not any other field in the pod spec which has this special treatment. The serviceAccount field is auto-populated in a special way (not by pod spec defaulting) because it derives its value from serviceAccountName. We incorrectly detect the absence of serviceAccount in the desired spec as a change in the pod template, which causes the unsolicited update.

NOTE for this to happen, ComputeHash would have had to have changed meaning from underneath us. Something which I will address in a v1.1 specific PR.

Thanks for the detailed explanation