Add support for envFrom and volumeMounts #393

igsong · 2019-08-28T17:51:06Z

Proposed Changes

To use NOT-YET supported expressions of ksvc, following flags, which can be set in service create or service update, are added.

Support envFrom expression. By giving the flag --env-from (config-map | secret):CONFIG_MAP_OR_SECRET_NAME, a config map or secret is embedded using envFrom. To unset, a - suffix can be used like --env or --label.
Support volumeMount expression. By giving the flag --volume-mount VOLUME_NAME=(config-map|secret):CONFIG_MAP_OR_SECRET_NAME@/mount/here, a config map or secret is mounted on the user-container. To unset, a - suffix can be used like --env or --label.

Unlike initial proposal, the support for serviceAccountName will be shown another PR and imagePullPolicy has removed because Knative Serving does not support the feature.

Release Note

NONE

/lint

knative-prow-robot

@igsong: 5 warnings.

In response to this:

Proposed Changes

To use NOT-YET supported expressions of ksvc, following flags, which can be set in service create or service update, are added.

Support envFrom expression. By giving the flag --env-from (config-map | secret):CONFIG_MAP_OR_SECRET_NAME, a config map or secret is embedded using envFrom. To unset, a - suffix can be used like --env or --label.

Support volumeMount expression. By giving the flag --volume-mount VOLUME_NAME=(config-map|secret):CONFIG_MAP_OR_SECRET_NAME@/mount/here, a config map or secret is mounted on the user-container. To unset, a - suffix can be used like --env or --label.

Support serviceAccountName expression. By giving the flag --service-account-name=SERVICE_ACCOUNT_NAME, a user can set a service account for a ksvc.

Support imagePullPolicy expression. By giving the flag --image-pull-policy=POLICY, a user can set a image pull policy for a ksvc. POLICY should be one of always, if-not-present, never or - which can be used for unsetting imagePullPolicy.

Release Note
NONE
/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.

pkg/serving/config_changes.go

pkg/util/parsing_helper.go

knative-prow-robot · 2019-08-28T17:51:21Z

Hi @igsong. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

igsong · 2019-08-28T18:55:47Z

/lint

knative-prow-robot

@igsong: 0 warnings.

In response to this:

/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

/ok-to-test

igsong · 2019-08-29T23:33:45Z

/test pull-knative-client-build-tests

navidshaikh · 2019-08-30T06:38:03Z

Support imagePullPolicy expression. By giving the flag --image-pull-policy=POLICY, a user can set a image pull policy for a ksvc. POLICY should be one of always, if-not-present, never or - which can be used for unsetting imagePullPolicy.

We have lock-to-digest and no-lock-to-digest flags, which users can use to achieve desired state.

igsong · 2019-08-30T16:18:44Z

We have lock-to-digest and no-lock-to-digest flags, which users can use to achieve desired state.

Thank you for comments. But, I think that the lock-to-digest and no-lock-to-digest flags are different from image-pull-policy because image-pull-policy is a kind of caching policy of the docker image on each k8s worker node, while the lock-to-digest flag automatically alternates image to a digest form of image URL previously used when there is no image flag. Of course, when a user is using a static tag or a digest to specify an image, or lock-to-digest flag is set, image-pull-policy may be not so useful, but when using a dynamic tag with no-lock-to-digest, setting imagePullPolicy may be needed.

pkg/kn/commands/service/configuration_edit_flags.go

pkg/serving/config_changes.go

pkg/kn/commands/service/configuration_edit_flags.go

pkg/serving/config_changes.go

sixolet · 2019-09-03T20:37:09Z

pkg/serving/config_changes.go

+		}
+	}
+
+	for i := range volumeMounts {


This seems like this code may not handle mounting the same secret (or whatever volume) at two different paths, or handling a message where the same secret (or whatever volume) was already mounted at two different paths. There are a lot of corner cases here.

I agree with the decision not to make the user handle all the minutae of the difference between volume mounts and volumes if they do not have to, though.

OK, then I think that it would be better to have two flags instead of current one, like --volume-mount /mount/here=VOLUME_NAME and --volume VOLUME_NAME=(config-map|secret):CONFIG_MAP_OR_SECRET_NAME. How do you think of it?

I split up the flag --volume-mount into --volume and --volume-mount to cope with the case that @sixolet mentioned

docs/cmd/kn_service_update.md

maximilien

Wondering if there is a way to also include an integration test? Perhaps a modification to other tests that uses these features?

maximilien · 2019-09-19T22:28:52Z

You might need to rebase:

pkg/kn/commands/service/service_create_test.go
pkg/kn/commands/service/service_update_test.go

navidshaikh · 2019-10-01T11:58:27Z

@igsong : ping! let's rebase and get this in queue.

igsong · 2019-10-04T13:55:53Z

@igsong : ping! let's rebase and get this in queue.

I've rebased.

maximilien · 2019-10-04T20:24:17Z

You might need to make sure and include the newly generated docs.

…rder to enable multiple times mounting for the same volume

…olume automatically when the config-map or secret is directley used. In addition, the test cases are changed to new unit test style. To keep the original orderedness given via flags, OrderedMap implementation is added as well

igsong · 2019-11-03T18:44:29Z

I've done to apply all the suggested and discussed changes.

One thing I want to notice is that OrderedMap is newly adopted to make behaviors caused by --volume and --mount deterministic.

rhuss

Thanks a tons, looks really, really good !

I just started the review, but not yet finished. However there is one serious error which always occur for auto generated volume names (see below, ie.. a name must not contain a /). If you could sanitize the name before using as a volume name, that would be a good fix.

I will continue later, but wanted to already give you that partial review feedback.

docs/cmd/kn_service_create.md

rhuss · 2019-11-05T11:35:06Z

pkg/serving/config_changes.go

+}
+
+func generateVolumeName(path string) string {
+	return fmt.Sprintf("kn-managed-volume:%s", path)


This is too simplistic, as you have to sanitize the path that it's a well-formed kubernetes identifier. As the path has to start with / (as you can only mount absolute paths) you will always get an invalid name here. Unfortunately, such things can be only detected in integration tests, not unit tests, as unit tests don't do any validation.

Also, I would either don't use a prefix or, if required e.g. when for auto-deleting volumes, then just use kn:, which is just shorter and easier to read when examing the k8s files.

I've changed the part to use a sanitized path string + checksum of original path string without any prefix.

Thanks, works for me now !

I'm going to go over the rest of the PR quickly and lets get it merged today so that we get it into 0.10.0 (to be released today). We can then fix any outstanding issues (or add some real integrations tests) later.

knative-metrics-robot · 2019-11-05T19:20:39Z

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	83.9%	84.7%	0.9
pkg/serving/config_changes.go	82.3%	79.3%	-3.0
pkg/util/orderedmap.go	Do not exist	73.0%
pkg/util/parsing_helper.go	100.0%	95.7%	-4.3

rhuss

Thanks a lot for all your work (and especially for the comprehensive unit tests) and also for your patience with us reviewing it !

The PR looks good to merge and will be included in 0.10.0 (which by accident is released today ;-)

/lgtm

knative-prow-robot · 2019-11-06T10:41:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: igsong, 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~~ [rhuss]

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

rhuss · 2019-11-06T11:21:00Z

/retest

rhuss · 2019-11-06T11:21:42Z

The CI test error is a strange beast. See slack channel for the discussion about it. Hopefully its only a hickup.

navidshaikh · 2019-11-06T11:49:20Z

/test pull-knative-client-integration-tests

* Add more functions to shared gcs package These functions are needed for flaky tests reporting, which will need to list n number of latest finished builds, generate some files and upload to gcs to be consumed by testgrid * separate out prow, testgrid, and junit related functions

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 28, 2019

knative-prow-robot requested a review from evankanderson August 28, 2019 17:51

knative-prow-robot reviewed Aug 28, 2019

View reviewed changes

knative-prow-robot requested a review from sixolet August 28, 2019 17:51

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 28, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 28, 2019

knative-prow-robot reviewed Aug 28, 2019

View reviewed changes

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 29, 2019

maximilien reviewed Aug 29, 2019

View reviewed changes

sixolet suggested changes Sep 3, 2019

View reviewed changes

igsong requested a review from sixolet September 17, 2019 15:38

maximilien reviewed Sep 19, 2019

View reviewed changes

docs/cmd/kn_service_update.md Outdated Show resolved Hide resolved

maximilien reviewed Sep 19, 2019

View reviewed changes

docs/cmd/kn_service_update.md Outdated Show resolved Hide resolved

maximilien suggested changes Sep 19, 2019

View reviewed changes

igsong force-pushed the master branch from 322f1b3 to d7a43e1 Compare October 4, 2019 13:53

igsong force-pushed the master branch from d7a43e1 to 9ec234f Compare October 5, 2019 16:03

igsong added 8 commits November 2, 2019 22:45

Fix a bug on a test case, caused by ignoring random orderedness of map

5a55a26

Remove image-pull-policy because it is not supported by Knative

4bb98c7

Fix comments to clarify it as well as to fix a typo

2c9ffdf

Remove service-account-name flags in order to submit it as a seperate PR

ef02dc6

Split --volume-mount flag into --volume-mount and --volume flags in o…

d712aa6

…rder to enable multiple times mounting for the same volume

Change the name of local variable to simplify

b45d2ad

Update docs

fafee77

igsong force-pushed the master branch from 4a27432 to cd48c65 Compare November 3, 2019 05:51

igsong added 2 commits November 2, 2019 23:00

Fix usage descriptions for the mount flag

4490db6

Factor out the name existence checking from createEnvFromSource

431b063

igsong requested a review from rhuss November 3, 2019 18:44

rhuss requested changes Nov 5, 2019

View reviewed changes

igsong added 2 commits November 5, 2019 09:02

Fix the usage description for --mount

1ae8426

Sanitize a generated volume name

e8e0f76

rhuss approved these changes Nov 6, 2019

View reviewed changes

knative-prow-robot assigned rhuss Nov 6, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 6, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2019

rhuss removed request for sixolet and evankanderson November 6, 2019 12:11

knative-prow-robot merged commit 9d759ca into knative:master Nov 6, 2019

navidshaikh added a commit to navidshaikh/client that referenced this pull request Aug 5, 2020

Update operator setup (knative#393)

2c6bc14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for envFrom and volumeMounts #393

Add support for envFrom and volumeMounts #393

igsong commented Aug 28, 2019 •

edited

Loading

knative-prow-robot left a comment

knative-prow-robot commented Aug 28, 2019

igsong commented Aug 28, 2019

knative-prow-robot left a comment

maximilien left a comment

igsong commented Aug 29, 2019

navidshaikh commented Aug 30, 2019

igsong commented Aug 30, 2019 •

edited

Loading

sixolet Sep 3, 2019

igsong Sep 4, 2019 •

edited

Loading

igsong Sep 4, 2019

maximilien left a comment

maximilien commented Sep 19, 2019 •

edited

Loading

navidshaikh commented Oct 1, 2019

igsong commented Oct 4, 2019 •

edited

Loading

maximilien commented Oct 4, 2019

igsong commented Nov 3, 2019

rhuss left a comment

rhuss Nov 5, 2019

igsong Nov 6, 2019

rhuss Nov 6, 2019

knative-metrics-robot commented Nov 5, 2019

rhuss left a comment

knative-prow-robot commented Nov 6, 2019

rhuss commented Nov 6, 2019

rhuss commented Nov 6, 2019

navidshaikh commented Nov 6, 2019

Add support for envFrom and volumeMounts #393

Add support for envFrom and volumeMounts #393

Conversation

igsong commented Aug 28, 2019 • edited Loading

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

knative-prow-robot commented Aug 28, 2019

igsong commented Aug 28, 2019

knative-prow-robot left a comment

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

igsong commented Aug 29, 2019

navidshaikh commented Aug 30, 2019

igsong commented Aug 30, 2019 • edited Loading

sixolet Sep 3, 2019

Choose a reason for hiding this comment

igsong Sep 4, 2019 • edited Loading

Choose a reason for hiding this comment

igsong Sep 4, 2019

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

maximilien commented Sep 19, 2019 • edited Loading

navidshaikh commented Oct 1, 2019

igsong commented Oct 4, 2019 • edited Loading

maximilien commented Oct 4, 2019

igsong commented Nov 3, 2019

rhuss left a comment

Choose a reason for hiding this comment

rhuss Nov 5, 2019

Choose a reason for hiding this comment

igsong Nov 6, 2019

Choose a reason for hiding this comment

rhuss Nov 6, 2019

Choose a reason for hiding this comment

knative-metrics-robot commented Nov 5, 2019

rhuss left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Nov 6, 2019

rhuss commented Nov 6, 2019

rhuss commented Nov 6, 2019

navidshaikh commented Nov 6, 2019

igsong commented Aug 28, 2019 •

edited

Loading

igsong commented Aug 30, 2019 •

edited

Loading

igsong Sep 4, 2019 •

edited

Loading

maximilien commented Sep 19, 2019 •

edited

Loading

igsong commented Oct 4, 2019 •

edited

Loading