Skip to content
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

Merged
merged 24 commits into from
May 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
ede25b9
feat: add Stores interface to wrap operations on namespaced stores
binbin-li Apr 3, 2024
d76e2a9
feat: add Policies interface to wrap operations on namespaced policies
binbin-li Apr 2, 2024
0eb8df5
feat: add KMPManager interface to wrap operations on namespaced kmp
binbin-li Apr 12, 2024
c0b5769
feat: revert extra namespace mapping
binbin-li Apr 19, 2024
05e34fa
Merge branch 'dev' into multi-tenancy-pr-6
binbin-li Apr 20, 2024
6e69159
Merge remote-tracking branch 'upstream/dev' into multi-tenancy-pr-6
binbin-li Apr 25, 2024
5dc63d0
feat: add context to GetKeys
binbin-li Apr 25, 2024
7d93b96
feat: add ClusterPolicy CRD
binbin-li Apr 17, 2024
cf7c563
chore: address comments
binbin-li Apr 24, 2024
7521fb2
feat: add NamespacedStore CRD
binbin-li Apr 23, 2024
db364e6
Merge remote-tracking branch 'upstream/dev' into multi-tenancy-pr-8
binbin-li Apr 26, 2024
80a229e
Merge branch 'dev' into multi-tenancy-pr-8
binbin-li Apr 28, 2024
d59067a
Merge remote-tracking branch 'upstream/dev' into multi-tenancy-pr-8
binbin-li Apr 29, 2024
13131f7
chore: remove deprecated tests
binbin-li Apr 29, 2024
d06b1a0
feat: add NamespacedKMP and switch KMP scope to cluster
binbin-li Apr 29, 2024
e856f53
Merge remote-tracking branch 'upstream/dev' into multi-tenancy-pr-9
binbin-li Apr 29, 2024
96f8ad3
Merge branch 'dev' into multi-tenancy-pr-9
binbin-li Apr 30, 2024
4d68169
Merge remote-tracking branch 'upstream/dev' into multi-tenancy-pr-9
binbin-li Apr 30, 2024
7321d5c
Merge branch 'dev' into multi-tenancy-pr-9
binbin-li May 1, 2024
a95d858
feat: refactor certStore to support multi-tenancy
binbin-li Apr 29, 2024
f573a21
Merge remote-tracking branch 'upstream/dev' into multi-tenancy-pr-10
binbin-li May 2, 2024
bc16d4c
feat: remove default namespace setting
binbin-li May 2, 2024
f236803
feat: return errors when failing to access kmp/certStore
binbin-li May 3, 2024
2474502
Merge remote-tracking branch 'upstream/dev' into multi-tenancy-pr-10
binbin-li May 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions charts/ratify/templates/inline-key-management-provider.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
---
{{- if .Values.notationCert }}
apiVersion: config.ratify.deislabs.io/v1beta1
kind: NamespacedKeyManagementProvider
kind: KeyManagementProvider
metadata:
name: {{$fullname}}-notation-inline-cert
annotations:
Expand All @@ -17,7 +17,7 @@ spec:
---
{{- range $i, $cert := .Values.notationCerts }}
apiVersion: config.ratify.deislabs.io/v1beta1
kind: NamespacedKeyManagementProvider
kind: KeyManagementProvider
metadata:
name: {{$fullname}}-notation-inline-cert-{{$i}}
annotations:
Expand All @@ -32,7 +32,7 @@ spec:
---
{{- range $i, $key := .Values.cosignKeys }}
apiVersion: config.ratify.deislabs.io/v1beta1
kind: NamespacedKeyManagementProvider
kind: KeyManagementProvider
metadata:
name: {{$fullname}}-cosign-inline-key-{{$i}}
annotations:
Expand Down
4 changes: 2 additions & 2 deletions charts/ratify/templates/verifier.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,10 @@ spec:
{{- end }}
keys:
{{- range $i, $key := .Values.cosignKeys }}
- provider: {{ $.Release.Namespace }}/{{$fullname}}-cosign-inline-key-{{$i}}
- provider: {{$fullname}}-cosign-inline-key-{{$i}}
{{- end }}
{{- if and .Values.azurekeyvault.enabled (gt (len .Values.azurekeyvault.keys) 0) }}
- provider: {{ $.Release.Namespace }}/kmprovider-akv
- provider: kmprovider-akv
{{- end }}
{{- else }}
key: {{ .Values.cosign.key | quote }}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package controllers
package namespaceresource

import (
"context"
Expand All @@ -23,6 +23,7 @@ import (
"github.com/deislabs/ratify/pkg/certificateprovider"
_ "github.com/deislabs/ratify/pkg/certificateprovider/azurekeyvault" // register azure keyvault certificate provider
_ "github.com/deislabs/ratify/pkg/certificateprovider/inline" // register inline certificate provider
"github.com/deislabs/ratify/pkg/controllers"
"github.com/deislabs/ratify/pkg/utils"

"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -63,8 +64,7 @@ func (r *CertificateStoreReconciler) Reconcile(ctx context.Context, req ctrl.Req
if err := r.Get(ctx, req.NamespacedName, &certStore); err != nil {
if apierrors.IsNotFound(err) {
logger.Infof("deletion detected, removing certificate store %v", resource)
// TODO: pass the actual namespace once multi-tenancy is supported.
NamespacedCertStores.DeleteStore(constants.EmptyNamespace, resource)
controllers.NamespacedCertStores.DeleteStore(resource)
} else {
logger.Error(err, "unable to fetch certificate store")
}
Expand Down Expand Up @@ -95,8 +95,7 @@ func (r *CertificateStoreReconciler) Reconcile(ctx context.Context, req ctrl.Req
return ctrl.Result{}, fmt.Errorf("Error fetching certificates in store %v with %v provider, error: %w", resource, certStore.Spec.Provider, err)
}

// TODO: pass the actual namespace once multi-tenancy is supported.
NamespacedCertStores.AddStore(constants.EmptyNamespace, resource, certificates)
controllers.NamespacedCertStores.AddStore(resource, certificates)
isFetchSuccessful = true
emptyErrorString := ""
writeCertStoreStatus(ctx, r, certStore, logger, isFetchSuccessful, emptyErrorString, lastFetchedTime, certAttributes)
Expand Down Expand Up @@ -148,8 +147,8 @@ func writeCertStoreStatus(ctx context.Context, r *CertificateStoreReconciler, ce
func updateErrorStatus(certStore *configv1beta1.CertificateStore, errorString string, operationTime *metav1.Time) {
// truncate brief error string to maxBriefErrLength
briefErr := errorString
if len(errorString) > maxBriefErrLength {
briefErr = fmt.Sprintf("%s...", errorString[:maxBriefErrLength])
if len(errorString) > constants.MaxBriefErrLength {
briefErr = fmt.Sprintf("%s...", errorString[:constants.MaxBriefErrLength])
}
certStore.Status.IsSuccess = false
certStore.Status.Error = errorString
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package controllers
package namespaceresource

import (
"fmt"
Expand Down
28 changes: 0 additions & 28 deletions pkg/controllers/utils/cert_store.go

This file was deleted.

35 changes: 0 additions & 35 deletions pkg/controllers/utils/cert_store_test.go

This file was deleted.

28 changes: 1 addition & 27 deletions pkg/controllers/verifier_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,10 @@ import (
"context"
"encoding/json"
"fmt"
"os"

configv1beta1 "github.com/deislabs/ratify/api/v1beta1"
"github.com/deislabs/ratify/config"
re "github.com/deislabs/ratify/errors"
"github.com/deislabs/ratify/internal/constants"
"github.com/deislabs/ratify/pkg/utils"
vc "github.com/deislabs/ratify/pkg/verifier/config"
vf "github.com/deislabs/ratify/pkg/verifier/factory"
"github.com/deislabs/ratify/pkg/verifier/types"
Expand Down Expand Up @@ -76,13 +73,7 @@ func (r *VerifierReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
return ctrl.Result{}, client.IgnoreNotFound(err)
}

namespace, err := getCertStoreNamespace(req.Namespace)
if err != nil {
verifierLogger.Error(err, "unable to get default namespace for certstore specified in verifier crd")
return ctrl.Result{}, err
}

if err = verifierAddOrReplace(verifier.Spec, resource, namespace); err != nil {
if err := verifierAddOrReplace(verifier.Spec, resource, constants.EmptyNamespace); err != nil {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

verifierLogger.Error(err, "unable to create verifier from verifier crd")
writeVerifierStatus(ctx, r, &verifier, verifierLogger, false, err.Error())
return ctrl.Result{}, err
Expand Down Expand Up @@ -152,23 +143,6 @@ func (r *VerifierReconciler) SetupWithManager(mgr ctrl.Manager) error {
Complete(r)
}

// Historically certStore defined in trust policy only contains name which means the CertStore cannot be uniquely identified
// If verifierNamespace is not empty, this method returns the default cert store namespace else returns the ratify deployed namespace
func getCertStoreNamespace(verifierNamespace string) (string, error) {
// first, check if we can use the verifier namespace as the cert store namespace
if verifierNamespace != "" {
return verifierNamespace, nil
}

// next, return the ratify deployed namespace
ns, found := os.LookupEnv(utils.RatifyNamespaceEnvVar)
if !found {
return "", re.ErrorCodeEnvNotSet.WithComponentType(re.Verifier).WithDetail(fmt.Sprintf("environment variable %s not set", utils.RatifyNamespaceEnvVar))
}

return ns, nil
}

func writeVerifierStatus(ctx context.Context, r client.StatusClient, verifier *configv1beta1.Verifier, logger *logrus.Entry, isSuccess bool, errorString string) {
if isSuccess {
verifier.Status.IsSuccess = true
Expand Down
25 changes: 0 additions & 25 deletions pkg/controllers/verifier_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,31 +212,6 @@ func TestWriteVerifierStatus(t *testing.T) {
}
}

func TestGetCertStoreNamespace(t *testing.T) {
// error scenario, everything is empty, expect error
_, err := getCertStoreNamespace("")
if err.Error() == "environment variable" {
t.Fatalf("env not set should trigger an error")
}

ratifyDeployedNamespace := "sample"
os.Setenv(utils.RatifyNamespaceEnvVar, ratifyDeployedNamespace)
defer os.Unsetenv(utils.RatifyNamespaceEnvVar)

// scenario1, when default namespace is provided, then we should expect default
verifierNamespace := "verifierNamespace"
ns, _ := getCertStoreNamespace(verifierNamespace)
if ns != verifierNamespace {
t.Fatalf("default namespace expected")
}

// scenario2, default is empty, should return ratify installed namespace
ns, _ = getCertStoreNamespace("")
if ns != ratifyDeployedNamespace {
t.Fatalf("default namespace expected")
}
}

func resetVerifierMap() {
NamespacedVerifiers = verifiers.NewActiveVerifiers()
}
Expand Down
18 changes: 9 additions & 9 deletions pkg/customresources/certificatestores/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,19 @@ limitations under the License.

package certificatestores

import "crypto/x509"
import (
"context"
"crypto/x509"
)

// CertStoreManager is an interface that defines the methods for managing certificate stores across different scopes.
type CertStoreManager interface {
// GetCertStores returns certificates for the given scope.
GetCertStores(scope string) map[string][]*x509.Certificate
// GetCertsFromStore returns certificates from the given certificate store.
GetCertsFromStore(ctx context.Context, storeName string) ([]*x509.Certificate, error)

// AddStore adds the given certificate under the given scope.
AddStore(scope, storeName string, cert []*x509.Certificate)
// AddStore adds the given certificate.
AddStore(storeName string, cert []*x509.Certificate)

// DeleteStore deletes the certificate from the given scope.
DeleteStore(scope, storeName string)

// IsEmpty returns true if there are no certificates.
IsEmpty() bool
DeleteStore(storeName string)
}
Loading
Loading