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.

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

Fix Evaluate logic #3

Merged
merged 1 commit into from
Sep 25, 2023
Merged

Conversation

iskey
Copy link
Contributor

@iskey iskey commented Sep 24, 2023

related to eclipse-xpanse/xpanse#896
related to eclipse-xpanse/xpanse#900

Remove the isAllow from the request and response.

As the opa will try to evaluate all the variables into the bindings, we just need to care about the allow and the deny variables in the rego. If no such variables, this rego will be ignored.

$ opa eval --data policy.rego --input input.json "result=data.example.auth"
{
  "result": [
    {
      "expressions": [
        {
          "value": true,
          "text": "result=data.example.auth",
          "location": {
            "row": 1,
            "col": 1
          }
        }
      ],
      "bindings": {
        "result": {
          "allow": true,
          "deny": true,
          "x": 1,
          "y": "hello world"
        }
      }
    }
  ]
}

@iskey iskey requested a review from swaroopar September 24, 2023 15:00
@@ -20,18 +20,17 @@ import (
)

type EvalRego struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to move all structs to a different package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, If no complex business logic, putting the structs where they have been used is also a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

was only thinking to keep all data models in once place. This will help us in future to easy check the correct places instead of searching.

if err != nil {
abortWithError(c, 500, err.Error())
return
}

c.JSON(200, gin.H{
"isAllow": cmd.Rego.IsAllow,
"isSuccessful": decision,
Copy link
Contributor

Choose a reason for hiding this comment

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

its better to define a struct for the return type as well.

@@ -32,18 +32,20 @@ Flags:
-p, --port string The port of the HTTP server
```

## Eval a policy
## Evaluate the input by a policy list
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to make policy-man more generic.. that it simply executes and returns results of all rules. And only in xpanse, we can build a limitation that the rego rules must contain allow/deny rules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we do it like this, then the response of this API will not be a stable structure. and the Xpanse must deserialize the response and check the allow/deny rules itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. Then we can provide two services. one with specific allow and deny.. and one with a generic one.


policyRegoEx := fmt.Sprintf("package policyman.auth\n\n%v", policyRego)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to append this? What I thought is, since we are loading the string fully, the rego must contain the package name already.

Copy link
Contributor Author

@iskey iskey Sep 25, 2023

Choose a reason for hiding this comment

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

If we let the users define the package name by themselves, we will not be able to be aware of which package to evaluate, or we must extract the package name from the rego files,

actually, this is an one-off policy here, the package name here is useless.

Copy link
Contributor

Choose a reason for hiding this comment

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

@iskey should we only then control if the package name already exists and overwrite it?

Copy link
Contributor

@swaroopar swaroopar left a comment

Choose a reason for hiding this comment

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

LGTM

@swaroopar swaroopar merged commit 90b0d9b into eclipse-xpanse:main Sep 25, 2023
@iskey iskey deleted the iskey/evaluate branch October 26, 2023 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants