Add --tag flag to service create and allow traffic split <100 when @l…

…atest is specified (#1514)

* Add --tag flag to service create

* added comments

* Added tests

* handled @latest tag in traffic split

* added unit tests

* added e2e tests

* added comments

* simplified code

* add e2e tests for error cases

* Add handling for non latest revisions with mutation bool

docs/cmd/kn_service_create.md

@@ -92,6 +92,7 @@ kn service create NAME --image IMAGE
       --scale-utilization int             Percentage of concurrent requests utilization before scaling up. (default 70)
       --scale-window string               Duration to look back for making auto-scaling decisions. The service is scaled to zero if no request was received in during that time. (eg: 10s)
       --service-account string            Service account name to set. An empty argument ("") clears the service account. The referenced service account must exist in the service's namespace.
+      --tag strings                       Set tag (format: --tag revisionRef=tagName) where revisionRef can be a revision or '@latest' string representing latest ready revision. This flag can be specified multiple times.
       --target string                     Work on local directory instead of a remote cluster (experimental)
       --user int                          The user ID to run the container (e.g., 1001).
       --volume stringArray                Add a volume from a ConfigMap (prefix cm: or config-map:) or a Secret (prefix secret: or sc:). Example: --volume myvolume=cm:myconfigmap or --volume myvolume=secret:mysecret. You can use this flag multiple times. To unset a ConfigMap/Secret reference, append "-" to the name, e.g. --volume myvolume-.

pkg/kn/commands/flags/traffic.go

@@ -25,22 +25,37 @@ type Traffic struct {
 }
 
 func (t *Traffic) Add(cmd *cobra.Command) {
-	cmd.Flags().StringSliceVar(&t.RevisionsPercentages,
-		"traffic",
+	t.AddTrafficFlag(cmd)
+
+	t.AddTagFlag(cmd)
+
+	t.AddUntagFlag(cmd)
+}
+
+// AddUntagFlag adds the flag --untag to the command
+func (t *Traffic) AddUntagFlag(cmd *cobra.Command) {
+	cmd.Flags().StringSliceVar(&t.UntagRevisions,
+		"untag",
 		nil,
-		"Set traffic distribution (format: --traffic revisionRef=percent) where revisionRef can be a revision or a tag or '@latest' string "+
-			"representing latest ready revision. This flag can be given multiple times with percent summing up to 100%.")
+		"Untag revision (format: --untag tagName). This flag can be specified multiple times.")
+}
 
+// AddTagFlag adds the flag --tag to the command
+func (t *Traffic) AddTagFlag(cmd *cobra.Command) {
 	cmd.Flags().StringSliceVar(&t.RevisionsTags,
 		"tag",
 		nil,
 		"Set tag (format: --tag revisionRef=tagName) where revisionRef can be a revision or '@latest' string representing latest ready revision. "+
 			"This flag can be specified multiple times.")
+}
 
-	cmd.Flags().StringSliceVar(&t.UntagRevisions,
-		"untag",
+// AddTrafficFlag adds the flag --traffic to the command
+func (t *Traffic) AddTrafficFlag(cmd *cobra.Command) {
+	cmd.Flags().StringSliceVar(&t.RevisionsPercentages,
+		"traffic",
 		nil,
-		"Untag revision (format: --untag tagName). This flag can be specified multiple times.")
+		"Set traffic distribution (format: --traffic revisionRef=percent) where revisionRef can be a revision or a tag or '@latest' string "+
+			"representing latest ready revision. This flag can be given multiple times with percent summing up to 100%.")
 }
 
 func (t *Traffic) PercentagesChanged(cmd *cobra.Command) bool {

pkg/kn/commands/service/create.go

@@ -23,6 +23,8 @@ import (
 
 	"knative.dev/client/pkg/config"
 	"knative.dev/client/pkg/kn/commands"
+	"knative.dev/client/pkg/kn/commands/flags"
+	"knative.dev/client/pkg/kn/traffic"
 	servinglib "knative.dev/client/pkg/serving"
 
 	"knative.dev/serving/pkg/apis/serving"
@@ -82,6 +84,7 @@ var create_example = `
 func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
 	var editFlags ConfigurationEditFlags
 	var waitFlags commands.WaitFlags
+	var trafficFlags flags.Traffic
 
 	serviceCreateCommand := &cobra.Command{
 		Use:     "create NAME --image IMAGE",
@@ -118,6 +121,15 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
 			if err != nil {
 				return err
 			}
+			if trafficFlags.TagsChanged(cmd) {
+				trafficFlags.RevisionsPercentages = []string{"@latest=100"}
+
+				traffic, err := traffic.Compute(cmd, service, &trafficFlags, nil, editFlags.AnyMutation(cmd))
+				if err != nil {
+					return err
+				}
+				service.Spec.Traffic = traffic
+			}
 			serviceExists, err := serviceExists(cmd.Context(), client, service.Name)
 			if err != nil {
 				return err
@@ -143,6 +155,7 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
 	commands.AddNamespaceFlags(serviceCreateCommand.Flags(), false)
 	commands.AddGitOpsFlags(serviceCreateCommand.Flags())
 	editFlags.AddCreateFlags(serviceCreateCommand)
+	trafficFlags.AddTagFlag(serviceCreateCommand)
 	waitFlags.AddConditionWaitFlags(serviceCreateCommand, commands.WaitDefaultTimeout, "create", "service", "ready")
 	return serviceCreateCommand
 }

pkg/kn/commands/service/create_test.go

@@ -1084,3 +1084,18 @@ func TestServiceCreateFromYAMLWithOverrideError(t *testing.T) {
 	assert.Assert(t, err != nil)
 	assert.Assert(t, util.ContainsAll(err.Error(), "expected", "0", "<=", "2147483647", autoscaling.MaxScaleAnnotationKey))
 }
+
+func TestServiceCreateTag(t *testing.T) {
+	action, created, _, err := fakeServiceCreate([]string{
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--tag", "foo"}, false)
+	assert.NilError(t, err)
+	assert.Assert(t, action.Matches("create", "services"))
+	assert.Equal(t, len(created.Spec.Traffic), 1)
+	assert.Equal(t, created.Spec.Traffic[0].Tag, "foo")
+}
+
+func TestServiceCreateTagWithError(t *testing.T) {
+	_, _, _, err := fakeServiceCreate([]string{
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--tag", "foo,bar"}, false)
+	assert.Error(t, err, "repetition of identifier @latest is not allowed, use only once with --tag flag")
+}

pkg/kn/commands/service/update.go

@@ -107,7 +107,7 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
 					if err != nil {
 						return nil, err
 					}
-					traffic, err := traffic.Compute(cmd, service.Spec.Traffic, &trafficFlags, service.Name, revisions.Items)
+					traffic, err := traffic.Compute(cmd, service, &trafficFlags, revisions.Items, editFlags.AnyMutation(cmd))
 					if err != nil {
 						return nil, err
 					}

pkg/kn/traffic/compute.go

@@ -31,7 +31,6 @@ var latestRevisionRef = "@latest"
 
 const (
 	errorDistributionRevisionCount = iota
-	errorDistributionLatestTag
 	errorDistributionRevisionNotFound
 )
 
@@ -44,6 +43,9 @@ func newServiceTraffic(traffic []servingv1.TrafficTarget) ServiceTraffic {
 
 func splitByEqualSign(pair string) (string, string, error) {
 	parts := strings.Split(pair, "=")
+	if len(parts) == 1 {
+		return latestRevisionRef, parts[0], nil
+	}
 	if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
 		return "", "", fmt.Errorf("expecting the value format in value1=value2, given %s", pair)
 	}
@@ -224,12 +226,10 @@ func errorTrafficDistribution(sum int, reason int) error {
 	switch reason {
 	case errorDistributionRevisionCount:
 		errMsg = "incorrect number of revisions specified. Only 1 revision should be missing"
-	case errorDistributionLatestTag:
-		errMsg = "cannot determine traffic split when @latest tag is specified"
 	case errorDistributionRevisionNotFound:
 		errMsg = "cannot determine the missing revision"
 	}
-	return fmt.Errorf("unable to allocate the remaining traffic %d. %s", 100-sum, errMsg)
+	return fmt.Errorf("unable to allocate the remaining traffic: %d%%. Reason: %s", 100-sum, errMsg)
 }
 
 func errorSumGreaterThan100(sum int) error {
@@ -240,12 +240,14 @@ func errorSumGreaterThan100(sum int) error {
 // verifyInput checks:
 // - if user has repeated @latest field in --tag or --traffic flags
 // - if provided traffic portion are integers
-func verifyInput(trafficFlags *flags.Traffic, revisions []servingv1.Revision) error {
+// - if traffic as per flags sums to 100
+// - if traffic as per flags < 100, can remaining traffic be automatically directed
+func verifyInput(trafficFlags *flags.Traffic, svc *servingv1.Service, revisions []servingv1.Revision, mutation bool) error {
 	// check if traffic is being sent to @latest tag
 	var latestRefTraffic bool
 
-	// number of revisions
-	var revisionCount = len(revisions)
+	// number of existing revisions
+	var existingRevisionCount = len(revisions)
 
 	err := verifyLatestTag(trafficFlags)
 	if err != nil {
@@ -257,36 +259,62 @@ func verifyInput(trafficFlags *flags.Traffic, revisions []servingv1.Revision) er
 		return err
 	}
 
+	// further verification is not required if sum >= 100
+	if sum == 100 {
+		return nil
+	}
+	if sum > 100 {
+		return errorSumGreaterThan100(sum)
+	}
+
 	if _, ok := revisionRefMap[latestRevisionRef]; ok {
 		// traffic has been routed to @latest tag
 		latestRefTraffic = true
 	}
 
 	// number of revisions specified in traffic flags
 	specRevPercentCount := len(trafficFlags.RevisionsPercentages)
-	// equivalent check for `cmd.Flags().Changed("traffic")` as we don't have `cmd` in this function
-	if specRevPercentCount > 0 {
-		if sum > 100 {
-			return errorSumGreaterThan100(sum)
+
+	// no traffic to route
+	if specRevPercentCount == 0 {
+		return nil
+	}
+
+	// cannot determine remaining revision. Only 1 should be left out
+	if specRevPercentCount < existingRevisionCount-1 {
+		return errorTrafficDistribution(sum, errorDistributionRevisionCount)
+	}
+
+	// if mutation is set, a new revision will be created after service update.
+	// specRevPercentCount should be equal to existingRevisionCount
+	if mutation {
+		if specRevPercentCount != existingRevisionCount {
+			return errorTrafficDistribution(sum, errorDistributionRevisionCount)
 		}
-		if sum < 100 && specRevPercentCount != revisionCount-1 {
+		if _, ok := revisionRefMap[latestRevisionRef]; !ok {
+			// remaining % can only go to the @latest tag
+			trafficFlags.RevisionsPercentages = append(trafficFlags.RevisionsPercentages, fmt.Sprintf("%s=%d", latestRevisionRef, 100-sum))
+			return nil
+		}
+	} else {
+		// if mutation is not set, specRevPercentCount should be equal to existingRevisionCount
+		if specRevPercentCount != existingRevisionCount-1 {
 			return errorTrafficDistribution(sum, errorDistributionRevisionCount)
 		}
-		if sum < 100 && specRevPercentCount == revisionCount-1 {
-			if latestRefTraffic {
-				return errorTrafficDistribution(sum, errorDistributionLatestTag)
-			}
-			for _, rev := range revisions {
-				if !checkRevisionPresent(revisionRefMap, rev) {
-					trafficFlags.RevisionsPercentages = append(trafficFlags.RevisionsPercentages, fmt.Sprintf("%s=%d", rev.Name, 100-sum))
-					return nil
-				}
-			}
-			return errorTrafficDistribution(sum, errorDistributionRevisionNotFound)
+		if latestRefTraffic {
+			revisionRefMap[svc.Status.LatestReadyRevisionName] = 0
 		}
 	}
 
-	return nil
+	// remaining % to 100
+	for _, rev := range revisions {
+		if !checkRevisionPresent(revisionRefMap, rev) {
+			trafficFlags.RevisionsPercentages = append(trafficFlags.RevisionsPercentages, fmt.Sprintf("%s=%d", rev.Name, 100-sum))
+			return nil
+		}
+	}
+
+	return errorTrafficDistribution(sum, errorDistributionRevisionNotFound)
 }
 
 func verifyRevisionSumAndReferences(trafficFlags *flags.Traffic) (revisionRefMap map[string]int, sum int, err error) {
@@ -352,10 +380,14 @@ func checkRevisionPresent(refMap map[string]int, rev servingv1.Revision) bool {
 	return tagExists || nameExists
 }
 
-// Compute takes service traffic targets and updates per given traffic flags
-func Compute(cmd *cobra.Command, targets []servingv1.TrafficTarget,
-	trafficFlags *flags.Traffic, serviceName string, revisions []servingv1.Revision) ([]servingv1.TrafficTarget, error) {
-	err := verifyInput(trafficFlags, revisions)
+// Compute takes service object, computes traffic per given traffic flags and returns the new traffic. If
+// total traffic per flags < 100, the params 'revisions' and 'mutation' are used to direct remaining
+// traffic. Param 'mutation' is set to true if a new revision will be created on service update
+func Compute(cmd *cobra.Command, svc *servingv1.Service,
+	trafficFlags *flags.Traffic, revisions []servingv1.Revision, mutation bool) ([]servingv1.TrafficTarget, error) {
+	targets := svc.Spec.Traffic
+	serviceName := svc.Name
+	err := verifyInput(trafficFlags, svc, revisions, mutation)
 	if err != nil {
 		return nil, err
 	}
@@ -416,7 +448,7 @@ func Compute(cmd *cobra.Command, targets []servingv1.TrafficTarget,
 		traffic = traffic.TagRevision(tag, revision)
 	}
 
-	if cmd.Flags().Changed("traffic") {
+	if cmd.Flags().Changed("traffic") || (cmd.Name() == "create" && len(trafficFlags.RevisionsTags) > 0) {
 		// reset existing traffic portions as what's on CLI is desired state of traffic split portions
 		traffic.ResetAllTargetPercent()