Merge pull request #3 from Skyscanner/linting

Enabling golangci-lint and addresing its issues

.golangci.yml

@@ -40,8 +40,6 @@ linters-settings:
     check-shadowing: true
   maligned:
     suggest-new: true
-  misspell:
-    locale: UK
 
 linters:
   disable-all: true

.pre-commit-config.yaml

@@ -8,6 +8,13 @@ repos:
           - --allow-multiple-documents
       - id: end-of-file-fixer
       - id: trailing-whitespace
+  - repo: https://github.com/golangci/golangci-lint
+    rev: v1.29.0
+    hooks:
+      - id: golangci-lint
+        args:
+        - --timeout
+        - 2m
   - repo: local
     hooks:
       - id: fmt

Makefile

@@ -66,7 +66,7 @@ vet:
 
 # Run golangci-lint against code
 lint: vet
-	# golangci-lint run --fix
+	golangci-lint run --fix --timeout 2m
 
 # Generate code
 generate: controller-gen

api/v1alpha1/kmsissuer_types.go

@@ -21,7 +21,7 @@ import (
 )
 
 // EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!
-// NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialized.
+// NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialised.
 
 // Condition reasons
 const (
@@ -37,7 +37,7 @@ type KMSIssuerSpec struct {
 	// deletion. When unspecified, a RSA 2048 key is created and managed by
 	// the operator.
 	// +optional
-	KeyId string `json:"keyId,omitempty"`
+	KeyID string `json:"keyId,omitempty"`
 
 	// CommonName is a common name to be used on the Certificate.
 	// The CommonName should have a length of 64 characters or fewer to avoid

api/v1alpha1/kmskey_types.go

@@ -28,7 +28,7 @@ const (
 )
 
 // EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!
-// NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialized.
+// NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialised.
 
 // KMSKeySpec defines the desired state of KMSKey
 type KMSKeySpec struct {
@@ -59,7 +59,7 @@ type KMSKeySpec struct {
 type KMSKeyStatus struct {
 	Status `json:",inline"`
 	// KeyID is the unique identifier for the customer master key (CMK)
-	KeyId string `json:"keyId,omitempty"`
+	KeyID string `json:"keyId,omitempty"`
 }
 
 // +kubebuilder:object:root=true

api/v1alpha1/status.go

@@ -102,13 +102,12 @@ func (status *Status) GetCondition(condType ConditionType) *Condition {
 
 // SetCondition adds/replaces the given condition in the KMSIssuer status. If the condition that we
 // are about to add already exists and has the same status and reason then we are not going to update.
-func (status *Status) SetCondition(condition Condition) {
+func (status *Status) SetCondition(condition *Condition) {
 	currentCond := status.GetCondition(condition.Type)
 	if currentCond != nil && currentCond.Status == condition.Status && currentCond.Reason == condition.Reason {
 		return
 	}
-	newConditions := status.filterOutCondition(condition.Type)
-	status.Conditions = append(newConditions, condition)
+	status.Conditions = append(status.filterOutCondition(condition.Type), *condition)
 }
 
 // RemoveCondition removes the condition with the provided type from the replicaset status.

api/v1alpha1/status_test.go

@@ -39,48 +39,52 @@ var _ = Describe("Status", func() {
 		Describe("SetCondition", func() {
 			It("should add a new conditions", func() {
 				status := &Status{}
-				condition := NewCondition(ConditionReady, ConditionFalse, KMSIssuerReasonFailed, "oups")
-				status.SetCondition(condition)
-				Expect(status.Conditions[0]).To(Equal(condition))
+				oups := NewCondition(ConditionReady, ConditionFalse, KMSIssuerReasonFailed, "oups")
+				status.SetCondition(&oups)
+				Expect(status.Conditions[0]).To(Equal(oups))
 				Expect(len(status.Conditions)).To(Equal(1))
 			})
 
 			It("should replace a pre-existing conditions", func() {
 				status := &Status{}
-				status.SetCondition(NewCondition(ConditionReady, ConditionTrue, KMSIssuerReasonIssued, ""))
-				condition := NewCondition(ConditionReady, ConditionFalse, KMSIssuerReasonFailed, "oups")
-				status.SetCondition(condition)
-				Expect(status.Conditions[0]).To(Equal(condition))
+				empty := NewCondition(ConditionReady, ConditionTrue, KMSIssuerReasonIssued, "")
+				status.SetCondition(&empty)
+				oups := NewCondition(ConditionReady, ConditionFalse, KMSIssuerReasonFailed, "oups")
+				status.SetCondition(&oups)
+				Expect(status.Conditions[0]).To(Equal(oups))
 				Expect(len(status.Conditions)).To(Equal(1))
 			})
 
 			It("should not update the condition if it already exists and has the same status and reason.", func() {
 				status := &Status{}
-				condition := NewCondition(ConditionReady, ConditionFalse, KMSIssuerReasonFailed, "because")
-				status.SetCondition(condition)
-				status.SetCondition(NewCondition(ConditionReady, ConditionFalse, KMSIssuerReasonFailed, "othermsg"))
-				Expect(status.Conditions[0]).To(Equal(condition))
+				because := NewCondition(ConditionReady, ConditionFalse, KMSIssuerReasonFailed, "because")
+				status.SetCondition(&because)
+				otherMsg := NewCondition(ConditionReady, ConditionFalse, KMSIssuerReasonFailed, "othermsg")
+				status.SetCondition(&otherMsg)
+				Expect(status.Conditions[0]).To(Equal(because))
 				Expect(len(status.Conditions)).To(Equal(1))
 			})
 		})
 		Describe("GetCondition", func() {
 			It("should return a condition by type", func() {
 				status := &Status{}
-				condition := NewCondition(ConditionReady, ConditionTrue, KMSIssuerReasonIssued, "")
-				status.SetCondition(condition)
-				Expect(*status.GetCondition(ConditionReady)).To(Equal(condition))
+				empty := NewCondition(ConditionReady, ConditionTrue, KMSIssuerReasonIssued, "")
+				status.SetCondition(&empty)
+				Expect(*status.GetCondition(ConditionReady)).To(Equal(empty))
 			})
 		})
 		Describe("IsReady", func() {
 			It("should return true when the Ready Condition is true", func() {
 				status := &Status{}
-				status.SetCondition(NewCondition(ConditionReady, ConditionTrue, KMSIssuerReasonIssued, ""))
+				empty := NewCondition(ConditionReady, ConditionTrue, KMSIssuerReasonIssued, "")
+				status.SetCondition(&empty)
 				Expect(status.IsReady()).To(BeTrue())
 			})
 
 			It("should return false with the Ready Condition is false", func() {
 				status := &Status{}
-				status.SetCondition(NewCondition(ConditionReady, ConditionFalse, KMSIssuerReasonIssued, ""))
+				empty := NewCondition(ConditionReady, ConditionFalse, KMSIssuerReasonIssued, "")
+				status.SetCondition(&empty)
 				Expect(status.IsReady()).To(BeFalse())
 			})
 			It("should return false with the Ready Condition is not set", func() {

controllers/certificaterequest_controller.go

@@ -70,14 +70,14 @@ func (r *CertificateRequestReconciler) Reconcile(req ctrl.Request) (ctrl.Result,
 	// Check the CertificateRequest's issuerRef and if it does not match the api
 	// group name, log a message at a debug level and stop processing.
 	if cr.Spec.IssuerRef.Group != "" && cr.Spec.IssuerRef.Group != kmsiapi.GroupVersion.Group {
-		log.V(4).Info("resource does not specify an issuerRef group name that we are responsible for", "group", cr.Spec.IssuerRef.Group)
+		log.V(4).Info("resource does not specify an issuerRef group name that we are responsible for", "group", cr.Spec.IssuerRef.Group) //nolint:gomnd // TODO: fix when refactoring the logger
 		return ctrl.Result{}, nil
 	}
 
 	// If the certificate data is already set then we skip this request as it
 	// has already been completed in the past.
 	if len(cr.Status.Certificate) > 0 {
-		log.V(4).Info("existing certificate data found in status, skipping already completed CertificateRequest")
+		log.V(4).Info("existing certificate data found in status, skipping already completed CertificateRequest") //nolint:gomnd // TODO: fix when refactoring the logger
 		return ctrl.Result{}, nil
 	}
 
@@ -120,7 +120,7 @@ func (r *CertificateRequestReconciler) Reconcile(req ctrl.Request) (ctrl.Result,
 	}
 
 	signed, err := r.KMSCA.SignCertificate(&kmsca.IssueCertificateInput{
-		KeyId:     issuer.Spec.KeyId,
+		KeyID:     issuer.Spec.KeyID,
 		Parent:    parent,
 		Cert:      cert,
 		PublicKey: cert.PublicKey,
@@ -135,7 +135,7 @@ func (r *CertificateRequestReconciler) Reconcile(req ctrl.Request) (ctrl.Result,
 	return ctrl.Result{}, r.setStatus(ctx, cr, cmmeta.ConditionTrue, cmapi.CertificateRequestReasonIssued, "Certificate issued")
 }
 
-// SetupWithManager initializes the CertificateRequest controller into the
+// SetupWithManager initialises the CertificateRequest controller into the
 // controller runtime.
 func (r *CertificateRequestReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).

controllers/certificaterequest_controller_test.go

@@ -37,7 +37,7 @@ var _ = Context("CertificateRequestReconciler", func() {
 	Describe("when a new CertificateRequest is created", func() {
 		It("should sign the certificate request", func() {
 			By("Creating a KMSIssuer")
-			keyId, err := ca.CreateKey(&kmsca.CreateKeyInput{
+			keyID, err := ca.CreateKey(&kmsca.CreateKeyInput{
 				AliasName: "alias/test-key",
 			})
 			Expect(err).To(BeNil())
@@ -51,7 +51,7 @@ var _ = Context("CertificateRequestReconciler", func() {
 					Namespace: issuerKey.Namespace,
 				},
 				Spec: kmsiapi.KMSIssuerSpec{
-					KeyId:      keyId,
+					KeyID:      keyID,
 					CommonName: "RootCA",
 					Duration:   &metav1.Duration{},
 				},
@@ -77,7 +77,7 @@ var _ = Context("CertificateRequestReconciler", func() {
 				[]byte{1, 1, 1, 1},
 			}
 			exampleURIs := []string{"spiffe://foo.foo.example.net", "spiffe://foo.bar.example.net"}
-			cr, _, err := util.NewCertManagerBasicCertificateRequest(
+			cr, _, err := util.NewCertManagerBasicCertificateRequest( //nolint:staticcheck // TODO: fixed when refactored
 				crKey.Name, issuerKey.Name, "KMSIssuer",
 				&metav1.Duration{
 					Duration: time.Hour * 24 * 90,

controllers/helpers.go

@@ -16,15 +16,6 @@ limitations under the License.
 
 package controllers
 
-func containsString(slice []string, s string) bool {
-	for _, item := range slice {
-		if item == s {
-			return true
-		}
-	}
-	return false
-}
-
 func removeString(slice []string, s string) (result []string) {
 	for _, item := range slice {
 		if item == s {

controllers/kmsissuer_controller.go

@@ -27,7 +27,6 @@ import (
 
 	kmsiapi "github.com/Skyscanner/kms-issuer/api/v1alpha1"
 	"github.com/Skyscanner/kms-issuer/pkg/kmsca"
-	"github.com/aws/aws-sdk-go/service/kms/kmsiface"
 	"github.com/go-logr/logr"
 	core "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -37,8 +36,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 )
 
-var kmsApi kmsiface.KMSAPI
-
 // KMSIssuerReconciler reconciles a KMSIssuer object.
 type KMSIssuerReconciler struct {
 	client.Client
@@ -75,15 +72,15 @@ func (r *KMSIssuerReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 	}
 
 	// validation
-	if len(issuer.Spec.KeyId) == 0 {
-		return ctrl.Result{}, r.manageFailure(ctx, log, issuer, errors.New("INVALID KeyId"), fmt.Sprintf("Not a valid key: %s", issuer.Spec.KeyId))
+	if issuer.Spec.KeyID == "" {
+		return ctrl.Result{}, r.manageFailure(ctx, log, issuer, errors.New("INVALID KeyId"), fmt.Sprintf("Not a valid key: %s", issuer.Spec.KeyID))
 	}
 
 	// Generate ca certificate
 	if len(issuer.Status.Certificate) == 0 {
 		log.Info("generate certificate")
 		cert, err := r.KMSCA.GenerateCertificateAuthorityCertificate(&kmsca.GenerateCertificateAuthorityCertificateInput{
-			KeyId: issuer.Spec.KeyId,
+			KeyID: issuer.Spec.KeyID,
 			Subject: pkix.Name{
 				CommonName: issuer.Spec.CommonName,
 			},
@@ -112,23 +109,19 @@ func (r *KMSIssuerReconciler) SetupWithManager(mgr ctrl.Manager) error {
 func (r *KMSIssuerReconciler) manageSuccess(ctx context.Context, log logr.Logger, issuer *kmsiapi.KMSIssuer) error {
 	reason := kmsiapi.KMSIssuerReasonIssued
 	msg := ""
-	log.Info("successfuly reconciled issuer")
+	log.Info("successfully reconciled issuer")
 	r.Recorder.Event(issuer, core.EventTypeNormal, reason, msg)
-	issuer.Status.SetCondition(kmsiapi.NewCondition(kmsiapi.ConditionReady, kmsiapi.ConditionTrue, reason, msg))
-	if err := r.Client.Status().Update(ctx, issuer); err != nil {
-		return err
-	}
-	return nil
+	ready := kmsiapi.NewCondition(kmsiapi.ConditionReady, kmsiapi.ConditionTrue, reason, msg)
+	issuer.Status.SetCondition(&ready)
+	return r.Client.Status().Update(ctx, issuer)
 }
 
 // manageFailure
 func (r *KMSIssuerReconciler) manageFailure(ctx context.Context, log logr.Logger, issuer *kmsiapi.KMSIssuer, issue error, message string) error {
 	reason := kmsiapi.KMSIssuerReasonFailed
 	log.Error(issue, message)
 	r.Recorder.Event(issuer, core.EventTypeWarning, reason, message)
-	issuer.Status.SetCondition(kmsiapi.NewCondition(kmsiapi.ConditionReady, kmsiapi.ConditionFalse, reason, message))
-	if err := r.Client.Status().Update(ctx, issuer); err != nil {
-		return err
-	}
-	return nil
+	ready := kmsiapi.NewCondition(kmsiapi.ConditionReady, kmsiapi.ConditionFalse, reason, message)
+	issuer.Status.SetCondition(&ready)
+	return r.Client.Status().Update(ctx, issuer)
 }

controllers/kmsissuer_controller_test.go

@@ -46,7 +46,7 @@ var _ = Context("KMSIssuer", func() {
 	Describe("when a new resources is created", func() {
 		It("should sign the intermediate certificate", func() {
 			By("Creating a KMS Key")
-			keyId, err := ca.CreateKey(&kmsca.CreateKeyInput{
+			keyID, err := ca.CreateKey(&kmsca.CreateKeyInput{
 				AliasName: "alias/test-key",
 			})
 			Expect(err).To(BeNil())
@@ -62,7 +62,7 @@ var _ = Context("KMSIssuer", func() {
 					Namespace: key.Namespace,
 				},
 				Spec: kmsiapi.KMSIssuerSpec{
-					KeyId:      keyId,
+					KeyID:      keyID,
 					CommonName: "RootCA",
 					Duration:   &metav1.Duration{},
 				},

controllers/kmskey_controller.go

@@ -98,7 +98,7 @@ func (r *KMSKeyReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 	}
 
 	// Create the KMS key
-	keyId, err := r.KMSCA.CreateKey(&kmsca.CreateKeyInput{
+	keyID, err := r.KMSCA.CreateKey(&kmsca.CreateKeyInput{
 		AliasName:             kmsKey.Spec.AliasName,
 		Description:           kmsKey.Spec.Description,
 		CustomerMasterKeySpec: kmsKey.Spec.CustomerMasterKeySpec,
@@ -108,7 +108,7 @@ func (r *KMSKeyReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 	if err != nil {
 		return ctrl.Result{}, r.manageFailure(ctx, log, kmsKey, err, "Failed to create the kms key")
 	}
-	kmsKey.Status.KeyId = keyId
+	kmsKey.Status.KeyID = keyID
 	if err := r.Client.Status().Update(ctx, kmsKey); err != nil {
 		return ctrl.Result{}, r.manageFailure(ctx, log, kmsKey, err, "Failed to update kmsKey.Status.KeyId")
 	}
@@ -125,23 +125,19 @@ func (r *KMSKeyReconciler) SetupWithManager(mgr ctrl.Manager) error {
 func (r *KMSKeyReconciler) manageSuccess(ctx context.Context, log logr.Logger, kmskey *kmsiapi.KMSKey) error {
 	reason := kmsiapi.KMSKeyReasonIssued
 	msg := ""
-	log.Info("successfuly reconciled kms key")
+	log.Info("successfully reconciled kms key")
 	r.Recorder.Event(kmskey, core.EventTypeNormal, reason, msg)
-	kmskey.Status.SetCondition(kmsiapi.NewCondition(kmsiapi.ConditionReady, kmsiapi.ConditionTrue, reason, msg))
-	if err := r.Client.Status().Update(ctx, kmskey); err != nil {
-		return err
-	}
-	return nil
+	ready := kmsiapi.NewCondition(kmsiapi.ConditionReady, kmsiapi.ConditionTrue, reason, msg)
+	kmskey.Status.SetCondition(&ready)
+	return r.Client.Status().Update(ctx, kmskey)
 }
 
 // manageFailure
-func (r *KMSKeyReconciler) manageFailure(ctx context.Context, log logr.Logger, kmskey *kmsiapi.KMSKey, issue error, message string) error {
+func (r *KMSKeyReconciler) manageFailure(ctx context.Context, log logr.Logger, kmskey *kmsiapi.KMSKey, issue error, msg string) error {
 	reason := kmsiapi.KMSKeyReasonFailed
-	log.Error(issue, message)
-	r.Recorder.Event(kmskey, core.EventTypeWarning, reason, message)
-	kmskey.Status.SetCondition(kmsiapi.NewCondition(kmsiapi.ConditionReady, kmsiapi.ConditionFalse, reason, message))
-	if err := r.Client.Status().Update(ctx, kmskey); err != nil {
-		return err
-	}
-	return nil
+	log.Error(issue, msg)
+	r.Recorder.Event(kmskey, core.EventTypeWarning, reason, msg)
+	ready := kmsiapi.NewCondition(kmsiapi.ConditionReady, kmsiapi.ConditionFalse, reason, msg)
+	kmskey.Status.SetCondition(&ready)
+	return r.Client.Status().Update(ctx, kmskey)
 }

controllers/kmskey_controller_test.go

@@ -86,7 +86,7 @@ var _ = Context("KMSKey", func() {
 			By("Checking the finalizer has been set")
 			Expect(NeedToAddFinalizer(kmsKey)).To(BeFalse())
 			By("Checking the Status.KeyId has been set")
-			Expect(kmsKey.Status.KeyId).NotTo(BeEmpty())
+			Expect(kmsKey.Status.KeyID).NotTo(BeEmpty())
 		})
 	})
 

main.go

@@ -37,6 +37,7 @@ import (
 var (
 	scheme   = runtime.NewScheme()
 	setupLog = ctrl.Log.WithName("setup")
+	port     = 9443
 )
 
 func init() {
@@ -60,7 +61,7 @@ func main() {
 	mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
 		Scheme:             scheme,
 		MetricsBindAddress: metricsAddr,
-		Port:               9443,
+		Port:               port,
 		LeaderElection:     enableLeaderElection,
 		LeaderElectionID:   "8655c165.skyscanner.net",
 	})