feat: Add log wrapper for more expressive code (#6)

* feat: Add log wrapper for more expressive code

Signed-off-by: Leonardo Luz Almeida <[email protected]>

* Add docs

Signed-off-by: Leonardo Luz Almeida <[email protected]>

---------

Signed-off-by: Leonardo Luz Almeida <[email protected]>

api/v1alpha1/accessrequest_types.go

@@ -132,11 +132,11 @@ func (ar *AccessRequest) Validate() error {
 	return nil
 }
 
-// IsExpired will return true if this AccessRequest is expired. Otherwise
-// it returns false.
-func (ar *AccessRequest) IsExpired() bool {
+// IsExpiring will return true if this AccessRequest is expired by
+// verifying the .status.ExpiresAt field. Otherwise it returns false.
+func (ar *AccessRequest) IsExpiring() bool {
 	if ar.Status.ExpiresAt != nil &&
-		ar.Status.ExpiresAt.Time.After(time.Now()) {
+		ar.Status.ExpiresAt.Time.Before(time.Now()) {
 		return true
 	}
 	return false

cmd/main.go

@@ -23,6 +23,7 @@ import (
 
 	// Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.)
 	// to ensure that exec-entrypoint and run can make use of them.
+	"go.uber.org/zap/zapcore"
 	_ "k8s.io/client-go/plugin/pkg/client/auth"
 
 	"k8s.io/apimachinery/pkg/runtime"
@@ -69,6 +70,7 @@ func main() {
 		"If set, HTTP/2 will be enabled for the metrics and webhook servers")
 	opts := zap.Options{
 		Development: true,
+		TimeEncoder: zapcore.ISO8601TimeEncoder,
 	}
 	opts.BindFlags(flag.CommandLine)
 	flag.Parse()

internal/controller/accessrequest_controller.go

@@ -27,9 +27,9 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
-	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	api "github.com/argoproj-labs/ephemeral-access/api/v1alpha1"
+	"github.com/argoproj-labs/ephemeral-access/internal/log"
 )
 
 // AccessRequestReconciler reconciles a AccessRequest object
@@ -67,23 +67,23 @@ const (
 // 9. update the accessrequest status to "granted"
 // 10. set the RequeueAfter in Result
 func (r *AccessRequestReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	logger := log.FromContext(ctx).
-		WithValues("controller", "AccessRequest", "reconcile", req.NamespacedName)
+	logger := log.NewFromContext(ctx)
+	logger.Info("Reconciliation started", "bla", "bli")
 
-	logger.Info("retrieving AccessRequest k8s state")
 	ar := &api.AccessRequest{}
 	if err := r.Get(ctx, req.NamespacedName, ar); err != nil {
 		if apierrors.IsNotFound(err) {
 			return ctrl.Result{}, nil
 		}
-		logger.Error(err, "error retrieving AccessRequest from k8s")
+		logger.Error(err, "Error retrieving AccessRequest from k8s")
 		return ctrl.Result{}, err
 	}
 
 	// check if the object is being deleted and properly handle it
+	logger.Debug("Handling finalizer")
 	deleted, err := r.handleFinalizer(ctx, ar)
 	if err != nil {
-		logger.Error(err, "handleFinalizer error")
+		logger.Error(err, "HandleFinalizer error")
 		return ctrl.Result{}, fmt.Errorf("error handling finalizer: %w", err)
 	}
 	// stop the reconciliation as the object was deleted
@@ -92,22 +92,29 @@ func (r *AccessRequestReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 		return ctrl.Result{}, nil
 	}
 
+	logger.Debug("Validating AccessRequest")
 	err = ar.Validate()
 	if err != nil {
-		logger.Info("validation error: %s", err)
+		logger.Info("Validation error: %s", err)
 		return ctrl.Result{}, fmt.Errorf("error validating the AccessRequest: %w", err)
 	}
 
 	// initialize the status if not done yet
 	if ar.Status.RequestState == "" {
+		logger.Debug("Initializing status")
 		ar.UpdateStatus(api.RequestedStatus, "")
 		r.Status().Update(ctx, ar)
 	}
 
-	if ar.IsExpired() {
+	if reconciliationConcluded(ar) {
+		logger.Info("Reconciliation concluded", "status", ar.Status.RequestState)
+		return ctrl.Result{}, nil
+	}
+
+	if ar.IsExpiring() {
 		err := r.handleAccessExpired(ctx, ar)
 		if err != nil {
-			logger.Error(err, "handleAccessExpired error")
+			logger.Error(err, "HandleAccessExpired error")
 			return ctrl.Result{}, fmt.Errorf("error handling access expired: %w", err)
 		}
 		// Stop the reconciliation if the access is expired
@@ -116,15 +123,27 @@ func (r *AccessRequestReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 	}
 
 	// check subject is sudoer
+	logger.Debug("Handling permission")
 	status, err := r.handlePermission(ctx, ar)
 	if err != nil {
-		logger.Error(err, "handlePermission error")
+		logger.Error(err, "HandlePermission error")
 		return ctrl.Result{}, fmt.Errorf("error handling permission: %w", err)
 	}
 
 	return buildResult(status, ar), nil
 }
 
+// reconciliationConcluded will check the status of the given AccessRequest
+// to determine if the reconciliation is concluded.
+func reconciliationConcluded(ar *api.AccessRequest) bool {
+	switch ar.Status.RequestState {
+	case api.DeniedStatus, api.ExpiredStatus:
+		return true
+	default:
+		return false
+	}
+}
+
 // buildResult will verify the given status and determine when this access
 // request should be requeued.
 func buildResult(status api.Status, ar *api.AccessRequest) ctrl.Result {
@@ -159,9 +178,12 @@ func (r *AccessRequestReconciler) handlePermission(ctx context.Context, ar *api.
 	if err != nil {
 		return "", fmt.Errorf("error updating AppProject RBAC: %w", err)
 	}
-	err = r.updateStatusWithRetry(ctx, ar, api.GrantedStatus, resp.Message)
-	if err != nil {
-		return "", fmt.Errorf("error updating access request status to granted: %w", err)
+	// only update the status if transitioning to granted
+	if ar.Status.RequestState != api.GrantedStatus {
+		err = r.updateStatusWithRetry(ctx, ar, api.GrantedStatus, resp.Message)
+		if err != nil {
+			return "", fmt.Errorf("error updating access request status to granted: %w", err)
+		}
 	}
 	return api.GrantedStatus, nil
 }
@@ -193,7 +215,7 @@ type AllowedResponse struct {
 
 // TODO
 func (r *AccessRequestReconciler) Allowed(ctx context.Context, ar *api.AccessRequest) (AllowedResponse, error) {
-	return AllowedResponse{}, nil
+	return AllowedResponse{Allowed: true}, nil
 }
 
 // handleAccessExpired will remove the Argo CD access for the subject and update the
@@ -266,7 +288,7 @@ func (r *AccessRequestReconciler) handleFinalizer(ctx context.Context, ar *api.A
 
 // TODO
 func (r *AccessRequestReconciler) removeArgoCDAccess(ar *api.AccessRequest) error {
-	return fmt.Errorf("cleanupArgoCDAccess not implemented")
+	return nil
 }
 
 // SetupWithManager sets up the controller with the Manager.

internal/log/log.go

@@ -0,0 +1,61 @@
+package log
+
+import (
+	"context"
+
+	logr "github.com/go-logr/logr"
+	k8slog "sigs.k8s.io/controller-runtime/pkg/log"
+)
+
+const (
+	INFO  = 0
+	WARN  = 1
+	DEBUG = 2
+)
+
+// logWrapper provides more expressive methods than the ones provided
+// by the logr.Logger interface abstracting away the usage of numeric
+// log levels.
+type logWrapper struct {
+	Logger logr.Logger
+}
+
+// New will initialize a new log wrapper with the provided logger.
+func New(l logr.Logger) *logWrapper {
+	return &logWrapper{
+		Logger: l,
+	}
+}
+
+// NewFromContext will initialize a new log wrapper extracting the logger
+// from the given context.
+func NewFromContext(ctx context.Context, keysAndValues ...interface{}) *logWrapper {
+	l := k8slog.FromContext(ctx, keysAndValues...)
+	return &logWrapper{
+		Logger: l,
+	}
+}
+
+// Info logs a non-error message with info level. If provided, the given
+// key/value pairs are added in the log entry context.
+func (l *logWrapper) Info(msg string, keysAndValues ...any) {
+	l.Logger.V(INFO).Info(msg, keysAndValues...)
+}
+
+// Warn logs a non-error message with warn level. If provided, the given
+// key/value pairs are added in the log entry context.
+func (l *logWrapper) Warn(msg string, keysAndValues ...any) {
+	l.Logger.V(WARN).Info(msg, keysAndValues...)
+}
+
+// Debug logs a non-error message with debug level. If provided, the given
+// key/value pairs are added in the log entry context.
+func (l *logWrapper) Debug(msg string, keysAndValues ...any) {
+	l.Logger.V(DEBUG).Info(msg, keysAndValues...)
+}
+
+// Error logs an error message. If provided, the given
+// key/value pairs are added in the log entry context.
+func (l *logWrapper) Error(err error, msg string, keysAndValues ...any) {
+	l.Logger.Error(err, msg, keysAndValues...)
+}

test/manifests/accessrequest.yaml

@@ -0,0 +1,13 @@
+apiVersion: ephemeral-access.argoproj-labs.io/v1alpha1
+kind: AccessRequest
+metadata:
+  name: some-application-username
+  namespace: ephemeral
+spec:
+  duration: '1m'
+  targetRoleName: ephemeral-write-access
+  application: 
+    name: someArgoCDApplication
+    namespace: applicationNamespace
+  subjects:
+    - username: [email protected]