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

feat: add csi-secrets-store addon #2936

Merged
merged 7 commits into from
Mar 30, 2020
Merged

Conversation

aramase
Copy link
Member

@aramase aramase commented Mar 19, 2020

Reason for Change:

  • Adds csi-secrets-store addon
  • Enabled by default instead of keyvault-flexvolume for 1.16+ clusters
  • If both keyvault-flexvolume and csi-secrets-store addon enabled simultaneously, then will result in an error
Error: validating API model after populating values: Both keyvault-flexvolume and csi-secrets-store addons are enabled, only one of these may be enabled on a cluster
  • If keyvault-flexvolume addon enabled explicitly for 1.16 clusters, a warning message will be displayed and csi-secrets-store addon will not be enabled.
WARN[0000] keyvault-flexvolume add-on will be DEPRECATED in favor of csi-secrets-store addon for 1.16+ 
  • Disable keyvault-flexvol addon for 1.16+ clusters
  • Failure to enable csi-secrets-store addon for < 1.16 clusters
Error: validating API model after populating values: csi-secrets-store add-on can only be used in 1.16+

Issue Fixed:

fixes #2695

Requirements:

Notes:

@aramase
Copy link
Member Author

aramase commented Mar 19, 2020

/cc @ritazh

@acs-bot acs-bot requested a review from ritazh March 19, 2020 21:48
@aramase aramase force-pushed the csi-secrets-store branch 5 times, most recently from 70c4c46 to ce2c837 Compare March 19, 2020 22:52
@codecov
Copy link

codecov bot commented Mar 19, 2020

Codecov Report

Merging #2936 into master will decrease coverage by 0.47%.
The diff coverage is 94.87%.

@@            Coverage Diff            @@
##           master   #2936      +/-   ##
=========================================
- Coverage   72.58%   72.1%   -0.48%     
=========================================
  Files         141     141              
  Lines       25893   26251     +358     
=========================================
+ Hits        18794   18929     +135     
- Misses       6005    6228     +223     
  Partials     1094    1094

@@ -0,0 +1,287 @@
apiVersion: storage.k8s.io/v1beta1
Copy link
Member

Choose a reason for hiding this comment

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

Because we're installing this by default and it's so much larger in terms of its yaml byte payload compared to keyvault-flexvol, we may run into cloud-init data limits. We'll want to test this with other data-heavy configurations (e.g., calico).

If we do find that this touches the limit we can schedule some cloud-init payload optimization refactor work, but that would become a blocker to this landing.

@ritazh FYI

Copy link
Member Author

Choose a reason for hiding this comment

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

Going to test it now with calico enabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jackfrancis I was able to successfully deploy 1.16 cluster with csi-secrets-store and calico enabled. Any other heavy config that you are aware of we should test with?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for validating! I'm testing across the 1.17 matrix, will report back if any cluster configs complain about customData limits.

Copy link
Member

Choose a reason for hiding this comment

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

Validation passed, so we don't have to worry about this (for now!)

pkg/api/addons.go Outdated Show resolved Hide resolved
pkg/api/addons.go Outdated Show resolved Hide resolved
pkg/api/addons.go Outdated Show resolved Hide resolved
pkg/api/addons.go Outdated Show resolved Hide resolved
@aramase aramase force-pushed the csi-secrets-store branch 2 times, most recently from acff520 to e1839b0 Compare March 28, 2020 03:51
pkg/api/addons.go Outdated Show resolved Hide resolved
Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

/lgtm

@acs-bot acs-bot added the lgtm label Mar 30, 2020
@jackfrancis jackfrancis merged commit 228c9a0 into Azure:master Mar 30, 2020
@acs-bot
Copy link

acs-bot commented Mar 31, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aramase, jackfrancis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@aramase aramase deleted the csi-secrets-store branch March 31, 2020 00:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate keyvault flexvol and switch to csi driver
5 participants