Code review feedback

Co-authored-by: Josh Van Leeuwen <joshua.vanleeuwen@jetstack.io>
Signed-off-by: irbekrm <irbekrm@gmail.com>

cmd/controller/app/controller.go

@@ -273,8 +273,8 @@ func buildControllerContext(ctx context.Context, stopCh <-chan struct{}, opts *o
 			DefaultAutoCertificateAnnotations: opts.DefaultAutoCertificateAnnotations,
 		},
 		CertificateOptions: controller.CertificateOptions{
-			EnableOwnerRef:    opts.EnableCertificateOwnerRef,
-			CopiedAnnotations: opts.CopiedAnnotations,
+			EnableOwnerRef:           opts.EnableCertificateOwnerRef,
+			CopiedAnnotationPrefixes: opts.CopiedAnnotationPrefixes,
 		},
 		SchedulerOptions: controller.SchedulerOptions{
 			MaxConcurrentChallenges: opts.MaxConcurrentChallenges,

cmd/controller/app/options/options.go

@@ -110,7 +110,7 @@ type ControllerOptions struct {
 	// Annotations copied Certificate -> CertificateRequest,
 	// CertificateRequest -> Order. Slice of string literals that are
 	// treated as prefixes for annotation keys.
-	CopiedAnnotations []string
+	CopiedAnnotationPrefixes []string
 }
 
 const (
@@ -208,7 +208,7 @@ var (
 	}
 	// Annotations that will be copied from Certificate to CertificateRequest and to Order.
 	// By default, copy all annotations except for the ones applied by kubectl, fluxcd, argocd.
-	defaultCopiedAnnotations = []string{
+	defaultCopiedAnnotationPrefixes = []string{
 		"*",
 		"-kubectl.kubernetes.io/",
 		"-fluxcd.io/",
@@ -337,9 +337,9 @@ func (s *ControllerOptions) AddFlags(fs *pflag.FlagSet) {
 	fs.BoolVar(&s.EnableCertificateOwnerRef, "enable-certificate-owner-ref", defaultEnableCertificateOwnerRef, ""+
 		"Whether to set the certificate resource as an owner of secret where the tls certificate is stored. "+
 		"When this flag is enabled, the secret will be automatically removed when the certificate resource is deleted.")
-	fs.StringSliceVar(&s.CopiedAnnotations, "copied-annotations", defaultCopiedAnnotations, "Annotations that should/shouldn't be copied"+
-		"Certificate -> CertificateRequest, CertificateRequest -> Order. Each value is considered as a prefix for annotation key."+
-		"Prefix annotation with '-' to specify that it should not be copied. Example: '*,-kubectl.kuberenetes.io/'- all annotations"+
+	fs.StringSliceVar(&s.CopiedAnnotationPrefixes, "copied-annotation-prefixes", defaultCopiedAnnotationPrefixes, "Specify which annotations should/shouldn't be copied"+
+		"from Certificate to CertificateRequest and Order by passing a list of annotation key prefixes."+
+		"A prefix starting with a dash(-) specifies an annotation that shouldn't be copied. Example: '*,-kubectl.kuberenetes.io/'- all annotations"+
 		"will be copied apart from the ones where the key is prefixed with 'kubectl.kubernetes.io/'.")
 
 	fs.IntVar(&s.MaxConcurrentChallenges, "max-concurrent-challenges", defaultMaxConcurrentChallenges, ""+

pkg/controller/certificates/requestmanager/requestmanager_controller.go

@@ -68,7 +68,7 @@ type controller struct {
 	client                   cmclient.Interface
 	recorder                 record.EventRecorder
 	clock                    clock.Clock
-	copiedAnnotations        []string
+	copiedAnnotationPrefixes []string
 }
 
 func NewController(
@@ -117,7 +117,7 @@ func NewController(
 		client:                   client,
 		recorder:                 recorder,
 		clock:                    clock,
-		copiedAnnotations:        certificateControllerOptions.CopiedAnnotations,
+		copiedAnnotationPrefixes: certificateControllerOptions.CopiedAnnotationPrefixes,
 	}, queue, mustSync
 }
 
@@ -356,7 +356,7 @@ func (c *controller) createNewCertificateRequest(ctx context.Context, crt *cmapi
 		return err
 	}
 
-	annotations := certificates.BuildAnnotationsToCopy(crt, c.copiedAnnotations)
+	annotations := certificates.BuildAnnotationsToCopy(crt, c.copiedAnnotationPrefixes)
 	annotations[cmapi.CertificateRequestRevisionAnnotationKey] = strconv.Itoa(nextRevision)
 	annotations[cmapi.CertificateRequestPrivateKeyAnnotationKey] = nextPrivateKeySecretName
 	annotations[cmapi.CertificateNameKey] = crt.Name

pkg/controller/certificates/util.go

@@ -312,6 +312,8 @@ func RenewalTime(notBefore, notAfter time.Time, renewBeforeOverride *metav1.Dura
 	return &rt
 }
 
+// BuildAnnotationsToCopy builds a map of annotations from a Certificate that
+// should be copied to the CertificateRequests for that Certificate.
 func BuildAnnotationsToCopy(cert *cmapi.Certificate, copiedAnnotations []string) map[string]string {
 	annotations := make(map[string]string)
 	all := false

pkg/controller/context.go

@@ -149,10 +149,9 @@ type CertificateOptions struct {
 	// EnableOwnerRef controls whether the certificate is configured as an owner of
 	// secret where the effective TLS certificate is stored.
 	EnableOwnerRef bool
-	// Annotations copied Certificate -> CertificateRequest,
-	// CertificateRequest -> Order. Slice of string literals that are
-	// treated as prefixes for annotation keys.
-	CopiedAnnotations []string
+	// CopiedAnnotationPrefixes defines which annotations should be copied
+	// Certificate -> CertificateRequest, CertificateRequest -> Order.
+	CopiedAnnotationPrefixes []string
 }
 
 type SchedulerOptions struct {