Merge pull request #173 from justinsb/set_namespace_direct_applier

Fix namespace setting with direct applier

Author: k8s-ci-robot

pkg/patterns/declarative/pkg/applier/direct.go

@@ -2,48 +2,71 @@ package applier
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"strings"
 
+	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/cli-runtime/pkg/genericclioptions"
 	"k8s.io/cli-runtime/pkg/printers"
 	"k8s.io/cli-runtime/pkg/resource"
+	"k8s.io/client-go/discovery"
+	"k8s.io/client-go/rest"
 	"k8s.io/kubectl/pkg/cmd/apply"
 	cmdDelete "k8s.io/kubectl/pkg/cmd/delete"
 	cmdutil "k8s.io/kubectl/pkg/cmd/util"
 )
 
 type DirectApplier struct {
-	a apply.ApplyOptions
 }
 
+var _ Applier = &DirectApplier{}
+
 func NewDirectApplier() *DirectApplier {
 	return &DirectApplier{}
 }
 
-func (d *DirectApplier) Apply(ctx context.Context,
-	namespace string,
-	manifest string,
-	validate bool,
-	extraArgs ...string,
-) error {
+func (d *DirectApplier) Apply(ctx context.Context, opt ApplierOptions) error {
 	ioStreams := genericclioptions.IOStreams{
 		In:     os.Stdin,
 		Out:    os.Stdout,
 		ErrOut: os.Stderr,
 	}
-	restClient := genericclioptions.NewConfigFlags(true).WithDeprecatedPasswordFlag()
-	ioReader := strings.NewReader(manifest)
+	ioReader := strings.NewReader(opt.Manifest)
+
+	restClientGetter := &staticRESTClientGetter{
+		RESTMapper: opt.RESTMapper,
+		RESTConfig: opt.RESTConfig,
+	}
+	b := resource.NewBuilder(restClientGetter)
+
+	if opt.Validate {
+		// This potentially causes redundant work, but validation isn't the common path
+		v, err := cmdutil.NewFactory(&genericclioptions.ConfigFlags{}).Validator(true)
+		if err != nil {
+			return err
+		}
+		b.Schema(v)
+	}
 
-	b := resource.NewBuilder(restClient)
 	res := b.Unstructured().Stream(ioReader, "manifestString").Do()
 	infos, err := res.Infos()
 	if err != nil {
 		return err
 	}
 
+	// Populate the namespace on any namespace-scoped objects
+	if opt.Namespace != "" {
+		visitor := resource.SetNamespace(opt.Namespace)
+		for _, info := range infos {
+			if err := info.Visit(visitor); err != nil {
+				return fmt.Errorf("error from SetNamespace: %w", err)
+			}
+		}
+	}
+
 	applyOpts := apply.NewApplyOptions(ioStreams)
-	applyOpts.Namespace = namespace
+	applyOpts.Namespace = opt.Namespace
 	applyOpts.SetObjects(infos)
 	applyOpts.ToPrinter = func(operation string) (printers.ResourcePrinter, error) {
 		applyOpts.PrintFlags.NamePrintFlags.Operation = operation
@@ -56,3 +79,31 @@ func (d *DirectApplier) Apply(ctx context.Context,
 
 	return applyOpts.Run()
 }
+
+// staticRESTClientGetter returns a fixed RESTClient
+type staticRESTClientGetter struct {
+	RESTConfig      *rest.Config
+	DiscoveryClient discovery.CachedDiscoveryInterface
+	RESTMapper      meta.RESTMapper
+}
+
+var _ resource.RESTClientGetter = &staticRESTClientGetter{}
+
+func (s *staticRESTClientGetter) ToRESTConfig() (*rest.Config, error) {
+	if s.RESTConfig == nil {
+		return nil, fmt.Errorf("RESTConfig not set")
+	}
+	return s.RESTConfig, nil
+}
+func (s *staticRESTClientGetter) ToDiscoveryClient() (discovery.CachedDiscoveryInterface, error) {
+	if s.DiscoveryClient == nil {
+		return nil, fmt.Errorf("DiscoveryClient not set")
+	}
+	return s.DiscoveryClient, nil
+}
+func (s *staticRESTClientGetter) ToRESTMapper() (meta.RESTMapper, error) {
+	if s.RESTMapper == nil {
+		return nil, fmt.Errorf("RESTMapper not set")
+	}
+	return s.RESTMapper, nil
+}

pkg/patterns/declarative/pkg/applier/exec.go

@@ -37,6 +37,8 @@ type ExecKubectl struct {
 	cmdSite commandSite
 }
 
+var _ Applier = &ExecKubectl{}
+
 // commandSite allows for tests to mock cmd.Run() events
 type commandSite interface {
 	Run(*exec.Cmd) error
@@ -49,26 +51,25 @@ func (console) Run(c *exec.Cmd) error {
 }
 
 // Apply runs the kubectl apply with the provided manifest argument
-func (c *ExecKubectl) Apply(ctx context.Context, namespace string, manifest string, validate bool,
-	extraArgs ...string) error {
+func (c *ExecKubectl) Apply(ctx context.Context, opt ApplierOptions) error {
 	log := log.Log
 
 	log.Info("applying manifest")
 
 	args := []string{"apply"}
-	if namespace != "" {
-		args = append(args, "-n", namespace)
+	if opt.Namespace != "" {
+		args = append(args, "-n", opt.Namespace)
 	}
 
 	// Not doing --validate avoids downloading the OpenAPI
 	// which can save a lot work & memory
-	args = append(args, "--validate="+strconv.FormatBool(validate))
+	args = append(args, "--validate="+strconv.FormatBool(opt.Validate))
 
-	args = append(args, extraArgs...)
+	args = append(args, opt.ExtraArgs...)
 	args = append(args, "-f", "-")
 
 	cmd := exec.Command("kubectl", args...)
-	cmd.Stdin = strings.NewReader(manifest)
+	cmd.Stdin = strings.NewReader(opt.Manifest)
 
 	var stdout bytes.Buffer
 	var stderr bytes.Buffer
@@ -80,7 +81,7 @@ func (c *ExecKubectl) Apply(ctx context.Context, namespace string, manifest stri
 	err := c.cmdSite.Run(cmd)
 	if err != nil {
 		log.WithValues("stdout", stdout.String()).WithValues("stderr", stderr.String()).Error(err, "error from running kubectl apply")
-		log.Info(fmt.Sprintf("manifest:\n%v", manifest))
+		log.Info(fmt.Sprintf("manifest:\n%v", opt.Manifest))
 		return fmt.Errorf("error from running kubectl apply: %v", err)
 	}
 

pkg/patterns/declarative/pkg/applier/exec_test.go

@@ -85,7 +85,13 @@ func TestKubectlApply(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			cs := collector{Error: test.err}
 			kubectl := &ExecKubectl{cmdSite: &cs}
-			err := kubectl.Apply(context.Background(), test.namespace, test.manifest, test.validate, test.args...)
+			opts := ApplierOptions{
+				Namespace: test.namespace,
+				Manifest:  test.manifest,
+				Validate:  test.validate,
+				ExtraArgs: test.args,
+			}
+			err := kubectl.Apply(context.Background(), opts)
 
 			if test.err != nil && err == nil {
 				t.Error("expected error to occur")

pkg/patterns/declarative/pkg/applier/type.go

@@ -2,8 +2,21 @@ package applier
 
 import (
 	"context"
+
+	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/client-go/rest"
 )
 
 type Applier interface {
-	Apply(ctx context.Context, namespace string, manifest string, validate bool, extraArgs ...string) error
+	Apply(ctx context.Context, options ApplierOptions) error
+}
+
+type ApplierOptions struct {
+	Manifest string
+
+	RESTConfig *rest.Config
+	RESTMapper meta.RESTMapper
+	Namespace  string
+	Validate   bool
+	ExtraArgs  []string
 }

pkg/patterns/declarative/reconciler.go

@@ -256,7 +256,16 @@ func (r *Reconciler) reconcileExists(ctx context.Context, name types.NamespacedN
 		}
 	}
 
-	if err := r.kubectl.Apply(ctx, ns, manifestStr, r.options.validate, extraArgs...); err != nil {
+	applyOpt := applier.ApplierOptions{
+		RESTConfig: r.config,
+		RESTMapper: r.restMapper,
+		Namespace:  ns,
+		Manifest:   manifestStr,
+		Validate:   r.options.validate,
+		ExtraArgs:  extraArgs,
+	}
+
+	if err := r.kubectl.Apply(ctx, applyOpt); err != nil {
 		log.Error(err, "applying manifest")
 		return reconcile.Result{}, fmt.Errorf("error applying manifest: %v", err)
 	}
@@ -564,14 +573,13 @@ func (r *Reconciler) CollectMetrics() bool {
 	return r.options.metrics
 }
 
-func GetObjectFromCluster(obj *manifest.Object, r *Reconciler) (*unstructured.
-	Unstructured, error) {
+func GetObjectFromCluster(obj *manifest.Object, r *Reconciler) (*unstructured.Unstructured, error) {
 	getOptions := metav1.GetOptions{}
 	gvk := obj.GroupVersionKind()
 
 	mapping, err := r.restMapper.RESTMapping(obj.GroupKind(), gvk.Version)
 	if err != nil {
-		return nil, fmt.Errorf("unable to get mapping for resource: %w", err)
+		return nil, fmt.Errorf("unable to get mapping for resource %v: %w", gvk, err)
 	}
 	ns := obj.UnstructuredObject().GetNamespace()
 	unstruct, err := r.dynamicClient.Resource(mapping.Resource).Namespace(ns).Get(context.Background(),