ast: Adding rule_head_general_refs capabilities feature flag #6346

johanfylling · 2023-10-26T10:21:54Z

To signal support for general refs in rule heads.

To signal support for general refs in rule heads. Fixes: open-policy-agent#6334 Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

netlify · 2023-10-26T10:23:16Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`a47427a`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/6554978dd8545800087ad051
😎 Deploy Preview	https://deploy-preview-6346--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

srenatus · 2023-10-26T11:17:44Z

docs/content/deployments.md

+{
+    "features": [ 
+      "rule_head_ref_string_prefixes", 
+      "rule_head_general_refs" 


Keep wondering if...

the implication should somehow be reflected -- you cannot have rule_head_general_refs without rule_head_ref_string_prefixes, since the latter is implied.

naming: rule_head_ref could be the general case, _string_prefixes the limited one?

I don't think I follow. You currently can't have rule_head_general_refs without also rule_head_ref_string_prefixes. We could add a more specific error message for when the former is present but not the latter.

would that not change the semantics of rule_head_ref_string_prefixes?

Sorry I was a bit too brief:

I was wondering if it made sense to declare that both features as supported when one subsumes the other one. It's a bit like saying "All animals are allowed. And cows are allowed." 🙃 However, it probably simplifies things to keep them both, so it's OK, I think.

I was thinking that "general refs" are just refs. "string prefixes" are the special case. So my proposal was to rename the new feature to "rule_head_refs"

the reason for why I didn't let one feature imply the other is out of concern that someone already has a procedure for stripping out rule_head_ref_string_prefixes from a capabilities-file generated by OPA, and if we introduce a new feature flag that will enable refs anyways, then their setup will start behaving against their configuration.
Of course, I think the risk of this is close to non-existent; but I'm hesitant to break an already established contract for rule_head_ref_string_prefixes.

Agreed. What do you think about calling the new feature simply "rule_head_refs"?

@srenatus @anderseknert wdyt about rule_head_general_refs implying rule_head_ref_string_prefixes?

I'd be in favor of that. Technically, I think we could even consider deprecating rule_head_ref_string_prefixes, since it's been a byproduct of how this feature got introduced; not really a desirable feature switch to have, in my opinion.

Technically, I think we could even consider deprecating rule_head_ref_string_prefixes

Could we just remove it, without first deprecating it? Then we'd effectively just replace rule_head_ref_string_prefixes with rule_head_refs now.

Again, I'm not very familiar with how this is actually used, and what impact breaking changes have; but if we're willing to break an egg, perhaps we can thoroughly stomp on it 😈 😄.

Could we just remove it, without first deprecating it? Then we'd effectively just replace rule_head_ref_string_prefixes with rule_head_refs now.

👍 I think that's OK. @ashutosh-narkar ?

Sure. Let's just make sure we add a change log entry, message this in slack etc. to let the community know about this after we merge this PR.

Given how the minimal compatible version is calculated, I opted to go with @srenatus's first suggestion, and let the rule_head_refs capability map to all ref head rule permutations, and let rule_head_ref_string_prefixes map to the special case of string-prefix refs.

tsandall

Now that #6348 has merged we need to update this PR to include the feature in the required capabilities that the compiler generates.

…le version calculation Signed-off-by: Johan Fylling <johan.dev@fylling.se>

johanfylling · 2023-11-01T16:06:59Z

capabilities/v0.59.0.json

Will it mess anything up that we add this now before the actual release?

johanfylling · 2023-11-01T16:19:31Z

ast/capabilities_test.go

+				p.a.b[c].d if { c := "foo" }
+			`,
+			version: "0.59.0",
+		},


I've added support for the rule_head_refs capability feature to the minimum compatible version calculation @tsandall .

How do we envision this working when we start removing capabilities, such as the deprecated built-ins?

Then, the following policy

package x p { all([true, false]) }

will still output 0.17.0 as minimum version, but it won't be compatible with 1.0.
Will we just add a MaximumCompatibleVersion(), that'll output whatever the last supported version is for the capability?

I hadn't thought about the deprecated case. I wonder if a better interface would be to return the compatible version range?

Yes, that makes sense 👍 .
I wonder if this change fits better as part of #6361, though, as it wouldn't make much difference here.

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

johanfylling added 3 commits October 26, 2023 11:11

ast: Adding rule_head_general_refs capabilities feature flag

d7b8476

To signal support for general refs in rule heads. Fixes: open-policy-agent#6334 Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Adding tests for capabilities feature flags

651faae

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Adding documentation

d48b730

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

srenatus previously approved these changes Oct 26, 2023

View reviewed changes

tsandall requested changes Oct 31, 2023

View reviewed changes

johanfylling added 2 commits November 1, 2023 15:11

Merge branch 'main' into general_refs/capabilities_feature

9388a94

Adding rule_head_refs capabilities feature flag to minimum compatib…

b34461e

…le version calculation Signed-off-by: Johan Fylling <johan.dev@fylling.se>

johanfylling dismissed srenatus’s stale review via b34461e November 1, 2023 16:06

johanfylling commented Nov 1, 2023

View reviewed changes

johanfylling requested a review from tsandall November 1, 2023 16:24

johanfylling added 2 commits November 7, 2023 14:14

Updating docs

164a51e

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Merge branch 'main' into general_refs/capabilities_feature

3896ac5

johanfylling mentioned this pull request Nov 7, 2023

ast: Adding capability feature for the rego.v1 import #6375

Merged

johanfylling requested a review from ashutosh-narkar November 7, 2023 14:26

Merge branch 'main' into general_refs/capabilities_feature

e200ff4

ashutosh-narkar approved these changes Nov 14, 2023

View reviewed changes

Merge branch 'main' into general_refs/capabilities_feature

a47427a

johanfylling merged commit 56f2350 into open-policy-agent:main Nov 15, 2023
24 checks passed

johanfylling deleted the general_refs/capabilities_feature branch November 15, 2023 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ast: Adding rule_head_general_refs capabilities feature flag #6346

ast: Adding rule_head_general_refs capabilities feature flag #6346

johanfylling commented Oct 26, 2023

netlify bot commented Oct 26, 2023 •

edited

srenatus Oct 26, 2023

johanfylling Oct 26, 2023

srenatus Oct 26, 2023

johanfylling Oct 26, 2023

srenatus Oct 26, 2023

srenatus Oct 30, 2023

johanfylling Oct 31, 2023

srenatus Oct 31, 2023

ashutosh-narkar Oct 31, 2023

johanfylling Nov 1, 2023

tsandall left a comment

johanfylling Nov 1, 2023

johanfylling Nov 1, 2023

tsandall Nov 7, 2023

johanfylling Nov 8, 2023

ast: Adding rule_head_general_refs capabilities feature flag #6346

ast: Adding rule_head_general_refs capabilities feature flag #6346

Conversation

johanfylling commented Oct 26, 2023

netlify bot commented Oct 26, 2023 • edited

✅ Deploy Preview for openpolicyagent ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsandall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Oct 26, 2023 •

edited