From 317234305c10f82a2f4e38b56c5cd07694eb6740 Mon Sep 17 00:00:00 2001 From: Kermit Alexander II Date: Fri, 13 Jan 2023 18:14:53 +0000 Subject: [PATCH 1/5] Update for inclusion of string.format in CEL. --- .../3488-cel-admission-control/README.md | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/keps/sig-api-machinery/3488-cel-admission-control/README.md b/keps/sig-api-machinery/3488-cel-admission-control/README.md index 9937ffddf53..51552f6fae2 100644 --- a/keps/sig-api-machinery/3488-cel-admission-control/README.md +++ b/keps/sig-api-machinery/3488-cel-admission-control/README.md @@ -407,7 +407,7 @@ spec: validations: - name: max-replicas expression: "object.spec.replicas <= params.maxReplicas" - messageExpression: "'object.spec.replicas must be no greater than ' + string(params.maxReplicas)" + messageExpression: "'object.spec.replicas must be no greater than {}'.format([params.maxReplicas])" reason: Invalid # ...other rule related fields here... ``` @@ -850,6 +850,9 @@ Policy definitions: - Each validation may define a message: - `message` - plain string message - `messageExpression: ""` (mutually exclusive with `message`) + - As part of [the KEP update to add expression composition](https://github.com/kubernetes/enhancements/pull/3669/files), + expressions defined under `variables` will be accessible from `messageExpression` + - While messageExpression is a CEL expression, it does not factor into the runtime cost limit. - If `message` and `messageExpression` are absent, `expression` and `name` will be included in the failure message - If `messageExpression` results in an error: `expression` and `name` will be @@ -871,7 +874,7 @@ spec: validations: - expression: "self.name.startsWith('xyz-')" name: name-prefix - messageExpression: "self.name + ' must start with xyz-'" + messageExpression: "'{} must start with xyz-'.format([self.name])" reason: Unauthorized - expression: "self.name.contains('bad')" name: bad-name @@ -880,7 +883,7 @@ spec: reason: Invalid - expression: "self.name.contains('suspicious')" name: suspicious-name - messageExpression: "self.name + ' contains suspicious'" + messageExpression: "'{} contains suspicious'.format([self.name])" code: 400 reason: Invalid ``` @@ -1223,7 +1226,10 @@ Plan: To consider: - labelSelector evaluation functions or other match evaluator functions ([original comment thread](https://github.com/kubernetes/enhancements/pull/3492#discussion_r981747317)) -- `string.format(string, list(dyn))` to make `messageExpression` more convenient. + +To implement: + +- `string.format` for CEL/cel-go ([tracking PR](https://github.com/google/cel-go/pull/617)) #### Audit Annotations @@ -2872,7 +2878,7 @@ For example, to validate all containers: validations: - scope: "spec.containers[*]" expression: "scope.name.startsWith('xyz-')" - messageExpression: "scope.name + 'does not start with \'xyz\''" + messageExpression: "'{} does not start with \'xyz\'-'.format([scope.name])" ``` To make it possible to access the path information in the scope, we can offer a @@ -2886,7 +2892,7 @@ spec.x[xKey].y[yIndex].field validations: - scope: "x[xKey].y[yIndex].field" expression: "scope.startsWith('xyz-')" - messageExpression: "scopePath.xKey + ', ' + scopePath.yIndex + ': some problem'" + messageExpression: "'{}, {}: some problem'.format([scopePath.xKey, scopePath.yIndex])" ``` Prior art: @@ -2907,7 +2913,7 @@ Note: We considered extending to a list of scopes, e.g.: validations: - scopes: ["spec.containers[*]", "initContainers[*]", "spec.ephemeralContainers[*]"] expression: "scope.name.startsWith('xyz-')" - messageExpression: "scope.name + ' does not start with \'xyz\''" + messageExpression: "'{} does not start with \'xyz\''.format([scope.name])" ``` But feedback was this is signficantly more difficult to understand. From bd2ca8393cefc747cd9918bb0e195cb96aea7539 Mon Sep 17 00:00:00 2001 From: Kermit Alexander II Date: Mon, 23 Jan 2023 19:39:30 +0000 Subject: [PATCH 2/5] Update messageExpression behavior re: cost limits. --- keps/sig-api-machinery/3488-cel-admission-control/README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/keps/sig-api-machinery/3488-cel-admission-control/README.md b/keps/sig-api-machinery/3488-cel-admission-control/README.md index 51552f6fae2..d3e72ab87d9 100644 --- a/keps/sig-api-machinery/3488-cel-admission-control/README.md +++ b/keps/sig-api-machinery/3488-cel-admission-control/README.md @@ -852,7 +852,9 @@ Policy definitions: - `messageExpression: ""` (mutually exclusive with `message`) - As part of [the KEP update to add expression composition](https://github.com/kubernetes/enhancements/pull/3669/files), expressions defined under `variables` will be accessible from `messageExpression` - - While messageExpression is a CEL expression, it does not factor into the runtime cost limit. + - `messageExpression` is a CEL expression and thus factors into the runtime cost limit. + If the runtime cost limit is exceeded during `messageExpression` execution, then this is logged. + Whether or not the action is admitted after that depends upon failure policy. - If `message` and `messageExpression` are absent, `expression` and `name` will be included in the failure message - If `messageExpression` results in an error: `expression` and `name` will be From 39263f5cd970492946b53111985ec53d884ea694 Mon Sep 17 00:00:00 2001 From: Kermit Alexander II Date: Mon, 23 Jan 2023 19:53:30 +0000 Subject: [PATCH 3/5] Add TODO and note on CEL upstream. --- keps/sig-api-machinery/3488-cel-admission-control/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/keps/sig-api-machinery/3488-cel-admission-control/README.md b/keps/sig-api-machinery/3488-cel-admission-control/README.md index d3e72ab87d9..3c3adbc4186 100644 --- a/keps/sig-api-machinery/3488-cel-admission-control/README.md +++ b/keps/sig-api-machinery/3488-cel-admission-control/README.md @@ -1231,7 +1231,7 @@ To consider: To implement: -- `string.format` for CEL/cel-go ([tracking PR](https://github.com/google/cel-go/pull/617)) +- `string.format` into CEL upstream ([tracking PR](https://github.com/google/cel-go/pull/617)) (TODO @DangerOnTheRanger: add tracking cel-go issue once available) #### Audit Annotations From 3448066f3c5aae14aa92684a2a6f16b83f4eb18d Mon Sep 17 00:00:00 2001 From: Kermit Alexander II Date: Thu, 2 Feb 2023 20:47:35 +0000 Subject: [PATCH 4/5] Use field names instead of field values for some examples. --- .../3488-cel-admission-control/README.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/keps/sig-api-machinery/3488-cel-admission-control/README.md b/keps/sig-api-machinery/3488-cel-admission-control/README.md index 3c3adbc4186..fd3dc3968e8 100644 --- a/keps/sig-api-machinery/3488-cel-admission-control/README.md +++ b/keps/sig-api-machinery/3488-cel-admission-control/README.md @@ -876,7 +876,7 @@ spec: validations: - expression: "self.name.startsWith('xyz-')" name: name-prefix - messageExpression: "'{} must start with xyz-'.format([self.name])" + message: "self.name must start with xyz-" reason: Unauthorized - expression: "self.name.contains('bad')" name: bad-name @@ -885,7 +885,7 @@ spec: reason: Invalid - expression: "self.name.contains('suspicious')" name: suspicious-name - messageExpression: "'{} contains suspicious'.format([self.name])" + message: "'self.name contains suspicious'" code: 400 reason: Invalid ``` @@ -2880,7 +2880,7 @@ For example, to validate all containers: validations: - scope: "spec.containers[*]" expression: "scope.name.startsWith('xyz-')" - messageExpression: "'{} does not start with \'xyz\'-'.format([scope.name])" + message: "scope.name does not start with 'xyz'" ``` To make it possible to access the path information in the scope, we can offer a @@ -2915,7 +2915,7 @@ Note: We considered extending to a list of scopes, e.g.: validations: - scopes: ["spec.containers[*]", "initContainers[*]", "spec.ephemeralContainers[*]"] expression: "scope.name.startsWith('xyz-')" - messageExpression: "'{} does not start with \'xyz\''.format([scope.name])" + message: "scope.name does not start with 'xyz'" ``` But feedback was this is signficantly more difficult to understand. From dbd173db8bf0b7f4045d69b685e300667911606a Mon Sep 17 00:00:00 2001 From: Kermit Alexander II Date: Thu, 2 Feb 2023 20:50:10 +0000 Subject: [PATCH 5/5] Adjust messageExpression examples from {} to printf style. --- keps/sig-api-machinery/3488-cel-admission-control/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/keps/sig-api-machinery/3488-cel-admission-control/README.md b/keps/sig-api-machinery/3488-cel-admission-control/README.md index fd3dc3968e8..b8b09603b28 100644 --- a/keps/sig-api-machinery/3488-cel-admission-control/README.md +++ b/keps/sig-api-machinery/3488-cel-admission-control/README.md @@ -407,7 +407,7 @@ spec: validations: - name: max-replicas expression: "object.spec.replicas <= params.maxReplicas" - messageExpression: "'object.spec.replicas must be no greater than {}'.format([params.maxReplicas])" + messageExpression: "'object.spec.replicas must be no greater than %d'.format([params.maxReplicas])" reason: Invalid # ...other rule related fields here... ``` @@ -2894,7 +2894,7 @@ spec.x[xKey].y[yIndex].field validations: - scope: "x[xKey].y[yIndex].field" expression: "scope.startsWith('xyz-')" - messageExpression: "'{}, {}: some problem'.format([scopePath.xKey, scopePath.yIndex])" + messageExpression: "'%s, %d: some problem'.format([scopePath.xKey, scopePath.yIndex])" ``` Prior art: