Merge pull request #493 from qu1queee/pairing/build_watch_secrets

Build watch secrets

docs/development/authentication.md

@@ -9,6 +9,7 @@ SPDX-License-Identifier: Apache-2.0
 The following document provides an introduction around the different authentication methods that can take place during an image build when using the Build operator.
 
 - [Overview](#overview)
+- [Build Secrets Annotation](#build-secrets-annotation)
 - [Authentication for Git](#authentication-for-git)
   - [Basic authentication](#basic-authentication)
   - [SSH authentication](#ssh-authentication)
@@ -20,7 +21,32 @@ The following document provides an introduction around the different authenticat
 
 ## Overview
 
-There are two places where users might need to define authentication when building images. Authentication to a container registry is the most common one, but also users might have the need to define authentications for pulling source-code from Git.
+There are two places where users might need to define authentication when building images. Authentication to a container registry is the most common one, but also users might have the need to define authentications for pulling source-code from Git. Overall, the authentication is done via the definion of [secrets](https://kubernetes.io/docs/concepts/configuration/secret/) in which the require sensitive data will be stored.
+
+## Build Secrets Annotation
+
+Users need to add an annotation `build.build.dev/referenced.secret: "true"` to a build secret so that build controller can decide to take a reconcile action when a secret event (`create`, `update` and `delete`) happens. Below is a secret example with build annotation:
+
+```yaml
+apiVersion: v1
+data:
+  .dockerconfigjson: xxxxx
+kind: Secret
+metadata:
+  annotations:
+    build.build.dev/referenced.secret: "true"
+  name: secret-docker
+type: kubernetes.io/dockerconfigjson
+```
+
+This annotation will help us filter secrets which are not referenced on a Build instance. That means if a secret doesn't have this annotation, then although event happens on this secret, Build controller will not reconcile. Being able to reconcile on secrets events allow the Build controller to re-trigger validations on the Build configuration, allowing users to understand if a dependency is missing.
+
+If you are using `kubectl` command create secrets, then you can first create build secret using `kubectl create secret` command and annotate this secret using `kubectl annotate secrets`. Below is an example:
+
+```sh
+kubectl -n ${namespace} create secret docker-registry example-secret --docker-server=${docker-server} --docker-username="${username}" --docker-password="${password}" [email protected]
+kubectl -n ${namespace} annotate secrets example-secret build.build.dev/referenced.secret='true'
+```
 
 ## Authentication for Git
 
@@ -44,6 +70,7 @@ metadata:
   annotations:
     tekton.dev/git-0: github.com
     tekton.dev/git-1: gitlab.com
+    build.build.dev/referenced.secret: "true"
 type: kubernetes.io/ssh-auth
 data:
   ssh-privatekey: <base64 <~/.ssh/id_rsa>
@@ -64,6 +91,7 @@ metadata:
   annotations:
     tekton.dev/git-0: https://github.com
     tekton.dev/git-1: https://gitlab.com
+    build.build.dev/referenced.secret: "true"
 type: kubernetes.io/basic-auth
 stringData:
   username: <cleartext username>
@@ -118,6 +146,7 @@ kubectl --namespace <YOUR_NAMESPACE> create secret docker-registry <CONTAINER_RE
   --docker-username=<USERNAME> \
   --docker-password=<PASSWORD> \
   [email protected]
+kubectl --namespace <YOUR_NAMESPACE> annotate secrets <CONTAINER_REGISTRY_SECRET_NAME> build.build.dev/referenced.secret='true'
 ```
 
 _Notes:_ When generating a secret to access docker hub, the `REGISTRY_HOST` value should be `https://index.docker.io/v1/`, the username is the Docker ID.

pkg/apis/build/v1alpha1/build_types.go

@@ -18,6 +18,10 @@ const (
 
 	// AnnotationBuildRunDeletion is a label key for enabling/disabling the BuildRun deletion
 	AnnotationBuildRunDeletion = "build.build.dev/build-run-deletion"
+
+	// AnnotationBuildRefSecret is an annotation that tells the Build Controller to reconcile on
+	// events of the secret only if is referenced by a Build in the same namespace
+	AnnotationBuildRefSecret = "build.build.dev/referenced.secret"
 )
 
 // BuildSpec defines the desired state of Build

pkg/controller/build/build_controller.go

@@ -17,6 +17,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -99,6 +100,96 @@ func add(ctx context.Context, mgr manager.Manager, r reconcile.Reconciler) error
 
 	// Watch for changes to primary resource Build
 	err = c.Watch(&source.Kind{Type: &build.Build{}}, &handler.EnqueueRequestForObject{}, pred)
+
+	preSecret := predicate.Funcs{
+
+		// Only filter events where the secret have the Build specific annotation
+		CreateFunc: func(e event.CreateEvent) bool {
+			objectAnnotations := e.Meta.GetAnnotations()
+			if _, ok := buildSecretRefAnnotationExist(objectAnnotations); ok {
+				return true
+			}
+			return false
+		},
+
+		// Only filter events where the secret have the Build specific annotation,
+		// but only if the Build specific annotation changed
+		UpdateFunc: func(e event.UpdateEvent) bool {
+			oldAnnotations := e.MetaOld.GetAnnotations()
+			newAnnotations := e.MetaNew.GetAnnotations()
+
+			if _, oldBuildKey := buildSecretRefAnnotationExist(oldAnnotations); !oldBuildKey {
+				if _, newBuildKey := buildSecretRefAnnotationExist(newAnnotations); newBuildKey {
+					return true
+				}
+			}
+			return false
+		},
+
+		// Only filter events where the secret have the Build specific annotation
+		DeleteFunc: func(e event.DeleteEvent) bool {
+			objectAnnotations := e.Meta.GetAnnotations()
+			if _, ok := buildSecretRefAnnotationExist(objectAnnotations); ok {
+				return true
+			}
+			return false
+		},
+	}
+
+	err = c.Watch(&source.Kind{Type: &corev1.Secret{}}, &handler.EnqueueRequestsFromMapFunc{
+		ToRequests: handler.ToRequestsFunc(func(o handler.MapObject) []reconcile.Request {
+
+			secret := o.Object.(*corev1.Secret)
+
+			buildList := &build.BuildList{}
+
+			// List all builds in the namespace of the current secret
+			if err := mgr.GetClient().List(ctx, buildList, &client.ListOptions{Namespace: secret.Namespace}); err != nil {
+				// Avoid entering into the Reconcile space
+				ctxlog.Info(ctx, "unexpected error happened while listing builds", namespace, secret.Namespace, "error", err)
+				return []reconcile.Request{}
+			}
+
+			if len(buildList.Items) == 0 {
+				// Avoid entering into the Reconcile space
+				return []reconcile.Request{}
+			}
+
+			// Only enter the Reconcile space if the secret is referenced on
+			// any Build in the same namespaces
+
+			reconcileList := []reconcile.Request{}
+			flagReconcile := false
+
+			for _, build := range buildList.Items {
+				if build.Spec.Source.SecretRef != nil {
+					if build.Spec.Source.SecretRef.Name == secret.Name {
+						flagReconcile = true
+					}
+				}
+				if build.Spec.Output.SecretRef != nil {
+					if build.Spec.Output.SecretRef.Name == secret.Name {
+						flagReconcile = true
+					}
+				}
+				if build.Spec.BuilderImage != nil && build.Spec.BuilderImage.SecretRef != nil {
+					if build.Spec.BuilderImage.SecretRef.Name == secret.Name {
+						flagReconcile = true
+					}
+				}
+				if flagReconcile {
+					reconcileList = append(reconcileList, reconcile.Request{
+						NamespacedName: types.NamespacedName{
+							Name:      build.Name,
+							Namespace: build.Namespace,
+						},
+					})
+				}
+			}
+			return reconcileList
+		}),
+	}, preSecret)
+
 	if err != nil {
 		return err
 	}
@@ -164,8 +255,15 @@ func (r *ReconcileBuild) Reconcile(request reconcile.Request) (reconcile.Result,
 	if len(secretNames) > 0 {
 		if err := r.validateSecrets(ctx, secretNames, b.Namespace); err != nil {
 			b.Status.Reason = err.Error()
-			updateErr := r.client.Status().Update(ctx, b)
-			return reconcile.Result{}, fmt.Errorf("errors: %v %v", err, updateErr)
+			if updateErr := r.client.Status().Update(ctx, b); updateErr != nil {
+				// return an error in case of transient failure, and expect the next
+				// reconciliation to be able to update the Status of the object
+				return reconcile.Result{}, fmt.Errorf("errors: %v %v", err, updateErr)
+			}
+			// The Secret Resource watcher will Reconcile again once the missing
+			// secret is created, therefore no need to return an error and enter on an infinite
+			// reconciliation
+			return reconcile.Result{}, nil
 		}
 	}
 
@@ -275,7 +373,6 @@ func (r *ReconcileBuild) validateClusterBuildStrategy(ctx context.Context, n str
 	}
 	return nil
 }
-
 func (r *ReconcileBuild) validateSecrets(ctx context.Context, secretNames []string, ns string) error {
 	list := &corev1.SecretList{}
 
@@ -393,3 +490,10 @@ func (r *ReconcileBuild) retrieveBuildRunsfromBuild(ctx context.Context, b *buil
 func removeOwnerReferenceByIndex(references []metav1.OwnerReference, i int) []metav1.OwnerReference {
 	return append(references[:i], references[i+1:]...)
 }
+
+func buildSecretRefAnnotationExist(annotation map[string]string) (string, bool) {
+	if val, ok := annotation[build.AnnotationBuildRefSecret]; ok {
+		return val, true
+	}
+	return "", false
+}

pkg/controller/build/build_controller_test.go

@@ -103,9 +103,8 @@ var _ = Describe("Reconcile Build", func() {
 				statusWriter.UpdateCalls(statusCall)
 
 				_, err := reconciler.Reconcile(request)
-				Expect(err).To(HaveOccurred())
+				Expect(err).To(BeNil())
 				Expect(statusWriter.UpdateCallCount()).To(Equal(1))
-				Expect(err.Error()).To(ContainSubstring("secret non-existing does not exist"))
 			})
 
 			It("succeeds when the secret exists", func() {
@@ -166,9 +165,8 @@ var _ = Describe("Reconcile Build", func() {
 				statusWriter.UpdateCalls(statusCall)
 
 				_, err := reconciler.Reconcile(request)
-				Expect(err).To(HaveOccurred())
+				Expect(err).To(BeNil())
 				Expect(statusWriter.UpdateCallCount()).To(Equal(1))
-				Expect(err.Error()).To(ContainSubstring("secret non-existing does not exist"))
 			})
 
 			It("succeeds when the secret exists", func() {
@@ -225,9 +223,8 @@ var _ = Describe("Reconcile Build", func() {
 				statusWriter.UpdateCalls(statusCall)
 
 				_, err := reconciler.Reconcile(request)
-				Expect(err).To(HaveOccurred())
+				Expect(err).To(BeNil())
 				Expect(statusWriter.UpdateCallCount()).To(Equal(1))
-				Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("secret %s does not exist", registrySecret)))
 			})
 			It("succeed when the secret exists", func() {
 
@@ -270,9 +267,8 @@ var _ = Describe("Reconcile Build", func() {
 				statusWriter.UpdateCalls(statusCall)
 
 				_, err := reconciler.Reconcile(request)
-				Expect(err).To(HaveOccurred())
+				Expect(err).To(BeNil())
 				Expect(statusWriter.UpdateCallCount()).To(Equal(1))
-				Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("there are no secrets in namespace %s", namespace)))
 			})
 		})
 
@@ -300,11 +296,8 @@ var _ = Describe("Reconcile Build", func() {
 				})
 
 				_, err := reconciler.Reconcile(request)
-				Expect(err).To(HaveOccurred())
+				Expect(err).To(BeNil())
 				Expect(statusWriter.UpdateCallCount()).To(Equal(1))
-				Expect(err.Error()).To(ContainSubstring("do not exist"))
-				Expect(err.Error()).To(ContainSubstring("non-existing-source"))
-				Expect(err.Error()).To(ContainSubstring("non-existing-output"))
 			})
 		})
 

test/build_samples.go

@@ -111,6 +111,80 @@ spec:
       name: fake-secret
 `
 
+// BuildWithOutputRefSecret defines a Build with a
+// referenced secret under spec.output
+const BuildWithOutputRefSecret = `
+apiVersion: build.dev/v1alpha1
+kind: Build
+spec:
+  source:
+    url: "https://github.com/sbose78/taxi"
+  strategy:
+    kind: ClusterBuildStrategy
+  output:
+    image: image-registry.openshift-image-registry.svc:5000/example/buildpacks-app
+    credentials:
+      name: output-secret
+  timeout: 5s
+`
+
+// BuildWithSourceRefSecret defines a Build with a
+// referenced secret under spec.source
+const BuildWithSourceRefSecret = `
+apiVersion: build.dev/v1alpha1
+kind: Build
+spec:
+  source:
+    url: "https://github.com/sbose78/taxi"
+    credentials:
+      name: source-secret
+  strategy:
+    kind: ClusterBuildStrategy
+  output:
+    image: image-registry.openshift-image-registry.svc:5000/example/buildpacks-app
+  timeout: 5s
+`
+
+// BuildWithBuilderRefSecret defines a Build with a
+// referenced secret under spec.builder
+const BuildWithBuilderRefSecret = `
+apiVersion: build.dev/v1alpha1
+kind: Build
+spec:
+  source:
+    url: "https://github.com/sbose78/taxi"
+  builder:
+    image: heroku/buildpacks:18
+    credentials:
+      name: builder-secret
+  strategy:
+    kind: ClusterBuildStrategy
+  output:
+    image: image-registry.openshift-image-registry.svc:5000/example/buildpacks-app
+  timeout: 5s
+`
+
+// BuildWithMultipleRefSecrets defines a Build with
+// multiple referenced secrets under spec
+const BuildWithMultipleRefSecrets = `
+apiVersion: build.dev/v1alpha1
+kind: Build
+spec:
+  source:
+    url: "https://github.com/sbose78/taxi"
+    credentials:
+      name: source-secret
+  builder:
+    image: heroku/buildpacks:18
+    credentials:
+      name: builder-secret
+  strategy:
+    kind: ClusterBuildStrategy
+  output:
+    image: image-registry.openshift-image-registry.svc:5000/example/buildpacks-app
+  timeout: 5s
+`
+
 // BuildCBSWithShortTimeOut defines a Build with a
 // ClusterBuildStrategy and a short timeout
 const BuildCBSWithShortTimeOut = `

test/catalog.go

@@ -30,6 +30,27 @@ import (
 // Catalog allows you to access helper functions
 type Catalog struct{}
 
+// SecretWithAnnotation gives you a secret with build annotation
+func (c *Catalog) SecretWithAnnotation(name string, ns string) *corev1.Secret {
+	return &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        name,
+			Namespace:   ns,
+			Annotations: map[string]string{build.AnnotationBuildRefSecret: "true"},
+		},
+	}
+}
+
+// SecretWithoutAnnotation gives you a secret without build annotation
+func (c *Catalog) SecretWithoutAnnotation(name string, ns string) *corev1.Secret {
+	return &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: ns,
+		},
+	}
+}
+
 // BuildWithClusterBuildStrategy gives you an specific Build CRD
 func (c *Catalog) BuildWithClusterBuildStrategy(name string, ns string, strategyName string, secretName string) *build.Build {
 	buildStrategy := build.ClusterBuildStrategyKind
@@ -176,7 +197,7 @@ func (c *Catalog) FakeSecretList() corev1.SecretList {
 	}
 }
 
-// FakeSecretListInNamespace to support test
+// FakeNoSecretListInNamespace returns an empty secret list
 func (c *Catalog) FakeNoSecretListInNamespace() corev1.SecretList {
 	return corev1.SecretList{
 		Items: []corev1.Secret{},