MAISTRA-1071 Add manifest namespace validation

pkg/controller/common/util.go

@@ -8,10 +8,12 @@ import (
 	core "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/record"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
+	maistrav1 "github.com/maistra/istio-operator/pkg/apis/maistra/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -141,3 +143,15 @@ func BoolToConditionStatus(b bool) core.ConditionStatus {
 		return core.ConditionFalse
 	}
 }
+
+// GetMeshNamespaces returns all namespaces that are part of a mesh.
+func GetMeshNamespaces(smcp *maistrav1.ServiceMeshControlPlane, smmr *maistrav1.ServiceMeshMemberRoll) sets.String {
+	if smcp == nil {
+		return sets.NewString()
+	}
+	meshNamespaces := sets.NewString(smcp.GetNamespace())
+	if smmr != nil {
+		meshNamespaces.Insert(smmr.Status.ConfiguredMembers...)
+	}
+	return meshNamespaces
+}

pkg/controller/servicemesh/controlplane/reconciler.go

@@ -242,6 +242,16 @@ func (r *controlPlaneInstanceReconciler) Reconcile(ctx context.Context) (result
 		}
 	}
 
+	// validate generated manifests
+	// this has to be done always before applying because the memberroll might have changed
+	err = r.validateManifests(ctx)
+	if err != nil {
+		reconciliationReason = v1.ConditionReasonReconcileError
+		reconciliationMessage = "Error validating generated manifests"
+		err = errors.Wrap(err, reconciliationMessage)
+		return
+	}
+
 	// create components
 	for _, key := range r.getChartsInInstallationOrder() {
 		if ready, err = r.processComponentManifests(ctx, key); !ready {
@@ -646,6 +656,55 @@ func (r *controlPlaneInstanceReconciler) renderCharts(ctx context.Context) error
 	return nil
 }
 
+func (r *controlPlaneInstanceReconciler) validateManifests(ctx context.Context) error {
+	log := common.LogFromContext(ctx)
+	allErrors := []error{}
+	// validate resource namespaces
+	smmr := &v1.ServiceMeshMemberRoll{}
+	var smmrRetrievalError error
+	if smmrRetrievalError = r.Client.Get(context.TODO(), client.ObjectKey{Namespace: r.Instance.GetNamespace(), Name: common.MemberRollName}, smmr); smmrRetrievalError != nil {
+		if !apierrors.IsNotFound(smmrRetrievalError) {
+			// log error, but don't fail validation just yet: we'll just assume that the control plane namespace is the only namespace for now
+			// if we end up failing validation because of this assumption, we'll return this error
+			log.Error(smmrRetrievalError, "failed to retrieve SMMR for SMCP")
+			smmr = nil
+		}
+	}
+	meshNamespaces := common.GetMeshNamespaces(r.Instance, smmr)
+	for _, manifestList := range r.renderings {
+		for _, manifestBundle := range manifestList {
+			for _, manifest := range strings.Split(manifestBundle.Content, "---") {
+				obj := map[string]interface{}{}
+				err := yaml.Unmarshal([]byte(manifest), &obj)
+				if err != nil || obj == nil {
+					continue
+				}
+				metadata, ok := obj["metadata"].(map[string]interface{})
+				if !ok {
+					// if it doesn't have a metadata section, ignore
+					continue
+				}
+				objNs, ok := metadata["namespace"].(string)
+				if !ok {
+					// if namespace is not set, ignore
+					continue
+				}
+				if !meshNamespaces.Has(objNs) {
+					allErrors = append(allErrors, fmt.Errorf("%s: namespace of manifest %s/%s not in mesh", manifestBundle.Name, metadata["namespace"], metadata["name"]))
+				}
+			}
+		}
+	}
+	if len(allErrors) > 0 {
+		// if validation fails because we couldn't Get() the SMMR, return that error
+		if smmrRetrievalError != nil {
+			return smmrRetrievalError
+		}
+		return utilerrors.NewAggregate(allErrors)
+	}
+	return nil
+}
+
 func (r *controlPlaneInstanceReconciler) PostStatus(ctx context.Context) error {
 	log := common.LogFromContext(ctx)
 	instance := &v1.ServiceMeshControlPlane{}

pkg/controller/servicemesh/controlplane/reconciler_test.go

@@ -2,13 +2,15 @@ package controlplane
 
 import (
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/record"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"k8s.io/client-go/kubernetes/scheme"
 
@@ -171,6 +173,181 @@ func TestInstallationErrorDoesNotUpdateLastTransitionTimeWhenNoStateTransitionOc
 	assert.DeepEquals(newStatus, initialStatus, "didn't expect SMCP status to be updated", t)
 }
 
+type customSetup func(client.Client, *test.EnhancedTracker)
+
+func TestManifestValidation(t *testing.T) {
+	testCases := []struct {
+		name         string
+		controlPlane *maistrav1.ServiceMeshControlPlane
+		memberRoll   *maistrav1.ServiceMeshMemberRoll
+		setupFn      customSetup
+		errorMessage string
+	}{
+		{
+			name: "error getting smmr",
+			controlPlane: &maistrav1.ServiceMeshControlPlane{
+				ObjectMeta: newControlPlane().ObjectMeta,
+				Spec: maistrav1.ControlPlaneSpec{
+					Template: "maistra",
+					Version:  "v1.1",
+					Istio: map[string]interface{}{
+						"gateways": map[string]interface{}{
+							"istio-ingressgateway": map[string]interface{}{
+								"namespace": "somewhere",
+							},
+						},
+					},
+				},
+				Status: maistrav1.ControlPlaneStatus{},
+			},
+			memberRoll: &maistrav1.ServiceMeshMemberRoll{},
+			setupFn: func(cl client.Client, tracker *test.EnhancedTracker) {
+				tracker.AddReactor("get", "servicemeshmemberrolls", test.ClientFails())
+			},
+			errorMessage: "some error",
+		},
+		{
+			name: "gateways outside of mesh",
+			controlPlane: &maistrav1.ServiceMeshControlPlane{
+				ObjectMeta: newControlPlane().ObjectMeta,
+				Spec: maistrav1.ControlPlaneSpec{
+					Template: "maistra",
+					Version:  "v1.1",
+					Istio: map[string]interface{}{
+						"gateways": map[string]interface{}{
+							"another-gateway": map[string]interface{}{
+								"enabled":   "true",
+								"namespace": "b",
+								"labels":    map[string]interface{}{},
+							},
+							"istio-ingressgateway": map[string]interface{}{
+								"namespace": "c",
+							},
+							"istio-egressgateway": map[string]interface{}{
+								"namespace": "d",
+							},
+						},
+					},
+				},
+				Status: maistrav1.ControlPlaneStatus{},
+			},
+			memberRoll: &maistrav1.ServiceMeshMemberRoll{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "default",
+					Namespace: controlPlaneNamespace,
+				},
+				Spec: maistrav1.ServiceMeshMemberRollSpec{
+					Members: []string{
+						"a",
+					},
+				},
+				Status: maistrav1.ServiceMeshMemberRollStatus{
+					ConfiguredMembers: []string{
+						"a",
+					},
+				},
+			},
+			errorMessage: "namespace of manifest c/istio-ingressgateway not in mesh",
+		},
+		{
+			name: "valid namespaces",
+			controlPlane: &maistrav1.ServiceMeshControlPlane{
+				ObjectMeta: newControlPlane().ObjectMeta,
+				Spec: maistrav1.ControlPlaneSpec{
+					Template: "maistra",
+					Version:  "v1.1",
+					Istio: map[string]interface{}{
+						"gateways": map[string]interface{}{
+							"istio-ingressgateway": map[string]interface{}{
+								"namespace": "a",
+							},
+							"istio-egressgateway": map[string]interface{}{
+								"namespace": "c",
+							},
+						},
+					},
+				},
+				Status: maistrav1.ControlPlaneStatus{},
+			},
+			memberRoll: &maistrav1.ServiceMeshMemberRoll{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "default",
+					Namespace: controlPlaneNamespace,
+				},
+				Spec: maistrav1.ServiceMeshMemberRollSpec{
+					Members: []string{
+						"a",
+						"c",
+					},
+				},
+				Status: maistrav1.ServiceMeshMemberRollStatus{
+					ConfiguredMembers: []string{
+						"a",
+						"c",
+					},
+				},
+			},
+		},
+	}
+
+	operatorNamespace := "istio-operator"
+	InitializeGlobals(operatorNamespace)()
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			tc.controlPlane.Status.SetCondition(maistrav1.Condition{
+				Type:               maistrav1.ConditionTypeReconciled,
+				Status:             maistrav1.ConditionStatusFalse,
+				Reason:             "",
+				Message:            "",
+				LastTransitionTime: oneMinuteAgo,
+			})
+
+			namespace := &v1.Namespace{
+				ObjectMeta: metav1.ObjectMeta{Name: tc.controlPlane.Namespace},
+			}
+
+			cl, tracker := test.CreateClient(tc.controlPlane, tc.memberRoll, namespace)
+			fakeEventRecorder := &record.FakeRecorder{}
+
+			r := NewControlPlaneInstanceReconciler(
+				common.ControllerResources{
+					Client:            cl,
+					Scheme:            scheme.Scheme,
+					EventRecorder:     fakeEventRecorder,
+					PatchFactory:      common.NewPatchFactory(cl),
+					OperatorNamespace: operatorNamespace,
+				},
+				tc.controlPlane,
+				common.CNIConfig{Enabled: true})
+
+			if tc.setupFn != nil {
+				tc.setupFn(cl, tracker)
+			}
+			// run initial reconcile to update the SMCP status
+			_, err := r.Reconcile(ctx)
+
+			if tc.errorMessage != "" {
+				if err == nil {
+					t.Fatal(tc.name, "-", "Expected reconcile to fail, but it didn't")
+				}
+
+				updatedControlPlane := &maistrav1.ServiceMeshControlPlane{}
+				test.PanicOnError(cl.Get(ctx, common.ToNamespacedName(tc.controlPlane.ObjectMeta), updatedControlPlane))
+				newStatus := &updatedControlPlane.Status
+
+				assert.True(strings.Contains(newStatus.GetCondition(maistrav1.ConditionTypeReconciled).Message, tc.errorMessage), "Expected reconciliation error: "+tc.errorMessage+", but got:"+newStatus.GetCondition(maistrav1.ConditionTypeReconciled).Message, t)
+			} else {
+				if err != nil {
+					t.Fatal(tc.name, "-", "Expected no errors, but got error: ", err)
+				}
+			}
+		})
+
+	}
+
+}
+
 func newTestReconciler() *controlPlaneInstanceReconciler {
 	reconciler := NewControlPlaneInstanceReconciler(
 		common.ControllerResources{},

pkg/controller/servicemesh/webhooks/validation/controlplane_test.go

@@ -459,6 +459,88 @@ func TestControlPlaneValidation(t *testing.T) {
 			},
 			valid: false,
 		},
+		{
+			name: "gateway-outside-mesh",
+			controlPlane: &maistrav1.ServiceMeshControlPlane{
+				ObjectMeta: meta.ObjectMeta{
+					Name:      "some-smcp",
+					Namespace: "istio-system",
+				},
+				Spec: maistrav1.ControlPlaneSpec{
+					Version: maistra.V1_1.String(),
+					Istio: map[string]interface{}{
+						"gateways": map[string]interface{}{
+							"istio-ingressgateway": map[string]interface{}{
+								"namespace": "outside",
+							},
+							"istio-egressgateway": map[string]interface{}{
+								"namespace": "inside",
+							},
+						},
+					},
+				},
+			},
+			resources: []runtime.Object{
+				&maistrav1.ServiceMeshMemberRoll{
+					ObjectMeta: meta.ObjectMeta{
+						Name:      "default",
+						Namespace: "istio-system",
+					},
+					Spec: maistrav1.ServiceMeshMemberRollSpec{
+						Members: []string{
+							"inside",
+						},
+					},
+					Status: maistrav1.ServiceMeshMemberRollStatus{
+						ConfiguredMembers: []string{
+							"inside",
+						},
+					},
+				},
+			},
+			valid: false,
+		},
+		{
+			name: "gateway-inside-mesh",
+			controlPlane: &maistrav1.ServiceMeshControlPlane{
+				ObjectMeta: meta.ObjectMeta{
+					Name:      "some-smcp",
+					Namespace: "istio-system",
+				},
+				Spec: maistrav1.ControlPlaneSpec{
+					Version: maistra.V1_1.String(),
+					Istio: map[string]interface{}{
+						"gateways": map[string]interface{}{
+							"istio-ingressgateway": map[string]interface{}{
+								"namespace": "inside",
+							},
+							"istio-egressgateway": map[string]interface{}{
+								"namespace": "inside",
+							},
+						},
+					},
+				},
+			},
+			resources: []runtime.Object{
+				&maistrav1.ServiceMeshMemberRoll{
+					ObjectMeta: meta.ObjectMeta{
+						Name:      "default",
+						Namespace: "istio-system",
+					},
+					Spec: maistrav1.ServiceMeshMemberRollSpec{
+						Members: []string{
+							"inside",
+						},
+					},
+					Status: maistrav1.ServiceMeshMemberRollStatus{
+						ConfiguredMembers: []string{
+							"inside",
+						},
+					},
+				},
+			},
+			valid: true,
+		},
 	}
 
 	for _, tc := range cases {