Skip to content

Commit

Permalink
Add FieldRef support.
Browse files Browse the repository at this point in the history
  • Loading branch information
JRBANCEL committed Jun 10, 2020
1 parent 42e22ba commit 04cbe7e
Show file tree
Hide file tree
Showing 8 changed files with 287 additions and 76 deletions.
5 changes: 4 additions & 1 deletion config/core/configmaps/features.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ metadata:
labels:
serving.knative.dev/release: devel
annotations:
knative.dev/example-checksum: 84c3c6c3
knative.dev/example-checksum: f9125b41
data:
_example: |
################################
Expand All @@ -40,3 +40,6 @@ data:
# Indicates whether multi container support is enabled
multi-container: "disabled"
# Indicates whether Kubernetes FieldRef support is enabled
kubernetes/field-ref: "disabled"
10 changes: 7 additions & 3 deletions pkg/apis/config/features.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,18 @@ const (

func defaultFeaturesConfig() *Features {
return &Features{
MultiContainer: Disabled,
MultiContainer: Disabled,
KubernetesFieldRef: Disabled,
}
}

// NewFeaturesConfigFromMap creates a Features from the supplied Map
func NewFeaturesConfigFromMap(data map[string]string) (*Features, error) {
nc := defaultFeaturesConfig()

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 {
return nil, err
}
return nc, nil
Expand All @@ -57,7 +60,8 @@ func NewFeaturesConfigFromConfigMap(config *corev1.ConfigMap) (*Features, error)

// Features specifies which features are allowed by the webhook.
type Features struct {
MultiContainer Flag
MultiContainer Flag
KubernetesFieldRef Flag
}

// asFlag parses the value at key as a Flag into the target, if it exists.
Expand Down
58 changes: 54 additions & 4 deletions pkg/apis/config/features_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package config

import (
"reflect"
"testing"

"github.com/google/go-cmp/cmp"
Expand Down Expand Up @@ -57,21 +58,57 @@ func TestFeaturesConfiguration(t *testing.T) {
}, {
name: "multi-container Allowed",
wantErr: false,
wantFeatures: &Features{
wantFeatures: defaultWith(&Features{
MultiContainer: Allowed,
},
}),
data: map[string]string{
"multi-container": "Allowed",
},
}, {
name: "multi-container Enabled",
wantErr: false,
wantFeatures: &Features{
wantFeatures: defaultWith(&Features{
MultiContainer: Enabled,
},
}),
data: map[string]string{
"multi-container": "Enabled",
},
}, {
name: "multi-container Disabled",
wantErr: false,
wantFeatures: defaultWith(&Features{
MultiContainer: Disabled,
}),
data: map[string]string{
"multi-container": "Disabled",
},
}, {
name: "kubernetes/field-ref Allowed",
wantErr: false,
wantFeatures: defaultWith(&Features{
KubernetesFieldRef: Allowed,
}),
data: map[string]string{
"kubernetes/field-ref": "Allowed",
},
}, {
name: "kubernetes/field-ref Enabled",
wantErr: false,
wantFeatures: defaultWith(&Features{
KubernetesFieldRef: Enabled,
}),
data: map[string]string{
"kubernetes/field-ref": "Enabled",
},
}, {
name: "kubernetes/field-ref Disabled",
wantErr: false,
wantFeatures: defaultWith(&Features{
KubernetesFieldRef: Disabled,
}),
data: map[string]string{
"kubernetes/field-ref": "Disabled",
},
}}

for _, tt := range configTests {
Expand All @@ -91,3 +128,16 @@ func TestFeaturesConfiguration(t *testing.T) {
})
}
}

// defaultWith returns the default *Feature patched with the provided *Features.
func defaultWith(p *Features) *Features {
f := defaultFeaturesConfig()
pType := reflect.ValueOf(p).Elem()
fType := reflect.ValueOf(f).Elem()
for i := 0; i < pType.NumField(); i++ {
if pType.Field(i).Interface().(Flag) != "" {
fType.Field(i).Set(pType.Field(i))
}
}
return f
}
10 changes: 5 additions & 5 deletions pkg/apis/serving/fieldmask.go
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ func EnvVarMask(in *corev1.EnvVar) *corev1.EnvVar {
// EnvVarSourceMask performs a _shallow_ copy of the Kubernetes EnvVarSource object to a new
// Kubernetes EnvVarSource object bringing over only the fields allowed in the Knative API. This
// does not validate the contents or the bounds of the provided fields.
func EnvVarSourceMask(in *corev1.EnvVarSource) *corev1.EnvVarSource {
func EnvVarSourceMask(in *corev1.EnvVarSource, fieldRef bool) *corev1.EnvVarSource {
if in == nil {
return nil
}
Expand All @@ -383,10 +383,10 @@ func EnvVarSourceMask(in *corev1.EnvVarSource) *corev1.EnvVarSource {
out.ConfigMapKeyRef = in.ConfigMapKeyRef
out.SecretKeyRef = in.SecretKeyRef

// Disallowed
// This list is unnecessary, but added here for clarity
out.FieldRef = nil
out.ResourceFieldRef = nil
if fieldRef {
out.FieldRef = in.FieldRef
out.ResourceFieldRef = in.ResourceFieldRef
}

return out
}
Expand Down
78 changes: 53 additions & 25 deletions pkg/apis/serving/fieldmask_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -415,31 +415,59 @@ func TestEnvVarMask(t *testing.T) {
}

func TestEnvVarSourceMask(t *testing.T) {
want := &corev1.EnvVarSource{
ConfigMapKeyRef: &corev1.ConfigMapKeySelector{},
SecretKeyRef: &corev1.SecretKeySelector{},
}
in := &corev1.EnvVarSource{
ConfigMapKeyRef: &corev1.ConfigMapKeySelector{},
SecretKeyRef: &corev1.SecretKeySelector{},
FieldRef: &corev1.ObjectFieldSelector{},
ResourceFieldRef: &corev1.ResourceFieldSelector{},
}

got := EnvVarSourceMask(in)

if &want == &got {
t.Error("Input and output share addresses. Want different addresses")
}

if diff, err := kmp.SafeDiff(want, got); err != nil {
t.Errorf("Got error comparing output, err = %v", err)
} else if diff != "" {
t.Errorf("EnvVarSourceMask (-want, +got): %s", diff)
}

if got = EnvVarSourceMask(nil); got != nil {
t.Errorf("EnvVarSourceMask(nil) = %v, want: nil", got)
tests := []struct {
name string
fieldRef bool
in *corev1.EnvVarSource
want *corev1.EnvVarSource
}{{
name: "FieldRef false",
fieldRef: false,
in: &corev1.EnvVarSource{
ConfigMapKeyRef: &corev1.ConfigMapKeySelector{},
SecretKeyRef: &corev1.SecretKeySelector{},
FieldRef: &corev1.ObjectFieldSelector{},
ResourceFieldRef: &corev1.ResourceFieldSelector{},
},
want: &corev1.EnvVarSource{
ConfigMapKeyRef: &corev1.ConfigMapKeySelector{},
SecretKeyRef: &corev1.SecretKeySelector{},
},
}, {
name: "FieldRef true",
fieldRef: true,
in: &corev1.EnvVarSource{
ConfigMapKeyRef: &corev1.ConfigMapKeySelector{},
SecretKeyRef: &corev1.SecretKeySelector{},
FieldRef: &corev1.ObjectFieldSelector{},
ResourceFieldRef: &corev1.ResourceFieldSelector{},
},
want: &corev1.EnvVarSource{
ConfigMapKeyRef: &corev1.ConfigMapKeySelector{},
SecretKeyRef: &corev1.SecretKeySelector{},
FieldRef: &corev1.ObjectFieldSelector{},
ResourceFieldRef: &corev1.ResourceFieldSelector{},
},
}}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
got := EnvVarSourceMask(test.in, test.fieldRef)

if &test.want == &got {
t.Error("Input and output share addresses. Want different addresses")
}

if diff, err := kmp.SafeDiff(test.want, got); err != nil {
t.Errorf("Got error comparing output, err = %v", err)
} else if diff != "" {
t.Errorf("EnvVarSourceMask (-want, +got): %s", diff)
}

if got = EnvVarSourceMask(nil, test.fieldRef); got != nil {
t.Errorf("EnvVarSourceMask(nil) = %v, want: nil", got)
}
})
}
}

Expand Down
31 changes: 16 additions & 15 deletions pkg/apis/serving/k8s_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,14 +181,15 @@ func validateKeyToPath(k2p corev1.KeyToPath) *apis.FieldError {
return errs
}

func validateEnvValueFrom(source *corev1.EnvVarSource) *apis.FieldError {
func validateEnvValueFrom(ctx context.Context, source *corev1.EnvVarSource) *apis.FieldError {
if source == nil {
return nil
}
return apis.CheckDisallowedFields(*source, *EnvVarSourceMask(source))
features := config.FromContextOrDefaults(ctx).Features
return apis.CheckDisallowedFields(*source, *EnvVarSourceMask(source, features.KubernetesFieldRef != config.Disabled))
}

func validateEnvVar(env corev1.EnvVar) *apis.FieldError {
func validateEnvVar(ctx context.Context, env corev1.EnvVar) *apis.FieldError {
errs := apis.CheckDisallowedFields(env, *EnvVarMask(&env))

if env.Name == "" {
Expand All @@ -200,13 +201,13 @@ func validateEnvVar(env corev1.EnvVar) *apis.FieldError {
})
}

return errs.Also(validateEnvValueFrom(env.ValueFrom).ViaField("valueFrom"))
return errs.Also(validateEnvValueFrom(ctx, env.ValueFrom).ViaField("valueFrom"))
}

func validateEnv(envVars []corev1.EnvVar) *apis.FieldError {
func validateEnv(ctx context.Context, envVars []corev1.EnvVar) *apis.FieldError {
var errs *apis.FieldError
for i, env := range envVars {
errs = errs.Also(validateEnvVar(env).ViaIndex(i))
errs = errs.Also(validateEnvVar(ctx, env).ViaIndex(i))
}
return errs
}
Expand Down Expand Up @@ -257,7 +258,7 @@ func ValidatePodSpec(ctx context.Context, ps corev1.PodSpec) *apis.FieldError {
case 0:
errs = errs.Also(apis.ErrMissingField("containers"))
case 1:
errs = errs.Also(ValidateContainer(ps.Containers[0], volumes).
errs = errs.Also(ValidateContainer(ctx, ps.Containers[0], volumes).
ViaFieldIndex("containers", 0))
default:
errs = errs.Also(validateContainers(ctx, ps.Containers, volumes))
Expand All @@ -282,9 +283,9 @@ func validateContainers(ctx context.Context, containers []corev1.Container, volu
// Probes are not allowed on other than serving container,
// ref: http://bit.ly/probes-condition
if len(containers[i].Ports) == 0 {
errs = errs.Also(validateSidecarContainer(containers[i], volumes).ViaFieldIndex("containers", i))
errs = errs.Also(validateSidecarContainer(ctx, containers[i], volumes).ViaFieldIndex("containers", i))
} else {
errs = errs.Also(ValidateContainer(containers[i], volumes).ViaFieldIndex("containers", i))
errs = errs.Also(ValidateContainer(ctx, containers[i], volumes).ViaFieldIndex("containers", i))
}
}
}
Expand All @@ -309,7 +310,7 @@ func validateContainersPorts(containers []corev1.Container) *apis.FieldError {
}

// validateSidecarContainer validate fields for non serving containers
func validateSidecarContainer(container corev1.Container, volumes sets.String) *apis.FieldError {
func validateSidecarContainer(ctx context.Context, container corev1.Container, volumes sets.String) *apis.FieldError {
var errs *apis.FieldError
if container.LivenessProbe != nil {
errs = errs.Also(apis.CheckDisallowedFields(*container.LivenessProbe,
Expand All @@ -319,19 +320,19 @@ func validateSidecarContainer(container corev1.Container, volumes sets.String) *
errs = errs.Also(apis.CheckDisallowedFields(*container.ReadinessProbe,
*ProbeMask(&corev1.Probe{})).ViaField("readinessProbe"))
}
return errs.Also(validate(container, volumes))
return errs.Also(validate(ctx, container, volumes))
}

// ValidateContainer validate fields for serving containers
func ValidateContainer(container corev1.Container, volumes sets.String) *apis.FieldError {
func ValidateContainer(ctx context.Context, container corev1.Container, volumes sets.String) *apis.FieldError {
var errs *apis.FieldError
// Single container cannot have multiple ports
errs = errs.Also(portValidation(container.Ports).ViaField("ports"))
// Liveness Probes
errs = errs.Also(validateProbe(container.LivenessProbe).ViaField("livenessProbe"))
// Readiness Probes
errs = errs.Also(validateReadinessProbe(container.ReadinessProbe).ViaField("readinessProbe"))
return errs.Also(validate(container, volumes))
return errs.Also(validate(ctx, container, volumes))
}

func portValidation(containerPorts []corev1.ContainerPort) *apis.FieldError {
Expand All @@ -345,7 +346,7 @@ func portValidation(containerPorts []corev1.ContainerPort) *apis.FieldError {
return nil
}

func validate(container corev1.Container, volumes sets.String) *apis.FieldError {
func validate(ctx context.Context, container corev1.Container, volumes sets.String) *apis.FieldError {
if equality.Semantic.DeepEqual(container, corev1.Container{}) {
return apis.ErrMissingField(apis.CurrentField)
}
Expand All @@ -360,7 +361,7 @@ func validate(container corev1.Container, volumes sets.String) *apis.FieldError
}

// Env
errs = errs.Also(validateEnv(container.Env).ViaField("env"))
errs = errs.Also(validateEnv(ctx, container.Env).ViaField("env"))
// EnvFrom
errs = errs.Also(validateEnvFrom(container.EnvFrom).ViaField("envFrom"))
// Image
Expand Down
Loading

0 comments on commit 04cbe7e

Please sign in to comment.