Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the dependency between create namespace command and generators #96556

Merged
merged 12 commits into from Feb 5, 2021
139 changes: 116 additions & 23 deletions staging/src/k8s.io/kubectl/pkg/cmd/create/create_namespace.go
Expand Up @@ -17,12 +17,20 @@ limitations under the License.
package create

import (
"context"
"fmt"
"github.com/spf13/cobra"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"k8s.io/apimachinery/pkg/runtime"
"k8s.io/cli-runtime/pkg/resource"
coreclient "k8s.io/client-go/kubernetes/typed/core/v1"
"k8s.io/kubectl/pkg/scheme"
"k8s.io/kubectl/pkg/util"

"k8s.io/cli-runtime/pkg/genericclioptions"
cmdutil "k8s.io/kubectl/pkg/cmd/util"
"k8s.io/kubectl/pkg/generate"
generateversioned "k8s.io/kubectl/pkg/generate/versioned"
"k8s.io/kubectl/pkg/util/i18n"
"k8s.io/kubectl/pkg/util/templates"
)
Expand All @@ -36,16 +44,37 @@ var (
kubectl create namespace my-namespace`))
)

// NamespaceOpts is the options for 'create namespace' sub command
type NamespaceOpts struct {
CreateSubcommandOptions *CreateSubcommandOptions
// NamespaceOptions is the options for 'create namespace' sub command
type NamespaceOptions struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CreateNamespaceOptions

// PrintFlags holds options necessary for obtaining a printer
PrintFlags *genericclioptions.PrintFlags
// Name of resource being created
Name string

DryRunStrategy cmdutil.DryRunStrategy
DryRunVerifier *resource.DryRunVerifier
CreateAnnotation bool
FieldManager string

Client *coreclient.CoreV1Client

PrintObj func(obj runtime.Object) error

genericclioptions.IOStreams
}

// NewNamespaceOptions creates a new *NamespaceOptions with sane defaults
func NewNamespaceOptions(ioStreams genericclioptions.IOStreams) *NamespaceOptions {
return &NamespaceOptions{
PrintFlags: genericclioptions.NewPrintFlags("created").WithTypeSetter(scheme.Scheme),
IOStreams: ioStreams,
}
}

// NewCmdCreateNamespace is a macro command to create a new namespace
func NewCmdCreateNamespace(f cmdutil.Factory, ioStreams genericclioptions.IOStreams) *cobra.Command {
options := &NamespaceOpts{
CreateSubcommandOptions: NewCreateSubcommandOptions(ioStreams),
}

o := NewNamespaceOptions(ioStreams)

cmd := &cobra.Command{
Use: "namespace NAME [--dry-run=server|client|none]",
Expand All @@ -55,40 +84,104 @@ func NewCmdCreateNamespace(f cmdutil.Factory, ioStreams genericclioptions.IOStre
Long: namespaceLong,
Example: namespaceExample,
Run: func(cmd *cobra.Command, args []string) {
cmdutil.CheckErr(options.Complete(f, cmd, args))
cmdutil.CheckErr(options.Run())
cmdutil.CheckErr(o.Complete(f, cmd, args))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the code walk through video(Check out the video starting from 43.30) usually it has Complete, Validate, and Run

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know if we need the Validate() as this is a really simple function and the validation of the name is already done by NameFromCommandArgs, but for now let's keep this and maybe cleanup later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we need a more complicated validation here.As described in the url,the name of a namespace must be a valid DNS label which is described here.But it conflicts with previous code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hum, but we can improve the code. I guess anyway who returns an error if the name is or is not DNS formated is the APIServer.

Let me test your PR here and see how things happen :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up with the test:

_output/bin/kubectl create ns lalala###123
The Namespace "lalala###123" is invalid: metadata.name: Invalid value: "lalala###123": a lowercase RFC 1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character (e.g. 'my-name',  or '123-abc', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?')

Because all the objects are validated against the validation.IsDNS1123Label() function (from api-machinery) I guess this is the normal behavior and we don't need to take care of this (for now, but maybe an improvement in the future validating this before sending to the APIServer and making also dry-run complain, wdyt @soltysh @pwittrock ?)

So for now what you can do is keep the Validate() as is (porting the code from the generator), and removing the call of Validate() from createNamespace() :)

cmdutil.CheckErr(o.Validate())
cmdutil.CheckErr(o.Run())
},
}

options.CreateSubcommandOptions.PrintFlags.AddFlags(cmd)
o.PrintFlags.AddFlags(cmd)

cmdutil.AddApplyAnnotationFlags(cmd)
cmdutil.AddValidateFlags(cmd)
cmdutil.AddGeneratorFlags(cmd, generateversioned.NamespaceV1GeneratorName)
cmdutil.AddFieldManagerFlagVar(cmd, &options.CreateSubcommandOptions.FieldManager, "kubectl-create")
cmdutil.AddDryRunFlag(cmd)
cmdutil.AddFieldManagerFlagVar(cmd, &o.FieldManager, "kubectl-create")

return cmd
}

// Complete completes all the required options
func (o *NamespaceOpts) Complete(f cmdutil.Factory, cmd *cobra.Command, args []string) error {
func (o *NamespaceOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []string) error {
name, err := NameFromCommandArgs(cmd, args)
if err != nil {
return err
}

var generator generate.StructuredGenerator
switch generatorName := cmdutil.GetFlagString(cmd, "generator"); generatorName {
case generateversioned.NamespaceV1GeneratorName:
generator = &generateversioned.NamespaceGeneratorV1{Name: name}
default:
return errUnsupportedGenerator(cmd, generatorName)
restConfig, err := f.ToRESTConfig()
if err != nil {
return err
}
o.Client, err = coreclient.NewForConfig(restConfig)
if err != nil {
return err
}

return o.CreateSubcommandOptions.Complete(f, cmd, args, generator)
o.Name = name
o.DryRunStrategy, err = cmdutil.GetDryRunStrategy(cmd)
if err != nil {
return err
}
dynamicClient, err := f.DynamicClient()
if err != nil {
return err
}
discoveryClient, err := f.ToDiscoveryClient()
if err != nil {
return err
}
o.DryRunVerifier = resource.NewDryRunVerifier(dynamicClient, discoveryClient)
o.CreateAnnotation = cmdutil.GetFlagBool(cmd, cmdutil.ApplyAnnotationsFlag)
cmdutil.PrintFlagsWithDryRunStrategy(o.PrintFlags, o.DryRunStrategy)
printer, err := o.PrintFlags.ToPrinter()
if err != nil {
return err
}
o.PrintObj = func(obj runtime.Object) error {
return printer.PrintObj(obj, o.Out)
}
return nil
}

// Run calls the CreateSubcommandOptions.Run in NamespaceOpts instance
func (o *NamespaceOpts) Run() error {
return o.CreateSubcommandOptions.Run()
func (o *NamespaceOptions) Run() error {
namespace := o.createNamespace()
if err := util.CreateOrUpdateAnnotation(o.CreateAnnotation, namespace, scheme.DefaultJSONEncoder()); err != nil {
return err
}

if o.DryRunStrategy != cmdutil.DryRunClient {
createOptions := metav1.CreateOptions{}
if o.FieldManager != "" {
createOptions.FieldManager = o.FieldManager
}
if o.DryRunStrategy == cmdutil.DryRunServer {
if err := o.DryRunVerifier.HasSupport(namespace.GroupVersionKind()); err != nil {
return err
}
createOptions.DryRun = []string{metav1.DryRunAll}
}
var err error
namespace, err = o.Client.Namespaces().Create(context.TODO(), namespace, createOptions)
if err != nil {
return err
}
}
return o.PrintObj(namespace)
}

// createNamespace outputs a namespace object using the configured fields
func (o *NamespaceOptions) createNamespace() *corev1.Namespace {
namespace := &corev1.Namespace{
TypeMeta: metav1.TypeMeta{APIVersion: corev1.SchemeGroupVersion.String(), Kind: "Namespace"},
ObjectMeta: metav1.ObjectMeta{Name: o.Name},
}
return namespace
}

// Validate validates required fields are set to support structured generation
func (o *NamespaceOptions) Validate() error {
if len(o.Name) == 0 {
return fmt.Errorf("name must be specified")
}
return nil
}
62 changes: 28 additions & 34 deletions staging/src/k8s.io/kubectl/pkg/cmd/create/create_namespace_test.go
Expand Up @@ -17,45 +17,39 @@ limitations under the License.
package create

import (
"net/http"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"testing"

"k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/cli-runtime/pkg/genericclioptions"
"k8s.io/client-go/rest/fake"
cmdtesting "k8s.io/kubectl/pkg/cmd/testing"
"k8s.io/kubectl/pkg/scheme"
apiequality "k8s.io/apimachinery/pkg/api/equality"
)

func TestCreateNamespace(t *testing.T) {
namespaceObject := &v1.Namespace{}
namespaceObject.Name = "my-namespace"
tf := cmdtesting.NewTestFactory()
defer tf.Cleanup()

codec := scheme.Codecs.LegacyCodec(scheme.Scheme.PrioritizedVersionsAllGroups()...)
ns := scheme.Codecs.WithoutConversion()

tf.Client = &fake.RESTClient{
GroupVersion: schema.GroupVersion{Version: "v1"},
NegotiatedSerializer: ns,
Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) {
switch p, m := req.URL.Path, req.Method; {
case p == "/namespaces" && m == "POST":
return &http.Response{StatusCode: http.StatusCreated, Header: cmdtesting.DefaultHeader(), Body: cmdtesting.ObjBody(codec, namespaceObject)}, nil
default:
t.Fatalf("unexpected request: %#v\n%#v", req.URL, req)
return nil, nil
}
}),
tests := map[string]struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More tests can be found here, and of course you can add more test cases if needed.
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kubectl/pkg/generate/versioned/namespace_test.go

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, let's at least migrate the old test suite please :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But not all the old test suite can be migrated into new one.Previous code uses a generator to generate a namespace and takes a map as paras which has a less stricted type check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeap you are right :)

I don't think there's much more to test here, so I'll close this comment :)

options *NamespaceOptions
expected *corev1.Namespace
}{
"success_create": {
options: &NamespaceOptions{
Name: "my-namespace",
},
expected: &corev1.Namespace{
TypeMeta: metav1.TypeMeta{
Kind: "Namespace",
APIVersion: "v1",
},
ObjectMeta: metav1.ObjectMeta{
Name: "my-namespace",
},
},
},
}
ioStreams, _, buf, _ := genericclioptions.NewTestIOStreams()
cmd := NewCmdCreateNamespace(tf, ioStreams)
cmd.Flags().Set("output", "name")
cmd.Run(cmd, []string{namespaceObject.Name})
expectedOutput := "namespace/" + namespaceObject.Name + "\n"
if buf.String() != expectedOutput {
t.Errorf("expected output: %s, but got: %s", expectedOutput, buf.String())
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
namespace := tc.options.createNamespace()
if !apiequality.Semantic.DeepEqual(namespace, tc.expected) {
t.Errorf("expected:\n%#v\ngot:\n%#v", tc.expected, namespace)
}
})
}
}