What happened? Applied CRD: <div class="highlight highlight-so

Not only duration format but others as well: <p d

<a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/us

ValidatingAdmissionPolicy objects have different runtime type compared to CRDValidationRules about kubernetes HOT 13 OPEN

alexzielenski commented on September 28, 2024

ValidatingAdmissionPolicy objects have different runtime type compared to CRDValidationRules

from kubernetes.

Comments (13)

alexzielenski commented on September 28, 2024

/sig api-machinery

from kubernetes.

jpbetz commented on September 28, 2024

We have schemas at runtime don't we? So even though we don't compile, we should be able to run unstructured through UnstructuredToVal and get a correctly typed runtime object?

As a short term workaround, duration(object.duration) >= duration(\"2h\") should work.

from kubernetes.

alexzielenski commented on September 28, 2024

Not only duration format but others as well:

kubernetes/staging/src/k8s.io/apiserver/pkg/cel/common/values.go

Lines 132 to 162 in c6b3a2f

 str, ok := unstructured.(string) 

 if !ok { 

 return types.NewErr("invalid data, expected string, got %T", unstructured) 

 } 

 switch schema.Format() { 

 case "duration": 

 d, err := strfmt.ParseDuration(str) 

 if err != nil { 

 return types.NewErr("Invalid duration %s: %v", str, err) 

 } 

 return types.Duration{Duration: d} 

 case "date": 

 d, err := time.Parse(strfmt.RFC3339FullDate, str) // strfmt uses this format for OpenAPIv3 value validation 

 if err != nil { 

 return types.NewErr("Invalid date formatted string %s: %v", str, err) 

 } 

 return types.Timestamp{Time: d} 

 case "date-time": 

 d, err := strfmt.ParseDateTime(str) 

 if err != nil { 

 return types.NewErr("Invalid date-time formatted string %s: %v", str, err) 

 } 

 return types.Timestamp{Time: time.Time(d)} 

 case "byte": 

 base64 := strfmt.Base64{} 

 err := base64.UnmarshalText([]byte(str)) 

 if err != nil { 

 return types.NewErr("Invalid byte formatted string %s: %v", str, err) 

 } 

 return types.Bytes(base64) 

 }

In addition to these, there seem to be different types for maplist and setlist, causing subtle differences CEL for how expressions operate on these values:

kubernetes/staging/src/k8s.io/apiserver/pkg/cel/common/values.go

Lines 105 to 129 in c6b3a2f

 if schema.Type() == "array" { 

 l, ok := unstructured.([]interface{}) 

 if !ok { 

 return types.NewErr("invalid data, expected an array for the provided schema with type=array") 

 } 

 if schema.Items() == nil { 

 return types.NewErr("invalid array type, expected Items with a non-empty Schema") 

 } 

 typedList := unstructuredList{elements: l, itemsSchema: schema.Items()} 

 listType := schema.XListType() 

 if listType != "" { 

 switch listType { 

 case "map": 

 mapKeys := schema.XListMapKeys() 

 return &unstructuredMapList{unstructuredList: typedList, escapedKeyProps: escapeKeyProps(mapKeys)} 

 case "set": 

 return &unstructuredSetList{unstructuredList: typedList} 

 case "atomic": 

 return &typedList 

 default: 

 return types.NewErr("invalid x-kubernetes-list-type, expected 'map', 'set' or 'atomic' but got %s", listType) 

 } 

 } 

 return &typedList 

 }

from kubernetes.

jpbetz commented on September 28, 2024

UnstructuredToVal is primarily for encoding maximum sizes etc for static cost checking, so the few differences seem to be limited to the formats which yield different types:

I meant common.UnstructuredToVal. I don't see anything in UnstructuredToVal that I wouldn't want applied to ValidationAdmissionPolicy. duration, date, date-time and byte all need to be coerced, but there is also numeric handling that seems appropriate. And yes, the handling of maps, lists and sets for equality is important.

from kubernetes.

jpbetz commented on September 28, 2024

Note that changing a field from type string to type duration, even for runtime, is a breaking change that is observable with expressions like type(object.duration)

EDIT: There are other ways to observe the breaking change too, this is just one example.

from kubernetes.

cici37 commented on September 28, 2024

/cc
I agree that it would be great to transfer object to CEL value in VAP as what has been done for CRD validation rule. Since VAP is in GA and it would be a breaking change so a feature gate might be needed. Thanks for bringing it up!

from kubernetes.

jpbetz commented on September 28, 2024

+1 to the feature gate approach.

from kubernetes.

seans3 commented on September 28, 2024

/triage accepted

from kubernetes.

T-Lakshmi commented on September 28, 2024

/cc

from kubernetes.

jpbetz commented on September 28, 2024

@liggitt Is this the one you'd be okay with just fixing w/o a feature gate? It is possible to depend on the broken behavior but it's VERY unlikely (depending on a runtime type check, or expecting a duration to be a string, not a CEL duration type)

from kubernetes.

liggitt commented on September 28, 2024

@liggitt Is this the one you'd be okay with just fixing w/o a feature gate?

what's the proposed fix?

from kubernetes.

alexzielenski commented on September 28, 2024

@liggitt Is this the one you'd be okay with just fixing w/o a feature gate?

what's the proposed fix?

To change all CEL environments (VAP, MatchConditions) which expose a Kubernetes object to CEL to first call UnstructuredToVal so that the schema is considered in the CEL type.

from kubernetes.

liggitt commented on September 28, 2024

We have schemas at runtime don't we?

I ... didn't think we did for admission of CRDs

from kubernetes.

ValidatingAdmissionPolicy objects have different runtime type compared to CRDValidationRules about kubernetes HOT 13 OPEN

Comments (13)

Related Issues (20)

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent

	str, ok := unstructured.(string)
	if !ok {
	return types.NewErr("invalid data, expected string, got %T", unstructured)
	}
	switch schema.Format() {
	case "duration":
	d, err := strfmt.ParseDuration(str)
	if err != nil {
	return types.NewErr("Invalid duration %s: %v", str, err)
	}
	return types.Duration{Duration: d}
	case "date":
	d, err := time.Parse(strfmt.RFC3339FullDate, str) // strfmt uses this format for OpenAPIv3 value validation
	if err != nil {
	return types.NewErr("Invalid date formatted string %s: %v", str, err)
	}
	return types.Timestamp{Time: d}
	case "date-time":
	d, err := strfmt.ParseDateTime(str)
	if err != nil {
	return types.NewErr("Invalid date-time formatted string %s: %v", str, err)
	}
	return types.Timestamp{Time: time.Time(d)}
	case "byte":
	base64 := strfmt.Base64{}
	err := base64.UnmarshalText([]byte(str))
	if err != nil {
	return types.NewErr("Invalid byte formatted string %s: %v", str, err)
	}
	return types.Bytes(base64)
	}

	if schema.Type() == "array" {
	l, ok := unstructured.([]interface{})
	if !ok {
	return types.NewErr("invalid data, expected an array for the provided schema with type=array")
	}
	if schema.Items() == nil {
	return types.NewErr("invalid array type, expected Items with a non-empty Schema")
	}
	typedList := unstructuredList{elements: l, itemsSchema: schema.Items()}
	listType := schema.XListType()
	if listType != "" {
	switch listType {
	case "map":
	mapKeys := schema.XListMapKeys()
	return &unstructuredMapList{unstructuredList: typedList, escapedKeyProps: escapeKeyProps(mapKeys)}
	case "set":
	return &unstructuredSetList{unstructuredList: typedList}
	case "atomic":
	return &typedList
	default:
	return types.NewErr("invalid x-kubernetes-list-type, expected 'map', 'set' or 'atomic' but got %s", listType)
	}
	}
	return &typedList
	}