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

cert: use MeshRootCertificate on startup #4816

Merged
merged 1 commit into from
Jun 20, 2022

Conversation

keithmattix
Copy link
Contributor

@keithmattix keithmattix commented Jun 13, 2022

Description:
Use MRC to initialize certificate manager on startup. Fixes #4713

Testing done:
Added unit test to verify that the MRC is used during startup

Affected area:

Functional Area
Certificate Management [X]

Please answer the following questions with yes/no.

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

    • Did you notify the maintainers and provide attribution? no
  2. Is this a breaking change? no

  3. Has documentation corresponding to this change been updated in the osm-docs repo (if applicable)? no

@keithmattix keithmattix changed the title Abstract message broker cert: use MeshRootCertificate on startup Jun 13, 2022
pkg/certificate/errors.go Outdated Show resolved Hide resolved
pkg/certificate/fake_manager.go Show resolved Hide resolved
pkg/certificate/manager.go Outdated Show resolved Hide resolved
pkg/certificate/manager.go Show resolved Hide resolved
pkg/certificate/manager.go Outdated Show resolved Hide resolved
pkg/certificate/providers/config.go Outdated Show resolved Hide resolved
pkg/certificate/types.go Show resolved Hide resolved
pkg/configurator/client.go Outdated Show resolved Hide resolved
pkg/configurator/methods.go Outdated Show resolved Hide resolved
pkg/certificate/manager.go Outdated Show resolved Hide resolved
@keithmattix keithmattix force-pushed the abstract-message-broker branch from cf607ad to 821f517 Compare June 14, 2022 19:33
@keithmattix keithmattix marked this pull request as ready for review June 14, 2022 19:33
@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2022

Codecov Report

Merging #4816 (6589808) into main (5ab34a3) will decrease coverage by 0.04%.
The diff coverage is 73.14%.

@@            Coverage Diff             @@
##             main    #4816      +/-   ##
==========================================
- Coverage   68.54%   68.49%   -0.05%     
==========================================
  Files         224      225       +1     
  Lines       16170    16325     +155     
==========================================
+ Hits        11083    11182      +99     
- Misses       5035     5091      +56     
  Partials       52       52              
Flag Coverage Δ
unittests 68.49% <73.14%> (-0.05%) ⬇️

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

Impacted Files Coverage Δ
cmd/osm-controller/osm-controller.go 13.76% <0.00%> (+0.16%) ⬆️
pkg/certificate/types.go 100.00% <ø> (ø)
pkg/certificate/providers/mrc.go 35.48% <35.48%> (ø)
cmd/osm-bootstrap/osm-bootstrap.go 48.05% <50.00%> (+1.22%) ⬆️
pkg/certificate/manager.go 77.43% <65.85%> (-10.36%) ⬇️
pkg/certificate/fake_manager.go 71.64% <94.44%> (+3.89%) ⬆️
pkg/certificate/providers/config.go 76.96% <95.83%> (+1.45%) ⬆️
pkg/certificate/providers/compat.go 71.42% <100.00%> (-28.58%) ⬇️
... and 4 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 5ab34a3...6589808. Read the comment docs.

@keithmattix keithmattix requested review from steeling and jaellio June 15, 2022 20:04
pkg/certificate/providers/config.go Show resolved Hide resolved
},
var mrcClient certificate.MRCClient
if ic == nil || len(ic.List(informers.InformerKeyMeshRootCertificate)) == 0 {
// no MRCs detected; use the compat client
Copy link
Contributor

Choose a reason for hiding this comment

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

If osm-bootstrap always creates a MRC, will this branch ever be hit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaellio can correct me, but I think we're still allowing customers to NOT use MRCs and to keep using MeshConfigs for a couple of releases (configurable somehow through helm?). I'm totally down to remove the logic if that's not the case and we're making a hard cut in v1.2. I'm just trying to be mindful of backwards compatibility

Copy link
Contributor

Choose a reason for hiding this comment

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

Right now a MRC is always created by osm-bootstrap. If we want to support the option of not using the MRC created on startup to configure the Manager, maybe it would be best to make using the MRC an opt in option on install?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have strong opinions either way. If we ARE making a hard cut though, we're likely going to want some documentation on the change and instructions on how to switch

pkg/certificate/manager.go Outdated Show resolved Hide resolved
pkg/certificate/providers/mrc.go Outdated Show resolved Hide resolved
pkg/certificate/providers/mrc.go Outdated Show resolved Hide resolved
@keithmattix keithmattix requested a review from nojnhuh June 15, 2022 22:06
pkg/certificate/types.go Show resolved Hide resolved
pkg/certificate/providers/mrc.go Show resolved Hide resolved
pkg/certificate/providers/mrc.go Show resolved Hide resolved
@keithmattix keithmattix force-pushed the abstract-message-broker branch 2 times, most recently from 0ef747d to 4626499 Compare June 17, 2022 01:54
@keithmattix keithmattix requested a review from steeling June 17, 2022 16:55
pkg/certificate/manager.go Show resolved Hide resolved
pkg/certificate/manager.go Show resolved Hide resolved
pkg/certificate/manager.go Outdated Show resolved Hide resolved
pkg/certificate/manager.go Outdated Show resolved Hide resolved
pkg/certificate/providers/mrc.go Outdated Show resolved Hide resolved
pkg/certificate/providers/mrc.go Outdated Show resolved Hide resolved
@keithmattix keithmattix force-pushed the abstract-message-broker branch from 6589808 to 34030b7 Compare June 17, 2022 19:53
@keithmattix keithmattix requested a review from nojnhuh June 17, 2022 19:55
Copy link
Contributor

@nojnhuh nojnhuh left a comment

Choose a reason for hiding this comment

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

LGTM once conflicts are resolved.

By default, read MRCs from the cluster in order to build out the
certificate manager. From there, allow the certificate manager to watch
for changes to the MRCs in the cluster

Signed-off-by: Keith Mattix II <[email protected]>
@keithmattix keithmattix force-pushed the abstract-message-broker branch from 34030b7 to c284106 Compare June 17, 2022 20:08
@jaellio jaellio merged commit 30885c9 into openservicemesh:main Jun 20, 2022
@keithmattix keithmattix deleted the abstract-message-broker branch June 22, 2022 19:11
@jaellio jaellio mentioned this pull request Aug 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use MeshRootCertificate resource to configure OSM certificate manager on startup
5 participants