Expand variables in containers' env, cmd, args #8149

pmorie · 2015-05-12T23:50:36Z

Depends on #7936, not ready for review.

pmorie · 2015-05-18T23:59:44Z

@smarterclayton @derekwaynecarr @a-robinson PTAL

pmorie · 2015-05-19T17:11:42Z

derekwaynecarr · 2015-05-19T17:21:01Z

pkg/kubelet/kubelet.go

+	//
+	// 1.  Determine the runtime value of all variables by resolving
+	//     alternate env var sources.
+	// 2.  Determine the final value of each variable by expanding `$var` and `${var}`


$(var) references

pmorie · 2015-05-19T17:34:12Z

pkg/kubelet/kubelet.go

+	// 3.  Create the container's environment in the order variables are declared
+
+	// Step 1: resolve alternate env var sources
+	tmpEnv := make(map[string]string)
 	for _, value := range container.Env {


I think this method has a bug -- currently, you could have incorrect expansion of a var declared later in a container's environment, which is not supposed to be legal according to the spec.

Verified it is a bug, will be fixed in next push.

pmorie · 2015-05-19T18:08:35Z

For the record, I decided when coding this that adding a new env var source was unnecessary / inconsistent with the rest of the agreed upon changes.

pmorie · 2015-05-19T18:17:17Z

pkg/api/types.go

@@ -513,7 +513,8 @@ type EnvVar struct {
 	// Required: This must be a C_IDENTIFIER.
 	Name string `json:"name"`
 	// Optional: no more than one of the following may be specified.
-	// Optional: Defaults to "".
+	// Optional: Defaults to ""; variable references ("$(VAR_NAME)") are


@derekwaynecarr if you like the copy here and in the v1 types.go file, I will change the other API versions.

FTR I am hands-off on this - just swamped. @a-robinson is running point

On Tue, May 19, 2015 at 11:17 AM, Paul Morie notifications@github.com
wrote:

In pkg/api/types.go
#8149 (comment)
:

@@ -513,7 +513,8 @@ type EnvVar struct {
// Required: This must be a C_IDENTIFIER.
Name string json:"name"
// Optional: no more than one of the following may be specified.

// Optional: Defaults to "".

// Optional: Defaults to ""; variable references ("$(VAR_NAME)") are

@derekwaynecarr https://github.com/derekwaynecarr if you like the copy
here and in the v1 types.go file, I will change the other API versions.

—
Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/8149/files#r30629300
.

That's a new acronym I'm going to steal from Tim.

----- Original Message -----

@@ -513,7 +513,8 @@ type EnvVar struct {
// Required: This must be a C_IDENTIFIER.
Name string json:"name"
// Optional: no more than one of the following may be specified.

// Optional: Defaults to "".

// Optional: Defaults to ""; variable references ("$(VAR_NAME)") are

FTR I am hands-off on this - just swamped. @a-robinson is running point

On Tue, May 19, 2015 at 11:17 AM, Paul Morie notifications@github.com
wrote:

In pkg/api/types.go
#8149 (comment)
:

@@ -513,7 +513,8 @@ type EnvVar struct {
// Required: This must be a C_IDENTIFIER.
Name string json:"name"
// Optional: no more than one of the following may be specified.

// Optional: Defaults to "".

// Optional: Defaults to ""; variable references ("$(VAR_NAME)") are

@derekwaynecarr https://github.com/derekwaynecarr if you like the copy
here and in the v1 types.go file, I will change the other API versions.

—
Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/8149/files#r30629300
.

Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/8149/files#r30641318

This is totally open to discussion, but think it's more clear to get rid of the extraneous symbols around the example, leaving something like -
variable references of the form $(VAR_NAME) are expanded to the value of VAR_NAME according to the variable expansion spec

And it would be even better if you could briefly explain the behavior rather than only referring to a spec that the reader has to search for

Like @a-robinson , I would prefer that the pertinent documentation piece goes in the description field for the value. Also note if you change these you will need to regen the swagger docs.

pmorie · 2015-05-19T20:51:09Z

@a-robinson if you have bandwidth I'd love your input on this

a-robinson · 2015-05-19T21:08:21Z

pkg/api/v1/types.go

-	Command []string `json:"command,omitempty" description:"entrypoint array; not executed within a shell; the docker image's entrypoint is used if this is not provided; cannot be updated"`
-	// Optional: The docker image's cmd is used if this is not provided; cannot be updated.
-	Args []string `json:"args,omitempty" description:"command array; the docker image's cmd is used if this is not provided; arguments to the entrypoint; cannot be updated"`
+	// Optional: The docker image's entrypoint is used if this is not provided; cannot be updated;


You seem to have missed the comment on EnvVar.Value in this file

a-robinson · 2015-05-23T23:32:23Z

pkg/api/types.go

@@ -639,9 +645,13 @@ type Container struct {
 	Name string `json:"name"`
 	// Required.
 	Image string `json:"image"`
-	// Optional: The docker image's entrypoint is used if this is not provided; cannot be updated.
+	// Optional: The docker image's entrypoint is used if this is not provided;
+	// cannot be updated; variable references ("$(VAR_NAME)") are expanded according


drop the extra parentheses and quotes around $(VAR_NAME) as you did above

You could also refer the reader to the rules for how EnvVar.Value is expanded rather than to the full spec, or just copy what you put there as you did in the versioned API files

a-robinson · 2015-05-23T23:43:21Z

LGTM other than a few really minor cleanups. Thanks for the revisions!

pmorie · 2015-05-24T00:05:48Z

Re: ordering sure thing
On Sat, May 23, 2015 at 7:43 PM Alex Robinson notifications@github.com
wrote:

LGTM other than a few really minor cleanups. Thanks for the revisions!

—
Reply to this email directly or view it on GitHub
#8149 (comment)
.

pmorie · 2015-05-26T15:23:54Z

@a-robinson PR deloused, PTAL

a-robinson · 2015-05-26T17:41:53Z

@derekwaynecarr, any other comments?

pmorie · 2015-05-27T02:41:21Z

@a-robinson, derek is too stunned to speak, clearly. any chance the on-call can TAL tomorrow?

pmorie · 2015-05-27T02:44:28Z

@a-robinson unfortunately I need to remove LGTM status from this, i believe there is something I missed that needs to be updated.

pmorie · 2015-05-27T02:45:10Z

elaborating on my prior comment, looks like the dns test needs to be updated since it uses the subshell $() syntax in the image's command.

pmorie · 2015-05-27T02:47:46Z

Cluster logging e2e is affected too. They should both pass but I don't want to be sloppy.

a-robinson · 2015-05-27T03:56:41Z

Good catch, let me know when you've fixed them up.

The oncall is still catching up with the backlog from last week's merge freeze, so the current protocol is just to slap the status/lgtm on it and wait for them to come by and merge it.

pmorie · 2015-05-27T04:15:39Z

@a-robinson Latest code has fixes. Thnx for clarification re: on-call.

a-robinson · 2015-05-27T04:46:38Z

LGTM, assuming you made sure the modified e2e tests pass.

pmorie · 2015-05-27T04:50:11Z

Yeah, i got a clean e2e run in GCE
On Wed, May 27, 2015 at 12:46 AM Alex Robinson notifications@github.com
wrote:

LGTM, assuming you made sure the modified e2e tests pass.

—
Reply to this email directly or view it on GitHub
#8149 (comment)
.

Expand variables in containers' env, cmd, args

googlebot added the cla: yes label May 12, 2015

pmorie force-pushed the container-env-resolution branch 5 times, most recently from 0ab9ad2 to c1d9f0a Compare May 18, 2015 23:28

pmorie changed the title ~~WIP: Expand env var values in kubelet~~ Expand variables in containers' env, cmd, args May 19, 2015

pmorie force-pushed the container-env-resolution branch from c1d9f0a to 95f75af Compare May 19, 2015 00:01

derekwaynecarr reviewed May 19, 2015
View reviewed changes

derekwaynecarr self-assigned this May 19, 2015

pmorie reviewed May 19, 2015
View reviewed changes

pmorie force-pushed the container-env-resolution branch from 95f75af to c84931b Compare May 19, 2015 18:07

pmorie force-pushed the container-env-resolution branch 2 times, most recently from 5b70258 to dc84fc5 Compare May 19, 2015 18:15

pmorie reviewed May 19, 2015
View reviewed changes

a-robinson reviewed May 19, 2015
View reviewed changes

pmorie force-pushed the container-env-resolution branch 2 times, most recently from 8c6f7de to be12662 Compare May 22, 2015 22:42

a-robinson reviewed May 23, 2015
View reviewed changes

pmorie force-pushed the container-env-resolution branch from be12662 to c247852 Compare May 26, 2015 15:23

a-robinson added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2015

a-robinson assigned a-robinson and unassigned derekwaynecarr May 26, 2015

pmorie mentioned this pull request May 26, 2015

Finalize Kube items openshift/origin#2233

Closed

9 tasks

pmorie removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 27, 2015

Make kubelet expand var refs in cmd, args, env

8b33886

pmorie force-pushed the container-env-resolution branch from c247852 to 8b33886 Compare May 27, 2015 04:14

a-robinson added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 27, 2015

thockin added a commit that referenced this pull request May 27, 2015

Merge pull request #8149 from pmorie/container-env-resolution

57df04e

Expand variables in containers' env, cmd, args

thockin merged commit 57df04e into kubernetes:master May 27, 2015

pmorie mentioned this pull request May 28, 2015

Support limited substitution syntax in env var values #7739

Closed

pmorie mentioned this pull request Jun 17, 2015

UPSTREAM: Variable expansion openshift/origin#3155

Merged

jim-minter mentioned this pull request Mar 14, 2019

etcd metrics shim openshift/openshift-azure#1176

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand variables in containers' env, cmd, args #8149

Expand variables in containers' env, cmd, args #8149

pmorie commented May 12, 2015

pmorie commented May 18, 2015

pmorie commented May 19, 2015

derekwaynecarr May 19, 2015

pmorie May 19, 2015

pmorie May 19, 2015

pmorie May 19, 2015

pmorie commented May 19, 2015

pmorie May 19, 2015

thockin May 19, 2015

smarterclayton May 19, 2015

a-robinson May 19, 2015

a-robinson May 19, 2015

derekwaynecarr May 20, 2015

pmorie commented May 19, 2015

a-robinson May 19, 2015

a-robinson May 23, 2015

a-robinson May 23, 2015

a-robinson commented May 23, 2015

pmorie commented May 24, 2015

pmorie commented May 26, 2015

a-robinson commented May 26, 2015

pmorie commented May 27, 2015

pmorie commented May 27, 2015

pmorie commented May 27, 2015

pmorie commented May 27, 2015

a-robinson commented May 27, 2015

pmorie commented May 27, 2015

a-robinson commented May 27, 2015

pmorie commented May 27, 2015

Expand variables in containers' env, cmd, args #8149

Expand variables in containers' env, cmd, args #8149

Conversation

pmorie commented May 12, 2015

pmorie commented May 18, 2015

pmorie commented May 19, 2015

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

pmorie commented May 19, 2015

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

pmorie commented May 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-robinson commented May 23, 2015

pmorie commented May 24, 2015

pmorie commented May 26, 2015

a-robinson commented May 26, 2015

pmorie commented May 27, 2015

pmorie commented May 27, 2015

pmorie commented May 27, 2015

pmorie commented May 27, 2015

a-robinson commented May 27, 2015

pmorie commented May 27, 2015

a-robinson commented May 27, 2015

pmorie commented May 27, 2015