Cleanup some TODOs.

* Use LocalObjectReference for the auth.
* Improve error messages.

Signed-off-by: Kevin McDermott <[email protected]>

Author: bigkevmcd

api/v1alpha1/polledrepository_types.go

@@ -65,13 +65,11 @@ type PolledRepositorySpec struct {
 }
 
 // AuthSecret references a secret for authenticating the request.
-// TODO: This should use a LocalObjectReference, and not allow the secret
-// reference to go across namespaces.
 type AuthSecret struct {
 	// This is a local reference to the named secret to fetch.
 	// This secret is expected to have a "token" key with a valid GitHub/GitLab
 	// auth token.
-	corev1.SecretReference `json:"secretRef,omitempty"`
+	SecretRef corev1.LocalObjectReference `json:"secretRef,omitempty"`
 	//+kubebuilder:default:="token"
 	Key string `json:"key,omitempty"`
 }

api/v1alpha1/zz_generated.deepcopy.go

@@ -65,12 +65,13 @@ spec:
                       auth token.
                     properties:
                       name:
-                        description: name is unique within a namespace to reference
-                          a secret resource.
-                        type: string
-                      namespace:
-                        description: namespace defines the space within which the
-                          secret name must be unique.
+                        default: ""
+                        description: |-
+                          Name of the referent.
+                          This field is effectively required, but due to backwards compatibility is
+                          allowed to be empty. Instances of this type with an empty value here are
+                          almost certainly wrong.
+                          More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
                         type: string
                     type: object
                     x-kubernetes-map-type: atomic

config/crd/bases/polling.gitops.tools_polledrepositories.yaml

@@ -85,7 +85,8 @@ func (r *PolledRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Req
 
 	authToken, err := r.authTokenForRepo(ctx, reqLogger, req.Namespace, repo)
 	if err != nil {
-		return reconcile.Result{}, fmt.Errorf("failed to get auth token")
+		reqLogger.Error(err, "Getting the auth token failed")
+		return reconcile.Result{}, fmt.Errorf("failed to get auth token: %w", err)
 	}
 
 	// TODO: handle pollerFactory returning nil/error
@@ -117,14 +118,12 @@ func (r *PolledRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Req
 	repo.Status.PollStatus = newStatus
 	if err := r.Client.Status().Update(ctx, &repo); err != nil {
 		reqLogger.Error(err, "unable to update Repository status")
-		// TODO: improve the error
-		return ctrl.Result{}, err
+		return ctrl.Result{}, fmt.Errorf("failed to update status after change detected: %w", err)
 	}
 
 	if err := r.EventDispatcher.Dispatch(ctx, repo, commit); err != nil {
 		reqLogger.Error(err, "failed to dispatch commit")
-		// TODO: improve the error
-		return ctrl.Result{}, err
+		return ctrl.Result{}, fmt.Errorf("failed to send notification to %q: %w", repo.Spec.Endpoint, err)
 	}
 
 	reqLogger.Info("requeueing next check", "frequency", repo.Spec.Frequency.Duration)
@@ -147,9 +146,8 @@ func (r *PolledRepositoryReconciler) authTokenForRepo(ctx context.Context, logge
 	if repo.Spec.Auth.Key != "" {
 		key = repo.Spec.Auth.Key
 	}
-	authToken, err := r.SecretGetter.SecretToken(ctx, types.NamespacedName{Name: repo.Spec.Auth.Name, Namespace: namespace}, key)
+	authToken, err := r.SecretGetter.SecretToken(ctx, types.NamespacedName{Name: repo.Spec.Auth.SecretRef.Name, Namespace: namespace}, key)
 	if err != nil {
-		logger.Error(err, "Getting the auth token failed", "name", repo.Spec.Auth.Name, "namespace", namespace, "key", key)
 		return "", err
 	}
 
@@ -159,13 +157,16 @@ func (r *PolledRepositoryReconciler) authTokenForRepo(ctx context.Context, logge
 func repoFromURL(s string) (string, string, error) {
 	parsed, err := url.Parse(s)
 	if err != nil {
-		return "", "", fmt.Errorf("failed to parse repo from URL %#v: %s", s, err)
+		return "", "", fmt.Errorf("failed to parse repo from URL %s: %s", s, err)
 	}
 	host := parsed.Host
 	if strings.HasSuffix(host, "github.com") {
 		host = "api." + host
 	}
+
+	// TODO: This should create a new URL with scheme and host?
 	endpoint := fmt.Sprintf("%s://%s", parsed.Scheme, host)
+
 	return strings.TrimPrefix(strings.TrimSuffix(parsed.Path, ".git"), "/"), endpoint, nil
 }
 

internal/controller/polledrepository_controller.go

@@ -225,7 +225,7 @@ func TestReconciliation(t *testing.T) {
 func withSecretRef(s *corev1.Secret) func(*pollingv1.PolledRepository) {
 	return func(r *pollingv1.PolledRepository) {
 		r.Spec.Auth = &pollingv1.AuthSecret{
-			SecretReference: corev1.SecretReference{
+			SecretRef: corev1.LocalObjectReference{
 				Name: s.Name,
 			},
 		}

internal/controller/polledrepository_controller_test.go

@@ -46,7 +46,7 @@ func (k KubeSecretGetter) SecretToken(ctx context.Context, id types.NamespacedNa
 	}
 	token, ok := loaded.Data[key]
 	if !ok {
-		return "", fmt.Errorf("secret invalid, no %#v key in %s/%s", key, id.Namespace, id.Name)
+		return "", fmt.Errorf("secret invalid, no key %q in %s", key, id)
 	}
 	return string(token), nil
 }

pkg/secrets/secrets.go

@@ -46,7 +46,7 @@ func TestSecretTokenWithMissingKey(t *testing.T) {
 	g := New(fake.NewFakeClient(createSecret(testID, "secret-token")))
 
 	_, err := g.SecretToken(context.TODO(), testID, "unknown")
-	if err.Error() != `secret invalid, no "unknown" key in test-ns/test-secret` {
+	if err.Error() != `secret invalid, no key "unknown" in test-ns/test-secret` {
 		t.Fatal(err)
 	}
 }

pkg/secrets/secrets_test.go

@@ -0,0 +1,39 @@
+package utils
+
+import (
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+// NewSecret creates and returns a new Secret.
+//
+// The data is converted to map[string][]byte as a convenience.
+func NewSecret(data map[string]string, opts ...func(*corev1.Secret)) *corev1.Secret {
+	cm := &corev1.Secret{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "v1",
+			Kind:       "Secret",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "demo-secret",
+			// TODO: pass in a types.NamespacedName
+			Namespace: "testing",
+		},
+		Data: dataToBytes(data),
+	}
+
+	for _, o := range opts {
+		o(cm)
+	}
+
+	return cm
+}
+
+func dataToBytes(src map[string]string) map[string][]byte {
+	result := map[string][]byte{}
+	for k, v := range src {
+		result[k] = []byte(v)
+	}
+
+	return result
+}