Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: RawExtension to string conversion #501

Merged
merged 3 commits into from
Nov 4, 2024
Merged

fix: RawExtension to string conversion #501

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f0890d6
fix: RawExtension to string conversion
guicassolato Nov 4, 2024
ff2afd9
handle conversion error for spicedb permission value
guicassolato Nov 4, 2024
113717d
avoid unnecessary json decoding/encoding when the value already is a …
guicassolato Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions pkg/evaluators/authorization/authzed.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

"github.com/kuadrant/authorino/pkg/auth"
"github.com/kuadrant/authorino/pkg/expressions"
"github.com/kuadrant/authorino/pkg/json"
"google.golang.org/grpc"
insecuregrpc "google.golang.org/grpc/credentials/insecure"

Expand Down Expand Up @@ -60,10 +61,11 @@ func (a *Authzed) Call(pipeline auth.AuthPipeline, ctx gocontext.Context) (inter
if err != nil {
return nil, err
}
permissionStr, err := json.StringifyJSON(permission)
KevFan marked this conversation as resolved.
Show resolved Hide resolved
resp, err := client.CheckPermission(ctx, &authzedpb.CheckPermissionRequest{
Resource: resource,
Subject: &authzedpb.SubjectReference{Object: object},
Permission: fmt.Sprintf("%s", permission),
Permission: permissionStr,
})
if err != nil {
return nil, err
Expand Down Expand Up @@ -91,12 +93,20 @@ func authzedObjectFor(name, kind expressions.Value, authJSON string) (*authzedpb
if err != nil {
return nil, err
}
objectIdStr, err := json.StringifyJSON(objectId)
if err != nil {
return nil, err
}
objectType, err := kind.ResolveFor(authJSON)
if err != nil {
return nil, err
}
objectTypeStr, err := json.StringifyJSON(objectType)
if err != nil {
return nil, err
}
return &authzedpb.ObjectReference{
ObjectId: fmt.Sprintf("%s", objectId),
ObjectType: fmt.Sprintf("%s", objectType),
ObjectId: objectIdStr,
ObjectType: objectTypeStr,
}, nil
}
3 changes: 2 additions & 1 deletion pkg/evaluators/authorization/kubernetes_authz.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/kuadrant/authorino/pkg/auth"
"github.com/kuadrant/authorino/pkg/context"
"github.com/kuadrant/authorino/pkg/expressions"
"github.com/kuadrant/authorino/pkg/json"
"github.com/kuadrant/authorino/pkg/log"

kubeAuthz "k8s.io/api/authorization/v1"
Expand Down Expand Up @@ -71,7 +72,7 @@ func (k *KubernetesAuthz) Call(pipeline auth.AuthPipeline, ctx gocontext.Context
if err != nil {
return "", err
}
return fmt.Sprintf("%s", resolved), nil
return json.StringifyJSON(resolved)
}

user, err := jsonValueToStr(k.User)
Expand Down
6 changes: 5 additions & 1 deletion pkg/evaluators/metadata/generic_http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,11 @@ func (h *GenericHttp) buildRequest(ctx gocontext.Context, endpoint, authJSON str
if err != nil {
return nil, err
}
req.Header.Set(header.Name, fmt.Sprintf("%s", headerValue))
headerValueStr, err := json.StringifyJSON(headerValue)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that always true tho? Couldn't this be "just a plain string" ever?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the types we used internally won't contribute much to make this clear, but, in short, yes, that is always true.

First thing to keep in mind is that the type that matters the most here is not of the source, but the one of where the value is used. The value of a header must always be a string, regardless of the source.

A second point – and the one less clear from the types used internally – regards to what json.StringifyJSON does with the multiple possible outputs from ResolveFor for the different types ultimately supported as input, i.e.:

  • value → runtime.RawExtension (the one whose conversion this PR fixes)
  • selector → a JSON-compatible Golang type as implemented by gjson (not affected by adding now another step of JSON marshalling and gjson parsing)
  • expression → a CEL-resolved value (recently reduced back into the previous case anyway)

You can try this out if you want using this other example:

apiVersion: authorino.kuadrant.io/v1beta3
kind: AuthConfig
metadata:
  name: talker-api-protection
spec:
  hosts:
  - talker-api.127.0.0.1.nip.io
  metadata:
    "http-request":
      http:
        url: http://ip-api.com/json
        headers:
          "Accept":
            value: application/json
          "x-object-static":
            value:
              foo: bar
          "x-object-selector":
            selector: request.headers
          "x-object-expression":
            expression: request.headers
  response:
    success:
      headers:
        "geo-info":
          plain:
            expression: auth.metadata["http-request"]

You should see in the logs something similar to:

{
  "level": "debug",
  "ts": "2024-11-04T12:09:13Z",
  "logger": "authorino.service.auth.authpipeline.metadata.http",
  "msg": "sending request",
  "request id": "fce0cae9-a3af-4491-beab-076178c78261",
  "config": "http-request",
  "method": "GET",
  "url": "http://ip-api.com/json",
  "headers": {
    "Accept": [
      "application/json"
    ],
    "Content-Type": [
      "text/plain"
    ],
    "X-Object-Expression": [
      "{\":authority\":\"talker-api.127.0.0.1.nip.io:8000\",\":method\":\"GET\",\":path\":\"/my-secret\",\":scheme\":\"http\",\"accept\":\"*/*\",\"user-agent\":\"curl/8.7.1\",\"x-envoy-internal\":\"true\",\"x-forwarded-for\":\"10.244.0.10\",\"x-forwarded-proto\":\"http\",\"x-request-id\":\"fce0cae9-a3af-4491-beab-076178c78261\"}"
    ],
    "X-Object-Selector": [
      "{\":authority\":\"talker-api.127.0.0.1.nip.io:8000\",\":method\":\"GET\",\":path\":\"/my-secret\",\":scheme\":\"http\",\"accept\":\"*/*\",\"user-agent\":\"curl/8.7.1\",\"x-envoy-internal\":\"true\",\"x-forwarded-for\":\"10.244.0.10\",\"x-forwarded-proto\":\"http\",\"x-request-id\":\"fce0cae9-a3af-4491-beab-076178c78261\"}"
    ],
    "X-Object-Static": [
      "{\"foo\":\"bar\"}"
    ]
  }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing we can do to mitigate the impact of this PR over the other affected sources for the conversion is to make json.StringifyJSON return earlier if the interface{} input is a string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed a commit: 113717d

Copy link
Member

@alexsnaps alexsnaps Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so it was not always the case? And now a "plain string" remains a plain string... do I get this right? In anycase... I think looking into something like what's proposed in #500 wrt to Value.ResolveFor's signature would be a good refactor for ... future us. Today's interface{} (and I know that's sourced in golang's type system) is just too broad I think. Further more the asymmetry of the function wrt its input & output is definitively a code smell to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was the case, I played with these functions a bit... I get what's going on now

if err != nil {
return nil, err
}
req.Header.Set(header.Name, headerValueStr)
}

req.Header.Set("Content-Type", contentType)
Expand Down
Loading