Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

fix: filter out KeyVault resources during upgrade #4072

Merged
merged 5 commits into from
Dec 2, 2020
Merged

fix: filter out KeyVault resources during upgrade #4072

merged 5 commits into from
Dec 2, 2020

Conversation

jackfrancis
Copy link
Member

Reason for Change:

This PR addresses edge case behavior in aks-engine upgrade when more than one master node and enableEncryptionWithExternalKms is true.

Issue Fixed:

Fixes #4071

Credit Where Due:

Does this change contain code from or inspired by another project?

  • No
  • Yes

If "Yes," did you notify that project's maintainers and provide attribution?

  • No
  • Yes

Requirements:

Notes:

@codecov
Copy link

codecov bot commented Dec 1, 2020 •
edited

Codecov Report

Merging #4072 (bbd5652) into master (11d6609) will decrease coverage by 0.01%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4072      +/-   ##
==========================================
- Coverage   73.19%   73.18%   -0.02%     
==========================================
  Files         135      135              
  Lines       20555    20576      +21     
==========================================
+ Hits        15046    15058      +12     
- Misses       4539     4545       +6     
- Partials      970      973       +3
Impacted Files Coverage Δ
pkg/operations/kubernetesupgrade/upgrader.go 62.16% <0.00%> (-0.49%) ⬇️
pkg/engine/transform/transform.go 71.71% <70.58%> (-0.06%) ⬇️

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 11d6609...bbd5652. Read the comment docs.

@jackfrancis jackfrancis changed the title fix: filter out KeyVault resources during upgrade [WIP] fix: filter out KeyVault resources during upgrade Dec 1, 2020
@jackfrancis jackfrancis changed the title [WIP] fix: filter out KeyVault resources during upgrade fix: filter out KeyVault resources during upgrade Dec 2, 2020
@jadarsie
Copy link
Member

jadarsie commented Dec 2, 2020

I would just add UTs to avoid future regressions, the template manipulation code is not a pleasant read.

I would try to callRemoveKMSResourcesFromTemplate unconditionally from NormalizeResourcesForK8sMasterUpgrade.

Then, TestNormalizeResourcesForK8sMasterUpgrade should cover this scenario.

jackfrancis
jackfrancis commented Dec 2, 2020
View reviewed changes
pkg/engine/transform/transformtestfiles/k8s_slb_template.json
@@ -2298,7 +2298,8 @@
"name": "vmLoopNode"
},
"dependsOn": [
"[concat('Microsoft.Compute/virtualMachines/', variables('masterVMNamePrefix'), copyIndex(variables('masterOffset')))]"
"[concat('Microsoft.Compute/virtualMachines/', variables('masterVMNamePrefix'), copyIndex(variables('masterOffset')))]",
"[concat('Microsoft.KeyVault/vaults/', variables('clusterKeyVaultName'))]"
Copy link
Member Author

Choose a reason for hiding this comment

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

This demonstrates that this dependency will be removed if it's found (k8s_slb_template.json is the input file before we do the translation/filtering)

@jadarsie
Copy link
Member

jadarsie commented Dec 2, 2020

/lgtm

@jackfrancis jackfrancis merged commit 539069f into Azure:master Dec 2, 2020
@jackfrancis jackfrancis deleted the upgrade-kms branch December 2, 2020 22:15
@fmotrifork fmotrifork mentioned this pull request Dec 8, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade fails if enableEncryptionWithExternalKms is true
2 participants
@jackfrancis @jadarsie