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

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

Merged
merged 9 commits into from
Jul 11, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
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-.
--file string Create service from file.
--force Create service forcefully, replaces existing service if any.
-h, --help help for create
--image string Image to run.
Expand Down
6 changes: 5 additions & 1 deletion pkg/kn/commands/service/configuration_edit_flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ type ConfigurationEditFlags struct {
GenerateRevisionName bool
ForceCreate bool

File string
Copy link
Contributor

Choose a reason for hiding this comment

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

I always debate using Filename instead of File since there is invariably a File object created somewhere later. Of course, File is shorter as a flag name so makes sense too... Argh.

Copy link
Contributor Author

@dsimansk dsimansk Jul 2, 2020

Choose a reason for hiding this comment

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

Following the kubectl example we could settle on -f, --filename as --force is not using that shorthand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the name in 9a3c1b9.

dsimansk marked this conversation as resolved.
Show resolved Hide resolved

// Bookkeeping
flags []string
}
Expand Down Expand Up @@ -272,7 +274,9 @@ 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.MarkFlagRequired("image")
command.Flags().StringVarP(&p.File, "file", "", "", "Create service from file.")
command.MarkFlagFilename("file")
}

// Apply mutates the given service according to the flags in the command.
Expand Down
46 changes: 41 additions & 5 deletions pkg/kn/commands/service/create.go
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 @@ -79,7 +82,7 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
return errors.New("'service create' requires the service name given as single argument")
}
name := args[0]
if editFlags.Image == "" {
if editFlags.Image == "" && editFlags.File == "" {
return errors.New("'service create' requires the image name to run provided with the --image option")
}

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

service, err := constructService(cmd, editFlags, name, namespace)
var service *servingv1.Service
if editFlags.File == "" {
service, err = constructService(cmd, editFlags, name, namespace)
} else {
service, err = constructServiceFromFile(cmd, editFlags, name, namespace)
}
if err != nil {
return err
}
Expand All @@ -97,8 +105,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 +115,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 +267,32 @@ 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.File)
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 service.Name != name {
return nil, fmt.Errorf("Provided service name '%s' doesn't match name from file '%s'.", name, service.Name)
dsimansk marked this conversation as resolved.
Show resolved Hide resolved
}
// 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

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
}
99 changes: 99 additions & 0 deletions pkg/kn/commands/service/create_test.go
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,99 @@ 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")
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", "--file", tempFile}, false)
assert.NilError(t, err)
assert.Assert(t, action.Matches("create", "services"))
fmt.Println("Name: " + created.Name)
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")
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", "--file", 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 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)

_, _, _, err = fakeServiceCreate([]string{
"service", "create", "anotherFoo", "--file", tempFile}, false)

assert.Assert(t, util.ContainsAllIgnoreCase(err.Error(), "provided", "'anotherFoo'", "name", "match", "from", "file", "'foo'"))
}

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

assert.Assert(t, util.ContainsAll(err.Error(), "no", "such", "file", "directory", "filepath"))
}
122 changes: 122 additions & 0 deletions test/e2e/service_file_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
// Copyright 2020 The Knative Authors

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at

// http://www.apache.org/licenses/LICENSE-2.0

// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

// +build e2e
// +build !eventing

package e2e

import (
"fmt"
"io/ioutil"
"path/filepath"
"testing"

"gotest.tools/assert"

"knative.dev/client/lib/test"
"knative.dev/client/pkg/util"
)

const (
ServiceYAML string = `
apiVersion: serving.knative.dev/v1
kind: Service
metadata:
name: foo-yaml
spec:
template:
spec:
containers:
- image: %s
env:
- name: TARGET
value: "Go Sample v1"`

ServiceJSON string = `
{
"apiVersion": "serving.knative.dev/v1",
"kind": "Service",
"metadata": {
"name": "foo-json"
},
"spec": {
"template": {
"spec": {
"containers": [
{
"image": "%s",
"env": [
{
"name": "TARGET",
"value": "Go Sample v1"
}
]
}
]
}
}
}
}`
)

func TestServiceCreateFromFile(t *testing.T) {
t.Parallel()
it, err := test.NewKnTest()
assert.NilError(t, err)
defer func() {
assert.NilError(t, it.Teardown())
}()

r := test.NewKnRunResultCollector(t, it)
defer r.DumpIfFailed()

tempDir, err := ioutil.TempDir("", "kn-file")
dsimansk marked this conversation as resolved.
Show resolved Hide resolved
assert.NilError(t, err)

test.CreateFile("foo.json", fmt.Sprintf(ServiceJSON, test.KnDefaultTestImage), tempDir, test.FileModeReadWrite)
test.CreateFile("foo.yaml", fmt.Sprintf(ServiceYAML, test.KnDefaultTestImage), tempDir, test.FileModeReadWrite)

t.Log("create foo-json service from JSON file")
serviceCreateFromFile(r, "foo-json", filepath.Join(tempDir, "foo.json"))

t.Log("create foo-yaml service from YAML file")
serviceCreateFromFile(r, "foo-yaml", filepath.Join(tempDir, "foo.yaml"))

t.Log("error message for non-existing file")
serviceCreateFromFileNameMismatch(r, "foo", filepath.Join(tempDir, "foo.yaml"))
serviceCreateFromFileNameMismatch(r, "foo", filepath.Join(tempDir, "foo.json"))
}

func serviceCreateFromFile(r *test.KnRunResultCollector, serviceName, filePath string) {
out := r.KnTest().Kn().Run("service", "create", serviceName, "--file", filePath)
r.AssertNoError(out)
assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stdout, "service", serviceName, "creating", "namespace", r.KnTest().Kn().Namespace(), "ready"))

out = r.KnTest().Kn().Run("service", "describe", serviceName, "--verbose")
r.AssertNoError(out)
assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stdout, serviceName))
}

func serviceCreateFromFileError(r *test.KnRunResultCollector, serviceName, filePath string) {
out := r.KnTest().Kn().Run("service", "create", serviceName, "--file", filePath)
r.AssertError(out)
assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stderr, "no", "such", "file", "directory", filePath))
}

func serviceCreateFromFileNameMismatch(r *test.KnRunResultCollector, serviceName, filePath string) {
out := r.KnTest().Kn().Run("service", "create", serviceName, "--file", filePath)
r.AssertError(out)
assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stderr, "provided", "'"+serviceName+"'", "name", "match", "from", "file"))
}