Env var sources / downward API #6739

pmorie · 2015-04-12T06:26:12Z

As discussed in #6000 and #2316, relevant also to #386.

googlebot · 2015-04-12T06:26:14Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

pmorie · 2015-04-12T06:29:39Z

@bgrant0607 @smarterclayton @erictune @thockin @derekwaynecarr @mfojtik PTAL.

Preliminary WIP for comment. I don't think there's any controversy about the concepts here, based on the last community hangout. Looking for feedback on the factorings.

smarterclayton · 2015-04-13T13:48:35Z

pkg/kubelet/kubelet.go

+		switch {
+		case envVar.ObjectInfo != nil:
+			objInfo := envVar.ObjectInfo
+			internalFieldPath, _, err := api.Scheme.ConvertFieldLabel(objInfo.APIVersion, "Pod", objInfo.FieldPath, "")


add the TODO: use Kind from ObjectInfo so it doesn't confuse people looking at this.

pmorie · 2015-04-13T15:21:58Z

Taking this a little further today in terms of factoring within the kubelet. If anyone would like info added to the skeletal API, speak out.

I'll making the versioned API changes once people have a chance to chime in here.

pmorie · 2015-04-13T15:26:44Z

@sdminonne fyi

sdminonne · 2015-04-13T15:41:01Z

@pmorie thanks

derekwaynecarr · 2015-04-13T19:32:29Z

@pmorie - my first impression is that I am good with this approach. my initial reaction was unsure if i liked the metadata.* syntax, but I have no other good alternative in mind so that lgtm.

ironcladlou · 2015-04-14T14:34:34Z

Is there any reason to place security restrictions on the host source? Is it okay that any user can access host information via this mechanism? Right now it's limited to hostname, but I assume the set of fields will increase over time.

pmorie · 2015-04-14T19:27:10Z

pkg/api/serialization_test.go

@@ -164,6 +164,20 @@ func TestEncode_Ptr(t *testing.T) {
 		Spec: api.PodSpec{
 			RestartPolicy: api.RestartPolicyAlways,
 			DNSPolicy:     api.DNSClusterFirst,
+			Containers: []api.Container{


Debugging cruft; will remove

pmorie · 2015-04-14T20:36:22Z

@bgrant0607 This is shaping up; what supplemental stuff does it need to go in with? I can think of:

Pass through the examples to see where the downward API can be used instead
Docs
E2E test

@smarterclayton @thockin @derekwaynecarr any opinions on the above?

bgrant0607 · 2015-04-14T20:55:04Z

pkg/api/v1beta3/types.go

@@ -509,6 +509,33 @@ type EnvVar struct {
 	Name string `json:"name" description:"name of the environment variable; must be a C_IDENTIFIER"`
 	// Optional: defaults to "".
 	Value string `json:"value,omitempty" description:"value of the environment variable; defaults to empty string"`
+	// Optional: specify a source for the value of the EnvVar.
+	*EnvVarSource `json:"source,omitempty" description:"source for the environment variable's value; cannot be used if value is set"`


A pointer to an embedded struct is confusing. It needs to not be embedded, which would make sense because the json isn't embedded.

Re. the description: We can't tell whether value is "set", only whether it is empty.

@bgrant0607 Are you suggesting instead:

// EnvVar represents an environment variable present in a Container. type EnvVar struct { // Required: This must be a C_IDENTIFIER. Name string `json:"name" description:"name of the environment variable; must be a C_IDENTIFIER"` // Optional: defaults to "". Value string `json:"value,omitempty" description:"value of the environment variable; defaults to empty string"` // Optional; ObjectInfo represents a downward API request for a field of the object. // // Valid fields of this reference are: // // 1. APIVersion // 2. FieldPath // // No other fields may be specified. // // Currently ObjectInfo may be used to address the Name and Namespace // only. ObjectInfo *ObjectReference `json:"objectInfo,omitempty" description:"describes a field of the pod to use the value of; only APIVersion and FieldPath are accepted fields"` // Optional; HostInfo represents a downward API request for information about // the host. HostInfo *HostInfoSource `json:"hostInfo,omitempty" description:"describes information about the host to use"` }

@bgrant0607 @smarterclayton I would be okay with having ObjectInfo become a field of EnvVar -- it would simplify the API greatly.

If we think there may be multiple sources in the future, then I prefer the extra level of nesting. I was objecting to the specific combination of a pointer to an anonymous Go struct. Name the field Source.

I'm fine with that.

Perhaps we should call it fieldValue, though, rather than objectInfo, to strengthen the association with value.

What do we want the JSON to look like?

{ "name": "myvar", "source": { "objectInfo": { "fieldPath": ".metadata.name" } } }

or

{ "name": "myvar", "objectInfo": { "fieldPath": ".metadata.name" } }

or

{ "name": "myvar", "value": { "objectInfo": {"fieldPath": ".metadata.name" }} }

If I parse this correctly it's #2 that we are getting, yes?

What about calling the sources something like "FromPod" or "ValueFromPod" ?

@thockin Either of those names is more descriptive -- unless we feel like we want open-ended info about objects, which I don't think we do.

pmorie · 2015-04-16T02:07:34Z

Gonna go with this factoring in my next pass unless someone speaks soon:

// EnvVar represents an environment variable present in a Container.
type EnvVar struct {
    // Required: This must be a C_IDENTIFIER.
    Name string `json:"name" description:"name of the environment variable; must be a C_IDENTIFIER"`
    // Optional: defaults to "".
    Value string `json:"value,omitempty" description:"value of the environment variable; defaults to empty string"`
    // Optional; ObjectInfo represents a downward API request for a field of the object.
    //
    // Valid fields of this reference are:
    //
    // 1. APIVersion
    // 2. FieldPath
    //
    // No other fields may be specified.
    //
    // Currently ObjectInfo may be used to address the Name and Namespace
    // only.
    ObjectInfo *ObjectReference `json:"objectInfo,omitempty" description:"describes a field of the pod to use the value of; only APIVersion and FieldPath are accepted fields"`
}

Scratching HostInfo, we can add it back in later if there's a need.

thockin · 2015-04-16T03:44:01Z

pkg/api/types.go

+	// 1. APIVersion
+	// 2. FieldPath
+	//
+	// No other fields may be specified.


This feels like an abuse of an existing type. Why not just define your own type?

pmorie · 2015-04-27T15:23:27Z

@bgrant0607 All comments should be fixed now

smarterclayton · 2015-04-27T15:35:56Z

So now that this awesome feature is almost done... how do I choose my own names for environment variables for services?

pmorie · 2015-04-27T15:39:18Z

@smarterclayton We can implement a new env var source for that info

smarterclayton · 2015-04-27T15:41:08Z

Is it a different source? Seems like it's the spec.portalIP and spec.port[0] field of a service with a given name and namespace.

pmorie · 2015-04-27T16:02:05Z

@smarterclayton

with a given name and namespace.

Wouldn't it just be the name, since we don't support cross-ns services?

smarterclayton · 2015-04-27T16:09:29Z

It could be eventually. But yes, for now, just name.

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

@smarterclayton

with a given name and namespace.

Wouldn't it just be the name, since we don't support cross-ns services?

Reply to this email directly or view it on GitHub:
#6739 (comment)

pmorie · 2015-04-27T16:16:00Z

@smarterclayton That sounds to me like:

type EnvVarSource struct {
  FieldPath        *ObjectFieldSelector
  ServiceFieldPath *NamedObjectFieldSelector
}

type NamedObjectFieldSelector {
    ObjectFieldSelector

    Name string
}

pmorie · 2015-04-27T16:16:31Z

Kicked shippable

smarterclayton · 2015-04-27T16:17:05Z

And then we say... why do we have ServiceFieldPath and FieldPath together?

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

@smarterclayton That sounds to me like:

type EnvVarSource struct {
  FieldPath        *ObjectFieldSelector
  ServiceFieldPath *NamedObjectFieldSelector
}

type NamedObjectFieldSelector {
  ObjectFieldSelector

  Name string
}

Reply to this email directly or view it on GitHub:
#6739 (comment)

pmorie · 2015-04-27T16:17:47Z

@smarterclayton this would definitely lurch us in the direction of ObjectReference

smarterclayton · 2015-04-27T16:18:59Z

Yes. I'm not proposing any changes for this pull (since the types are the same) but I realized we got really focused on pod name/namespace. The follow-on will need to deal with the implications.

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

@smarterclayton this would definitely lurch us in the direction of
ObjectReference

Reply to this email directly or view it on GitHub:
#6739 (comment)

pmorie · 2015-04-27T16:20:02Z

@smarterclayton Agree, I will make a new issue for service downward API

thockin · 2015-04-27T16:26:05Z

Services wanders into a whole different discussion - the pre-declaring
service-deps topic. I don't yet have an opinion as to whether we should
have an env var source way to do this, but I do think it is more
complicated that you have listed here.

Service have multiple ports - do I have to call out each one I want? How
do I express named ports, rather than indexed? etc.

On Mon, Apr 27, 2015 at 9:20 AM, Paul Morie notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton Agree, I will make a
new issue for service downward API

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

pmorie · 2015-04-27T16:28:20Z

@thockin I am looking at the downward API for service envs as orthogonal to pre-declaring service dependencies. I think dependencies will take pre-start hooks and should be separate in the API from how the vars are constructed.

smarterclayton · 2015-04-27T16:28:44Z

Exactly. But exposing services via this mechanism is strictly better in my opinion than making up a bunch of env vars (making up one env var I can get behind, just not 100 different variants).

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

Services wanders into a whole different discussion - the pre-declaring
service-deps topic. I don't yet have an opinion as to whether we should
have an env var source way to do this, but I do think it is more
complicated that you have listed here.

Service have multiple ports - do I have to call out each one I want? How
do I express named ports, rather than indexed? etc.

On Mon, Apr 27, 2015 at 9:20 AM, Paul Morie notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton Agree, I will make a
new issue for service downward API

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

Reply to this email directly or view it on GitHub:
#6739 (comment)

pmorie · 2015-04-27T18:15:48Z

Ugh. Rebasing

pmorie · 2015-04-27T18:43:33Z

Rebased

pmorie · 2015-04-27T19:01:41Z

Local e2e run looks good.

pmorie · 2015-04-27T19:01:54Z

@bgrant0607 Does this need anything else?

bgrant0607 · 2015-04-27T19:43:18Z

pkg/api/v1beta1/defaults.go

@@ -166,6 +166,11 @@ func init() {
 				obj.ExternalID = obj.ID
 			}
 		},
+		func(obj *ObjectFieldSelector) {


Did you confirm that this isn't called when the pointer is nil?

bgrant0607 · 2015-04-27T19:45:57Z

The ObjectFieldSelector field descriptions didn't get changed as I asked, but my review latency is very high right now, so I'm not going to block on that.

LGTM.

Env var sources / downward API

pmorie · 2015-04-27T19:53:19Z

@bgrant0607 Dang, I thought I had absolutely everything. I will fix them in a follow-up.

googlebot added the cla: no label Apr 12, 2015

thockin assigned bgrant0607 Apr 13, 2015

smarterclayton reviewed Apr 13, 2015
View reviewed changes

pmorie force-pushed the downward-api branch from f830755 to 6c1d9d0 Compare April 13, 2015 16:09

pmorie force-pushed the downward-api branch from 6c1d9d0 to 3c74773 Compare April 14, 2015 06:34

pmorie reviewed Apr 14, 2015
View reviewed changes

bgrant0607 reviewed Apr 14, 2015
View reviewed changes

thockin reviewed Apr 16, 2015
View reviewed changes

thockin mentioned this pull request Apr 16, 2015

downward api volume plugin #5093

Merged

pmorie force-pushed the downward-api branch from 471a41b to c0d8fb8 Compare April 17, 2015 14:27

pmorie force-pushed the downward-api branch from 2202b39 to 3112fc2 Compare April 27, 2015 15:15

This was referenced Apr 27, 2015

Add KUBERNETES_NAMESPACE variable to Pod containers #6354

Closed

Downward API for service environment vars #7365

Closed

Add downward API for environment vars

7d30f09

pmorie force-pushed the downward-api branch from 3112fc2 to 7d30f09 Compare April 27, 2015 18:42

bgrant0607 reviewed Apr 27, 2015
View reviewed changes

bgrant0607 added a commit that referenced this pull request Apr 27, 2015

Merge pull request #6739 from pmorie/downward-api

2648ae8

Env var sources / downward API

bgrant0607 merged commit 2648ae8 into kubernetes:master Apr 27, 2015

pmorie mentioned this pull request May 1, 2015

Rename EnvVarSource.FieldPath -> FieldRef and add example #7592

Merged

Env var sources / downward API #6739

Env var sources / downward API #6739

Conversation

pmorie commented Apr 12, 2015

googlebot commented Apr 12, 2015

pmorie commented Apr 12, 2015

Choose a reason for hiding this comment

pmorie commented Apr 13, 2015

pmorie commented Apr 13, 2015

sdminonne commented Apr 13, 2015

derekwaynecarr commented Apr 13, 2015

ironcladlou commented Apr 14, 2015

Choose a reason for hiding this comment

pmorie commented Apr 14, 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

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 Apr 16, 2015

Choose a reason for hiding this comment

pmorie commented Apr 27, 2015

smarterclayton commented Apr 27, 2015

pmorie commented Apr 27, 2015

smarterclayton commented Apr 27, 2015

pmorie commented Apr 27, 2015

smarterclayton commented Apr 27, 2015

pmorie commented Apr 27, 2015

pmorie commented Apr 27, 2015

smarterclayton commented Apr 27, 2015

pmorie commented Apr 27, 2015

smarterclayton commented Apr 27, 2015

pmorie commented Apr 27, 2015

thockin commented Apr 27, 2015

pmorie commented Apr 27, 2015

smarterclayton commented Apr 27, 2015

pmorie commented Apr 27, 2015

pmorie commented Apr 27, 2015

pmorie commented Apr 27, 2015

pmorie commented Apr 27, 2015

Choose a reason for hiding this comment

bgrant0607 commented Apr 27, 2015

pmorie commented Apr 27, 2015