Initial codebase #1

jakexks · 2020-11-04T15:28:17Z

No description provided.

Signed-off-by: Jake Sanders <i@am.so-aweso.me>

JoshVanL

Submitting now before I loose everything 😅

api/v1alpha1/googlecasclusterissuer_types.go

JoshVanL · 2020-11-05T11:57:31Z

api/v1alpha1/googlecasissuer_types.go

+type GoogleCASIssuerSpec struct {
+	// INSERT ADDITIONAL SPEC FIELDS - desired state of cluster
+	// Important: Run "make" to regenerate code after modifying this file
+
+	// Project is the Google Cloud Project ID
+	Project string `json:"project,omitempty"`
+
+	// Location is the Google Cloud Project Location
+	Location string `json:"location,omitempty"`
+
+	// CertificateAuthorityID is The ID of the Google Private certificate authority that will sign certificates
+	CertificateAuthorityID string `json:"certificateAuthorityID,omitempty"`
+
+	// Credentials is a reference to a Kubernetes Secret Key that contains Google Service Account Credentials
+	// +optional
+	Credentials SecretKeySelector `json:"credentials,omitempty"`
+}


Looks like we can use the same Spec struct for both cluster and non cluster scoped Issuers

If the secret namespace is set on a flag, they become identical

api/v1alpha1/googlecasissuer_types.go

JoshVanL · 2020-11-05T12:38:44Z

controllers/certificaterequest_controller.go

+import (
+	"bytes"
+	"context"
+	"errors"
+	"fmt"
+	"github.com/go-logr/logr"
+	apiutil "github.com/jetstack/cert-manager/pkg/api/util"
+	cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1"
+	cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1"
+	api "github.com/jetstack/google-cas-issuer/api/v1alpha1"
+	"github.com/jetstack/google-cas-issuer/util/cas"
+	core "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/tools/record"
+	"math/rand"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)


nit: Break out imports into blocks;

local
external
internal

JoshVanL · 2020-11-05T12:40:00Z

controllers/certificaterequest_controller.go

+	if err := r.Client.Get(ctx, req.NamespacedName, cr); err != nil {
+		if apierrors.IsNotFound(err) {
+			return ctrl.Result{}, nil
+		}
+
+		log.Error(err, "failed to retrieve CertificateRequest resource")
+		return ctrl.Result{}, err
+	}


nit: I prefer the err blocks to be two distinct blocks, rather than in the same if: https://github.com/jetstack/cert-manager/blob/d4c9a7beb4ab7c734c84f2f3a46b9613997e6172/pkg/controller/certificaterequests/sync.go#L74

controllers/googlecasissuer_controller.go

main.go

JoshVanL · 2020-11-05T12:59:18Z

main.go

+import (
+	"flag"
+	cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1"
+	"os"
+
+	"k8s.io/apimachinery/pkg/runtime"
+	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
+	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/log/zap"
+
+	issuersv1alpha1 "github.com/jetstack/google-cas-issuer/api/v1alpha1"
+	"github.com/jetstack/google-cas-issuer/controllers"
+	// +kubebuilder:scaffold:imports
+)
+


nit: imports

JoshVanL · 2020-11-05T12:59:58Z

main.go

+	// +kubebuilder:scaffold:scheme
+}
+
+func main() {


nit: move business logic into /cmd/app or something similar. main should contain the bare minimum to execute an app.

main.go

JoshVanL

Made a few more comments, let me know if it doesn't make sense!

/assign @jakexks

JoshVanL · 2020-11-05T14:18:14Z

controllers/googlecasclusterissuer_controller.go

+)
+
+// ClusterIssuers is a store for reconciled ClusterIssuers
+var ClusterIssuers *sync.Map


sync.Map is slow. Can we safely use a map[string]struct{} instead?

JoshVanL · 2020-11-05T14:19:45Z

controllers/googlecasclusterissuer_controller.go

+			r.setStatusCondition(ctx, log, &issuer, issuersv1alpha1.IssuerConditionReady, issuersv1alpha1.ConditionFalse, "CASError", "Cas error: "+err.Error())
+			return ctrl.Result{}, err
+		}
+		ClusterIssuers.Store(req.NamespacedName, casClient)


Are these client keeping a long lived connection for the entire issue lifecycle? What happens if the client looses connection?

I'm not against having long lived for the entire lifecycle, but we should be able to properly handle loosing connection etc., and mark the issuer as not ready.

JoshVanL · 2020-11-05T14:20:23Z

controllers/googlecasissuer_controller.go

+func (r *GoogleCASIssuerReconciler) setStatusCondition(ctx context.Context,
+	log logr.Logger,
+	issuer *issuersv1alpha1.GoogleCASIssuer,
+	conditionType issuersv1alpha1.GoogleCASIssuerConditionType,
+	status issuersv1alpha1.ConditionStatus,
+	reason string,
+	message string) {
+	newCondition := issuersv1alpha1.GoogleCASIssuerCondition{
+		Type:    conditionType,
+		Status:  status,
+		Reason:  reason,
+		Message: message,
+	}
+	nowTime := metav1.NewTime(time.Now())
+	newCondition.LastTransitionTime = &nowTime
+	// Search through existing conditions
+	for i, cond := range issuer.Status.Conditions {
+		// Skip unrelated conditions
+		if cond.Type != conditionType {
+			continue
+		}
+
+		// If this update doesn't contain a state transition, we don't update
+		// the conditions LastTransitionTime to Now()
+		if cond.Status == status {
+			newCondition.LastTransitionTime = cond.LastTransitionTime
+		} else {
+			log.Info("Updating last transition time for issuer "+issuer.Name, "condition", conditionType, "old_status", cond.Status, "new_status", status, "time", nowTime.Time)
+		}
+
+		// Overwrite the existing condition
+		issuer.Status.Conditions[i] = newCondition
+		if err := r.Client.Status().Update(ctx, issuer); err != nil {
+			log.Info("Couldn't update issuer condition:", "err", err)
+		}
+		return
+	}
+}


Would be nice to see some table tests for these condition settings.

main.go

JoshVanL · 2020-11-05T14:22:26Z

main.go

+
+	ctrl.SetLogger(zap.New(zap.UseDevMode(true)))
+
+	mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{


Are we exposing flags for the kube client config?

JoshVanL · 2020-11-05T14:33:10Z

util/cas/certificateauthority.go

+// CreateCA creates a new certificate authority
+func (c *casClient) CreateCA(options *CreateCAOptions) error {
+	// TODO: should the timeout be configurable?
+	timeout, cancel := context.WithTimeout(context.Background(), 30*time.Second)


timeout -> ctx

JoshVanL · 2020-11-05T14:35:06Z

util/cas/certificateauthority.go

+	if err != nil {
+		return err
+	}
+	_, err = operation.Wait(timeout)


_, err = operation.Wait(timeout); if err != nil

Can you add a comment to what this is doing? It seems like it is waiting for either a success/fail of the call, or the timeout context to expire, but this timeout has already been passed to the previous call.
It would make sense to change the CreateCertificateAuthority to pass in the original context, and then create a "timeout" context, just for this Wait call.

JoshVanL · 2020-11-05T14:35:19Z

util/cas/certificateauthority.go

+}
+
+// ListCA returns a list of all certificate authorities we have access to
+func (c *casClient) ListCA(options *ListCAOptions) ([]string, error) {


Pass in context

JoshVanL · 2020-11-05T14:36:30Z

util/cas/certificate.go

+
+// CreateCertificate Signs a certificate
+func (c *casClient) CreateCertificate(options *CreateCertificateOptions) (string, []string, error) {
+	timeout, cancel := context.WithTimeout(context.Background(), 30*time.Second)


Pass in context to this func. Not sure a timeout is really required here.

JoshVanL · 2020-11-05T14:37:15Z

main.go

+	// +kubebuilder:scaffold:builder
+
+	setupLog.Info("starting manager")
+	if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil {


We should create a context here based upon the signal handler, this can then be passed and wrapped to all other recievers.

Signed-off-by: Jake Sanders <i@am.so-aweso.me>

james-w · 2020-11-11T09:46:44Z

Merging so we can make this public, we can make changes later.

/lgtm

jetstack-bot added the size/XXL label Nov 4, 2020

jakexks force-pushed the initial branch from d5815de to 1d5db59 Compare November 4, 2020 15:29

Initial Code

9ecc92a

Signed-off-by: Jake Sanders <i@am.so-aweso.me>

jakexks force-pushed the initial branch from 1d5db59 to 9ecc92a Compare November 4, 2020 16:23

JoshVanL suggested changes Nov 5, 2020

View reviewed changes

jetstack-bot assigned jakexks Nov 5, 2020

addressing some comments

cc336b9

Signed-off-by: Jake Sanders <i@am.so-aweso.me>

jetstack-bot assigned james-w Nov 11, 2020

jetstack-bot added the lgtm label Nov 11, 2020

jetstack-bot merged commit 73acd3b into main Nov 11, 2020

jakexks deleted the initial branch November 11, 2020 10:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial codebase #1

Initial codebase #1

jakexks commented Nov 4, 2020

JoshVanL left a comment

JoshVanL Nov 5, 2020

jakexks Nov 5, 2020

JoshVanL Nov 5, 2020

JoshVanL Nov 5, 2020

JoshVanL Nov 5, 2020

JoshVanL Nov 5, 2020

JoshVanL left a comment

JoshVanL Nov 5, 2020

JoshVanL Nov 5, 2020

JoshVanL Nov 5, 2020

JoshVanL Nov 5, 2020

JoshVanL Nov 5, 2020

JoshVanL Nov 5, 2020

JoshVanL Nov 5, 2020

JoshVanL Nov 5, 2020

JoshVanL Nov 5, 2020

james-w commented Nov 11, 2020


		ctrl.SetLogger(zap.New(zap.UseDevMode(true)))

		mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{

Initial codebase #1

Initial codebase #1

Conversation

jakexks commented Nov 4, 2020

JoshVanL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshVanL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-w commented Nov 11, 2020