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

Allow policy to force http.send to cache across queries #2666

Closed
tsandall opened this issue Aug 27, 2020 · 14 comments · Fixed by #2744
Closed

Allow policy to force http.send to cache across queries #2666

tsandall opened this issue Aug 27, 2020 · 14 comments · Fixed by #2744
Assignees

Comments

@tsandall
Copy link
Member

tsandall commented Aug 27, 2020

There are various reasons why users would need to override the caching directives defined by the server (e.g., the server may be outside their control and configured incorrectly.) To workaround these cases, we should allow users to override the caching behaviour and control it themselves. Initially, users should be able to force caching (overriding the directives in the server's response) and control how long the cached response will be valid for:

# execute an HTTP GET caching the response for up to 5 minutes
http.send({
  "method": "get",
  "url": "https://example.service/v1/foo/bar",
  "force_cache": true,
  "force_cache_duration_seconds": 300,
})

If "force_cache" is true, it overrides the "cache" parameter. If the force_cache parameter is supplied they should have to provide other cache control info like duration (force_cache_duration_seconds above).

@tsandall tsandall added this to TODO (Things That Should Be Done) in Open Policy Agent via automation Aug 27, 2020
@tsandall tsandall moved this from TODO (Things That Should Be Done) to Planned (Things We Are Going To Do) in Open Policy Agent Aug 27, 2020
@ashutosh-narkar ashutosh-narkar moved this from Planned (Things We Are Going To Do) to In Progress in Open Policy Agent Sep 29, 2020
@ashutosh-narkar ashutosh-narkar self-assigned this Sep 29, 2020
ashutosh-narkar added a commit to ashutosh-narkar/opa that referenced this issue Oct 6, 2020
This commit adds two new fields to the http.send builtin
that allow the user to override the caching directives defined
by the server and thus get more control over the caching
behavior.

Fixes: open-policy-agent#2666

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Open Policy Agent automation moved this from In Progress to Done Oct 6, 2020
ashutosh-narkar added a commit that referenced this issue Oct 6, 2020
This commit adds two new fields to the http.send builtin
that allow the user to override the caching directives defined
by the server and thus get more control over the caching
behavior.

Fixes: #2666

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
@pmeng99
Copy link

pmeng99 commented Oct 14, 2020

Is this fully implemented in v0.24.0? When I try it, the response does not seem to be cached. The end point got hit every single time. No caching seems to be taking place.

Anything else I need to do besides setting "force_cache" and "force_cache_duration_seconds"?

@ashutosh-narkar
Copy link
Member

@pmeng99 how are you running OPA ?

@pmeng99
Copy link

pmeng99 commented Oct 15, 2020

@pmeng99 how are you running OPA ?

we are running it as an envoy plugin.

module xxxxxxxxx/xxxxxxx/opa-plugin

go 1.14

require (
github.com/open-policy-agent/opa v0.24.0
github.com/open-policy-agent/opa-envoy-plugin v0.24.0
gopkg.in/square/go-jose.v2 v2.5.1
)

@ashutosh-narkar
Copy link
Member

I meant do you run OPA in server mode ? The inter-query cache is only used when OPA is run as a server.

@pmeng99
Copy link

pmeng99 commented Oct 15, 2020

Yes, we did, in server mode and as an envoy plugin..

@ashutosh-narkar
Copy link
Member

Can you share your deployment manifest ? It will help reproduce the issue.

@pmeng99
Copy link

pmeng99 commented Oct 15, 2020

we start the server like this:

$ ./opa-plugin run --server --log-format=json-pretty --set=decision_logs.console=true --set=plugins.envoy_ext_authz_grpc.addr=:8089 --set=plugins.envoy_ext_authz_grpc.path=authz/allow policy.rego data.json

the plugin was built as mentioned in the go.mod file I sent you earlier

Let me know if any other info you need. This feature is critical to us. Please help!

@ashutosh-narkar
Copy link
Member

Currently when the OPA-Envoy plugin creates the Rego object it does not set the inter-query cache and hence http.send falls backs to using the intra-query cache. So the plugin does not support inter-query caching atm.

@pmeng99
Copy link

pmeng99 commented Oct 15, 2020

Any plans as when to support it?

@ashutosh-narkar
Copy link
Member

It should be a pretty small addition something like below and then it should support inter-query caching.

$ git diff internal/internal.go
diff --git a/internal/internal.go b/internal/internal.go
index ed356a3b..771b3c80 100644
--- a/internal/internal.go
+++ b/internal/internal.go
@@ -38,6 +38,8 @@ import (
@@ -38,6 +38,8 @@ import (
        "github.com/open-policy-agent/opa/server"
        "github.com/open-policy-agent/opa/storage"
        "github.com/open-policy-agent/opa/util"
+
+       iCache "github.com/open-policy-agent/opa/topdown/cache"
 )

 const defaultAddr = ":9191"
@@ -104,10 +106,11 @@ func Validate(m *plugins.Manager, bs []byte) (*Config, error) {
 func New(m *plugins.Manager, cfg *Config) plugins.Plugin {

        plugin := &envoyExtAuthzGrpcServer{
-               manager:             m,
-               cfg:                 *cfg,
-               server:              grpc.NewServer(),
-               preparedQueryDoOnce: new(sync.Once),
+               manager:                m,
+               cfg:                    *cfg,
+               server:                 grpc.NewServer(),
+               preparedQueryDoOnce:    new(sync.Once),
+               interQueryBuiltinCache: iCache.NewInterQueryCache(m.InterQueryBuiltinCacheConfig()),
        }

        // Register Authorization Server
@@ -136,11 +139,12 @@ type Config struct {
 }

 type envoyExtAuthzGrpcServer struct {
-       cfg                 Config
-       server              *grpc.Server
-       manager             *plugins.Manager
-       preparedQuery       *rego.PreparedEvalQuery
-       preparedQueryDoOnce *sync.Once
+       cfg                    Config
+       server                 *grpc.Server
+       manager                *plugins.Manager
+       preparedQuery          *rego.PreparedEvalQuery
+       preparedQueryDoOnce    *sync.Once
+       interQueryBuiltinCache iCache.InterQueryCache
 }

 func (p *envoyExtAuthzGrpcServer) Start(ctx context.Context) error {
@@ -366,6 +370,7 @@ func (p *envoyExtAuthzGrpcServer) eval(ctx context.Context, input ast.Value, res
                        rego.EvalParsedInput(input),
                        rego.EvalTransaction(txn),
                        rego.EvalMetrics(result.metrics),
+                       rego.EvalInterQueryBuiltinCache(p.interQueryBuiltinCache),
                )

@pmeng99
Copy link

pmeng99 commented Oct 15, 2020

Can you make a 0.24.1 release to include this?

@ashutosh-narkar
Copy link
Member

Sure we could pick this up the next couple of weeks. Feel free to create a PR with this feature if you'd like and I can help out as well.

@pmeng99
Copy link

pmeng99 commented Oct 28, 2020

I see there is an edge release out. Is this fix in it?

@ashutosh-narkar
Copy link
Member

Yes. The change is included in the v0.24.0-envoy-1 release of the opa-envoy plugin. Docker image: openpolicyagent/opa: 0.24.0-envoy-1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

3 participants