diff --git a/pkg/controller/common/util.go b/pkg/controller/common/util.go index cc93c91f6f..08e1c5c192 100644 --- a/pkg/controller/common/util.go +++ b/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 +} diff --git a/pkg/controller/servicemesh/controlplane/reconciler.go b/pkg/controller/servicemesh/controlplane/reconciler.go index a117a1b639..68dbdfddb3 100644 --- a/pkg/controller/servicemesh/controlplane/reconciler.go +++ b/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{} diff --git a/pkg/controller/servicemesh/controlplane/reconciler_test.go b/pkg/controller/servicemesh/controlplane/reconciler_test.go index b91c04bab1..8ed58fd2f1 100644 --- a/pkg/controller/servicemesh/controlplane/reconciler_test.go +++ b/pkg/controller/servicemesh/controlplane/reconciler_test.go @@ -2,6 +2,7 @@ package controlplane import ( "reflect" + "strings" "testing" "time" @@ -9,6 +10,7 @@ import ( 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{}, diff --git a/pkg/controller/servicemesh/webhooks/validation/controlplane_test.go b/pkg/controller/servicemesh/webhooks/validation/controlplane_test.go index 6661f5adbb..54882f3735 100644 --- a/pkg/controller/servicemesh/webhooks/validation/controlplane_test.go +++ b/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 { diff --git a/pkg/controller/servicemesh/webhooks/validation/controlplane_util.go b/pkg/controller/servicemesh/webhooks/validation/controlplane_util.go index b1fb1d487b..48868d8a9e 100644 --- a/pkg/controller/servicemesh/webhooks/validation/controlplane_util.go +++ b/pkg/controller/servicemesh/webhooks/validation/controlplane_util.go @@ -6,6 +6,7 @@ import ( "strings" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/util/sets" ) func errForEnabledValue(obj map[string]interface{}, path string, disallowed bool) error { @@ -24,3 +25,34 @@ func errForEnabledValue(obj map[string]interface{}, path string, disallowed bool } return nil } + +func errForStringValue(obj map[string]interface{}, path string, allowedValues sets.String) error { + val, ok, _ := unstructured.NestedFieldNoCopy(obj, strings.Split(path, ".")...) + if ok { + switch typedVal := val.(type) { + case string: + if !allowedValues.Has(typedVal) { + return fmt.Errorf("%s=%s is not allowed", path, typedVal) + } + default: + return fmt.Errorf("expected string value at %s", path) + } + } + return nil +} + +func getMapKeys(obj map[string]interface{}, path string) []string { + val, ok, err := unstructured.NestedFieldNoCopy(obj, path) + if err != nil || !ok { + return []string{} + } + mapVal, ok := val.(map[string]interface{}) + if !ok { + return []string{} + } + keys := make([]string, len(mapVal)) + for k := range mapVal { + keys = append(keys, k) + } + return keys +} diff --git a/pkg/controller/servicemesh/webhooks/validation/controlplane_v1_1.go b/pkg/controller/servicemesh/webhooks/validation/controlplane_v1_1.go index d2d06da501..6c650538f8 100644 --- a/pkg/controller/servicemesh/webhooks/validation/controlplane_v1_1.go +++ b/pkg/controller/servicemesh/webhooks/validation/controlplane_v1_1.go @@ -5,13 +5,17 @@ import ( "fmt" "strings" + "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" utilerrors "k8s.io/apimachinery/pkg/util/errors" + logf "sigs.k8s.io/controller-runtime/pkg/runtime/log" maistrav1 "github.com/maistra/istio-operator/pkg/apis/maistra/v1" + "github.com/maistra/istio-operator/pkg/controller/common" ) func (v *ControlPlaneValidator) validateV1_1(ctx context.Context, smcp *maistrav1.ServiceMeshControlPlane) error { + logger := logf.Log.WithName("smcp-validator") var allErrors []error if zipkinAddress, ok, _ := unstructured.NestedString(smcp.Spec.Istio, strings.Split("global.tracer.zipkin.address", ".")...); ok && len(zipkinAddress) > 0 { @@ -48,5 +52,21 @@ func (v *ControlPlaneValidator) validateV1_1(ctx context.Context, smcp *maistrav } } + smmr, err := v.getSMMR(smcp) + if err != nil { + if !errors.IsNotFound(err) { + // log error, but don't fail validation: we'll just assume that the control plane namespace is the only namespace for now + logger.Error(err, "failed to retrieve SMMR for SMCP") + smmr = nil + } + } + + meshNamespaces := common.GetMeshNamespaces(smcp, smmr) + for _, gateway := range getMapKeys(smcp.Spec.Istio, "gateways") { + if err := errForStringValue(smcp.Spec.Istio, "gateways."+gateway+".namespace", meshNamespaces); err != nil { + allErrors = append(allErrors, fmt.Errorf("%v: namespace must be part of the mesh", err)) + } + } + return utilerrors.NewAggregate(allErrors) }