CEL expressions approver design #256
Conversation
jetstack-bot
added
dco-signoff: no
|
Hi @erikgb. Thanks for your PR. I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
jetstack-bot
added
the
size/XL
erikgb
force-pushed
the
cel-support-design
branch
from
747180d
to
963cd08
Compare
jetstack-bot
added
dco-signoff: yes
erikgb
force-pushed
the
cel-support-design
branch
from
963cd08
to
44a88d5
Compare
erikgb
force-pushed
the
cel-support-design
branch
from
2318647
to
dc4985b
Compare
erikgb
force-pushed
the
cel-support-design
branch
from
dc4985b
to
4f177f2
Compare
jetstack-bot
added
dco-signoff: no
erikgb
force-pushed
the
cel-support-design
branch
from
d191997
to
d918aea
Compare
jetstack-bot
added
dco-signoff: yes
|
I have now updated the proposal and moved the previous proposal into the alternatives. The new proposal is now significantly closer to the upstream CEL validations rules API. Please take another look. I would love to see some progress on this design proposal so we eventually can move over to implementing it. 😸 |
There was a problem hiding this comment.
/lgtm
/approve
/hold
(Holding so others can chime in)
First of all, this is well-written and makes sense to me. I understand the rationale and thank you for your patience with this!
I think the idea here is solid. It adds a lot of flexibility and power and the tradeoff in complexity is worth it.
A big part of why I'm really relaxed about this is that approver-policy isn't super-widely used and we're still at v1alpha1, which to me means we should be more relaxed about making changes and trying things out.
I think we should proceed with this on the basis that we should be experimenting with this. I'd also support making these changes in v1alpha2, leaving v1alpha1 as what we have today, which in my mind should make this even easier to approve. But I don't think that's required - I'd be happy to add this to v1alpha1.
As for the alternatives: I instinctively prefer having expressions be tied to fields rather than operating on the whole CSR (i.e. I prerfer the proposal to that alternative). I think the simpler the CEL the better.
jetstack-bot
added
do-not-merge/hold
|
|
||
| The last sentence describes the requirement that could need custom CEL function(s) to allow the user to specify allowed | ||
| CSR attributes. We cannot expect the user to include validations for **all** CSR attributes in the expression(s). So I | ||
| think we at least need a function to list allowed CSR attribute usage. |
There was a problem hiding this comment.
I think this is a very important aspect that I didn't think about.
I wonder if you can think of an easy way to eg allow all attributes or disallow only certain attributes.
Also, currently the validation logic is too permissive, it will allow any kind of extra X.509 extensions.
Is this something that CEL could help with?
There was a problem hiding this comment.
What makes you think the validation logic is too permissive? I haven't looked at the relevant source code (but I suspect you have), but as a user, I would expect usage of any X.509 extension to be denied if not explicitly allowed. Ref. the docs:
IMO allowing/disallowing attributes are better expressed in a standard API (as it is now) than using CEL. But feel free to disagree. How do you think CEL can help?
There was a problem hiding this comment.
I have now looked at the source code, and approver-policy only evaluates a policy against the CSR attributes that approver-policy supports. So you are correct that it might be considered too permissive in this sense. I suggest creating an issue for this if it is something that we should attempt to fix. Changing it would be breaking tho. 😉
| dnsNames: | ||
| validations: | ||
| - message: dnsName can be maximum 64 characters. | ||
| rule: self.size() =< 64 |
There was a problem hiding this comment.
How could we limit the number of dnsNames? Could we eg. have a "selfs" parameter (we should probably pick a better name)?
There was a problem hiding this comment.
This is an interesting and important comment! Strictly speaking, many CSR attributes are multi-valued - so it could make sense to make self refer to the list of attribute values in the CSR - which will allow for the validation you are requesting here. But I suspect this will make the feature less user-friendly. The validation example this review comment refers to would then have to be expressed as self.all(dnsName, dnsName.size() =< 64) - which is not too bad IMO.
But this could easily lead to user errors like self.size() =< 64 would now limit the number of dnsNames in the CSR to 63. 😉 Which is probably not what the user had in mind.
There was a problem hiding this comment.
IMO the most user-friendly is to keep the validations under allowed as per-item validations. This is how the value/values fields in the current API work. More complex CEL validations, typically cross-cutting concerns could be introduced later outside of the allowed API structure. I am updating the proposal with this clarification now.
erikgb
force-pushed
the
cel-support-design
branch
from
b185ab4
to
29b1c9a
Compare
There was a problem hiding this comment.
/lgtm
/approve
Fundamentally, I think we should be experimenting with approver-policy. Now's the time for it!
I'd be much more conservative about this if it were for cert-manager itself... but it's not. I think this is worth a go. It makes sense. It's well written. Thank you for this Erik! 👍
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: inteon, SgtCoDFish 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:
Approvers can indicate their approval by writing |
|
I have initiated the work on implementing this new feature: #277 |
erikgb
force-pushed
the
cel-support-design
branch
from
29b1c9a
to
9c3a79c
Compare
|
After working on improving the API docs and understanding more of the current API behavior, I have updated the proposed design (and examples with the following):
I think that is our safest option. The next API version could make this feature even more user-friendly. But I think it will be acceptable as proposed now. I am almost ready to move over to the implementation of this, so I would prefer a more "formal" approval of this design proposal by merging this PR. 😉 PTAL! |
|
Just wanted to add that the design I read just over 2 weeks ago looked great. It covered a few things I had not thought of and there was nothing I disagreed with. Now work has started, I will attempt to look over for understanding. The only minor thing is that there are a few empty sections in the design doc, might it be worth either a quick comment of |
erikgb
force-pushed
the
cel-support-design
branch
from
9c3a79c
to
885ac4d
Compare
|
/retest |
|
/unhold |
jetstack-bot
added
ok-to-test
and removed
do-not-merge/hold
Signed-off-by: Erik Godding Boye <egboye@gmail.com> Co-authored-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
erikgb
force-pushed
the
cel-support-design
branch
from
7fad7cd
to
b235207
Compare
|
/lgtm |
This PR contains an initial design proposal allowing policy authors to use CEL expressions to express policy rules. If this is ever implemented, it should address #62.
Also includes an initial design template copied from cert-manager.
/cc @inteon @SgtCoDFish