Add --pull-secret flag #617

navidshaikh · 2020-01-21T14:59:18Z

Fixes #616

Add --pull-secret flag for service create/update operations
Setting empty string to flag clears the pull secrets
List Image Pull Secret in service describe default output

/lint

knative-prow-robot

In response to this:

Fixes #616

Add --pull-secret flag for service create/update operations

Setting empty string to flag clears the pull secrets

List ImagePullSecrets for service in service describe default output

Run e2e tests against serving v0.11.1 (ImagePullSecrets introduced in this release)

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

maximilien

See comments. Thanks for contribution.

maximilien · 2020-01-21T22:25:41Z

docs/cmd/kn_service_create.md

@@ -61,6 +61,7 @@ kn service create NAME --image IMAGE [flags]
  -n, --namespace string         Specify the namespace to operate in.
      --no-lock-to-digest        do not keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)
  -p, --port int32               The port where application listens on.
+      --pull-secrets string      Image pull secrets to set. Empty image pull secrets will result to clear the pull secrets.


This "Empty image pull secrets will result to clear the pull secrets" reads odd... Perhaps: "Empty image pull secrets will clear the pull secrets field"

Maybe: "No pull secret is used when no secret is provided as an argument"

btw, this is another example why we might to separate better the create and update help messages as for a kn service create an empty --pull-secrets doesn't make sense at all (and would be a no-op)

this is another example why we might to separate better the create and update help messages as for a kn service create an empty --pull-secrets doesn't make sense at all (and would be a no-op)

+1, good point!

I kept the message in format as we've for ServiceAccount

--service-account string Service account name to set. Empty service account name will result to clear the service account.

The pull secrets are set per revision of the service, you can create a service specifying the pull secrets and a private image, then update service with another revision with a public image and clearing the pull secrets.
Also note, created a service with pull-secrets and private image, any subsequent updates to service without image and empty pull secrets ("") works fine too.

I've updated the help text as below for --pull-secrets and --service-account flag.

Image pull secrets to set. Empty image pull secrets will the clear the pull secrets.

Service account name to set. Empty service account name will clear the service account.

maximilien · 2020-01-21T22:29:23Z

pkg/serving/config_changes_test.go

+	assert.Equal(t, template.Spec.ImagePullSecrets[0].Name, "quay")
+
+	UpdateImagePullSecrets(template, " ")
+	assert.Check(t, template.Spec.ImagePullSecrets == nil)


Hmm, is that true. An empty space value will result in nil. From code above

if pullsecrets == "" { template.Spec.ImagePullSecrets = nil } else { template.Spec.ImagePullSecrets = []corev1.LocalObjectReference{{ Name: pullsecrets, }} }

seems like it would be an ImagePullSecrets with a LocalObjectReference of name " ". No?

No. The zero value for ImagePullSecrets (type slice) is nil here.

test/presubmit-integration-tests-latest-release.sh

rhuss

Looks good to me with some minor nits (adapting the usage message and maybe chaning the label)

rhuss · 2020-01-22T07:45:42Z

pkg/kn/commands/service/describe.go

@@ -171,6 +171,9 @@ func writeService(dw printers.PrefixWriter, service *v1alpha1.Service) {
 	if (service.Spec.Template != nil) && (service.Spec.Template.Spec.ServiceAccountName != "") {
 		dw.WriteAttribute("ServiceAccount", service.Spec.Template.Spec.ServiceAccountName)
 	}
+	if service.Spec.Template != nil && service.Spec.Template.Spec.ImagePullSecrets != nil {
+		dw.WriteAttribute("ImagePullSecrets", service.Spec.Template.Spec.ImagePullSecrets[0].Name)


I wonder whether we should use the label with spaces (like in "Image Pull Secrets", maybe also for "Service Account") as we use this for other parts in describe, too, for an easier consumption by a human reader.

But I just saw, that even for a kubectl describe this is not consistent (i.e. mixed usage of camel-case and space separated labels). So probably not a big thing :)

It occurred to me as well, though I went along with other fields as mentioned for ServiceAccount. Will change it to space separated.

Changed to Image Pull Secrets and Service Account.

rhuss · 2020-01-22T07:46:49Z

pkg/serving/config_changes.go

@@ -397,6 +397,18 @@ func UpdateServiceAccountName(template *servingv1alpha1.RevisionTemplateSpec, se
 	return nil
 }

+// UpdateImagePullSecrets updates the image pull secrets used for the corresponding knative service
+func UpdateImagePullSecrets(template *servingv1alpha1.RevisionTemplateSpec, pullsecrets string) {
+	pullsecrets = strings.TrimSpace(pullsecrets)


@maximilien this is where leading and trailing spaces are removed, therefor a test with " " works (and also tests that trim)

rhuss · 2020-01-22T07:49:42Z

/retest

Fixes knative#616 - Add --pull-secrets flag for service create/update operations - Setting empty string to flag clears the pull secrets - List ImagePullSecrets for service in `service describe` default output - Run e2e tests against latets serving v0.12.0 (ImagePullSecrets introduced in v0.11.1 release)

rhuss · 2020-01-22T10:55:17Z

docs/cmd/kn_service_create.md

@@ -62,10 +62,11 @@ kn service create NAME --image IMAGE [flags]
  -n, --namespace string          Specify the namespace to operate in.
      --no-lock-to-digest         do not keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)
  -p, --port int32                The port where application listens on.
+      --pull-secrets string       Image pull secrets to set. Empty image pull secrets will the clear the pull secrets.


Suggested change

--pull-secrets string Image pull secrets to set. Empty image pull secrets will the clear the pull secrets.

--pull-secrets string Image pull secret to set. An empty argument clears the pull secret. This must be the name of a secret in the service's namespace.

rhuss · 2020-01-22T10:56:19Z

pkg/kn/commands/service/describe.go

+		dw.WriteAttribute("Service Account", service.Spec.Template.Spec.ServiceAccountName)
+	}
+	if service.Spec.Template != nil && service.Spec.Template.Spec.ImagePullSecrets != nil {
+		dw.WriteAttribute("Image Pull Secrets", service.Spec.Template.Spec.ImagePullSecrets[0].Name)


So you are using only the first element, right ? IMO it would be more consequent to call the option --image-pull-secret (singular) than --image-pull-secrets

I removed image from it. Do you mean to add image in the flag as well apart from singular comment?

Yes, we're using only the first element.
IIUC only one image is allowed per Knative Service, but should we keep the flag name closer to the actual field name in spec?

nope, --pull-secret is ok as the context is clear. My bad.

rhuss

Looking again at the PR, I think it should be call --pull-secret (singular) as you only manage a single secret.

rhuss · 2020-01-22T10:59:12Z

docs/cmd/kn_service_create.md

      --requests-cpu string       The requested CPU (e.g., 250m).
      --requests-memory string    The requested memory (e.g., 64Mi).
      --revision-name string      The revision name to set. Must start with the service name and a dash as a prefix. Empty revision name will result in the server generating a name for the revision. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}")
-      --service-account string    Service account name to set. Empty service account name will result to clear the service account.
+      --service-account string    Service account name to set. Empty service account name will clear the service account.


Suggested change

--service-account string Service account name to set. Empty service account name will clear the service account.

--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.

well, actually I suggest the change on in the edit_flags.go file. But you know what I mean ;-)

- Update the key name in service describe: - ImagePullSecrets --> Image Pull Secrets - ServiceAccount --> Service Account - Update the help message for --service-account and --pull-secrets

knative-metrics-robot · 2020-01-22T11:24:58Z

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/service/configuration_edit_flags.go	82.5%	82.3%	-0.2
pkg/kn/commands/service/describe.go	80.0%	79.6%	-0.4
pkg/serving/config_changes.go	79.5%	79.8%	0.3

navidshaikh · 2020-01-22T11:51:11Z

/retest

Failed to successfully execute 'kn service create hello --image gcr.io/knative-samples/helloworld-go -n kne2etests6': Execution error: stderr: 'RevisionMissing: Revision "hello-mzmck-1" referenced in traffic not found.

navidshaikh · 2020-01-22T13:26:06Z

New Changes:

Renamed --pull-secrets to singular --pull-secret
describe output now lists keys as Service Account and Image Pull Secret
Help messages as follow:

--pull-secret string        Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.

and

--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.

rhuss

thanks!

/lgtm

knative-prow-robot · 2020-01-22T14:32:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, navidshaikh, rhuss

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [maximilien,navidshaikh,rhuss]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 21, 2020

knative-prow-robot requested review from mattmoor and rhuss January 21, 2020 14:59

knative-prow-robot reviewed Jan 21, 2020

View reviewed changes

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 21, 2020

navidshaikh force-pushed the pr/pull-secrets branch from e4270fd to 67f0728 Compare January 21, 2020 14:59

navidshaikh changed the title ~~Add --pull-secret flag~~ Add --pull-secrets flag Jan 21, 2020

navidshaikh force-pushed the pr/pull-secrets branch from 67f0728 to cbe685a Compare January 21, 2020 18:15

maximilien approved these changes Jan 21, 2020

View reviewed changes

rhuss requested changes Jan 22, 2020

View reviewed changes

navidshaikh force-pushed the pr/pull-secrets branch from cbe685a to c7b9bb2 Compare January 22, 2020 09:33

rhuss reviewed Jan 22, 2020

View reviewed changes

rhuss requested changes Jan 22, 2020

View reviewed changes

rhuss reviewed Jan 22, 2020

View reviewed changes

Space separated field name for describe and update flag help msg

f39e077

- Update the key name in service describe: - ImagePullSecrets --> Image Pull Secrets - ServiceAccount --> Service Account - Update the help message for --service-account and --pull-secrets

navidshaikh force-pushed the pr/pull-secrets branch from c7b9bb2 to f39e077 Compare January 22, 2020 11:22

navidshaikh changed the title ~~Add --pull-secrets flag~~ Add --pull-secret flag Jan 22, 2020

rhuss approved these changes Jan 22, 2020

View reviewed changes

knative-prow-robot assigned rhuss Jan 22, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 22, 2020

knative-prow-robot merged commit ff7dffc into knative:master Jan 22, 2020

coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020

enable dind for docs repo (knative#617)

02f3d54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --pull-secret flag #617

Add --pull-secret flag #617

navidshaikh commented Jan 21, 2020 •

edited

Loading

knative-prow-robot left a comment

maximilien left a comment

maximilien Jan 21, 2020

rhuss Jan 22, 2020

navidshaikh Jan 22, 2020

navidshaikh Jan 22, 2020

maximilien Jan 21, 2020

navidshaikh Jan 22, 2020

rhuss left a comment

rhuss Jan 22, 2020

navidshaikh Jan 22, 2020

navidshaikh Jan 22, 2020

rhuss Jan 22, 2020

rhuss commented Jan 22, 2020

rhuss Jan 22, 2020 •

edited

Loading

rhuss Jan 22, 2020

navidshaikh Jan 22, 2020

rhuss Jan 22, 2020

rhuss left a comment

rhuss Jan 22, 2020

rhuss Jan 22, 2020 •

edited

Loading

knative-metrics-robot commented Jan 22, 2020

navidshaikh commented Jan 22, 2020

navidshaikh commented Jan 22, 2020

rhuss left a comment

knative-prow-robot commented Jan 22, 2020

	--pull-secrets string Image pull secrets to set. Empty image pull secrets will the clear the pull secrets.
	--pull-secrets string Image pull secret to set. An empty argument clears the pull secret. This must be the name of a secret in the service's namespace.

	--service-account string Service account name to set. Empty service account name will clear the service account.
	--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.

Add --pull-secret flag #617

Add --pull-secret flag #617

Conversation

navidshaikh commented Jan 21, 2020 • edited Loading

knative-prow-robot left a comment

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss commented Jan 22, 2020

rhuss Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

knative-metrics-robot commented Jan 22, 2020

navidshaikh commented Jan 22, 2020

navidshaikh commented Jan 22, 2020

rhuss left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jan 22, 2020

navidshaikh commented Jan 21, 2020 •

edited

Loading

rhuss Jan 22, 2020 •

edited

Loading

rhuss Jan 22, 2020 •

edited

Loading