Add FieldRef support #8126

JRBANCEL · 2020-05-29T01:18:50Z

Addresses part of #4190

Proposed Changes

Introduce a Feature Flag kubernetes/field-ref (in config-features)
When that flag is enabled, a Knative Service can specify spec.template.spec.containers[].env[].valueFrom.fieldRef and spec.template.spec.containers[].env[].valueFrom.resourceFieldRef

Open questions

I am not 100% happy with the naming and scoping. I think it makes sense to scope features to the specific platform: e.g. multi-container is not platform specific, therefore not scoped, kubernetes/field-ref is specific to Kubernetes, so scoped to kubernetes. Should the flag in the code also be scoped? Right now it is called KubernetesFieldRef but since the code is specific to the implementation, wouldn't just FieldRef be cleaner?

Addresses part of #4190

Proposed Changes

Introduce a Feature Flag kubernetes/field-ref (in config-features)

When that flag is enabled, a Knative Service can specify spec.template.spec.containers[].env[].valueFrom.fieldRef and spec.template.spec.containers[].env[].valueFrom.resourceFieldRef

Open questions

I am not 100% happy with the naming and scoping. I think it makes sense to scope features to the specific platform: e.g. multi-container is not platform specific, therefore not scoped, kubernetes/field-ref is specific to Kubernetes, so scoped to kubernetes. Should the flag in the code also be scoped? Right now it is called KubernetesFieldRef but since the code is specific to the implementation, wouldn't just FieldRef be cleaner?

Next

Add E2E tests

Add documentation

/assign @dprotaso @mattmoor

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.

JRBANCEL · 2020-06-03T17:43:20Z

/ping

vagababov

/lint
Mostly looks good to me.

vagababov · 2020-06-03T17:52:54Z

pkg/apis/config/features_test.go

@@ -91,3 +128,16 @@ func TestFeaturesConfiguration(t *testing.T) {
 		})
 	}
 }
+
+// defaultWith returns the default *Feature patched with the provided *Features.
+func defaultWith(p *Features) *Features {


this is pure evil
😈

vagababov · 2020-06-03T17:53:23Z

pkg/apis/config/features_test.go

+	pType := reflect.ValueOf(p).Elem()
+	fType := reflect.ValueOf(f).Elem()
+	for i := 0; i < pType.NumField(); i++ {
+		if pType.Field(i).Interface().(Flag) != "" {


Are we sure no other types of values will be added? This is test, so 🤷 , but stil.

We can check the type assertion and fail but that doesn't help much compared to failing directly with a panic. It is for test only, I don't think we care.

knative-prow-robot

@vagababov: 0 warnings.

In response to this:

/lint
Mostly looks good to me.

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.

JRBANCEL · 2020-06-08T17:25:25Z

/ping @dprotaso @mattmoor

mattmoor

This generally LGTM. It needs a rebase, and not sure if @dprotaso has comments?

knative-metrics-robot · 2020-06-10T21:11:23Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/fieldmask.go	97.9%	97.9%	0.0
pkg/apis/serving/k8s_validation.go	99.2%	99.2%	0.0

dprotaso

/lgtm
/approve

Not going to block on the config map field formatting - hopefully we can come to a consensus before the release

dprotaso · 2020-06-11T17:26:24Z

pkg/apis/config/features.go

-	if err := cm.Parse(data, asFlag("multi-container", &nc.MultiContainer)); err != nil {
+	if err := cm.Parse(data,
+		asFlag("multi-container", &nc.MultiContainer),
+		asFlag("kubernetes/field-ref", &nc.KubernetesFieldRef)); err != nil {


note: there's a discussion about the format of these fields

https://knative.slack.com/archives/CU9DGL4FM/p1591710914146300

dprotaso

/hold
minor nit - drop if you want to do it in a later PR

dprotaso · 2020-06-11T17:54:34Z

pkg/apis/serving/k8s_validation_test.go

+					Name: "NODE_IP",
+					ValueFrom: &corev1.EnvVarSource{
+						FieldRef: &corev1.ObjectFieldSelector{
+							FieldPath: "status.IP",


Suggested change

FieldPath: "status.IP",

FieldPath: "status.hostIP",

dprotaso · 2020-06-11T17:54:48Z

pkg/apis/serving/k8s_validation_test.go

+					Name: "NODE_IP",
+					ValueFrom: &corev1.EnvVarSource{
+						FieldRef: &corev1.ObjectFieldSelector{
+							FieldPath: "status.IP",


Suggested change

FieldPath: "status.IP",

FieldPath: "status.hostIP",

knative-prow-robot · 2020-06-11T17:55:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, JRBANCEL

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:

~~config/OWNERS~~ [dprotaso]
~~pkg/apis/OWNERS~~ [dprotaso]

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

dprotaso · 2020-06-11T17:56:39Z

/hold cancel

knative-prow-robot assigned dprotaso and mattmoor May 29, 2020

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

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 29, 2020

knative-prow-robot requested review from evankanderson and grantr May 29, 2020 01:19

knative-prow-robot reviewed May 29, 2020

View reviewed changes

knative-prow-robot added the area/API API objects and controllers label May 29, 2020

evankanderson removed their request for review June 2, 2020 17:04

vagababov reviewed Jun 3, 2020

View reviewed changes

knative-prow-robot reviewed Jun 3, 2020

View reviewed changes

JRBANCEL mentioned this pull request Jun 9, 2020

Guard PodSpec dry run with a feature flag #8276

Merged

mattmoor reviewed Jun 10, 2020

View reviewed changes

Add FieldRef support.

04cbe7e

JRBANCEL force-pushed the downward-api branch from 036a82b to 04cbe7e Compare June 10, 2020 21:04

dprotaso approved these changes Jun 11, 2020

View reviewed changes

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 11, 2020

dprotaso approved these changes Jun 11, 2020

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 11, 2020

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 11, 2020

knative-prow-robot merged commit 7a6a278 into knative:master Jun 11, 2020

dprotaso mentioned this pull request Jun 22, 2020

Use valid FieldRef in test #8405

Merged

ianlewis mentioned this pull request Aug 14, 2020

knative workloads google/gvisor#3634

Closed

JRBANCEL mentioned this pull request Sep 28, 2020

Feature Request: Relax volume constraint to support more volume types #4417

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FieldRef support #8126

Add FieldRef support #8126

JRBANCEL commented May 29, 2020

knative-prow-robot left a comment

JRBANCEL commented Jun 3, 2020

vagababov left a comment

vagababov Jun 3, 2020

vagababov Jun 3, 2020

JRBANCEL Jun 3, 2020

knative-prow-robot left a comment

JRBANCEL commented Jun 8, 2020

mattmoor left a comment

knative-metrics-robot commented Jun 10, 2020

dprotaso left a comment

dprotaso Jun 11, 2020

dprotaso left a comment

dprotaso Jun 11, 2020

dprotaso Jun 11, 2020

knative-prow-robot commented Jun 11, 2020

dprotaso commented Jun 11, 2020

Add FieldRef support #8126

Add FieldRef support #8126

Conversation

JRBANCEL commented May 29, 2020

Proposed Changes

Open questions

Next

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Open questions

Next

JRBANCEL commented Jun 3, 2020

vagababov left a comment

Choose a reason for hiding this comment

vagababov Jun 3, 2020

Choose a reason for hiding this comment

vagababov Jun 3, 2020

Choose a reason for hiding this comment

JRBANCEL Jun 3, 2020

Choose a reason for hiding this comment

knative-prow-robot left a comment

Choose a reason for hiding this comment

JRBANCEL commented Jun 8, 2020

mattmoor left a comment

Choose a reason for hiding this comment

knative-metrics-robot commented Jun 10, 2020

dprotaso left a comment

Choose a reason for hiding this comment

dprotaso Jun 11, 2020

Choose a reason for hiding this comment

dprotaso left a comment

Choose a reason for hiding this comment

dprotaso Jun 11, 2020

Choose a reason for hiding this comment

dprotaso Jun 11, 2020

Choose a reason for hiding this comment

knative-prow-robot commented Jun 11, 2020

dprotaso commented Jun 11, 2020