-
Notifications
You must be signed in to change notification settings - Fork 40k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Env var sources / downward API #6739
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -397,6 +397,23 @@ type EnvVar struct { | |
Key string `json:"key,omitempty" description:"name of the environment variable; must be a C_IDENTIFIER; deprecated - use name instead"` | ||
// Optional: defaults to "". | ||
Value string `json:"value,omitempty" description:"value of the environment variable; defaults to empty string"` | ||
// Optional: specify a source the value of this var should come from. | ||
ValueFrom *EnvVarSource `json:"valueFrom,omitempty" description:"source for the environment variable's value; cannot be used if value is not empty"` | ||
} | ||
|
||
// EnvVarSource represents a source for the value of an EnvVar. | ||
// Only one of its members may be specified. | ||
type EnvVarSource struct { | ||
// Selects a field of the pod; only name and namespace are supported. | ||
FieldPath *ObjectFieldSelector `json:"fieldPath,omitempty" description:"selects a field of the pod; only name and namespace are supported"` | ||
} | ||
|
||
// ObjectFieldSelector selects an APIVersioned field of an object. | ||
type ObjectFieldSelector struct { | ||
// The API version the FieldPath is written in terms of. | ||
APIVersion string `json:"apiVersion,omitempty" description="The API version that FieldPath is written in terms of"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is fine for now - it's not your fault/problem, but I want to be on record that asking the user to spec an API version inside an object that is already API-versioned is asinine. We should endeavor to do better than this in API v2. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same problem as ObjectReference, validation errors, field selectors, and other field references. Sorry I missed the intervening discussion (too much going on), but podField and ObjectFieldSelector seem inelegant. ObjectReference was sufficient and more extensible. I understand that it seems overly general right now, but if we wanted more specific, I'd rather scrap both EnvVarSource and ObjectFieldSelector and just put ValueFromField in EnvVar. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. regarding ObjectRef vs ObjectFieldSelector, here's my thinking. It is a strict subset of ObejctRef. We should not reuse struct just because it exists. If it grows to be identical to ObjectRef, we can swap in that struct at will, and it will be compatible. We would not add fields to a struct just because we might need them. I feel we should be doing this MORE - use purpose-built structs that fit exactly what we need. The struct name is not really even part of the API. That's the last I'll say, if Brian wants to use Objectref, just do that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bgrant0607 is
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you want to use ObjectFieldSelector, document that it must be a strict subset of ObjectReference. My bigger beef is with the field name: podField. I'm 99% certain that we're going to need to be able to substitute fields of other objects. In the case that we support that in the API, podField is awkward. In the case that we push cross-object substitution to the client, podField seems both over- and under-specified (of course it must be a pod, but it can't be another pod). I'll try to provide a more constructive suggestion later tonight. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the argument against just "field"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. description nits: Delete leading "The" and refer to the json field names, not the Go struct names (so, fieldPath rather than FieldPath). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ping (here and other API versions) |
||
// The path of the field to select in the specified API version | ||
FieldPath string `json:"fieldPath,omitempty" description="The path of the field to select in the specified API version"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to document the format of this somewhere There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to documentation. For starters, an example in the description Delete leading "The". FieldPath isn't really optional. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ping (here and other API versions) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gah, |
||
} | ||
|
||
// HTTPGetAction describes an action based on HTTP Get requests. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1794,6 +1794,23 @@ func init() { | |
out.Path = in.Path | ||
return nil | ||
}, | ||
func(in *EnvVar, out *newer.EnvVar, s conversion.Scope) error { | ||
out.Name = in.Name | ||
out.Value = in.Value | ||
if err := s.Convert(&in.ValueFrom, &out.ValueFrom, 0); err != nil { | ||
return err | ||
} | ||
|
||
return nil | ||
}, | ||
func(in *newer.EnvVar, out *EnvVar, s conversion.Scope) error { | ||
out.Name = in.Name | ||
out.Value = in.Value | ||
if err := s.Convert(&in.ValueFrom, &out.ValueFrom, 0); err != nil { | ||
return err | ||
} | ||
return nil | ||
}, | ||
func(in *PodSpec, out *newer.PodSpec, s conversion.Scope) error { | ||
if in.Volumes != nil { | ||
out.Volumes = make([]newer.Volume, len(in.Volumes)) | ||
|
@@ -2715,6 +2732,7 @@ func init() { | |
func(label, value string) (string, string, error) { | ||
switch label { | ||
case "metadata.name", | ||
"metadata.namespace", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm resigned to the fact that starting with this syntax is the most expedient thing to do. We can reevaluate later. In the long run, we should strive to make all field references the same:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Forget I said anything. :-) I'd like to get this in. |
||
"status.phase", | ||
"spec.host": | ||
return label, value, nil | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -555,15 +555,58 @@ func validateEnv(vars []api.EnvVar) errs.ValidationErrorList { | |
vErrs := errs.ValidationErrorList{} | ||
if len(ev.Name) == 0 { | ||
vErrs = append(vErrs, errs.NewFieldRequired("name")) | ||
} | ||
if !util.IsCIdentifier(ev.Name) { | ||
} else if !util.IsCIdentifier(ev.Name) { | ||
vErrs = append(vErrs, errs.NewFieldInvalid("name", ev.Name, cIdentifierErrorMsg)) | ||
} | ||
vErrs = append(vErrs, validateEnvVarValueFrom(ev).Prefix("valueFrom")...) | ||
allErrs = append(allErrs, vErrs.PrefixIndex(i)...) | ||
} | ||
return allErrs | ||
} | ||
|
||
func validateEnvVarValueFrom(ev api.EnvVar) errs.ValidationErrorList { | ||
allErrs := errs.ValidationErrorList{} | ||
|
||
if ev.ValueFrom == nil { | ||
return allErrs | ||
} | ||
|
||
numSources := 0 | ||
|
||
switch { | ||
case ev.ValueFrom.FieldPath != nil: | ||
numSources++ | ||
allErrs = append(allErrs, validateObjectFieldSelector(ev.ValueFrom.FieldPath).Prefix("fieldPath")...) | ||
} | ||
|
||
if ev.Value != "" && numSources != 0 { | ||
allErrs = append(allErrs, errs.NewFieldInvalid("", "", "sources cannot be specified when value is not empty")) | ||
} | ||
|
||
return allErrs | ||
} | ||
|
||
var validFieldPathExpressions = util.NewStringSet("metadata.name", "metadata.namespace") | ||
|
||
func validateObjectFieldSelector(fs *api.ObjectFieldSelector) errs.ValidationErrorList { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At what point do we validate these are legitimate values? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @thockin At this point, that happens in the kubelet. I don't think that's great. Let me think on this a bit more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems totally feasible to validate them in apiserver even if expanded by Kubelet. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The user would prefer to get errors as early as possible. The plugin wouldn't have to be compiled into the apiserver or kubectl in order to enable that; we could use swagger. |
||
allErrs := errs.ValidationErrorList{} | ||
|
||
if fs.APIVersion == "" { | ||
allErrs = append(allErrs, errs.NewFieldRequired("apiVersion")) | ||
} else if fs.FieldPath == "" { | ||
allErrs = append(allErrs, errs.NewFieldRequired("fieldPath")) | ||
} else { | ||
internalFieldPath, _, err := api.Scheme.ConvertFieldLabel(fs.APIVersion, "Pod", fs.FieldPath, "") | ||
if err != nil { | ||
allErrs = append(allErrs, errs.NewFieldInvalid("fieldPath", fs.FieldPath, "error converting fieldPath")) | ||
} else if !validFieldPathExpressions.Has(internalFieldPath) { | ||
allErrs = append(allErrs, errs.NewFieldNotSupported("fieldPath", internalFieldPath)) | ||
} | ||
} | ||
|
||
return allErrs | ||
} | ||
|
||
func validateVolumeMounts(mounts []api.VolumeMount, volumes util.StringSet) errs.ValidationErrorList { | ||
allErrs := errs.ValidationErrorList{} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you confirm that this isn't called when the pointer is nil?