-
Notifications
You must be signed in to change notification settings - Fork 31
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 Support #495
base: main
Are you sure you want to change the base?
CEL Support #495
Conversation
fc65fbc
to
dc50681
Compare
00949f1
to
7f17d95
Compare
- apiGroups: | ||
- operator.authorino.kuadrant.io | ||
resources: | ||
- authorinos |
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.
There's something wrong going on here. It's somehow bringing to the operand permissions that only belong to the operator.
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.
Ah! Again... remember it happened to me already when I refactored the controller to v1beta2
🤔
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.
Could you try and run it on your machine and push the changes to this branch please? I have no idea what's screwed up on my end 🤷
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. 28e7fb9
❯ git st
On branch cel
Your branch is up to date with 'origin/cel'.
nothing to commit, working tree clean
❯ make generate manifests
go mod tidy
go: downloading github.com/envoyproxy/go-control-plane v0.12.1-0.20240621013728-1eb8caab5155
go: downloading github.com/prometheus/client_golang v1.20.2
go: downloading github.com/open-policy-agent/opa v0.68.0
go: downloading google.golang.org/grpc v1.66.0
go: downloading go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.53.0
go: downloading github.com/google/cel-go v0.21.0
go: downloading google.golang.org/genproto/googleapis/rpc v0.0.0-20240701130421-f6361c86f094
go: downloading github.com/klauspost/compress v1.17.9
go: downloading golang.org/x/time v0.6.0
go: downloading github.com/stoewer/go-strcase v1.2.0
go: downloading cloud.google.com/go/compute/metadata v0.3.0
go: downloading github.com/golang/glog v1.2.1
go: downloading golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d
go: downloading cloud.google.com/go/compute v1.23.0
go: downloading cloud.google.com/go v0.34.0
go mod vendor
controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
/Library/Developer/CommandLineTools/usr/bin/make fmt vet
go fmt ./...
go vet ./...
controller-gen crd:crdVersions=v1 rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=install/crd output:rbac:artifacts:config=install/rbac && /Users/guilherme.cassolato/Workspace/go/src/github.com/kuadrant/authorino/bin/kustomize build install > /Users/guilherme.cassolato/Workspace/go/src/github.com/kuadrant/authorino/install/manifests.yaml
/Library/Developer/CommandLineTools/usr/bin/make patch-webhook
envsubst \
< /Users/guilherme.cassolato/Workspace/go/src/github.com/kuadrant/authorino/install/manifests.yaml \
> /Users/guilherme.cassolato/Workspace/go/src/github.com/kuadrant/authorino/install/manifests.yaml.tmp && \
mv /Users/guilherme.cassolato/Workspace/go/src/github.com/kuadrant/authorino/install/manifests.yaml.tmp /Users/guilherme.cassolato/Workspace/go/src/github.com/kuadrant/authorino/install/manifests.yaml
❯ git st
On branch cel
Your branch is up to date with 'origin/cel'.
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git restore <file>..." to discard changes in working directory)
modified: install/manifests.yaml
modified: install/rbac/role.yaml
no changes added to commit (use "git add" and/or "git commit -a")
❯ grep "controller-gen@" Makefile
$(call go-get-tool,$(CONTROLLER_GEN),sigs.k8s.io/controller-tools/cmd/[email protected])
❯ ./bin/controller-gen --version
Version: v0.15.0
🤦 This should in
@guicassolato am I missing something else? |
return fmt.Sprintf("%s", value.ResolveFor(authJSON)) | ||
jsonValueToStr := func(value expressions.Value) (string, error) { | ||
if value == nil { | ||
return "", nil |
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.
Ugh... Now that it is a pointer, we could get nil
indeed... I don't know whether all these cases are now covered, hard to tell what can or not be nil
in those specs
901996d
to
067f83d
Compare
api/v1beta1/zz_generated.deepcopy.go
Outdated
@@ -1,5 +1,4 @@ | |||
//go:build !ignore_autogenerated | |||
// +build !ignore_autogenerated |
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.
The // +build
syntax has been deprecated since Go v1.17.
https://go.googlesource.com/proposal/+/master/design/draft-gobuild.md
api/v1beta3/auth_config_types.go
Outdated
// AuthConfig is the schema for Authorino's AuthConfig API | ||
// +kubebuilder:object:root=true | ||
// +kubebuilder:subresource:status | ||
// +kubebuilder:storageversion |
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.
Shall we remove it from v1beta2
?
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.
One and only one version must be marked as the storage version.
package v1beta3 | ||
|
||
// Hub marks this version as a conversion hub. | ||
func (a *AuthConfig) Hub() {} |
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 don't know for sure how this works, but can we have 2 versions marked as conversion hub? I believe right now v1beta2
is also marked as hub, right?
dc2777a
to
a25343f
Compare
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
This reverts commit c3fa020. Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
@KevFan could you have a look at this wrt the |
@alexsnaps Sure, I'll take a look 👀 |
Signed-off-by: KevFan <[email protected]>
Signed-off-by: KevFan <[email protected]>
@alexsnaps when running |
tests/v1beta3/authconfig.yaml
Outdated
- selector: context.request.http.path | ||
operator: matches | ||
value: ^/admin(/.*)?$ | ||
- predicate: request.http.path.matches("^/admin(/.*)?$") |
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.
Out of curiosity, are these changes still WIP for this file? 🤔
Just wondering cause running make e2e
is erroring due to invalid AuthConfig with this since it's not expecting predicate
currently:
authorino/api/v1beta3/auth_config_types.go
Line 118 in 3925889
NamedPatterns map[string]PatternExpressions `json:"patterns,omitempty"` authorino/api/v1beta3/auth_config_types.go
Lines 152 to 165 in 3925889
type PatternExpressions []PatternExpression type PatternExpression struct { // Path selector to fetch content from the authorization JSON (e.g. 'request.method'). // Any pattern supported by https://pkg.go.dev/github.com/tidwall/gjson can be used. // Authorino custom JSON path modifiers are also supported. Selector string `json:"selector,omitempty"` // The binary operator to be applied to the content fetched from the authorization JSON, for comparison with "value". // Possible values are: "eq" (equal to), "neq" (not equal to), "incl" (includes; for arrays), "excl" (excludes; for arrays), "matches" (regex) Operator PatternExpressionOperator `json:"operator,omitempty"` // The value of reference for the comparison with the content fetched from the authorization JSON. // If used with the "matches" operator, the value must compile to a valid Golang regex. Value string `json:"value,omitempty"` }
Error from server (BadRequest): error when creating "/Users/chfan/dev/authorino/tests/v1beta3/authconfig.yaml": AuthConfig in version "v1beta3" cannot be handled as a AuthConfig: strict decoding error: unknown field "spec.patterns.admin-path[0].predicate", unknown field "spec.patterns.resource-path[0].predicate"
waiting authconfig readyError from server (NotFound): authconfigs.authorino.kuadrant.io "e2e-test" not found
.Error from server (NotFound): authconfigs.authorino.kuadrant.io "e2e-test" not found
And also, failing on the oneOf
validations such as:
authorino/install/crd/patches/oneof_in_authconfigs.yaml
Lines 305 to 321 in 3925889
- op: add path: /spec/versions/1/schema/openAPIV3Schema/properties/spec/properties/authorization/additionalProperties/properties/patternMatching/properties/patterns/items/oneOf value: - properties: patternRef: {} required: [patternRef] - properties: operator: {} selector: {} value: {} required: [operator, selector] - properties: all: {} required: [all] - properties: any: {} required: [any]
The AuthConfig "e2e-test" is invalid:
* <nil>: Invalid value: "": "spec.response.success.headers.wristband.when[0]" must validate one and only one schema (oneOf). Found none valid
* spec.response.success.headers.wristband.when[0].patternRef: Required value
* <nil>: Invalid value: "": "spec.authentication.anonymous.when[0]" must validate one and only one schema (oneOf). Found none valid
* spec.authentication.anonymous.when[0].patternRef: Required value
* <nil>: Invalid value: "": "spec.authentication.anonymous.when[1]" must validate one and only one schema (oneOf). Found none valid
* spec.authentication.anonymous.when[1].patternRef: Required value
* <nil>: Invalid value: "": "spec.authorization.admin-kubernetes-rbac.when[1]" must validate one and only one schema (oneOf). Found none valid
* spec.authorization.admin-kubernetes-rbac.when[1].patternRef: Required value
* <nil>: Invalid value: "": "spec.authorization.admin-jwt-rbac.when[1]" must validate one and only one schema (oneOf). Found none valid
* spec.authorization.admin-jwt-rbac.when[1].patternRef: Required value
* <nil>: Invalid value: "": "spec.authorization.admin-jwt-rbac.patternMatching.patterns[0]" must validate one and only one schema (oneOf). Found none valid
* spec.authorization.admin-jwt-rbac.patternMatching.patterns[0].patternRef: Required value
waiting authconfig readyError from server (NotFound): authconfigs.authorino.kuadrant.io "e2e-test" not found
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.
Darn, looks like I screwed up my rebasing then 🤦
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.
Actually... no! I made this fail on purpose, I need @guicassolato 's opinion here... cause adding CEL there is either recursive or a breaking change, or needs another NamedCelExpression
mechanism, or...
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.
So, I'm thinking of adding a celFunctions
at the same level as
NamedPatterns map[string]PatternExpressions `json:"patterns,omitempty"`
Same "pattern", but map[string]CelExpression
that then register the key as a fn() in cel. So that:
celFunctions:
userGroup: request.headers['someHeader']
become usable in a cel expression:
expression: userGroup().startWith("admin")
If we think that makes sense, I'd add this in a subsequent PR tho and "just get rid of NamedPattern
s equivalence for Cel for now... wdyt?
manifests: generate v1beta3 changes
Signed-off-by: Alex Snaps <[email protected]>
Signed-off-by: Alex Snaps <[email protected]>
Fixes #475