Pass app env vars into the kpack image

* App environment consists of entries in the app env secret and
  `VCAP_SERVICES` that is built out of app service bindings
* Introduce a `env.Builder` to build the app environment which is used
  by the process controller and the build controller
* The `env.Builder` adds the `VCAP_SERVICES` entry into the app env
  secret under the hood
* If the app has no env secret name set, the builder would be a noop and
  no env would be passed to the process/build. We assume that in this
  case the developer does not care about the environment
* If the app has an env secret name set, but the secret does not exist,
  we return an error as we assume that the secret should have been already
  created

Issue: #774

Co-authored-by: Kieron Browne <kbrowne@vmware.com>
Co-authored-by: Danail Branekov <danailster@gmail.com>
Co-authored-by: Georgi Sabev <georgethebeatle@gmail.com>

api/tests/e2e/apps_test.go

@@ -305,6 +305,10 @@ var _ = Describe("Apps", func() {
 				Expect(resp).To(HaveRestyStatusCode(http.StatusOK))
 				Expect(result.State).To(Equal("STARTED"))
 			})
+
+			It("has the VCAP_SERVICES env var set", func() {
+				Expect(getEnv(appGUID)).To(HaveKeyWithValue("environment_variables", HaveKey("VCAP_SERVICES")))
+			})
 		})
 
 		Describe("Restart an app", func() {
@@ -324,6 +328,18 @@ var _ = Describe("Apps", func() {
 				Expect(resp).To(HaveRestyStatusCode(http.StatusOK))
 				Expect(result.State).To(Equal("STARTED"))
 			})
+
+			When("app environment has been changed", func() {
+				BeforeEach(func() {
+					setEnv(appGUID, map[string]interface{}{
+						"foo": "var",
+					})
+				})
+
+				It("sets the new env var to the app environment", func() {
+					Expect(getEnv(appGUID)).To(HaveKeyWithValue("environment_variables", HaveKeyWithValue("foo", "var")))
+				})
+			})
 		})
 
 		Describe("Stop an app", func() {

api/tests/e2e/e2e_suite_test.go

@@ -94,6 +94,12 @@ type appResource struct {
 	State    string `json:"state,omitempty"`
 }
 
+type appEnvResource map[string]interface{}
+
+type updateAppEnvVarsResource struct {
+	Var map[string]interface{} `json:"var"`
+}
+
 type typedResource struct {
 	resource `json:",inline"`
 	Type     string `json:"type,omitempty"`
@@ -414,6 +420,28 @@ func createApp(spaceGUID, name string) string {
 	return app.GUID
 }
 
+func setEnv(appName string, envVars map[string]interface{}) {
+	resp, err := adminClient.R().
+		SetBody(updateAppEnvVarsResource{
+			Var: envVars,
+		}).
+		Patch(fmt.Sprintf("/v3/apps/%s/environment_variables", appName))
+	ExpectWithOffset(1, err).NotTo(HaveOccurred())
+	ExpectWithOffset(1, resp).To(HaveRestyStatusCode(http.StatusOK))
+}
+
+func getEnv(appName string) map[string]interface{} {
+	var env appEnvResource
+
+	resp, err := adminClient.R().
+		SetResult(&env).
+		Get(fmt.Sprintf("/v3/apps/%s/env", appName))
+	ExpectWithOffset(1, err).NotTo(HaveOccurred())
+	ExpectWithOffset(1, resp).To(HaveRestyStatusCode(http.StatusOK))
+
+	return env
+}
+
 func getProcess(appGUID, processType string) string {
 	var processList resourceList
 	EventuallyWithOffset(1, func(g Gomega) {

controllers/config/rbac/role.yaml

@@ -12,6 +12,7 @@ rules:
   verbs:
   - get
   - list
+  - patch
   - watch
 - apiGroups:
   - ""
@@ -21,6 +22,7 @@ rules:
   verbs:
   - get
   - list
+  - patch
   - watch
 - apiGroups:
   - ""

controllers/controllers/workloads/cfbuild_controller.go

@@ -89,6 +89,7 @@ type CFBuildReconciler struct {
 	ControllerConfig    *config.ControllerConfig
 	RegistryAuthFetcher RegistryAuthFetcher
 	ImageProcessFetcher ImageProcessFetcher
+	EnvBuilder          EnvBuilder
 }
 
 //+kubebuilder:rbac:groups=workloads.cloudfoundry.org,resources=cfbuilds,verbs=get;list;watch;create;update;patch;delete
@@ -99,7 +100,7 @@ type CFBuildReconciler struct {
 //+kubebuilder:rbac:groups=kpack.io,resources=images/status,verbs=get;update;patch
 //+kubebuilder:rbac:groups=kpack.io,resources=images/finalizers,verbs=update
 
-//+kubebuilder:rbac:groups="",resources=serviceaccounts;secrets,verbs=get;list;watch
+//+kubebuilder:rbac:groups="",resources=serviceaccounts;secrets,verbs=get;list;watch;patch
 //+kubebuilder:rbac:groups="",resources=serviceaccounts/status;secrets/status,verbs=get
 
 // Reconcile is part of the main kubernetes reconciliation loop which aims to
@@ -154,20 +155,13 @@ func (r *CFBuildReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 		// Scenario: CFBuild newly created and all status conditions are unknown, it
 		// Creates a KpackImage resource to trigger staging.
 		// Updates status on CFBuild -> sets staging to True.
-		var appServiceBindings []servicesv1alpha1.CFServiceBinding
-		appServiceBindings, err = r.getAppServiceBindings(ctx, cfApp.Name, cfApp.Namespace)
-		if err != nil {
-			r.Log.Error(err, "Error when listing CFServiceBindings")
-			return ctrl.Result{}, err
-		}
-
 		err = r.ensureKpackImageRequirements(ctx, cfPackage)
 		if err != nil {
 			r.Log.Info("Kpack image requirements for CFPackage are not met", "guid", cfPackage.Name, "reason", err)
 			return ctrl.Result{}, err
 		}
 
-		err = r.createKpackImageAndUpdateStatus(ctx, cfBuild, cfApp, cfPackage, appServiceBindings)
+		err = r.createKpackImageAndUpdateStatus(ctx, cfBuild, cfApp, cfPackage)
 		if err != nil {
 			return ctrl.Result{}, err
 		}
@@ -228,24 +222,7 @@ func (r *CFBuildReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 	return ctrl.Result{}, nil
 }
 
-func (r *CFBuildReconciler) getAppServiceBindings(ctx context.Context, appGUID string, namespace string) ([]servicesv1alpha1.CFServiceBinding, error) {
-	serviceBindings := &servicesv1alpha1.CFServiceBindingList{}
-	err := r.Client.List(ctx, serviceBindings,
-		client.InNamespace(namespace),
-		client.MatchingFields{shared.IndexServiceBindingAppGUID: appGUID},
-	)
-	if err != nil {
-		return nil, fmt.Errorf("error listing CFServiceBindings: %w", err)
-	}
-
-	if len(serviceBindings.Items) == 0 {
-		return nil, nil
-	}
-
-	return serviceBindings.Items, nil
-}
-
-func (r *CFBuildReconciler) createKpackImageAndUpdateStatus(ctx context.Context, cfBuild *workloadsv1alpha1.CFBuild, cfApp *workloadsv1alpha1.CFApp, cfPackage *workloadsv1alpha1.CFPackage, serviceBindings []servicesv1alpha1.CFServiceBinding) error {
+func (r *CFBuildReconciler) createKpackImageAndUpdateStatus(ctx context.Context, cfBuild *workloadsv1alpha1.CFBuild, cfApp *workloadsv1alpha1.CFApp, cfPackage *workloadsv1alpha1.CFPackage) error {
 	serviceAccountName := kpackServiceAccount
 	kpackImageTag := path.Join(r.ControllerConfig.KpackImageTag, cfBuild.Name)
 	kpackImageName := cfBuild.Name
@@ -278,21 +255,20 @@ func (r *CFBuildReconciler) createKpackImageAndUpdateStatus(ctx context.Context,
 			},
 		},
 	}
-	for _, serviceBinding := range serviceBindings {
-		if serviceBinding.Status.Binding.Name != "" {
-			objRef := corev1.ObjectReference{
-				Kind:       "Secret",
-				Name:       serviceBinding.Status.Binding.Name,
-				APIVersion: "v1",
-			}
-			desiredKpackImage.Spec.Build.Services = append(desiredKpackImage.Spec.Build.Services, objRef)
-		} else {
-			r.Log.Info("binding secret name is empty")
-			return errors.New("binding secret name is empty")
-		}
+
+	buildServices, err := r.prepareBuildServices(ctx, kpackImageNamespace, cfApp.Name)
+	if err != nil {
+		return err
 	}
+	desiredKpackImage.Spec.Build.Services = buildServices
 
-	err := controllerutil.SetOwnerReference(cfBuild, &desiredKpackImage, r.Scheme)
+	imageEnvironment, err := r.prepareEnvironment(ctx, cfApp)
+	if err != nil {
+		return err
+	}
+	desiredKpackImage.Spec.Build.Env = imageEnvironment
+
+	err = controllerutil.SetOwnerReference(cfBuild, &desiredKpackImage, r.Scheme)
 	if err != nil {
 		r.Log.Error(err, "failed to set OwnerRef on Kpack Image")
 		return err
@@ -314,6 +290,51 @@ func (r *CFBuildReconciler) createKpackImageAndUpdateStatus(ctx context.Context,
 	return nil
 }
 
+func (r *CFBuildReconciler) prepareBuildServices(ctx context.Context, namespace, appGUID string) (buildv1alpha2.Services, error) {
+	serviceBindingsList := &servicesv1alpha1.CFServiceBindingList{}
+	err := r.Client.List(ctx, serviceBindingsList,
+		client.InNamespace(namespace),
+		client.MatchingFields{shared.IndexServiceBindingAppGUID: appGUID},
+	)
+	if err != nil {
+		return nil, fmt.Errorf("error listing CFServiceBindings: %w", err)
+	}
+
+	buildServices := buildv1alpha2.Services{}
+	for _, serviceBinding := range serviceBindingsList.Items {
+		if serviceBinding.Status.Binding.Name != "" {
+			objRef := corev1.ObjectReference{
+				Kind:       "Secret",
+				Name:       serviceBinding.Status.Binding.Name,
+				APIVersion: "v1",
+			}
+			buildServices = append(buildServices, objRef)
+		} else {
+			r.Log.Info("binding secret name is empty")
+			return nil, errors.New("binding secret name is empty")
+		}
+	}
+	return buildServices, nil
+}
+
+func (r *CFBuildReconciler) prepareEnvironment(ctx context.Context, cfApp *workloadsv1alpha1.CFApp) ([]corev1.EnvVar, error) {
+	env, err := r.EnvBuilder.BuildEnv(ctx, cfApp)
+	if err != nil {
+		r.Log.Error(err, "failed building environment")
+		return nil, err
+	}
+
+	imageEnvironment := []corev1.EnvVar{}
+	for k, v := range env {
+		imageEnvironment = append(imageEnvironment, corev1.EnvVar{
+			Name:  k,
+			Value: v,
+		})
+	}
+
+	return imageEnvironment, nil
+}
+
 func (r *CFBuildReconciler) createKpackImageIfNotExists(ctx context.Context, desiredKpackImage buildv1alpha2.Image) error {
 	var foundKpackImage buildv1alpha2.Image
 	err := r.Client.Get(ctx, types.NamespacedName{Name: desiredKpackImage.Name, Namespace: desiredKpackImage.Namespace}, &foundKpackImage)

controllers/controllers/workloads/cfbuild_controller_test.go

@@ -39,6 +39,7 @@ var _ = Describe("CFBuildReconciler", func() {
 
 		fakeRegistryAuthFetcher *fake.RegistryAuthFetcher
 		fakeImageProcessFetcher *fake.ImageProcessFetcher
+		fakeEnvBuilder          *fake.EnvBuilder
 
 		cfAppGUID               string
 		cfPackageGUID           string
@@ -94,7 +95,7 @@ var _ = Describe("CFBuildReconciler", func() {
 		kpackServiceAccount = BuildServiceAccount(kpackServiceAccountName, defaultNamespace, kpackRegistrySecretName)
 		kpackServiceAccountError = nil
 
-		fakeClient.GetStub = func(_ context.Context, _ types.NamespacedName, obj client.Object) error {
+		fakeClient.GetStub = func(_ context.Context, namespacedName types.NamespacedName, obj client.Object) error {
 			// cast obj to find its kind
 			switch obj := obj.(type) {
 			case *workloadsv1alpha1.CFBuild:
@@ -120,15 +121,13 @@ var _ = Describe("CFBuildReconciler", func() {
 			}
 		}
 
-		// Configure mock status update to succeed
-		fakeStatusWriter = &fake.StatusWriter{}
+		fakeStatusWriter = new(fake.StatusWriter)
 		fakeClient.StatusReturns(fakeStatusWriter)
 
-		// Configure a fake RegistryAuthFetcher
-		fakeRegistryAuthFetcher = &fake.RegistryAuthFetcher{}
-
-		// Configure a fake ImageProcessFetcher
-		fakeImageProcessFetcher = &fake.ImageProcessFetcher{}
+		fakeRegistryAuthFetcher = new(fake.RegistryAuthFetcher)
+		fakeImageProcessFetcher = new(fake.ImageProcessFetcher)
+		fakeEnvBuilder = new(fake.EnvBuilder)
+		fakeEnvBuilder.BuildEnvReturns(map[string]string{"foo": "var"}, nil)
 
 		// configure a CFBuildReconciler with the client
 		Expect(workloadsv1alpha1.AddToScheme(scheme.Scheme)).To(Succeed())
@@ -140,6 +139,7 @@ var _ = Describe("CFBuildReconciler", func() {
 			ControllerConfig:    &config.ControllerConfig{KpackImageTag: "image/registry/tag"},
 			RegistryAuthFetcher: fakeRegistryAuthFetcher.Spy,
 			ImageProcessFetcher: fakeImageProcessFetcher.Spy,
+			EnvBuilder:          fakeEnvBuilder,
 		}
 		req = ctrl.Request{
 			NamespacedName: types.NamespacedName{
@@ -166,8 +166,8 @@ var _ = Describe("CFBuildReconciler", func() {
 
 			It("should create kpack image with the same GUID as the CF Build", func() {
 				Expect(fakeClient.CreateCallCount()).To(Equal(1), "fakeClient Create was not called 1 time")
-				_, kpackImage, _ := fakeClient.CreateArgsForCall(0)
-				Expect(kpackImage.GetName()).To(Equal(cfBuildGUID))
+				_, actualKpackImage, _ := fakeClient.CreateArgsForCall(0)
+				Expect(actualKpackImage.GetName()).To(Equal(cfBuildGUID))
 			})
 
 			It("should update the status conditions on CFBuild", func() {
@@ -176,15 +176,30 @@ var _ = Describe("CFBuildReconciler", func() {
 
 			It("should create kpack Image with CFBuild owner reference", func() {
 				Expect(fakeClient.CreateCallCount()).To(Equal(1), "fakeClient Create was not called 1 time")
-				_, kpackImage, _ := fakeClient.CreateArgsForCall(0)
-				ownerRefs := kpackImage.GetOwnerReferences()
+				_, actualKpackImage, _ := fakeClient.CreateArgsForCall(0)
+				ownerRefs := actualKpackImage.GetOwnerReferences()
 				Expect(ownerRefs).To(ConsistOf(metav1.OwnerReference{
 					UID:        cfBuild.UID,
 					Kind:       cfBuild.Kind,
 					APIVersion: cfBuild.APIVersion,
 					Name:       cfBuild.Name,
 				}))
 			})
+
+			It("sets the env vars on the kpack image", func() {
+				Expect(fakeClient.CreateCallCount()).To(Equal(1))
+				_, obj, _ := fakeClient.CreateArgsForCall(0)
+				actualKpackImage, ok := obj.(*buildv1alpha2.Image)
+				Expect(ok).To(BeTrue(), "create wasn't passed a kpackImage")
+
+				Expect(fakeEnvBuilder.BuildEnvCallCount()).To(Equal(1))
+				_, actualApp := fakeEnvBuilder.BuildEnvArgsForCall(0)
+				Expect(actualApp).To(Equal(cfApp))
+
+				Expect(actualKpackImage.Spec.Build.Env).To(HaveLen(1))
+				Expect(actualKpackImage.Spec.Build.Env[0].Name).To(Equal("foo"))
+				Expect(actualKpackImage.Spec.Build.Env[0].Value).To(Equal("var"))
+			})
 		})
 
 		When("on unhappy path", func() {
@@ -271,6 +286,16 @@ var _ = Describe("CFBuildReconciler", func() {
 					Expect(reconcileErr).To(HaveOccurred())
 				})
 			})
+
+			When("building environment fails", func() {
+				BeforeEach(func() {
+					fakeEnvBuilder.BuildEnvReturns(nil, errors.New("boom"))
+				})
+
+				It("should return an error", func() {
+					Expect(reconcileErr).To(HaveOccurred())
+				})
+			})
 		})
 	})