Merge pull request cert-manager#125 from SgtCoDFish/allsigners

Remove issuerRef from approver

Author: cert-manager-prow[bot]

deploy/charts/csi-driver-spiffe/templates/deployment.yaml

@@ -34,9 +34,6 @@ spec:
           - --csi-driver-name={{ .Values.app.name }}
 
           - --certificate-request-duration={{ .Values.app.certificateRequestDuration }}
-          - --issuer-name={{ .Values.app.issuer.name }}
-          - --issuer-kind={{ .Values.app.issuer.kind }}
-          - --issuer-group={{ .Values.app.issuer.group }}
           - --trust-domain={{ .Values.app.trustDomain }}
 
           - --leader-election-namespace=$(POD_NAMESPACE)

internal/approver/app/app.go

@@ -86,7 +86,6 @@ func NewCommand(ctx context.Context) *cobra.Command {
 			})
 
 			if err := controller.AddApprover(ctx, opts.Logr, controller.Options{
-				IssuerRef: opts.CertManager.IssuerRef,
 				Evaluator: evaluator,
 				Manager:   mgr,
 			}); err != nil {

internal/approver/app/options/options.go

@@ -19,7 +19,6 @@ package options
 import (
 	"time"
 
-	cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1"
 	"github.com/spf13/pflag"
 
 	"github.com/cert-manager/csi-driver-spiffe/internal/flags"
@@ -57,17 +56,12 @@ type OptionsController struct {
 
 // OptionsCertManager are options specific to cert-manager and the evaluator.
 type OptionsCertManager struct {
-	// TrustDomain is the Trust Domain the evaluator will enforce requests request
-	// for.
+	// TrustDomain is the Trust Domain the evaluator will enforce requests request for.
 	TrustDomain string
 
 	// CertificateRequestDuration is the duration the evaluator will enforce
 	// CertificateRequest request for.
 	CertificateRequestDuration time.Duration
-
-	// IssuerRef is the issuer reference that will be used to match on created
-	// CertificateRequests.
-	IssuerRef cmmeta.ObjectReference
 }
 
 func New() *Options {
@@ -85,12 +79,27 @@ func (o *Options) addCertManagerFlags(fs *pflag.FlagSet) {
 	fs.DurationVar(&o.CertManager.CertificateRequestDuration, "certificate-request-duration", time.Hour,
 		"The duration which is enforced for requests to have.")
 
-	fs.StringVar(&o.CertManager.IssuerRef.Name, "issuer-name", "my-spiffe-ca",
-		"Name of issuer which is matched against to evaluate on.")
-	fs.StringVar(&o.CertManager.IssuerRef.Kind, "issuer-kind", "ClusterIssuer",
-		"Kind of issuer which is matched against to evaluate on.")
-	fs.StringVar(&o.CertManager.IssuerRef.Group, "issuer-group", "cert-manager.io",
-		"Group of issuer which is matched against to evaluate on.")
+	// allow issuer-* args to still be passed to avoid a backwards incompatible change
+	var dummyIssuerRefName, dummyIssuerRefKind, dummyIssuerRefGroup string
+
+	fs.StringVar(&dummyIssuerRefName, "issuer-name", "", "Deprecated; value is ignored")
+	fs.StringVar(&dummyIssuerRefKind, "issuer-kind", "", "Deprecated; value is ignored")
+	fs.StringVar(&dummyIssuerRefGroup, "issuer-group", "", "Deprecated; value is ignored")
+
+	err := fs.MarkDeprecated("issuer-name", "issuer-name is deprecated and will be ignored")
+	if err != nil {
+		panic("failed to mark issuer-name flag as deprecated; this is an internal programming error")
+	}
+
+	err = fs.MarkDeprecated("issuer-kind", "issuer-kind is deprecated and will be ignored")
+	if err != nil {
+		panic("failed to mark issuer-kind flag as deprecated; this is an internal programming error")
+	}
+
+	err = fs.MarkDeprecated("issuer-group", "issuer-group is deprecated and will be ignored")
+	if err != nil {
+		panic("failed to mark issuer-group flag as deprecated; this is an internal programming error")
+	}
 }
 
 func (o *Options) addControllerFlags(fs *pflag.FlagSet) {

internal/approver/controller/controller.go

@@ -30,14 +30,11 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
+	"github.com/cert-manager/csi-driver-spiffe/internal/annotations"
 	"github.com/cert-manager/csi-driver-spiffe/internal/approver/evaluator"
 )
 
 type Options struct {
-	// IssuerRef will be used to match against CertificateRequest that need
-	// evaluation.
-	IssuerRef cmmeta.ObjectReference
-
 	// Evaluator will be used to evaluate whether CertificateRequests should be
 	// Approved or Denied.
 	Evaluator evaluator.Interface
@@ -60,10 +57,6 @@ type approver struct {
 	// objects.
 	lister client.Reader
 
-	// issuerRef is the issuerRef that will be matched on CertificateRequests for
-	// evaluation.
-	issuerRef cmmeta.ObjectReference
-
 	// evaluator evaluates matched CertificateRequests for whether they should be
 	// approved or denied.
 	evaluator evaluator.Interface
@@ -75,7 +68,6 @@ func AddApprover(ctx context.Context, log logr.Logger, opts Options) error {
 		log:       log.WithName("controller"),
 		client:    opts.Manager.GetClient(),
 		lister:    opts.Manager.GetCache(),
-		issuerRef: opts.IssuerRef,
 		evaluator: opts.Evaluator,
 	}
 
@@ -103,7 +95,10 @@ func AddApprover(ctx context.Context, log logr.Logger, opts Options) error {
 				return false
 			}
 
-			return req.Spec.IssuerRef == a.issuerRef
+			// We expect that all csi-driver-spiffe certificates will have the identity
+			// annotation, so check for its existence as a final filter
+			_, annotationExists := req.ObjectMeta.Annotations[annotations.SPIFFEIdentityAnnnotationKey]
+			return annotationExists
 		})).
 		Complete(a)
 }

internal/approver/controller/test/suite.go

@@ -30,6 +30,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/metrics/server"
 
+	"github.com/cert-manager/csi-driver-spiffe/internal/annotations"
 	"github.com/cert-manager/csi-driver-spiffe/internal/approver/controller"
 	evaluatorfake "github.com/cert-manager/csi-driver-spiffe/internal/approver/evaluator/fake"
 
@@ -88,7 +89,6 @@ var _ = Context("Approval", func() {
 		Expect(controller.AddApprover(ctx, log, controller.Options{
 			Manager:   mgr,
 			Evaluator: evaluator,
-			IssuerRef: issuerRef,
 		})).NotTo(HaveOccurred())
 
 		By("Running Approver controller")
@@ -108,19 +108,18 @@ var _ = Context("Approval", func() {
 		cancel()
 	})
 
-	It("should ignore CertificateRequest that have the wrong IssuerRef", func() {
+	It("should ignore CertificateRequests that are missing the identity annotation", func() {
 		cr := cmapi.CertificateRequest{
 			ObjectMeta: metav1.ObjectMeta{
 				GenerateName: "cert-manager-csi-driver-spiffe-",
 				Namespace:    namespace.Name,
+				Annotations:  map[string]string{
+					// intentionally left blank
+				},
 			},
 			Spec: cmapi.CertificateRequestSpec{
-				Request: []byte("request"),
-				IssuerRef: cmmeta.ObjectReference{
-					Name:  "not-spiffe-ca",
-					Kind:  "ClusterIssuer",
-					Group: "cert-manager.io",
-				},
+				Request:   []byte("request"),
+				IssuerRef: issuerRef,
 			},
 		}
 		Expect(cl.Create(ctx, &cr)).NotTo(HaveOccurred())
@@ -142,6 +141,9 @@ var _ = Context("Approval", func() {
 			ObjectMeta: metav1.ObjectMeta{
 				GenerateName: "cert-manager-csi-driver-spiffe-",
 				Namespace:    namespace.Name,
+				Annotations: map[string]string{
+					annotations.SPIFFEIdentityAnnnotationKey: "sentinel",
+				},
 			},
 			Spec: cmapi.CertificateRequestSpec{
 				Request:   []byte("request"),
@@ -167,6 +169,9 @@ var _ = Context("Approval", func() {
 			ObjectMeta: metav1.ObjectMeta{
 				GenerateName: "cert-manager-csi-driver-spiffe-",
 				Namespace:    namespace.Name,
+				Annotations: map[string]string{
+					annotations.SPIFFEIdentityAnnnotationKey: "sentinel",
+				},
 			},
 			Spec: cmapi.CertificateRequestSpec{
 				Request:   []byte("request"),

test/e2e/suite/approval/approval.go

@@ -33,6 +33,7 @@ import (
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/cert-manager/csi-driver-spiffe/internal/annotations"
 	"github.com/cert-manager/csi-driver-spiffe/test/e2e/framework"
 
 	. "github.com/onsi/ginkgo/v2"
@@ -107,13 +108,17 @@ var _ = framework.CasesDescribe("Approval", func() {
 
 	It("should approve a valid request", func() {
 		By("Creating valid request")
+		spiffeID := fmt.Sprintf("spiffe://foo.bar/ns/%s/sa/%s", f.Namespace.Name, serviceAccount.Name)
 		certificateRequest := cmapi.CertificateRequest{
 			ObjectMeta: metav1.ObjectMeta{
 				GenerateName: "test-request-",
 				Namespace:    f.Namespace.Name,
+				Annotations: map[string]string{
+					annotations.SPIFFEIdentityAnnnotationKey: spiffeID,
+				},
 			},
 			Spec: cmapi.CertificateRequestSpec{
-				Request:   genCSRPEM(pk, cmapi.ECDSAKeyAlgorithm, fmt.Sprintf("spiffe://foo.bar/ns/%s/sa/%s", f.Namespace.Name, serviceAccount.Name)),
+				Request:   genCSRPEM(pk, cmapi.ECDSAKeyAlgorithm, spiffeID),
 				Duration:  &metav1.Duration{Duration: time.Hour},
 				IssuerRef: f.Config().IssuerRef,
 				IsCA:      false,
@@ -131,13 +136,18 @@ var _ = framework.CasesDescribe("Approval", func() {
 
 	It("should deny a request with the wrong duration", func() {
 		By("Creating request with wrong duration")
+
+		spiffeID := fmt.Sprintf("spiffe://foo.bar/ns/%s/sa/%s", f.Namespace.Name, serviceAccount.Name)
 		certificateRequest := cmapi.CertificateRequest{
 			ObjectMeta: metav1.ObjectMeta{
 				GenerateName: "test-request-",
 				Namespace:    f.Namespace.Name,
+				Annotations: map[string]string{
+					annotations.SPIFFEIdentityAnnnotationKey: spiffeID,
+				},
 			},
 			Spec: cmapi.CertificateRequestSpec{
-				Request:   genCSRPEM(pk, cmapi.ECDSAKeyAlgorithm, fmt.Sprintf("spiffe://foo.bar/ns/%s/sa/%s", f.Namespace.Name, serviceAccount.Name)),
+				Request:   genCSRPEM(pk, cmapi.ECDSAKeyAlgorithm, spiffeID),
 				Duration:  &metav1.Duration{Duration: time.Hour * 3},
 				IssuerRef: f.Config().IssuerRef,
 				IsCA:      false,
@@ -155,13 +165,19 @@ var _ = framework.CasesDescribe("Approval", func() {
 
 	It("should deny a request with the wrong SPIFFE ID", func() {
 		By("Creating request with wrong SPIFFE ID")
+
+		wrongSPIFFEID := fmt.Sprintf("spiffe://foo.bar/ns/%s/sa/%s", f.Namespace.Name, "not-the-right-sa")
+
 		certificateRequest := cmapi.CertificateRequest{
 			ObjectMeta: metav1.ObjectMeta{
 				GenerateName: "test-request-",
 				Namespace:    f.Namespace.Name,
+				Annotations: map[string]string{
+					annotations.SPIFFEIdentityAnnnotationKey: wrongSPIFFEID,
+				},
 			},
 			Spec: cmapi.CertificateRequestSpec{
-				Request:   genCSRPEM(pk, cmapi.ECDSAKeyAlgorithm, fmt.Sprintf("spiffe://foo.bar/ns/%s/sa/%s", f.Namespace.Name, "not-the-right-sa")),
+				Request:   genCSRPEM(pk, cmapi.ECDSAKeyAlgorithm, wrongSPIFFEID),
 				Duration:  &metav1.Duration{Duration: time.Hour},
 				IssuerRef: f.Config().IssuerRef,
 				IsCA:      false,
@@ -179,13 +195,19 @@ var _ = framework.CasesDescribe("Approval", func() {
 
 	It("should deny a request with the wrong key usages", func() {
 		By("Creating request with wrong key usages")
+
+		spiffeID := fmt.Sprintf("spiffe://foo.bar/ns/%s/sa/%s", f.Namespace.Name, serviceAccount.Name)
+
 		certificateRequest := cmapi.CertificateRequest{
 			ObjectMeta: metav1.ObjectMeta{
 				GenerateName: "test-request-",
 				Namespace:    f.Namespace.Name,
+				Annotations: map[string]string{
+					annotations.SPIFFEIdentityAnnnotationKey: spiffeID,
+				},
 			},
 			Spec: cmapi.CertificateRequestSpec{
-				Request:   genCSRPEM(pk, cmapi.ECDSAKeyAlgorithm, fmt.Sprintf("spiffe://foo.bar/ns/%s/sa/%s", f.Namespace.Name, serviceAccount.Name)),
+				Request:   genCSRPEM(pk, cmapi.ECDSAKeyAlgorithm, spiffeID),
 				Duration:  &metav1.Duration{Duration: time.Hour},
 				IssuerRef: f.Config().IssuerRef,
 				IsCA:      false,
@@ -206,13 +228,19 @@ var _ = framework.CasesDescribe("Approval", func() {
 		Expect(err).NotTo(HaveOccurred())
 
 		By("Creating request with wrong key type")
+
+		spiffeID := fmt.Sprintf("spiffe://foo.bar/ns/%s/sa/%s", f.Namespace.Name, serviceAccount.Name)
+
 		certificateRequest := cmapi.CertificateRequest{
 			ObjectMeta: metav1.ObjectMeta{
 				GenerateName: "test-request-",
 				Namespace:    f.Namespace.Name,
+				Annotations: map[string]string{
+					annotations.SPIFFEIdentityAnnnotationKey: spiffeID,
+				},
 			},
 			Spec: cmapi.CertificateRequestSpec{
-				Request:   genCSRPEM(pk, cmapi.RSAKeyAlgorithm, fmt.Sprintf("spiffe://foo.bar/ns/%s/sa/%s", f.Namespace.Name, serviceAccount.Name)),
+				Request:   genCSRPEM(pk, cmapi.RSAKeyAlgorithm, spiffeID),
 				Duration:  &metav1.Duration{Duration: time.Hour},
 				IssuerRef: f.Config().IssuerRef,
 				IsCA:      false,