-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: refactor certStore and KMP to support multi-tenancy [multi-tenancy PR 10] #1423
feat: refactor certStore and KMP to support multi-tenancy [multi-tenancy PR 10] #1423
Conversation
9fbd0b1
to
d5fdc08
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1423 +/- ##
==========================================
- Coverage 66.76% 66.31% -0.45%
==========================================
Files 116 117 +1
Lines 6030 6089 +59
==========================================
+ Hits 4026 4038 +12
- Misses 1620 1668 +48
+ Partials 384 383 -1 ☔ View full report in Codecov by Sentry. |
a6ed978
to
e5fc08b
Compare
e5fc08b
to
1d6262a
Compare
1d6262a
to
2a04be7
Compare
2a04be7
to
90f835f
Compare
90f835f
to
a95d858
Compare
71df42a
to
2f1d234
Compare
// } | ||
// Note: Scope is utilized for organizing and isolating cert stores. In a Kubernetes (K8s) environment, the scope can be either a namespace or an empty string ("") for cluster-wide cert stores. | ||
ScopedCertStores map[string]map[string][]*x509.Certificate | ||
// Note: The namespace "default" is reserved for cluster-wide scenario. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit (not blocking): I think this is out of date? "default" is not reserved right? It's the EmptyNamespace
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for catching! I did update in my last batch but forgot to push it. Actually as we disuccsed last week, there should be no cluster-wide concept while users keep using certStore since certStore will always prefixed with the namespace (default if users don't provide it). And a cluster-wide verifier could always access certStores in all namespaces as v1.1 supports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some clarification questions, thanks!
} | ||
|
||
if err = verifierAddOrReplace(verifier.Spec, resource, namespace); err != nil { | ||
if err := verifierAddOrReplace(verifier.Spec, resource, constants.EmptyNamespace); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need the namespace var, ? are we moving local from verifier to certStore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We won't need the namespace var since now. In the future, verifier will just keep the original configurations from users, like the ref to certStore/KMP. For the logics matching reference to certStore/KMP, it's moved to certStore/KMP implementation so that we could have different behavior on them. And it would be easier to manage it than splitting namespace fetching, namespace prefix and matching into different places.
// Cluster-wide verifier could access all certStores. | ||
// Note: the cluster-wide behavior is different from KMP as we need to keep the behavior backward compatible. | ||
func hasAccessToStore(ctx context.Context, storeName string) bool { | ||
namespace := ctxUtils.GetNamespace(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clarification , doe ctxUtils.GetNamespace(ctx) get the namespace of the verification request or the verifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fetching the namespace of the request, i.e. the namespace where image is deployed. I should update the comment to be more clear. Thanks for calling it out!
2f1d234
to
d0042d4
Compare
d0042d4
to
2474502
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
…ncy PR 10] (ratify-project#1423) Signed-off-by: akashsinghal <[email protected]>
Description
What this PR does / why we need it:
This is the 10th PR for multi-tenancy support. Please review #1422 first. Check diff between PR 9 and PR 10 at: binbin-li#141
pkg/customresources/certificatestores.go
to support multi-tenancy and backward compatible. MovedprependNamespace
logic toGetCertsFromStore
method. Removed extra namespace mapping fromScopedCertStores
, now the layout of certStore is similar to KMP.pkg/keymanagementprovider/keymanagementprovider.go
to support multi-tenancy.getCertStoreNamespace
from verifier_controller as we will not prepend namespace to certStore configs of Notation Verifiers.prependNamespaceToKMPName
fromcosign/trustpolicy.go
as we will not prepend namespace to store configs of Cosign Verifiers.User Bahavior
Since we don't have a cluster-wide CertStore CRD,
default
namespace will be reserved for cluster-wide CertStore usage.We can notice that behavior is different between certStore and kmp. Because KMP is following the multi-tenancy design but certStore keep the previous behavior to avoid breaking change.
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when the PR gets merged):Fixes #
Type of change
Please delete options that are not relevant.
main
branch)How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration
Checklist:
Post Merge Requirements
Helm Chart Change