Re-enable Mixer validation (#13379)

* cleaning up mixer validation Signed-off-by: Kuat Yessenov <kuat@google.com> * fixes Signed-off-by: Kuat Yessenov <kuat@google.com> * fix mixer tests Signed-off-by: Kuat Yessenov <kuat@google.com> * fix galley test Signed-off-by: Kuat Yessenov <kuat@google.com> * less diff Signed-off-by: Kuat Yessenov <kuat@google.com> * no edge case possible Signed-off-by: Kuat Yessenov <kuat@google.com> * fixing the adapter dependencies Signed-off-by: Kuat Yessenov <kuat@google.com> * enable validation Signed-off-by: Kuat Yessenov <kuat@google.com> * goimports Signed-off-by: Kuat Yessenov <kuat@google.com> * missed an adapter Signed-off-by: Kuat Yessenov <kuat@google.com> * edge case Signed-off-by: Kuat Yessenov <kuat@google.com> * coverage Signed-off-by: Kuat Yessenov <kuat@google.com>
istio · Apr 26, 2019 · 36eaeee · 36eaeee
1 parent 4991e3e
commit 36eaeee
Show file tree

Hide file tree

Showing 40 changed files with 723 additions and 1,096 deletions.
diff --git a/galley/pkg/crd/validation/validation.go b/galley/pkg/crd/validation/validation.go
@@ -25,12 +25,7 @@ import (
 
 	multierror "github.com/hashicorp/go-multierror"
 
-	"istio.io/istio/mixer/adapter"
-	"istio.io/istio/mixer/pkg/config"
-	"istio.io/istio/mixer/pkg/config/store"
-	runtimeConfig "istio.io/istio/mixer/pkg/runtime/config"
-	"istio.io/istio/mixer/pkg/template"
-	generatedTmplRepo "istio.io/istio/mixer/template"
+	mixervalidate "istio.io/istio/mixer/pkg/validate"
 	"istio.io/istio/pilot/pkg/model"
 	"istio.io/istio/pkg/cmd"
 	"istio.io/istio/pkg/kube"
@@ -52,21 +47,6 @@ type httpClient interface {
 	Do(req *http.Request) (*http.Response, error)
 }
 
-// createMixerValidator creates a mixer backend validator.
-// TODO(https://github.com/istio/istio/issues/4887) - refactor mixer
-// config validation to remove galley dependency on mixer internal
-// packages.
-func createMixerValidator() store.BackendValidator {
-	info := generatedTmplRepo.SupportedTmplInfo
-	templates := make(map[string]*template.Info, len(info))
-	for k := range info {
-		t := info[k]
-		templates[k] = &t
-	}
-	adapters := config.AdapterInfoMap(adapter.Inventory(), template.NewRepository(info).SupportsTemplate)
-	return store.NewValidator(nil, runtimeConfig.KindMap(adapters, templates))
-}
-
 func webhookHTTPSHandlerReady(client httpClient, vc *WebhookParameters) error {
 	readinessURL := &url.URL{
 		Scheme: "https",
@@ -95,7 +75,7 @@ func webhookHTTPSHandlerReady(client httpClient, vc *WebhookParameters) error {
 func RunValidation(vc *WebhookParameters, kubeConfig string,
 	livenessProbeController, readinessProbeController probe.Controller) {
 	log.Infof("Galley validation started with\n%s", vc)
-	mixerValidator := createMixerValidator()
+	mixerValidator := mixervalidate.NewDefaultValidator(false)
 	clientset, err := kube.CreateClientset(kubeConfig, "")
 	if err != nil {
 		log.Fatalf("could not create k8s clientset: %v", err)

diff --git a/galley/pkg/crd/validation/webhook.go b/galley/pkg/crd/validation/webhook.go
@@ -521,9 +521,12 @@ func (wh *Webhook) admitMixer(request *admissionv1beta1.AdmissionRequest) *admis
 		return &admissionv1beta1.AdmissionResponse{Allowed: true}
 	}
 
-	if err := wh.validator.Validate(ev); err != nil {
-		reportValidationFailed(request, reasonInvalidConfig)
-		return toAdmissionResponse(err)
+	// webhook skips deletions
+	if ev.Type == store.Update {
+		if err := wh.validator.Validate(ev); err != nil {
+			reportValidationFailed(request, reasonInvalidConfig)
+			return toAdmissionResponse(err)
+		}
 	}
 
 	reportValidationPass(request)

diff --git a/galley/pkg/crd/validation/webhook_test.go b/galley/pkg/crd/validation/webhook_test.go
@@ -60,6 +60,10 @@ func (fv *fakeValidator) Validate(*store.BackendEvent) error {
 	return fv.err
 }
 
+func (fv *fakeValidator) SupportsKind(string) bool {
+	return true
+}
+
 var (
 	dummyConfig = &admissionregistrationv1beta1.ValidatingWebhookConfiguration{
 		ObjectMeta: metav1.ObjectMeta{
@@ -432,7 +436,7 @@ func TestAdmitMixer(t *testing.T) {
 				Operation: admissionv1beta1.Delete,
 			},
 			validator: &fakeValidator{errors.New("fail")},
-			allowed:   false,
+			allowed:   true,
 		},
 		{
 			name: "invalid delete (missing name)",

diff --git a/istioctl/pkg/validate/validate.go b/istioctl/pkg/validate/validate.go
@@ -27,6 +27,9 @@ import (
 	"k8s.io/kubernetes/pkg/kubectl/genericclioptions"
 	"k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource"
 
+	mixercrd "istio.io/istio/mixer/pkg/config/crd"
+	mixerstore "istio.io/istio/mixer/pkg/config/store"
+	mixervalidate "istio.io/istio/mixer/pkg/validate"
 	"istio.io/istio/pilot/pkg/config/kube/crd"
 	"istio.io/istio/pilot/pkg/model"
 )
@@ -44,45 +47,15 @@ var (
 		"spec":       true,
 		"status":     true,
 	}
-)
-
-/*
-
-TODO(https://github.com/istio/istio/issues/4887)
-
-Reusing the existing mixer validation code pulls in all of the mixer
-adapter packages into istioctl. Not only is this not ideal (see
-issue), but it also breaks the istioctl build on windows as some mixer
-adapters use linux specific packges (e.g. syslog).
-
-func createMixerValidator() store.BackendValidator {
-	info := generatedTmplRepo.SupportedTmplInfo
-	templates := make(map[string]*template.Info, len(info))
-	for k := range info {
-		t := info[k]
-		templates[k] = &t
-	}
-	adapters := config.AdapterInfoMap(adapter.Inventory(), template.NewRepository(info).SupportsTemplate)
-	return store.NewValidator(nil, runtimeConfig.KindMap(adapters, templates))
-}
 
-var mixerValidator = createMixerValidator()
+	mixerAPIVersion = "config.istio.io/v1alpha2"
+)
 
-type validateArgs struct {
-	filenames []string
-	// TODO validateObjectStream namespace/object?
+type validator struct {
+	mixerValidator mixerstore.BackendValidator
 }
 
-func (args validateArgs) validate() error {
-	var errs *multierror.Error
-	if len(args.filenames) == 0 {
-		errs = multierror.Append(errs, errors.New("no filenames set (see --filename/-f)"))
-	}
-	return errs.ErrorOrNil()
-}
-*/
-
-func validateResource(un *unstructured.Unstructured) error {
+func (v *validator) validateResource(un *unstructured.Unstructured) error {
 	schema, exists := model.IstioConfigTypes.GetByType(crd.CamelCaseToKebabCase(un.GetKind()))
 	if exists {
 		obj, err := crd.ConvertObjectFromUnstructured(schema, un, "")
@@ -91,20 +64,22 @@ func validateResource(un *unstructured.Unstructured) error {
 		}
 		return schema.Validate(obj.Name, obj.Namespace, obj.Spec)
 	}
-	return fmt.Errorf("%s.%s validation is not supported", un.GetKind(), un.GetAPIVersion())
-	/*
-		TODO(https://github.com/istio/istio/issues/4887)
 
-		ev := &store.BackendEvent{
-			Key: store.Key{
+	if v.mixerValidator != nil && un.GetAPIVersion() == mixerAPIVersion {
+		if !v.mixerValidator.SupportsKind(un.GetKind()) {
+			return fmt.Errorf("%s not implemented", un.GetKind())
+		}
+		return v.mixerValidator.Validate(&mixerstore.BackendEvent{
+			Type: mixerstore.Update,
+			Key: mixerstore.Key{
 				Name:      un.GetName(),
 				Namespace: un.GetNamespace(),
 				Kind:      un.GetKind(),
 			},
-			Value: mixerCrd.ToBackEndResource(un),
-		}
-		return mixerValidator.Validate(ev)
-	*/
+			Value: mixercrd.ToBackEndResource(un),
+		})
+	}
+	return fmt.Errorf("%s.%s validation is not supported", un.GetKind(), un.GetAPIVersion())
 }
 
 var errMissingResource = errors.New(`error: you must specify resources by --filename.
@@ -136,6 +111,10 @@ func validateObjects(restClientGetter resource.RESTClientGetter, options resourc
 	}
 
 	var errs error
+	v := &validator{
+		mixerValidator: mixervalidate.NewDefaultValidator(true),
+	}
+
 	_ = r.Visit(func(info *resource.Info, err error) error {
 		if err != nil {
 			return err
@@ -154,13 +133,12 @@ func validateObjects(restClientGetter resource.RESTClientGetter, options resourc
 			}
 		}
 
-		if err := validateResource(un); err != nil {
+		if err := v.validateResource(un); err != nil {
 			errs = multierror.Append(errs, fmt.Errorf("error validating resource (%v Name=%v Namespace=%v): %v",
 				un.GetObjectKind().GroupVersionKind(), un.GetName(), un.GetNamespace(), err))
 		}
 		return nil
 	})
-
 	if errs != nil {
 		return errs
 	}

diff --git a/istioctl/pkg/validate/validate_test.go b/istioctl/pkg/validate/validate_test.go
@@ -24,6 +24,8 @@ import (
 
 	"github.com/ghodss/yaml"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+
+	mixervalidate "istio.io/istio/mixer/pkg/validate"
 )
 
 const (
@@ -140,18 +142,21 @@ func TestValidateResource(t *testing.T) {
 		{
 			name:  "valid mixer configuration",
 			in:    validMixerRule,
-			valid: false, // TODO(https://github.com/istio/istio/issues/4887)
+			valid: true,
 		},
 		{
 			name:  "invalid mixer configuration",
 			in:    invalidMixerRule,
-			valid: false, // TODO(https://github.com/istio/istio/issues/4887)
+			valid: false,
 		},
 	}
 
 	for i, c := range cases {
 		t.Run(fmt.Sprintf("[%v] %v ", i, c.name), func(tt *testing.T) {
-			err := validateResource(fromYAML(c.in))
+			v := &validator{
+				mixerValidator: mixervalidate.NewDefaultValidator(false),
+			}
+			err := v.validateResource(fromYAML(c.in))
 			if (err == nil) != c.valid {
 				tt.Fatalf("unexpected validation result: got %v want %v: err=%q", err == nil, c.valid, err)
 			}

diff --git a/mixer/adapter/bypass/bypass.go b/mixer/adapter/bypass/bypass.go
@@ -30,6 +30,7 @@ import (
 
 	"istio.io/api/mixer/adapter/model/v1beta1"
 	"istio.io/istio/mixer/adapter/bypass/config"
+	"istio.io/istio/mixer/adapter/metadata"
 	"istio.io/istio/mixer/pkg/adapter"
 	"istio.io/istio/mixer/template/checknothing"
 	"istio.io/istio/mixer/template/metric"
@@ -39,19 +40,9 @@ import (
 
 // GetInfo returns the Info associated with this adapter implementation.
 func GetInfo() adapter.Info {
-	return adapter.Info{
-		Name:        "bypass",
-		Impl:        "istio.io/istio/mixer/adapter/bypass",
-		Description: "Calls gRPC backends via the inline adapter model (useful for testing)",
-		SupportedTemplates: []string{
-			checknothing.TemplateName,
-			reportnothing.TemplateName,
-			metric.TemplateName,
-			quota.TemplateName,
-		},
-		DefaultConfig: &config.Params{},
-		NewBuilder:    func() adapter.HandlerBuilder { return &builder{} },
-	}
+	info := metadata.GetInfo("bypass")
+	info.NewBuilder = func() adapter.HandlerBuilder { return &builder{} }
+	return info
 }
 
 type builder struct {

diff --git a/mixer/adapter/circonus/circonus.go b/mixer/adapter/circonus/circonus.go
@@ -30,6 +30,7 @@ import (
 	"github.com/circonus-labs/circonus-gometrics/checkmgr"
 
 	"istio.io/istio/mixer/adapter/circonus/config"
+	"istio.io/istio/mixer/adapter/metadata"
 	"istio.io/istio/mixer/pkg/adapter"
 	"istio.io/istio/mixer/template/metric"
 )
@@ -193,15 +194,9 @@ func (h *handler) Close() error {
 
 // GetInfo returns the adapter.Info specific to this adapter.
 func GetInfo() adapter.Info {
-	return adapter.Info{
-		Name:        "circonus",
-		Description: "Emit metrics to Circonus.com monitoring endpoint",
-		SupportedTemplates: []string{
-			metric.TemplateName,
-		},
-		NewBuilder:    func() adapter.HandlerBuilder { return &builder{} },
-		DefaultConfig: &config.Params{SubmissionUrl: "", SubmissionInterval: 10 * time.Second},
-	}
+	info := metadata.GetInfo("circonus")
+	info.NewBuilder = func() adapter.HandlerBuilder { return &builder{} }
+	return info
 }
 
 // logToEnvLogger converts CGM log package writes to env.Logger()

diff --git a/mixer/adapter/cloudwatch/cloudwatch.go b/mixer/adapter/cloudwatch/cloudwatch.go
@@ -28,6 +28,7 @@ import (
 
 	istio_policy_v1beta1 "istio.io/api/policy/v1beta1"
 	"istio.io/istio/mixer/adapter/cloudwatch/config"
+	"istio.io/istio/mixer/adapter/metadata"
 	"istio.io/istio/mixer/pkg/adapter"
 	"istio.io/istio/mixer/template/logentry"
 	"istio.io/istio/mixer/template/metric"
@@ -197,14 +198,7 @@ func (h *handler) Close() error {
 
 // GetInfo returns the adapter.Info specific to this adapter.
 func GetInfo() adapter.Info {
-	return adapter.Info{
-		Name:        "cloudwatch",
-		Description: "Sends metrics to cloudwatch and logs to cloudwatchlogs",
-		SupportedTemplates: []string{
-			metric.TemplateName,
-			logentry.TemplateName,
-		},
-		NewBuilder:    func() adapter.HandlerBuilder { return &builder{} },
-		DefaultConfig: &config.Params{},
-	}
+	info := metadata.GetInfo("cloudwatch")
+	info.NewBuilder = func() adapter.HandlerBuilder { return &builder{} }
+	return info
 }
diff --git a/mixer/adapter/denier/denier.go b/mixer/adapter/denier/denier.go
@@ -30,6 +30,7 @@ import (
 	"github.com/gogo/googleapis/google/rpc"
 
 	"istio.io/istio/mixer/adapter/denier/config"
+	"istio.io/istio/mixer/adapter/metadata"
 	"istio.io/istio/mixer/pkg/adapter"
 	"istio.io/istio/mixer/pkg/status"
 	"istio.io/istio/mixer/template/checknothing"
@@ -77,18 +78,9 @@ func (*handler) Close() error { return nil }
 
 // GetInfo returns the Info associated with this adapter implementation.
 func GetInfo() adapter.Info {
-	return adapter.Info{
-		Name:        "denier",
-		Impl:        "istio.io/istio/mixer/adapter/denier",
-		Description: "Rejects any check and quota request with a configurable error",
-		SupportedTemplates: []string{
-			checknothing.TemplateName,
-			listentry.TemplateName,
-			quota.TemplateName,
-		},
-		DefaultConfig: defaultParam(),
-		NewBuilder:    func() adapter.HandlerBuilder { return &builder{} },
-	}
+	info := metadata.GetInfo("denier")
+	info.NewBuilder = func() adapter.HandlerBuilder { return &builder{} }
+	return info
 }
 
 type builder struct {

diff --git a/mixer/adapter/dogstatsd/dogstatsd.go b/mixer/adapter/dogstatsd/dogstatsd.go
@@ -29,6 +29,7 @@ import (
 
 	descriptor "istio.io/api/policy/v1beta1"
 	"istio.io/istio/mixer/adapter/dogstatsd/config"
+	"istio.io/istio/mixer/adapter/metadata"
 	"istio.io/istio/mixer/pkg/adapter"
 	"istio.io/istio/mixer/template/metric"
 )
@@ -201,18 +202,7 @@ func (h *handler) Close() error { return h.client.Close() }
 
 // GetInfo returns the adapter.Info specific to this adapter.
 func GetInfo() adapter.Info {
-	return adapter.Info{
-		Name:        "dogstatsd",
-		Description: "Produces dogstatsd metrics",
-		SupportedTemplates: []string{
-			metric.TemplateName,
-		},
-		NewBuilder: func() adapter.HandlerBuilder { return &builder{} },
-		DefaultConfig: &config.Params{
-			Address:      "localhost:8125",
-			Prefix:       "istio.",
-			BufferLength: 0,
-			GlobalTags:   map[string]string{},
-		},
-	}
+	info := metadata.GetInfo("dogstatsd")
+	info.NewBuilder = func() adapter.HandlerBuilder { return &builder{} }
+	return info
 }