-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cel: fix validation of expression result type #3526
Conversation
The earlier code used the proto.Equals from github.com/gogo/protobuf, which failed to compare two messages of the same type for some reason. Switching to proto.Equal from the canonical github.com/golang/protobuf fixes the issue.
modules/caddyhttp/celmatcher.go
Outdated
"github.com/golang/protobuf/proto" | ||
"github.com/golang/protobuf/ptypes/timestamp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the package github.com/golang/protobuf/proto is deprecated. We're instructed to use the "google.golang.org/protobuf/proto" package instead. I didn't want to change it now to avoid intrusive changes whose consequences are, currently, unknown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should, though, since deprecated packages spells lack of updates and fixes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. For a second I was worried this will break the code because cel-go uses the deprecated package github.com/golang/protobuf/ptypes/timestamp
. I checked the code in github.com/golang/protobuf/ptypes/timestamp
and found it uses type aliases, so it will all work fine. Kinda cool the type alias works perfectly as intended and argued for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this. Pretty disappointed in that dependency, but what can we do...
Let me know what you think about changing to the non-deprecated package path. That seems like the right thing to do, since we already have something that's broken, maybe we should be proactive in avoiding future breakages.
Also if you haven't yet, let's run go mod tidy
.
modules/caddyhttp/celmatcher.go
Outdated
"github.com/golang/protobuf/proto" | ||
"github.com/golang/protobuf/ptypes/timestamp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should, though, since deprecated packages spells lack of updates and fixes?
….golang.org/protobuf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thank you @mohammed90 !
This Caddy JSON config is rejected with the error
loading http app module: provision http: server srv0: setting up route handlers: route 0: loading handler modules: position 0: loading module 'subroute': provision http.handlers.subroute: setting up subroutes: route 1: loading matcher modules: module name 'not': provision http.matchers.not: loading matcher sets: module name 'expression': provision http.matchers.expression: CEL request matcher expects return type of bool, not primitive:BOOL
:Checking the types with a few log lines prints (manually prettified):
The types appear to be exactly the same. However, the
proto.Equal()
method prints:proto: don't know how to compare []
The earlier code used the proto.Equals from github.com/gogo/protobuf, which failed to compare two messages of the same type for some reason. Switching to proto.Equal from the canonical github.com/golang/protobuf fixes the issue.
Reported in the forum: https://caddy.community/t/example-configure-wordpress-with-a-static-cache/8215/7