Fix: improve operator robustness, performance and code quality (#39)

* fix: prevent nil pointer dereference and silent error handling in overcommit logic

- Return safe no-op values (1.0, 1.0) when errors occur instead of
  continuing with zero values or crashing on nil pointer dereference
- Remove unused getNamespaceYAML function and YAML roundtrip
- Properly handle all error paths in getNamespaceOvercommit and
  checkOvercommitType to avoid mutating pods with incorrect values
- Use direct client.Get for namespace instead of YAML marshal/unmarshal

* fix: add idempotency guard to prevent double overcommit mutation

- Add annotation 'overcommit.inditex.dev/applied' to track if a pod
  has already been mutated by the webhook
- Skip mutation on reinvocation (reinvocationPolicy=IfNeeded) if pod
  was already processed by the same overcommit class
- Store applied CPU/memory ratios in annotations for observability
- Resize operations always re-apply since limits may have changed

* perf: remove unnecessary 10s periodic requeue from controllers

- Remove RequeueAfter=10s from both Overcommit and OvercommitClass
  reconcilers to reduce unnecessary API server load
- Rely on event-driven reconciliation (watches) which is the standard
  controller-runtime pattern for reacting to resource changes

* refactor: propagate context instead of using context.Background/TODO

- Accept context parameter in Overcommit and OvercommitOnResize
  functions, propagated from the webhook admission handler
- Accept context in GetDefaultSpec and GetPodServiceAccount instead
  of creating context.Background()/context.TODO() internally
- Enables proper cancellation, timeouts, and distributed tracing
  through the full call chain

* refactor: remove redundant delegate methods from OvercommitClass

- Remove ~30 getter/setter methods that simply forwarded to the
  embedded metav1.ObjectMeta, which already implements the v1.Object
  interface via Go struct embedding
- Remove unused 'k8s.io/apimachinery/pkg/types' import
- Reduces ~140 lines of dead code improving maintainability

* fix: add ReDoS protection for excludedNamespaces regex validation

- Limit regex length to 512 characters to mitigate catastrophic
  backtracking (ReDoS) attacks via overly complex patterns
- Improve error message to include the actual regexp compilation error
  for better debugging

* fix: reduce TLS certificate duration from 10 years to 1 year

- Change certificate duration from 87600h (10y) to 8760h (1y)
- Shorter-lived certificates reduce the attack surface if a private
  key is compromised, following security best practices
- cert-manager will still auto-renew 30 days before expiry

* refactor: pass context to Overcommit function in tests

* chore: run go mod tidy

* docs: translate Spanish comments to English

* fix: correct certificate duration from 10 years to 1 year in generateResources tests

Author: enriqueavindi

api/v1alphav1/overcommitclass_types.go

@@ -8,7 +8,6 @@ package v1alphav1
 import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
 )
 
 // EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!
@@ -68,146 +67,6 @@ type OvercommitClass struct {
 	Status OvercommitClassStatus `json:"status,omitempty"`
 }
 
-// GetAnnotations implements v1.Object.
-func (in *OvercommitClass) GetAnnotations() map[string]string {
-	return in.ObjectMeta.Annotations
-}
-
-// GetCreationTimestamp implements v1.Object.
-func (in *OvercommitClass) GetCreationTimestamp() metav1.Time {
-	return in.ObjectMeta.CreationTimestamp
-}
-
-// GetDeletionGracePeriodSeconds implements v1.Object.
-func (in *OvercommitClass) GetDeletionGracePeriodSeconds() *int64 {
-	return in.ObjectMeta.DeletionGracePeriodSeconds
-}
-
-// GetDeletionTimestamp implements v1.Object.
-func (in *OvercommitClass) GetDeletionTimestamp() *metav1.Time {
-	return in.ObjectMeta.DeletionTimestamp
-}
-
-// GetFinalizers implements v1.Object.
-func (in *OvercommitClass) GetFinalizers() []string {
-	return in.ObjectMeta.Finalizers
-}
-
-// GetGenerateName implements v1.Object.
-func (in *OvercommitClass) GetGenerateName() string {
-	return in.ObjectMeta.GenerateName
-}
-
-// GetGeneration implements v1.Object.
-func (in *OvercommitClass) GetGeneration() int64 {
-	return in.ObjectMeta.Generation
-}
-
-// GetLabels implements v1.Object.
-func (in *OvercommitClass) GetLabels() map[string]string {
-	return in.ObjectMeta.Labels
-}
-
-// GetManagedFields implements v1.Object.
-func (in *OvercommitClass) GetManagedFields() []metav1.ManagedFieldsEntry {
-	return in.ObjectMeta.ManagedFields
-}
-
-// GetName implements v1.Object.
-func (in *OvercommitClass) GetName() string {
-	return in.ObjectMeta.Name
-}
-
-// GetNamespace implements v1.Object.
-func (in *OvercommitClass) GetNamespace() string {
-	return in.ObjectMeta.Namespace
-}
-
-// GetOwnerReferences implements v1.Object.
-func (in *OvercommitClass) GetOwnerReferences() []metav1.OwnerReference {
-	return in.ObjectMeta.OwnerReferences
-}
-
-// GetResourceVersion implements v1.Object.
-func (in *OvercommitClass) GetResourceVersion() string {
-	return in.ObjectMeta.ResourceVersion
-}
-
-// GetUID implements v1.Object.
-func (in *OvercommitClass) GetUID() types.UID {
-	return in.ObjectMeta.UID
-}
-
-// SetAnnotations implements v1.Object.
-func (in *OvercommitClass) SetAnnotations(annotations map[string]string) {
-	in.ObjectMeta.Annotations = annotations
-}
-
-// SetCreationTimestamp implements v1.Object.
-func (in *OvercommitClass) SetCreationTimestamp(timestamp metav1.Time) {
-	in.ObjectMeta.CreationTimestamp = timestamp
-}
-
-// SetDeletionGracePeriodSeconds implements v1.Object.
-func (in *OvercommitClass) SetDeletionGracePeriodSeconds(seconds *int64) {
-	in.ObjectMeta.DeletionGracePeriodSeconds = seconds
-}
-
-// SetDeletionTimestamp implements v1.Object.
-func (in *OvercommitClass) SetDeletionTimestamp(timestamp *metav1.Time) {
-	in.ObjectMeta.DeletionTimestamp = timestamp
-}
-
-// SetFinalizers implements v1.Object.
-func (in *OvercommitClass) SetFinalizers(finalizers []string) {
-	in.ObjectMeta.Finalizers = finalizers
-}
-
-// SetGenerateName implements v1.Object.
-func (in *OvercommitClass) SetGenerateName(name string) {
-	in.ObjectMeta.GenerateName = name
-}
-
-// SetGeneration implements v1.Object.
-func (in *OvercommitClass) SetGeneration(generation int64) {
-	in.ObjectMeta.Generation = generation
-}
-
-// SetLabels implements v1.Object.
-func (in *OvercommitClass) SetLabels(labels map[string]string) {
-	in.ObjectMeta.Labels = labels
-}
-
-// SetManagedFields implements v1.Object.
-func (in *OvercommitClass) SetManagedFields(managedFields []metav1.ManagedFieldsEntry) {
-	in.ObjectMeta.ManagedFields = managedFields
-}
-
-// SetName implements v1.Object.
-func (in *OvercommitClass) SetName(name string) {
-	in.ObjectMeta.Name = name
-}
-
-// SetNamespace implements v1.Object.
-func (in *OvercommitClass) SetNamespace(namespace string) {
-	in.ObjectMeta.Namespace = namespace
-}
-
-// SetOwnerReferences implements v1.Object.
-func (in *OvercommitClass) SetOwnerReferences(references []metav1.OwnerReference) {
-	in.ObjectMeta.OwnerReferences = references
-}
-
-// SetResourceVersion implements v1.Object.
-func (in *OvercommitClass) SetResourceVersion(version string) {
-	in.ObjectMeta.ResourceVersion = version
-}
-
-// SetUID implements v1.Object.
-func (in *OvercommitClass) SetUID(uid types.UID) {
-	in.ObjectMeta.UID = uid
-}
-
 // +kubebuilder:object:root=true
 
 // OvercommitClassList contains a list of OvercommitClass

api/v1alphav1/webhook_validations.go

@@ -70,9 +70,14 @@ func isClassDefault(class OvercommitClass, client client.Client) error {
 }
 
 func checkIsRegexValid(regex string) error {
+	// Limit regex length to prevent ReDoS (catastrophic backtracking)
+	const maxRegexLen = 512
+	if len(regex) > maxRegexLen {
+		return fmt.Errorf("regex is too long (%d chars), maximum allowed is %d", len(regex), maxRegexLen)
+	}
 	_, err := regexp.Compile(regex)
 	if err != nil {
-		return errors.New("Error: the regex is not valid")
+		return fmt.Errorf("invalid regex for excludedNamespaces: %w", err)
 	}
 	return nil
 }

cmd/main.go

@@ -150,7 +150,7 @@ func main() {
 			os.Exit(1)
 		}
 
-		serviceAccountName, err := utils.GetPodServiceAccount(mgr.GetAPIReader())
+		serviceAccountName, err := utils.GetPodServiceAccount(ctx, mgr.GetAPIReader())
 		if err != nil {
 			setupLog.Error(err, "unable to get pod service account")
 			os.Exit(1)

go.mod

@@ -12,7 +12,6 @@ require (
 	k8s.io/apimachinery v0.32.1
 	k8s.io/client-go v0.32.1
 	sigs.k8s.io/controller-runtime v0.19.0
-	sigs.k8s.io/yaml v1.4.0
 )
 
 require (
@@ -101,4 +100,5 @@ require (
 	sigs.k8s.io/gateway-api v1.1.0 // indirect
 	sigs.k8s.io/json v0.0.0-20241014173422-cfa47c3a1cc8 // indirect
 	sigs.k8s.io/structured-merge-diff/v4 v4.5.0 // indirect
+	sigs.k8s.io/yaml v1.4.0 // indirect
 )

internal/controller/overcommit/overcommit_controller.go

@@ -390,11 +390,8 @@ func (r *OvercommitReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		// Don't fail the reconciliation for status update errors
 	}
 
-	// Only requeue periodically for status checks, not immediately
-	logger.Info("Reconciliation completed successfully", "nextReconcile", "10 seconds", "time", time.Now().Format("15:04:05"))
-	return ctrl.Result{
-		RequeueAfter: time.Second * 10,
-	}, nil
+	logger.Info("Reconciliation completed successfully", "time", time.Now().Format("15:04:05"))
+	return ctrl.Result{}, nil
 }
 
 // +kubebuilder:rbac:groups=apps, resources=deployments;replicasets,verbs=get;list;watch;create;update;patch;delete

internal/controller/overcommitclass/overcommitclass_controller.go

@@ -393,9 +393,6 @@ func (r *OvercommitClassReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		return ctrl.Result{}, err
 	}
 
-	// Only requeue periodically for status checks, not immediately
-	logger.Info("Reconciliation completed successfully", "nextReconcile", "10 seconds", "time", time.Now().Format("15:04:05"))
-	return ctrl.Result{
-		RequeueAfter: 10 * time.Second,
-	}, nil
+	logger.Info("Reconciliation completed successfully", "time", time.Now().Format("15:04:05"))
+	return ctrl.Result{}, nil
 }

internal/resources/generate_resources_pod_mutating_webhooks.go

@@ -147,10 +147,10 @@ func CreateCertificate(name string, svc corev1.Service) *certmanager.Certificate
 		Spec: certmanager.CertificateSpec{
 			SecretName: name + "-webhook-secret",
 			Duration: &metav1.Duration{
-				Duration: 87600 * time.Hour,
+				Duration: 8760 * time.Hour, // 1 year
 			},
 			RenewBefore: &metav1.Duration{
-				Duration: 720 * time.Hour,
+				Duration: 720 * time.Hour, // 30 days
 			},
 			DNSNames: []string{
 				svc.Name + "." + svc.Namespace + ".svc",

internal/resources/generate_resources_pod_mutating_webhooks_test.go

@@ -123,7 +123,7 @@ func TestCreateCertificate(t *testing.T) {
 		t.Errorf("Expected secret name 'test-class-webhook-secret', got '%s'", certificate.Spec.SecretName)
 	}
 
-	expectedDuration := 87600 * time.Hour
+	expectedDuration := 8760 * time.Hour
 	if certificate.Spec.Duration.Duration != expectedDuration {
 		t.Errorf("Expected duration '%v', got '%v'", expectedDuration, certificate.Spec.Duration.Duration)
 	}

internal/utils/getOvercommitClass.go

@@ -45,14 +45,14 @@ func GetOvercommitClassSpec(ctx context.Context, name string, k8sClient client.C
 	return &overcommitClass.Spec, nil
 }
 
-func GetDefaultSpec(k8sClient client.Client) (*overcommit.OvercommitClassSpec, error) {
+func GetDefaultSpec(ctx context.Context, k8sClient client.Client) (*overcommit.OvercommitClassSpec, error) {
 	if k8sClient == nil {
 		return nil, errors.New("client parameter cannot be nil")
 	}
 
 	// List all OvercommitClass
 	var overcommitClasses overcommit.OvercommitClassList
-	if err := k8sClient.List(context.Background(), &overcommitClasses); err != nil {
+	if err := k8sClient.List(ctx, &overcommitClasses); err != nil {
 		return nil, fmt.Errorf("error listing OvercommitClass: %w", err)
 	}
 

internal/utils/getOvercommitClass_test.go

@@ -82,7 +82,7 @@ var _ = Describe("GetDefaultSpec", func() {
 
 	It("should retrieve the default OvercommitClassSpec correctly", func() {
 		// Try the GetDefaultSpec function
-		spec, err := GetDefaultSpec(k8sClient)
+		spec, err := GetDefaultSpec(context.Background(), k8sClient)
 		Expect(err).NotTo(HaveOccurred(), "Failed to get default OvercommitClassSpec")
 		Expect(spec).NotTo(BeNil(), "Spec should not be nil")
 		Expect(spec.IsDefault).To(BeTrue(), "Spec.IsDefault should be true")