Fail creating of revision is istio-injection label is not in namespace

pkg/apis/serving/v1alpha1/revision_types.go

@@ -398,6 +398,21 @@ func (rs *RevisionStatus) MarkInactive() {
 	})
 }
 
+func (rs *RevisionStatus) MarkNetworkProxyMissing() {
+	for _, cond := range []RevisionConditionType{
+		RevisionConditionContainerHealthy,
+		RevisionConditionReady,
+		RevisionConditionResourcesAvailable,
+	} {
+		rs.setCondition(&RevisionCondition{
+			Type:    cond,
+			Status:  corev1.ConditionFalse,
+			Reason:  "NetworkProxyMissing",
+			Message: "Missing istio proxy",
+		})
+	}
+}
+
 func (rs *RevisionStatus) MarkContainerMissing(message string) {
 	for _, cond := range []RevisionConditionType{
 		RevisionConditionContainerHealthy,

pkg/apis/serving/v1alpha1/revision_types_test.go

@@ -438,6 +438,33 @@ func TestTypicalFlowWithContainerMissing(t *testing.T) {
 	}
 }
 
+func TestMarkNetworkMissing(t *testing.T) {
+	r := &Revision{}
+	r.Status.InitializeConditions()
+	r.Status.InitializeBuildCondition()
+
+	checkConditionOngoingRevision(r.Status, RevisionConditionResourcesAvailable, t)
+	checkConditionOngoingRevision(r.Status, RevisionConditionContainerHealthy, t)
+	checkConditionOngoingRevision(r.Status, RevisionConditionReady, t)
+	checkConditionOngoingRevision(r.Status, RevisionConditionBuildSucceeded, t)
+
+	// Mark the revision status NetworkProxyMissing
+	r.Status.MarkNetworkProxyMissing()
+
+	// Marking NetworkProxyMissing shouldn't affect the build condition
+	checkConditionOngoingRevision(r.Status, RevisionConditionBuildSucceeded, t)
+
+	if got := checkConditionFailedRevision(r.Status, RevisionConditionReady, t); got == nil || got.Reason != "NetworkProxyMissing" {
+		t.Errorf("MarkNetworkProxyMissing = %v, want missing istio proxy", got)
+	}
+	if got := checkConditionFailedRevision(r.Status, RevisionConditionContainerHealthy, t); got == nil || got.Reason != "NetworkProxyMissing" {
+		t.Errorf("MarkNetworkProxyMissing = %v, want missing istio proxy", got)
+	}
+	if got := checkConditionFailedRevision(r.Status, RevisionConditionResourcesAvailable, t); got == nil || got.Reason != "NetworkProxyMissing" {
+		t.Errorf("MarkNetworkProxyMissing = %v, want missing istio proxy", got)
+	}
+}
+
 func TestTypicalFlowWithSuspendResume(t *testing.T) {
 	r := &Revision{}
 	r.Status.InitializeConditions()

pkg/controller/revision/revision.go

@@ -18,6 +18,7 @@ package revision
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"log"
 	"net/http"
@@ -44,7 +45,6 @@ import (
 	appsv1informers "k8s.io/client-go/informers/apps/v1"
 	corev1informers "k8s.io/client-go/informers/core/v1"
 
-	"k8s.io/apimachinery/pkg/api/errors"
 	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -60,6 +60,8 @@ import (
 )
 
 const (
+	istioInjectionLabel string = "istio-injection"
+
 	userContainerName string = "user-container"
 	userPortName      string = "user-port"
 	userPort                 = 8080
@@ -262,12 +264,13 @@ func (c *Controller) Reconcile(key string) error {
 	// Get the Revision resource with this namespace/name
 	rev, err := c.revisionLister.Revisions(namespace).Get(name)
 	// The resource may no longer exist, in which case we stop processing.
-	if errors.IsNotFound(err) {
+	if apierrs.IsNotFound(err) {
 		runtime.HandleError(fmt.Errorf("revision %q in work queue no longer exists", key))
 		return nil
 	} else if err != nil {
 		return err
 	}
+
 	// Don't modify the informer's copy.
 	rev = rev.DeepCopy()
 	rev.Status.InitializeConditions()
@@ -317,6 +320,15 @@ func (c *Controller) Reconcile(key string) error {
 		switch rev.Spec.ServingState {
 		case v1alpha1.RevisionServingStateActive:
 			logger.Info("Creating or reconciling resources for revision")
+			err := c.checkNamespaceHasIstioInjectionLabel(rev.Namespace)
+			if err != nil {
+				rev.Status.MarkNetworkProxyMissing()
+				if _, err := c.updateStatus(rev); err != nil {
+					logger.Error("Error recording inactivation of revision for: ", zap.Error(err))
+					return err
+				}
+				return apierrs.NewBadRequest("Missing Istio Proxy")
+			}
 			return c.createK8SResources(ctx, rev)
 
 		case v1alpha1.RevisionServingStateReserve:
@@ -445,6 +457,24 @@ func (c *Controller) SyncEndpoints(endpoint *corev1.Endpoints) {
 	return
 }
 
+func (c *Controller) checkNamespaceHasIstioInjectionLabel(namespace string) error {
+	ns, err := c.KubeClientSet.CoreV1().Namespaces().Get(namespace, metav1.GetOptions{})
+	if err != nil {
+		return fmt.Errorf("error fetching namespace: %s", err)
+	}
+
+	val, ok := ns.GetLabels()[istioInjectionLabel]
+
+	if !ok {
+		return errors.New("Missing istio injection label")
+	}
+
+	if val != "enabled" {
+		return errors.New("Expected istio injection label value to be 'enabled'")
+	}
+	return nil
+}
+
 func (c *Controller) deleteK8SResources(ctx context.Context, rev *v1alpha1.Revision) error {
 	logger := logging.FromContext(ctx)
 	logger.Info("Deleting the resources for revision")

pkg/controller/revision/revision_test.go

@@ -238,6 +238,7 @@ func newTestControllerWithConfig(t *testing.T, controllerConfig *ControllerConfi
 			Name:      ctrl.GetNetworkConfigMapName(),
 		},
 	})
+	createNamespace(kubeClient, testNamespace)
 
 	// Create informer factories with fake clients. The second parameter sets the
 	// resync period to zero, disabling it.
@@ -330,6 +331,18 @@ func updateRevision(elaClient *fakeclientset.Clientset, elaInformer informers.Sh
 	controller.Reconcile(KeyOrDie(rev))
 }
 
+func createNamespace(kubeClient *fakekubeclientset.Clientset, namespace string) {
+	kubeClient.CoreV1().Namespaces().Create(
+		&corev1.Namespace{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: namespace,
+				Labels: map[string]string{
+					istioInjectionLabel: "enabled",
+				},
+			},
+		})
+}
+
 type fixedResolver struct {
 	digest string
 }
@@ -1418,6 +1431,41 @@ func TestDoNotUpdateRevIfRevIsAlreadyReady(t *testing.T) {
 	controller.SyncEndpoints(endpoints)
 }
 
+func TestIstioLabelInjectionNotInNamespace(t *testing.T) {
+	kubeclient, _, elaClient, _, controller, _, _, elaInformer, _, _ := newTestController(t)
+
+	ns, err := kubeclient.CoreV1().Namespaces().Get(testNamespace, metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("Failed to get namespaces: %s", err)
+	}
+	ns.SetLabels(map[string]string{})
+
+	_, err = kubeclient.CoreV1().Namespaces().Update(ns)
+
+	rev := getTestRevision()
+	createRevision(elaClient, elaInformer, controller, rev)
+
+	revClient := elaClient.ServingV1alpha1().Revisions(testNamespace)
+	failedRev, err := revClient.Get(rev.Name, metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("Couldn't get revision: %v", err)
+	}
+
+	for _, ct := range []v1alpha1.RevisionConditionType{"ContainerHealthy", "Ready", "ResourcesAvailable"} {
+		got := failedRev.Status.GetCondition(ct)
+		want := &v1alpha1.RevisionCondition{
+			Type:               ct,
+			Status:             corev1.ConditionFalse,
+			Reason:             "NetworkProxyMissing",
+			Message:            "Missing istio proxy",
+			LastTransitionTime: got.LastTransitionTime,
+		}
+		if diff := cmp.Diff(want, got); diff != "" {
+			t.Errorf("Unexpected revision conditions diff (-want +got): %v", diff)
+		}
+	}
+}
+
 func TestDoNotUpdateRevIfRevIsMarkedAsFailed(t *testing.T) {
 	_, _, elaClient, _, controller, _, _, elaInformer, _, _ := newTestController(t)
 	rev := getTestRevision()

test/README.md

@@ -88,7 +88,12 @@ These tests require:
     kubectl create namespace pizzaplanet
     kubectl create namespace noodleburg
     ```
-3. A docker repo containing [the test images](#test-images)
+3. Label the `pizzaplanet` and `noodleburg` namespaces with `istio-injection=enabled`.
+    ```bash
+    kubectl label namespace pizzaplanet istio-injection=enabled
+    kubectl label namespace noodleburg istio-injection=enabled
+    ```
+4. A docker repo containing [the test images](#test-images)
 
 ## Test images
 

test/e2e-tests.sh

@@ -141,6 +141,7 @@ function dump_stack_info() {
 function run_e2e_tests() {
   header "Running tests in $1"
   kubectl create namespace $2
+  kubectl label namespace $2 istio-injection=enabled
   report_go_test -v -tags=e2e ./test/$1 -dockerrepo gcr.io/elafros-e2e-tests/$3
   local result=$?
   [[ ${result} -ne 0 ]] && dump_stack_info
@@ -157,6 +158,10 @@ function run_smoke_test() {
   local IMAGE="gcr.io/elafros-e2e-tests/ela-e2e-test/sample/helloworld"
   sed "s@github.com/knative/serving/sample/helloworld@${IMAGE}@g" \
     sample/helloworld/sample.yaml > ${YAML}
+
+  local TARGET_NAMESPACE=$(grep "namespace:" ${YAML} -m 1 | awk '{ print $2 }')
+  kubectl label namespace ${TARGET_NAMESPACE} istio-injection=enabled
+
   kubectl apply -f ${YAML}
   local service_host=""
   local service_ip=""
@@ -178,6 +183,7 @@ function run_smoke_test() {
     # service_host or service_ip might contain a useful error, dump it.
     echo "FAILED -- No ingress found. ${service_host}${service_ip}"
     kubectl delete -f ${YAML}
+    kubectl label namespace ${TARGET_NAMESPACE} istio-injection-
     return 1
   fi
   local failed=1
@@ -191,6 +197,7 @@ function run_smoke_test() {
     sleep 2
   done
   kubectl delete -f ${YAML}
+  kubectl label namespace ${TARGET_NAMESPACE} istio-injection-
   if (( failed )); then
     echo "FAILED -- Timeout waiting for app to be serving"
     return 1