-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Update docs and server binding addr per OPA v1.0 specs #7140
Update docs and server binding addr per OPA v1.0 specs #7140
Conversation
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
685eef0
to
f660edd
Compare
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.
Great work! Is the plan to add docs on how to "downgrade" OPA 1.0 for legacy compatibility here or in a later PR?
Yes @anderseknert. We'll add a section for 0.x compatibility later in this PR. |
@ashutosh-narkar neat! If you can remember to, ping me when that's in and I'd be happy to review 😃 |
73c0bb2
to
7215e50
Compare
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.
Just a few comments/questions
if !addrSetByUser && !rt.Params.V0Compatible && rt.Params.V1Compatible { | ||
rt.Params.Addrs = &[]string{defaultLocalAddr} | ||
if !addrSetByUser && rt.Params.V0Compatible { | ||
rt.Params.Addrs = &[]string{defaultAddr} |
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, should we really default to 0.0.0.0
if the --v0-compatible
flag is set? This has security implications. If you can add --v0-compatible
to your command, you can also add --addr ':8181'
.
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.
This is existing behavior which the flag is supposed to maintain. We already have a section on the security implications of this in the docs and we also log a warning on the command line. So I don't see a reason to change this.
docs/content/policy-language.md
Outdated
``` | ||
Name | Description | ||
--- |---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ||
Duplicate imports | Duplicate [imports](../policy-language/#imports), where one import shadows another, are prohibited. |
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.
We should drop this table entry, as this is part of the default v1
constraints.
We could keep it, with a comment, to let users know it's still relevant with --v0-compatible --strict
. But maybe it's more clear if that's a separate table. And then it should probably also be put in the v0 migration guide, with just a link here.
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've reworded this section to align with 1.0
docs/content/policy-language.md
Outdated
Duplicate imports | Duplicate [imports](../policy-language/#imports), where one import shadows another, are prohibited. | ||
Unused local assignments | Unused arguments or [assignments](../policy-reference/#assignment-and-equality) local to a rule, function or comprehension are prohibited | ||
Unused imports | Unused [imports](../policy-language/#imports) are prohibited. | ||
`input` and `data` reserved keywords | `input` and `data` are reserved keywords, and may not be used as names for rules and variable assignment. |
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.
Same here.
docs/content/policy-language.md
Outdated
Unused local assignments | Unused arguments or [assignments](../policy-reference/#assignment-and-equality) local to a rule, function or comprehension are prohibited | ||
Unused imports | Unused [imports](../policy-language/#imports) are prohibited. | ||
`input` and `data` reserved keywords | `input` and `data` are reserved keywords, and may not be used as names for rules and variable assignment. | ||
Use of deprecated built-ins | Use of deprecated functions is prohibited, and these will be removed in OPA 1.0. Deprecated built-in functions: `any`, `all`, `re_match`, `net.cidr_overlap`, `set_diff`, `cast_array`, `cast_set`, `cast_string`, `cast_boolean`, `cast_null`, `cast_object` |
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.
And here.
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.
Lookin' good :). Some comments.
docs/content/v0-compatibility.md
Outdated
`--rego-v1` flag below. | ||
- `inspect`: supports Rego v0 syntax modules, use of `import rego.v1` is optional. | ||
- `parse`: supports Rego v0 syntax modules, use of `import rego.v1` is optional. | ||
- `run`: supports modules (including discovery bundle) using Rego v0 syntax, use of `import rego.v1` is optional. Binds server listeners to all interfaces by default, rather than localhost. |
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.
Not sure if we should really bind to all interfaces with the --v0-compatible
flag ..
If you can add --v0-compatible
to your command, then surely you can add the flag to set an external listener too.
c72434b
to
6f8aa58
Compare
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.
Some more comments
8d41fd1
to
aaacc6c
Compare
77a3db8
to
e31b7d3
Compare
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.
Looks good 👍
Just a couple of notes.
|
||
### v0.x compatibility mode in Rego package | ||
|
||
There are three ways to enable v0.x compatibility mode in the [Rego package](https://pkg.go.dev/github.com/open-policy-agent/opa/rego): |
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 need this section at all. The old OPA API will retain the v0 behaviour, and you need to update your code to import the v1
packages to get the new v1 behaviour.
The techniques described here are however a good primer on how to manually control the applied rego-version regardless of whether you're using the v0 or v1 parts of the API. Is there a place where they would fit in?
|
||
### v0.x compatibility mode in Rego package | ||
|
||
There are three ways to enable v0.x compatibility mode in the [Rego package](https://pkg.go.dev/github.com/open-policy-agent/opa/rego): |
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.
OK maybe we shouldn't drop the section completely; it's probably worthwhile mentioning that the old parts of the API forks as before, to soothe some nerves.
We're gonna need to find a good place for describing the v1 API, though.
e31b7d3
to
1e835ab
Compare
Maybe Integrating OPA section? |
1e835ab
to
427330f
Compare
cfb3749
to
6d9847d
Compare
d8ea86c
to
adba53c
Compare
ef9d8b7
to
09d5a2f
Compare
docs/content/cli.md
Outdated
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.
Will this get regenerated? cc @johanfylling @charlieegan3
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 have rebased and regenerated it now. I have pushed directly to the branch.
This changes updates the docs and all the policy examples in them to be OPA v1.0-compliant. It also binds the OPA server to `localhost` interface by default per OPA v1.0 specs. Co-authored-by: Charlie Egan <[email protected]> Signed-off-by: Ashutosh Narkar <[email protected]>
09d5a2f
to
07d416e
Compare
Signed-off-by: Charlie Egan <[email protected]>
This changes updates the docs and all the policy examples in them to
be OPA v1.0-compliant. It also binds the OPA server to
localhost
interface by default per OPA v1.0 specs.
Co-authored-by: Charlie Egan [email protected]
Signed-off-by: Ashutosh Narkar [email protected]