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.

Sign up for GitHub

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

feat: Add --filename flag to service create command #913

Merged
merged 9 commits into from
Jul 11, 2020
Merged

feat: Add --filename flag to service create command #913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0218ea0
feat: Add --file flag to service create cmd
dsimansk Jul 2, 2020
228610a
fix: Fix linter warnings
dsimansk Jul 2, 2020
bd7ea6b
fix: Remove unecessary changes
dsimansk Jul 2, 2020
9a3c1b9
fix: Reflect review feedback
dsimansk Jul 2, 2020
07c1e81
fix: Change flag --file to -f,--filename
dsimansk Jul 2, 2020
d10ecd1
fix: Reflect review feedback
dsimansk Jul 3, 2020
e99a0e2
fix: Move test cleanup to correct place
dsimansk Jul 3, 2020
d1c8059
fix: Refactor name param restrictions
dsimansk Jul 3, 2020
5ace3b2
fix: Fix flag name string in func
dsimansk Jul 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/cmd/kn_service_create.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ kn service create NAME --image IMAGE
--concurrency-utilization int Percentage of concurrent requests utilization before scaling up. (default 70)
-e, --env stringArray Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables. To unset, specify the environment variable name followed by a "-" (e.g., NAME-).
--env-from stringArray Add environment variables from a ConfigMap (prefix cm: or config-map:) or a Secret (prefix secret:). Example: --env-from cm:myconfigmap or --env-from secret:mysecret. You can use this flag multiple times. To unset a ConfigMap/Secret reference, append "-" to the name, e.g. --env-from cm:myconfigmap-.
-f, --filename string Create a service from file.
--force Create service forcefully, replaces existing service if any.
-h, --help help for create
--image string Image to run.
Expand Down
5 changes: 4 additions & 1 deletion pkg/kn/commands/service/configuration_edit_flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ type ConfigurationEditFlags struct {
GenerateRevisionName bool
ForceCreate bool

Filename string

// Bookkeeping
flags []string
}
Expand Down Expand Up @@ -272,7 +274,8 @@ func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) {
p.addSharedFlags(command)
command.Flags().BoolVar(&p.ForceCreate, "force", false,
"Create service forcefully, replaces existing service if any.")
command.MarkFlagRequired("image")
command.Flags().StringVarP(&p.Filename, "filename", "f", "", "Create a service from file.")
command.MarkFlagFilename("filename")
}

// Apply mutates the given service according to the flags in the command.
Expand Down
63 changes: 56 additions & 7 deletions pkg/kn/commands/service/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"errors"
"fmt"
"io"
"os"

"knative.dev/client/pkg/kn/commands"
servinglib "knative.dev/client/pkg/serving"
Expand All @@ -28,6 +29,8 @@ import (
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/yaml"

servingv1 "knative.dev/serving/pkg/apis/serving/v1"

clientservingv1 "knative.dev/client/pkg/serving/v1"
Expand Down Expand Up @@ -75,11 +78,14 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
Short: "Create a service",
Example: create_example,
RunE: func(cmd *cobra.Command, args []string) (err error) {
if len(args) != 1 {
if len(args) != 1 && editFlags.Filename == "" {
return errors.New("'service create' requires the service name given as single argument")
}
name := args[0]
if editFlags.Image == "" {
name := ""
if len(args) == 1 {
name = args[0]
}
if editFlags.Image == "" && editFlags.Filename == "" {
return errors.New("'service create' requires the image name to run provided with the --image option")
}

Expand All @@ -88,7 +94,12 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
return err
}

service, err := constructService(cmd, editFlags, name, namespace)
var service *servingv1.Service
if editFlags.Filename == "" {
service, err = constructService(cmd, editFlags, name, namespace)
} else {
service, err = constructServiceFromFile(cmd, editFlags, name, namespace)
}
if err != nil {
return err
}
Expand All @@ -97,8 +108,7 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
if err != nil {
return err
}

serviceExists, err := serviceExists(client, name)
serviceExists, err := serviceExists(client, service.Name)
if err != nil {
return err
}
Expand All @@ -108,7 +118,7 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
if !editFlags.ForceCreate {
return fmt.Errorf(
"cannot create service '%s' in namespace '%s' "+
"because the service already exists and no --force option was given", name, namespace)
"because the service already exists and no --force option was given", service.Name, namespace)
}
err = replaceService(client, service, waitFlags, out)
} else {
Expand Down Expand Up @@ -260,3 +270,42 @@ func constructService(cmd *cobra.Command, editFlags ConfigurationEditFlags, name
}
return &service, nil
}

// constructServiceFromFile creates struct from provided file
func constructServiceFromFile(cmd *cobra.Command, editFlags ConfigurationEditFlags, name, namespace string) (*servingv1.Service, error) {
var service servingv1.Service
file, err := os.Open(editFlags.Filename)
if err != nil {
return nil, err
}
decoder := yaml.NewYAMLOrJSONDecoder(file, 512)

err = decoder.Decode(&service)
if err != nil {
return nil, err
}
dsimansk marked this conversation as resolved.
Show resolved Hide resolved
if name == "" && service.Name != "" {
// keep provided service.Name if name param is empty
} else if name != "" && service.Name == "" {
service.Name = name
} else if name != "" && service.Name != "" {
// throw error if names differ, otherwise use already set value
if name != service.Name {
return nil, fmt.Errorf("provided service name '%s' doesn't match name from file '%s'", name, service.Name)
}
} else {
return nil, fmt.Errorf("no service name provided in command parameter or file")
}

// Set namespace in case it's specified as --namespace
service.ObjectMeta.Namespace = namespace
Copy link
Collaborator

Choose a reason for hiding this comment

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

should the namespace also follow check as done above for service name ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think for the namespace we should just allow an override from the CLI. A typical use case is that you create your service maybe for a different environment in the file but want to apply it here in a different context (that's different from the "name" which is an identifier that does not change across environments).


// We need to generate revision to have --force replace working
revName, err := servinglib.GenerateRevisionName(editFlags.RevisionName, &service)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am thinking if the client-side revision name generation could be turned off if using files mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

➜  client git:(create-from-file) ✗ cat /tmp/hello.yaml
apiVersion: serving.knative.dev/v1
kind: Service
metadata:
  name: helloworld-go
spec:
  template:
    spec:
      containers:
        - image: gcr.io/knative-samples/helloworld-go
          env:
            - name: TARGET
              value: "Go Sample v1"
➜  client git:(create-from-file) ✗ kn service create helloworld-go -f /tmp/hello.yaml 
Creating service 'helloworld-go' in namespace 'default':
  0.028s The Route is still working to reflect the latest desired specification.
  0.061s Configuration "helloworld-go" is waiting for a Revision to become ready.
  0.106s ...
  3.751s ...
  3.795s Ingress has not yet been reconciled.
  3.868s Waiting for load balancer to be ready
  4.190s Ready to serve.
Service 'helloworld-go' created to latest revision 'helloworld-go-5j5mp' is available at URL:
http://helloworld-go.default.example.com
➜  client git:(create-from-file) ✗ kn service create helloworld-go -f /tmp/hello.yaml --force
Replacing service 'helloworld-go' in namespace 'default':

When I was testing the flag locally I've experienced the above behaviour. When trying to -recreate the service with --force the update hangs and timeouts eventually. However it's not happening for kn service update flow or imperative kn service create due to the fact that we generate revision name by default. May it a serving bug?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes could be, but we should be sure what it actually happen when using --force. Could you do a debug session and check where exactly it hangs ?

For opening an issue for serving we would need to recreate the steps via yaml files, I think.

Copy link
Contributor Author

@dsimansk dsimansk Jul 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Well, the issue is limited to a corner case when the same source is used twice, in other words to replace the same already created same Service. In that case such an update op never replaces the original Service content, causing no set of event to be produced in the cluster for wait to finish without timing out.
There's only one synthetic event ADDED received, but nothing else as the server-side doesn't update anything at all.
The same behaviour can be reproduced with kubectl replace, meaning that original service isn't actually replaced, is still the same with original timestamp etc., but there's no wait for ready that could be blocking the execution.

That kind of issue isn't present in imperative create flow due to the fact that generated revision name is enough of a change to trigger the update/replace mechanism, therefore desired set of events is received in wait function.

I'd suggest the following:

  • Implement @rhuss's suggestion from this comment
  • Mark filename flag with markFlagMakesRevision to enforce revision generation
    • Adding modification from other flags would always trigger new revision name generation, therefore we'll benefit from it for filename as well (although it might be seen as hack as well)
  • => Profit.

Copy link
Contributor

Choose a reason for hiding this comment

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

For a quick win, I would go for the second option. We then can implement that improvement to pick up other options later. wdyt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'm on board with quick win. :) Anyway it'll better to have improvement PR with proper amount of tests and bake time for options combination. I'll create a new issue for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow-up enhancement issue #923.

if err != nil {
return nil, err
}
Comment on lines +305 to +307
Copy link
Contributor

Choose a reason for hiding this comment

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

No tests for problem generating revision as I can comment these and pass tests. This is less important coverage than the comment above.

service.Spec.Template.Name = revName

Copy link
Contributor

Choose a reason for hiding this comment

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

Where does the override from all other options from the CLI happen ? I mean when someone uses

kn service create -f mysvc.yml --service-account mysa

then mysa should be set as service account in the generated service. For array/map like options that's a bit more tricky as we have to decided whether they override those or just work on the ones from the file like with an update (i would opt for the latter). I.e. I would think adding options in addition to the file would act like an update as if the service in the file has been already present on the cluster.

wdyt ?

As this looks a bit more involved I would be happy to add this in a second PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I've left it out, because of potential clashes. However from my limited tests so far it looks well good. E.g. env vars are merged, service account added etc. I'll add a few tests to cover those use cases and update the PR. Plus there's another benefit outlined here.

return &service, nil
}
196 changes: 196 additions & 0 deletions pkg/kn/commands/service/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ package service
import (
"errors"
"fmt"
"io/ioutil"
"os"
"path/filepath"
"reflect"
"strings"
"testing"
Expand Down Expand Up @@ -707,3 +710,196 @@ func TestServiceCreateWithClusterLocal(t *testing.T) {
t.Fatalf("Incorrect VisibilityClusterLocal value '%s'", labelValue)
}
}

var serviceYAML = `
apiVersion: serving.knative.dev/v1
kind: Service
metadata:
name: foo
spec:
template:
spec:
containers:
- image: gcr.io/foo/bar:baz
env:
- name: TARGET
value: "Go Sample v1"
`

var serviceJSON = `
{
"apiVersion": "serving.knative.dev/v1",
"kind": "Service",
"metadata": {
"name": "foo"
},
"spec": {
"template": {
"spec": {
"containers": [
{
"image": "gcr.io/foo/bar:baz",
"env": [
{
"name": "TARGET",
"value": "Go Sample v1"
}
]
}
]
}
}
}
}`

func TestServiceCreateFromYAML(t *testing.T) {
tempDir, err := ioutil.TempDir("", "kn-file")
defer os.RemoveAll(tempDir)
assert.NilError(t, err)

tempFile := filepath.Join(tempDir, "service.yaml")
err = ioutil.WriteFile(tempFile, []byte(serviceYAML), os.FileMode(0666))
assert.NilError(t, err)

action, created, _, err := fakeServiceCreate([]string{
"service", "create", "foo", "--filename", tempFile}, false)
assert.NilError(t, err)
assert.Assert(t, action.Matches("create", "services"))

assert.Equal(t, created.Name, "foo")
assert.Equal(t, created.Spec.Template.Spec.GetContainer().Image, "gcr.io/foo/bar:baz")
}

func TestServiceCreateFromJSON(t *testing.T) {
tempDir, err := ioutil.TempDir("", "kn-file")
defer os.RemoveAll(tempDir)
assert.NilError(t, err)

tempFile := filepath.Join(tempDir, "service.json")
err = ioutil.WriteFile(tempFile, []byte(serviceJSON), os.FileMode(0666))
assert.NilError(t, err)

action, created, _, err := fakeServiceCreate([]string{
"service", "create", "foo", "--filename", tempFile}, false)
assert.NilError(t, err)
assert.Assert(t, action.Matches("create", "services"))

assert.Equal(t, created.Name, "foo")
assert.Equal(t, created.Spec.Template.Spec.GetContainer().Image, "gcr.io/foo/bar:baz")
}

func TestServiceCreateFromFileWithName(t *testing.T) {
tempDir, err := ioutil.TempDir("", "kn-file")
defer os.RemoveAll(tempDir)
assert.NilError(t, err)

tempFile := filepath.Join(tempDir, "service.yaml")
err = ioutil.WriteFile(tempFile, []byte(serviceYAML), os.FileMode(0666))
assert.NilError(t, err)

t.Log("no NAME param provided")
action, created, _, err := fakeServiceCreate([]string{
"service", "create", "--filename", tempFile}, false)
assert.NilError(t, err)
assert.Assert(t, action.Matches("create", "services"))

assert.Equal(t, created.Name, "foo")
assert.Equal(t, created.Spec.Template.Spec.GetContainer().Image, "gcr.io/foo/bar:baz")

t.Log("no service.Name provided in file")
err = ioutil.WriteFile(tempFile, []byte(strings.ReplaceAll(serviceYAML, "name: foo", "")), os.FileMode(0666))
assert.NilError(t, err)
action, created, _, err = fakeServiceCreate([]string{
"service", "create", "cli-foo", "--filename", tempFile}, false)
assert.NilError(t, err)
assert.Assert(t, action.Matches("create", "services"))

assert.Equal(t, created.Name, "cli-foo")
assert.Equal(t, created.Spec.Template.Spec.GetContainer().Image, "gcr.io/foo/bar:baz")
}

func TestServiceCreateFileNameMismatch(t *testing.T) {
tempDir, err := ioutil.TempDir("", "kn-file")
assert.NilError(t, err)

tempFile := filepath.Join(tempDir, "service.json")
err = ioutil.WriteFile(tempFile, []byte(serviceJSON), os.FileMode(0666))
assert.NilError(t, err)

t.Log("NAME param nad service.Name differ")
_, _, _, err = fakeServiceCreate([]string{
"service", "create", "anotherFoo", "--filename", tempFile}, false)
assert.Assert(t, err != nil)
assert.Assert(t, util.ContainsAllIgnoreCase(err.Error(), "provided", "'anotherFoo'", "name", "match", "from", "file", "'foo'"))

t.Log("no NAME param & no service.Name provided in file")
err = ioutil.WriteFile(tempFile, []byte(strings.ReplaceAll(serviceYAML, "name: foo", "")), os.FileMode(0666))
assert.NilError(t, err)
_, _, _, err = fakeServiceCreate([]string{
"service", "create", "--filename", tempFile}, false)
assert.Assert(t, err != nil)
assert.Assert(t, util.ContainsAllIgnoreCase(err.Error(), "no", "service", "name", "provided", "parameter", "file"))
}

func TestServiceCreateFileError(t *testing.T) {
_, _, _, err := fakeServiceCreate([]string{
"service", "create", "foo", "--filename", "filepath"}, false)

assert.Assert(t, util.ContainsAll(err.Error(), "no", "such", "file", "directory", "filepath"))
}

func TestServiceCreateInvalidDataJSON(t *testing.T) {
tempDir, err := ioutil.TempDir("", "kn-file")
defer os.RemoveAll(tempDir)
assert.NilError(t, err)
tempFile := filepath.Join(tempDir, "invalid.json")

// Double curly bracket at the beginning of file
invalidData := strings.Replace(serviceJSON, "{\n", "{{\n", 1)
err = ioutil.WriteFile(tempFile, []byte(invalidData), os.FileMode(0666))
assert.NilError(t, err)
_, _, _, err = fakeServiceCreate([]string{"service", "create", "foo", "--filename", tempFile}, false)
assert.Assert(t, util.ContainsAll(err.Error(), "invalid", "character", "'{'", "beginning"))

// Remove closing quote on key
invalidData = strings.Replace(serviceJSON, "metadata\"", "metadata", 1)
err = ioutil.WriteFile(tempFile, []byte(invalidData), os.FileMode(0666))
assert.NilError(t, err)
_, _, _, err = fakeServiceCreate([]string{"service", "create", "foo", "--filename", tempFile}, false)
assert.Assert(t, util.ContainsAll(err.Error(), "invalid", "character", "'\\n'", "string", "literal"))

// Remove opening square bracket
invalidData = strings.Replace(serviceJSON, " [", "", 1)
err = ioutil.WriteFile(tempFile, []byte(invalidData), os.FileMode(0666))
assert.NilError(t, err)
_, _, _, err = fakeServiceCreate([]string{"service", "create", "foo", "--filename", tempFile}, false)
assert.Assert(t, util.ContainsAll(err.Error(), "invalid", "character", "']'", "after", "key:value"))
}

func TestServiceCreateInvalidDataYAML(t *testing.T) {
tempDir, err := ioutil.TempDir("", "kn-file")
defer os.RemoveAll(tempDir)
assert.NilError(t, err)
tempFile := filepath.Join(tempDir, "invalid.yaml")

// Remove dash
invalidData := strings.Replace(serviceYAML, "- image", "image", 1)
err = ioutil.WriteFile(tempFile, []byte(invalidData), os.FileMode(0666))
assert.NilError(t, err)
_, _, _, err = fakeServiceCreate([]string{"service", "create", "foo", "--filename", tempFile}, false)
assert.Assert(t, util.ContainsAll(err.Error(), "mapping", "values", "not", "allowed"))

// Remove name key
invalidData = strings.Replace(serviceYAML, "name:", "", 1)
err = ioutil.WriteFile(tempFile, []byte(invalidData), os.FileMode(0666))
assert.NilError(t, err)
_, _, _, err = fakeServiceCreate([]string{"service", "create", "foo", "--filename", tempFile}, false)
assert.Assert(t, util.ContainsAll(err.Error(), "cannot", "unmarshal", "Go", "struct", "Service.metadata"))

// Remove opening square bracket
invalidData = strings.Replace(serviceYAML, "env", "\tenv", 1)
err = ioutil.WriteFile(tempFile, []byte(invalidData), os.FileMode(0666))
assert.NilError(t, err)
_, _, _, err = fakeServiceCreate([]string{"service", "create", "foo", "--filename", tempFile}, false)
assert.Assert(t, util.ContainsAll(err.Error(), "found", "tab", "violates", "indentation"))
}
Loading