Repository: GoogleContainerTools/kpt
Pull request #3565 (2022-09-21 14:58:51)

Only run e2e tests if E2E env var is set
This allows us to run `go test ./...` without running integration tests.

Suggested by:
https://peter.bourgon.org/blog/2021/04/02/dont-use-build-tags-for-integration-tests.html


Patch:
diff --git a/porch/Makefile b/porch/Makefile
index 82e32962..530f947f 100644
--- a/porch/Makefile
+++ b/porch/Makefile
@@ -138,7 +138,7 @@ tidy:
 
 .PHONY: test
 test:
-	@for f in $(MODULES); do (cd $$f; echo "Testing $$f"; go test --count=1 ./...) || exit 1; done
+	@for f in $(MODULES); do (cd $$f; echo "Testing $$f"; E2E=1 go test --count=1 ./...) || exit 1; done
 
 .PHONY: vet
 vet:
diff --git a/porch/controllers/config/crd/bases/config.porch.kpt.dev_workloadidentitybindings.yaml b/porch/controllers/config/crd/bases/config.porch.kpt.dev_workloadidentitybindings.yaml
index 645e231c..2380f2a8 100644
--- a/porch/controllers/config/crd/bases/config.porch.kpt.dev_workloadidentitybindings.yaml
+++ b/porch/controllers/config/crd/bases/config.porch.kpt.dev_workloadidentitybindings.yaml
@@ -52,27 +52,19 @@ spec:
             type: object
           spec:
             description: WorkloadIdentityBindingSpec defines the desired state of
-              RemoteRootSync
+              WorkloadIdentityBinding
             properties:
-              resourceRef:
+              gcpServiceAccountRef:
                 properties:
-                  apiVersion:
-                    type: string
                   external:
                     type: string
-                  kind:
-                    type: string
                   name:
                     type: string
                   namespace:
                     type: string
                 type: object
-              serviceAccountRef:
+              kubernetesServiceAccountRef:
                 properties:
-                  apiVersion:
-                    type: string
-                  kind:
-                    type: string
                   name:
                     type: string
                   namespace:
diff --git a/porch/controllers/workloadidentitybindings/api/v1alpha1/workloadidentitybinding_types.go b/porch/controllers/workloadidentitybindings/api/v1alpha1/workloadidentitybinding_types.go
index bf2ff2a6..b18a3ce2 100644
--- a/porch/controllers/workloadidentitybindings/api/v1alpha1/workloadidentitybinding_types.go
+++ b/porch/controllers/workloadidentitybindings/api/v1alpha1/workloadidentitybinding_types.go
@@ -31,25 +31,21 @@ type WorkloadIdentityBinding struct {
 	Status WorkloadIdentityBindingStatus `json:"status,omitempty"`
 }
 
-// WorkloadIdentityBindingSpec defines the desired state of RemoteRootSync
+// WorkloadIdentityBindingSpec defines the desired state of WorkloadIdentityBinding
 type WorkloadIdentityBindingSpec struct {
-	ServiceAccountRef ServiceAccountRef `json:"serviceAccountRef,omitempty"`
-	ResourceRef       ResourceRef       `json:"resourceRef,omitempty"`
+	KubernetesServiceAccountRef KubernetesServiceAccountRef `json:"kubernetesServiceAccountRef,omitempty"`
+	GcpServiceAccountRef        GcpServiceAccountRef        `json:"gcpServiceAccountRef,omitempty"`
 }
 
-type ServiceAccountRef struct {
-	ApiVersion string `json:"apiVersion,omitempty"`
-	Kind       string `json:"kind,omitempty"`
-	Name       string `json:"name,omitempty"`
-	Namespace  string `json:"namespace,omitempty"`
+type KubernetesServiceAccountRef struct {
+	Name      string `json:"name,omitempty"`
+	Namespace string `json:"namespace,omitempty"`
 }
 
-type ResourceRef struct {
-	ApiVersion string `json:"apiVersion,omitempty"`
-	Kind       string `json:"kind,omitempty"`
-	Name       string `json:"name,omitempty"`
-	Namespace  string `json:"namespace,omitempty"`
-	External   string `json:"external,omitempty"`
+type GcpServiceAccountRef struct {
+	Name      string `json:"name,omitempty"`
+	Namespace string `json:"namespace,omitempty"`
+	External  string `json:"external,omitempty"`
 }
 
 // WorkloadIdentityBindingStatus defines the observed state of WorkloadIdentityBinding
diff --git a/porch/controllers/workloadidentitybindings/api/v1alpha1/zz_generated.deepcopy.go b/porch/controllers/workloadidentitybindings/api/v1alpha1/zz_generated.deepcopy.go
index 17f8755b..ae72e6b2 100644
--- a/porch/controllers/workloadidentitybindings/api/v1alpha1/zz_generated.deepcopy.go
+++ b/porch/controllers/workloadidentitybindings/api/v1alpha1/zz_generated.deepcopy.go
@@ -25,31 +25,31 @@ import (
 )
 
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
-func (in *ResourceRef) DeepCopyInto(out *ResourceRef) {
+func (in *GcpServiceAccountRef) DeepCopyInto(out *GcpServiceAccountRef) {
 	*out = *in
 }
 
-// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ResourceRef.
-func (in *ResourceRef) DeepCopy() *ResourceRef {
+// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GcpServiceAccountRef.
+func (in *GcpServiceAccountRef) DeepCopy() *GcpServiceAccountRef {
 	if in == nil {
 		return nil
 	}
-	out := new(ResourceRef)
+	out := new(GcpServiceAccountRef)
 	in.DeepCopyInto(out)
 	return out
 }
 
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
-func (in *ServiceAccountRef) DeepCopyInto(out *ServiceAccountRef) {
+func (in *KubernetesServiceAccountRef) DeepCopyInto(out *KubernetesServiceAccountRef) {
 	*out = *in
 }
 
-// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ServiceAccountRef.
-func (in *ServiceAccountRef) DeepCopy() *ServiceAccountRef {
+// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new KubernetesServiceAccountRef.
+func (in *KubernetesServiceAccountRef) DeepCopy() *KubernetesServiceAccountRef {
 	if in == nil {
 		return nil
 	}
-	out := new(ServiceAccountRef)
+	out := new(KubernetesServiceAccountRef)
 	in.DeepCopyInto(out)
 	return out
 }
@@ -116,8 +116,8 @@ func (in *WorkloadIdentityBindingList) DeepCopyObject() runtime.Object {
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *WorkloadIdentityBindingSpec) DeepCopyInto(out *WorkloadIdentityBindingSpec) {
 	*out = *in
-	out.ServiceAccountRef = in.ServiceAccountRef
-	out.ResourceRef = in.ResourceRef
+	out.KubernetesServiceAccountRef = in.KubernetesServiceAccountRef
+	out.GcpServiceAccountRef = in.GcpServiceAccountRef
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new WorkloadIdentityBindingSpec.
diff --git a/porch/controllers/workloadidentitybindings/config/rbac/role.yaml b/porch/controllers/workloadidentitybindings/config/rbac/role.yaml
index 13120698..bda98f03 100644
--- a/porch/controllers/workloadidentitybindings/config/rbac/role.yaml
+++ b/porch/controllers/workloadidentitybindings/config/rbac/role.yaml
@@ -27,6 +27,15 @@ rules:
   - get
   - list
   - watch
+- apiGroups:
+  - ""
+  resources:
+  - serviceaccounts
+  verbs:
+  - get
+  - list
+  - patch
+  - watch
 - apiGroups:
   - config.porch.kpt.dev
   resources:
@@ -65,3 +74,11 @@ rules:
   - patch
   - update
   - watch
+- apiGroups:
+  - iam.cnrm.cloud.google.com
+  resources:
+  - iamserviceaccounts
+  verbs:
+  - get
+  - list
+  - watch
diff --git a/porch/controllers/workloadidentitybindings/pkg/controllers/workloadidentitybinding/controller.go b/porch/controllers/workloadidentitybindings/pkg/controllers/workloadidentitybinding/controller.go
index e228595c..962aa7e2 100644
--- a/porch/controllers/workloadidentitybindings/pkg/controllers/workloadidentitybinding/controller.go
+++ b/porch/controllers/workloadidentitybindings/pkg/controllers/workloadidentitybinding/controller.go
@@ -16,6 +16,7 @@ package workloadidentitybinding
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 
 	"github.com/GoogleContainerTools/kpt/porch/controllers/remoterootsyncsets/pkg/applyset"
@@ -29,6 +30,11 @@ import (
 	"k8s.io/klog/v2"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+)
+
+const (
+	finalizerName = "config.porch.kpt.dev/workloadidentitybindings"
 )
 
 // WorkloadIdentityBindingReconciler reconciles WorkloadIdentityBinding objects
@@ -47,6 +53,8 @@ type WorkloadIdentityBindingReconciler struct {
 //+kubebuilder:rbac:groups=config.porch.kpt.dev,resources=workloadidentitybindings/finalizers,verbs=update
 //+kubebuilder:rbac:groups="",resources=namespaces,verbs=get;list;watch
 //+kubebuilder:rbac:groups=iam.cnrm.cloud.google.com,resources=iampolicymembers,verbs=get;list;watch;create;update;patch;delete
+//+kubebuilder:rbac:groups=iam.cnrm.cloud.google.com,resources=iamserviceaccounts,verbs=get;list;watch
+//+kubebuilder:rbac:groups="",resources=serviceaccounts,verbs=get;list;watch;patch
 
 // Reconcile implements the main kubernetes reconciliation loop.
 func (r *WorkloadIdentityBindingReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
@@ -54,39 +62,49 @@ func (r *WorkloadIdentityBindingReconciler) Reconcile(ctx context.Context, req c
 	if err := r.Get(ctx, req.NamespacedName, &subject); err != nil {
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
-	// myFinalizerName := "config.porch.kpt.dev/finalizer"
-	// if subject.ObjectMeta.DeletionTimestamp.IsZero() {
-	// 	// The object is not being deleted, so if it does not have our finalizer,
-	// 	// then lets add the finalizer and update the object. This is equivalent
-	// 	// registering our finalizer.
-	// 	if !controllerutil.ContainsFinalizer(&subject, myFinalizerName) {
-	// 		controllerutil.AddFinalizer(&subject, myFinalizerName)
-	// 		if err := r.Update(ctx, &subject); err != nil {
-	// 			return ctrl.Result{}, err
-	// 		}
-	// 	}
-	// } else {
-	// 	// The object is being deleted
-	// 	if controllerutil.ContainsFinalizer(&subject, myFinalizerName) {
-	// 		// // our finalizer is present, so lets handle any external dependency
-	// 		// if err := r.deleteExternalResources(ctx, &subject); err != nil {
-	// 		// 	// if fail to delete the external dependency here, return with error
-	// 		// 	// so that it can be retried
-	// 		// 	return ctrl.Result{}, fmt.Errorf("have problem to delete external resource: %w", err)
-	// 		// }
-	// 		// remove our finalizer from the list and update it.
-	// 		controllerutil.RemoveFinalizer(&subject, myFinalizerName)
-	// 		if err := r.Update(ctx, &subject); err != nil {
-	// 			return ctrl.Result{}, fmt.Errorf("failed to update %s after delete finalizer: %w", req.Name, err)
-	// 		}
-	// 	}
-	// 	// Stop reconciliation as the item is being deleted
-	// 	return ctrl.Result{}, nil
-	// }
+	if subject.ObjectMeta.DeletionTimestamp.IsZero() {
+		// The object is not being deleted, so if it does not have our finalizer,
+		// then lets add the finalizer and update the object. This is equivalent
+		// registering our finalizer.
+		if !controllerutil.ContainsFinalizer(&subject, finalizerName) {
+			controllerutil.AddFinalizer(&subject, finalizerName)
+			if err := r.Update(ctx, &subject); err != nil {
+				klog.Warningf("failed to update %s after adding finalizer: %v", req.Name, err)
+				return ctrl.Result{}, err
+			}
+		}
+	} else {
+		// The object is being deleted
+		if controllerutil.ContainsFinalizer(&subject, finalizerName) {
+			// // our finalizer is present, so lets remove the annotation from the SA
+			if err := r.removeWiAnnotation(ctx, &subject); err != nil {
+				// failed to remove the annotation, so return the error so it can be retried.
+				klog.Warningf("failed to remove SA annotation from %s: %v", req.Name, err)
+				return ctrl.Result{}, err
+			}
+			// remove our finalizer from the list and update it.
+			controllerutil.RemoveFinalizer(&subject, finalizerName)
+			if err := r.Update(ctx, &subject); err != nil {
+				klog.Warningf("failed to update %s after removing finalizer: %v", req.Name, err)
+				return ctrl.Result{}, err
+			}
+		}
+		// Stop reconciliation as the item is being deleted
+		return ctrl.Result{}, nil
+	}
 
 	var result ctrl.Result
 
-	results, err := r.applyToClusterRef(ctx, &subject)
+	projectID, err := r.findProjectID(ctx, &subject)
+	if err != nil {
+		return result, err
+	}
+
+	results, err := r.applyToClusterRef(ctx, projectID, &subject)
+	if err == nil && results.AllApplied() && results.AllHealthy() {
+		// If the IAMPolicyMember has been installed and reconciled, we add the annotation.
+		err = r.addWiAnnotation(ctx, projectID, &subject)
+	}
 	if updateStatus(&subject, results, err) {
 		if updateErr := r.Status().Update(ctx, &subject); updateErr != nil {
 			if err == nil {
@@ -124,21 +142,28 @@ func updateStatus(subject *api.WorkloadIdentityBinding, results *applyset.ApplyR
 	return true
 }
 
-func (r *WorkloadIdentityBindingReconciler) applyToClusterRef(ctx context.Context, subject *api.WorkloadIdentityBinding) (*applyset.ApplyResults, error) {
-
+func (r *WorkloadIdentityBindingReconciler) findProjectID(ctx context.Context, subject *api.WorkloadIdentityBinding) (string, error) {
 	ns := &corev1.Namespace{}
 	if err := r.Get(ctx, types.NamespacedName{Name: subject.GetNamespace()}, ns); err != nil {
-		return nil, fmt.Errorf("error getting namespace %q: %w", subject.GetNamespace(), err)
+		return "", fmt.Errorf("error getting namespace %q: %w", subject.GetNamespace(), err)
+	}
+
+	parentProjectID := ns.GetAnnotations()["cnrm.cloud.google.com/project-id"]
+	if parentProjectID == "" {
+		return "", fmt.Errorf("project-id not found for namespace %q", ns.GetName())
 	}
+	return parentProjectID, nil
+}
 
-	objects, err := r.BuildObjectsToApply(ctx, ns, subject)
+func (r *WorkloadIdentityBindingReconciler) applyToClusterRef(ctx context.Context, projectID string, subject *api.WorkloadIdentityBinding) (*applyset.ApplyResults, error) {
+	objects, err := r.BuildObjectsToApply(ctx, projectID, subject)
 	if err != nil {
 		return nil, err
 	}
 
 	// TODO: Cache applyset?
 	patchOptions := metav1.PatchOptions{
-		FieldManager: subject.GetObjectKind().GroupVersionKind().Kind + "-" + subject.GetNamespace() + "-" + subject.GetName(),
+		FieldManager: fieldManager(subject),
 	}
 
 	// We force to overcome errors like: Apply failed with 1 conflict: conflict with "kubectl-client-side-apply" using apps/v1: .spec.template.spec.containers[name="porch-server"].image
@@ -167,7 +192,7 @@ func (r *WorkloadIdentityBindingReconciler) applyToClusterRef(ctx context.Contex
 	return results, nil
 }
 
-func (r *WorkloadIdentityBindingReconciler) BuildObjectsToApply(ctx context.Context, ns *corev1.Namespace, subject *api.WorkloadIdentityBinding) ([]applyset.ApplyableObject, error) {
+func (r *WorkloadIdentityBindingReconciler) BuildObjectsToApply(ctx context.Context, projectID string, subject *api.WorkloadIdentityBinding) ([]applyset.ApplyableObject, error) {
 	var objects []applyset.ApplyableObject
 
 	{
@@ -177,7 +202,7 @@ func (r *WorkloadIdentityBindingReconciler) BuildObjectsToApply(ctx context.Cont
 		u.SetName("workloadidentitybinding-" + subject.GetName())
 		u.SetNamespace(subject.GetNamespace())
 
-		saRef := subject.Spec.ServiceAccountRef
+		saRef := subject.Spec.KubernetesServiceAccountRef
 
 		saNamespace := saRef.Namespace
 		saName := saRef.Name
@@ -185,16 +210,23 @@ func (r *WorkloadIdentityBindingReconciler) BuildObjectsToApply(ctx context.Cont
 			saNamespace = subject.GetNamespace()
 		}
 
-		parentProjectID := ns.GetAnnotations()["cnrm.cloud.google.com/project-id"]
-		if parentProjectID == "" {
-			return nil, fmt.Errorf("project-id not found for namespace %q", ns.GetName())
-		}
-		member := "serviceAccount:" + parentProjectID + ".svc.id.goog[" + saNamespace + "/" + saName + "]"
+		member := "serviceAccount:" + projectID + ".svc.id.goog[" + saNamespace + "/" + saName + "]"
 
+		resourceRef := map[string]string{
+			"apiVersion": "iam.cnrm.cloud.google.com/v1beta1",
+			"kind":       "IAMServiceAccount",
+			"name":       subject.Spec.GcpServiceAccountRef.Name,
+		}
+		if ns := subject.Spec.GcpServiceAccountRef.Namespace; ns != "" {
+			resourceRef["namespace"] = ns
+		}
+		if ext := subject.Spec.GcpServiceAccountRef.External; ext != "" {
+			resourceRef["external"] = ext
+		}
 		u.Object["spec"] = map[string]interface{}{
 			"member":      member,
 			"role":        "roles/iam.workloadIdentityUser",
-			"resourceRef": subject.Spec.ResourceRef,
+			"resourceRef": resourceRef,
 		}
 
 		objects = append(objects, u)
@@ -213,9 +245,115 @@ func (r *WorkloadIdentityBindingReconciler) BuildObjectsToApply(ctx context.Cont
 		})
 		obj.SetOwnerReferences(ownerRefs)
 	}
+
 	return objects, nil
 }
 
+func (r *WorkloadIdentityBindingReconciler) addWiAnnotation(ctx context.Context, projectID string, subject *api.WorkloadIdentityBinding) error {
+	// TODO: We should have a watch here so we can annotate the ServiceAccount even if it is
+	// applied later.
+	ksa, err := r.getKubernetesServiceAccount(ctx, subject)
+	if err != nil {
+		return client.IgnoreNotFound(err)
+	}
+
+	// TODO: Same as above, we should have a watch here so we can annotate the ksa when
+	// the gsa is created and reconciled (i.e. have the status.email field set)
+	gsa, err := r.getGcpServiceAccount(ctx, subject)
+	if err != nil {
+		return client.IgnoreNotFound(err)
+	}
+
+	gsaEmail, found, err := unstructured.NestedString(gsa.Object, "status", "email")
+	if err != nil {
+		return err
+	}
+	if !found || gsaEmail == "" {
+		return nil
+	}
+
+	u := &unstructured.Unstructured{}
+	u.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ServiceAccount"))
+	u.SetName(ksa.GetName())
+	u.SetNamespace(ksa.GetNamespace())
+	u.SetAnnotations(map[string]string{
+		"iam.gke.io/gcp-service-account": gsaEmail,
+	})
+
+	return r.updateServiceAccount(ctx, u, subject)
+}
+
+func (r *WorkloadIdentityBindingReconciler) removeWiAnnotation(ctx context.Context, subject *api.WorkloadIdentityBinding) error {
+	sa, err := r.getKubernetesServiceAccount(ctx, subject)
+	if err != nil {
+		return client.IgnoreNotFound(err)
+	}
+
+	u := &unstructured.Unstructured{}
+	u.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ServiceAccount"))
+	u.SetName(sa.GetName())
+	u.SetNamespace(sa.GetNamespace())
+	u.SetAnnotations(make(map[string]string))
+
+	return r.updateServiceAccount(ctx, u, subject)
+}
+
+func (r *WorkloadIdentityBindingReconciler) getKubernetesServiceAccount(ctx context.Context, subject *api.WorkloadIdentityBinding) (*unstructured.Unstructured, error) {
+	saRef := subject.Spec.KubernetesServiceAccountRef
+
+	saName := saRef.Name
+	saNamespace := saRef.Namespace
+	if saNamespace == "" {
+		saNamespace = subject.GetNamespace()
+	}
+
+	var sa unstructured.Unstructured
+	sa.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ServiceAccount"))
+	if err := r.Get(ctx, types.NamespacedName{Name: saName, Namespace: saNamespace}, &sa); err != nil {
+		return nil, err
+	}
+
+	return &sa, nil
+}
+
+func (r *WorkloadIdentityBindingReconciler) getGcpServiceAccount(ctx context.Context, subject *api.WorkloadIdentityBinding) (*unstructured.Unstructured, error) {
+	gsaRef := subject.Spec.GcpServiceAccountRef
+
+	gsaName := gsaRef.Name
+	gsaNamespace := gsaRef.Namespace
+	if gsaNamespace == "" {
+		gsaNamespace = subject.GetNamespace()
+	}
+
+	u := &unstructured.Unstructured{}
+	u.SetAPIVersion("iam.cnrm.cloud.google.com/v1beta1")
+	u.SetKind("IAMServiceAccount")
+
+	err := r.Get(ctx, types.NamespacedName{Name: gsaName, Namespace: gsaNamespace}, u)
+	return u, err
+}
+
+func (r *WorkloadIdentityBindingReconciler) updateServiceAccount(ctx context.Context, sa *unstructured.Unstructured, subject *api.WorkloadIdentityBinding) error {
+	data, err := json.Marshal(sa)
+	if err != nil {
+		return err
+	}
+
+	mapping, err := r.restMapper.RESTMapping(corev1.SchemeGroupVersion.WithKind("ServiceAccount").GroupKind())
+	if err != nil {
+		return err
+	}
+	dr := r.dynamicClient.Resource(mapping.Resource)
+	_, err = dr.Namespace(sa.GetNamespace()).Patch(ctx, sa.GetName(), types.ApplyPatchType, data, metav1.PatchOptions{
+		FieldManager: fieldManager(subject),
+	})
+	return client.IgnoreNotFound(err)
+}
+
+func fieldManager(subject *api.WorkloadIdentityBinding) string {
+	return subject.GetObjectKind().GroupVersionKind().Kind + "-" + subject.GetNamespace() + "-" + subject.GetName()
+}
+
 // SetupWithManager sets up the controller with the Manager.
 func (r *WorkloadIdentityBindingReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	if err := api.AddToScheme(mgr.GetScheme()); err != nil {


Test patch:
diff --git a/porch/test/e2e/e2e_test.go b/porch/test/e2e/e2e_test.go
index 2be126b7..62fcc83f 100644
--- a/porch/test/e2e/e2e_test.go
+++ b/porch/test/e2e/e2e_test.go
@@ -46,6 +46,11 @@ const (
 )
 
 func TestE2E(t *testing.T) {
+	e2e := os.Getenv("E2E")
+	if e2e == "" {
+		t.Skip("set E2E to run this test")
+	}
+
 	Run(&PorchSuite{}, t)
 }