ref: use binary flag to enable use of MeshRootCertificate #4871

jaellio · 2022-07-01T16:10:46Z

Description:

Uses a binary flag to enable the use of MeshRootCertificate.
Updates the MRC id to be the MRC name
Passes vault token secret reference options to the controller, injector, and bootstrap

Testing done:

CI
Demo
- verified the ability to use a secret to store the vault token and set the osm.vault.secret.name and osm.vault.secret.key on install

Affected area:

Functional Area
Certificate Management	[ x ]

Please answer the following questions with yes/no.

Does this change contain code from or inspired by another project? no
- Did you notify the maintainers and provide attribution?
Is this a breaking change? no
Has documentation corresponding to this change been updated in the osm-docs repo (if applicable)? docs: add vault token secret reference osm-docs#407

Signed-off-by: jaellio <[email protected]>

charts/osm/values.yaml

steeling · 2022-07-01T16:37:58Z

pkg/certificate/providers/config.go

@@ -44,14 +44,16 @@ var getCA func(certificate.Issuer) (pem.RootCertificate, error) = func(i certifi
 // NewCertificateManager returns a new certificate manager, with an MRC compat client.
 // TODO(4713): Use an informer behind a feature flag.
 func NewCertificateManager(ctx context.Context, kubeClient kubernetes.Interface, kubeConfig *rest.Config, cfg configurator.Configurator,
-	providerNamespace string, options Options, msgBroker *messaging.Broker, ic *informers.InformerCollection, checkInterval time.Duration) (*certificate.Manager, error) {
-	if err := options.Validate(); err != nil {
+	providerNamespace string, certOptions *CertProviderOptions, msgBroker *messaging.Broker, ic *informers.InformerCollection, checkInterval time.Duration) (*certificate.Manager, error) {


nit: instead of fiddling with the options maybe have 2 functions:

NewCertificateManagerFromMRC (takes options as a param, along with informers)

NewLegacyCertificateManager`. (doesn't take options, no ctx, etc)

pkg/certificate/providers/config_test.go

Signed-off-by: jaellio <[email protected]>

codecov-commenter · 2022-07-01T19:34:44Z

Codecov Report

Merging #4871 (bbbc59d) into main (3bf989a) will decrease coverage by 0.06%.
The diff coverage is 58.42%.

@@            Coverage Diff             @@
##             main    #4871      +/-   ##
==========================================
- Coverage   68.78%   68.71%   -0.07%     
==========================================
  Files         220      220              
  Lines       15911    15908       -3     
==========================================
- Hits        10944    10931      -13     
- Misses       4915     4925      +10     
  Partials       52       52

Flag	Coverage Δ
unittests	`68.71% <58.42%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/certificate/fake_manager.go	`67.69% <ø> (-1.43%)`	⬇️
cmd/osm-bootstrap/osm-bootstrap.go	`45.72% <12.50%> (-1.56%)`	⬇️
cmd/osm-controller/osm-controller.go	`15.41% <15.78%> (+0.50%)`	⬆️
pkg/certificate/providers/config.go	`81.36% <100.00%> (+2.13%)`	⬆️
pkg/certificate/providers/options.go	`100.00% <100.00%> (ø)`
pkg/certificate/manager.go	`76.70% <0.00%> (-0.81%)`	⬇️
pkg/certificate/certificate.go	`94.87% <0.00%> (ø)`
... and 1 more

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 3bf989a...bbbc59d. Read the comment docs.

use MRC name for the issuer ID. Signed-off-by: jaellio <[email protected]>

charts/osm/README.md

cmd/cli/install_test.go

keithmattix

LGTM pending CI

requirements to helm template Signed-off-by: jaellio <[email protected]>

shashankram

Looks good overall, other than the comment I left.

shashankram · 2022-07-05T18:21:29Z

charts/osm/values.yaml

+  # -- Experimental values. Behavior is not supported.
+  experimental:
+    # -- Enable the MeshRootCertificate to configure the OSM certificate provider.
+    enableMeshRootCertificate: false


I'd rather have this as a part of the featureFlags spec, which is the way we have been exposing experimental features.

That makes sense to me. Should the flag be added to the MeshConfig?

Only if you want it to be dynamically (post pod startup) configurable.

i'd vote no on that

I originally hesitated to add it as a part of the featureFlag spec since I thought all values in that spec supported dynamic configuration. I've added enableMeshRootCertificate to the featureFlag spec and did not include it in the MeshConfig

charts/osm/values.yaml

steeling · 2022-07-05T19:39:58Z

pkg/certificate/providers/config.go

-	}
-	return vaultClient, id, nil
+
+	return vaultClient, mrc.Name, nil


At this point, i would change the func signature to (issuer, error), and let the manager set the ID on it's own

Updated the signature to no longer return the issuer id.

- Allows manager to set issuer ID - Removes unnecessary newline Signed-off-by: jaellio <[email protected]>

ref: use binary flag to enable use of MeshRootCertificte

b38fe51

Signed-off-by: jaellio <[email protected]>

steeling reviewed Jul 1, 2022

View reviewed changes

charts/osm/values.yaml Outdated Show resolved Hide resolved

steeling reviewed Jul 1, 2022

View reviewed changes

pkg/certificate/providers/config_test.go Show resolved Hide resolved

Add NewCertificateManagerFromMRC instead of modifying options

d9fd7c0

Signed-off-by: jaellio <[email protected]>

jaellio force-pushed the revertMRCOnInstall branch from 5db50ff to bbbc59d Compare July 1, 2022 22:18

jaellio marked this pull request as ready for review July 1, 2022 22:45

jaellio requested review from shashankram, snehachhabria, nojnhuh, draychev, trstringer and keithmattix as code owners July 1, 2022 22:45

Fix failing tests, add tests for NewCertificateManagerFromMRC, and

96df713

use MRC name for the issuer ID. Signed-off-by: jaellio <[email protected]>

jaellio force-pushed the revertMRCOnInstall branch from bbbc59d to 96df713 Compare July 5, 2022 13:41

keithmattix reviewed Jul 5, 2022

View reviewed changes

charts/osm/README.md Outdated Show resolved Hide resolved

cmd/cli/install_test.go Show resolved Hide resolved

jaellio force-pushed the revertMRCOnInstall branch from 51aab9a to 6afb77d Compare July 5, 2022 15:01

keithmattix approved these changes Jul 5, 2022

View reviewed changes

jaellio force-pushed the revertMRCOnInstall branch 4 times, most recently from da93696 to 9227ca9 Compare July 5, 2022 16:32

Use experimental instead of unsafe and add vault token

d5b6645

requirements to helm template Signed-off-by: jaellio <[email protected]>

jaellio force-pushed the revertMRCOnInstall branch from 9227ca9 to d5b6645 Compare July 5, 2022 16:52

shashankram suggested changes Jul 5, 2022

View reviewed changes

steeling reviewed Jul 5, 2022

View reviewed changes

steeling approved these changes Jul 5, 2022

View reviewed changes

- Adds enableMeshRootCertificate to featureFlag values

b5855b0

- Allows manager to set issuer ID - Removes unnecessary newline Signed-off-by: jaellio <[email protected]>

jaellio force-pushed the revertMRCOnInstall branch from 8323bfb to b5855b0 Compare July 5, 2022 20:03

shashankram approved these changes Jul 5, 2022

View reviewed changes

jaellio mentioned this pull request Jul 5, 2022

Modify release notes #4865

Merged

nojnhuh approved these changes Jul 5, 2022

View reviewed changes

jaellio merged commit aa1abf1 into openservicemesh:main Jul 5, 2022

This was referenced Aug 25, 2022

Remove MRCCompatClient #5040

Closed

Add EnableMeshRootCertificate to MeshConfig #5041

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref: use binary flag to enable use of MeshRootCertificate #4871

ref: use binary flag to enable use of MeshRootCertificate #4871

jaellio commented Jul 1, 2022 •

edited

Loading

steeling Jul 1, 2022

codecov-commenter commented Jul 1, 2022 •

edited

Loading

keithmattix left a comment

shashankram left a comment

shashankram Jul 5, 2022

jaellio Jul 5, 2022

shashankram Jul 5, 2022

steeling Jul 5, 2022

jaellio Jul 5, 2022

steeling Jul 5, 2022

jaellio Jul 5, 2022

ref: use binary flag to enable use of MeshRootCertificate #4871

ref: use binary flag to enable use of MeshRootCertificate #4871

Conversation

jaellio commented Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Jul 1, 2022 • edited Loading

Codecov Report

keithmattix left a comment

Choose a reason for hiding this comment

shashankram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaellio commented Jul 1, 2022 •

edited

Loading

codecov-commenter commented Jul 1, 2022 •

edited

Loading