feat: adds http delete support to cel expressions#55
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #55 +/- ##
==========================================
+ Coverage 70.35% 70.84% +0.49%
==========================================
Files 80 80
Lines 2540 2679 +139
==========================================
+ Hits 1787 1898 +111
- Misses 602 615 +13
- Partials 151 166 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Gagan H R <hrgagan4@gmail.com>
Signed-off-by: Gagan H R <hrgagan4@gmail.com>
Signed-off-by: Gagan H R <hrgagan4@gmail.com>
e6b6aa4 to
63e4f9b
Compare
There was a problem hiding this comment.
Pull request overview
Adds additional HTTP verb support to the Kyverno SDK CEL http library so CEL expressions can perform more REST operations (notably DELETE as requested in the PR description).
Changes:
- Extends the HTTP CEL surface area with new member function overloads for
Put,Patch, andDelete(plus camelCase variants for v1.18+). - Implements
Put,Patch, andDeleterequest execution in the HTTP context implementation. - Adds unit tests covering PUT/PATCH/DELETE request execution and some argument validation paths.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
extensions/cel/libs/http/types.go |
Expands the exported HTTP context interface with additional verb methods. |
extensions/cel/libs/http/lib.go |
Registers new CEL member overloads for Put/Patch/Delete (Pascal + camelCase). |
extensions/cel/libs/http/impl.go |
Adds CEL binding implementations for PUT/PATCH/DELETE. |
extensions/cel/libs/http/http.go |
Implements PUT/PATCH/DELETE on the underlying HTTP client context. |
extensions/cel/libs/http/impl_test.go |
Adds tests for PUT/PATCH/DELETE behavior and some error cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type ContextInterface interface { | ||
| Get(url string, headers map[string]string) (any, error) | ||
| Post(url string, data any, headers map[string]string) (any, error) | ||
| Put(url string, data any, headers map[string]string) (any, error) | ||
| Patch(url string, data any, headers map[string]string) (any, error) | ||
| Delete(url string, headers map[string]string) (any, error) | ||
| Client(caBundle string) (ContextInterface, error) |
There was a problem hiding this comment.
Adding Put/Patch/Delete to the exported ContextInterface is a breaking change for any downstream code that provides its own implementation of this interface (their types will no longer satisfy it). To keep backward compatibility, consider keeping ContextInterface as-is and instead adding Put/Patch/Delete methods on the exported Context wrapper (delegating to the embedded value via optional interface assertions), or introduce a new extended interface and accept that type where needed.
| libraryDecls := map[string][]cel.FunctionOpt{ | ||
| "Get": buildGetOverloads("pascal"), | ||
| "Post": buildPostOverloads("pascal"), | ||
| "Put": buildPutOverloads("pascal"), | ||
| "Patch": buildPatchOverloads("pascal"), | ||
| "Delete": buildDeleteOverloads("pascal"), | ||
| "Client": buildClientOverloads("pascal"), | ||
| } | ||
|
|
||
| if c.version.AtLeast(version.MajorMinor(1, 18)) { | ||
| libraryDecls["get"] = buildGetOverloads("camel") | ||
| libraryDecls["post"] = buildPostOverloads("camel") | ||
| libraryDecls["put"] = buildPutOverloads("camel") | ||
| libraryDecls["patch"] = buildPatchOverloads("camel") | ||
| libraryDecls["delete"] = buildDeleteOverloads("camel") | ||
| libraryDecls["client"] = buildClientOverloads("camel") |
There was a problem hiding this comment.
The PR description is about adding DELETE support, but this change set also introduces PUT and PATCH declarations/overloads. If those are coming from a dependent PR, it would be better to rebase/split so this PR only contains the DELETE-related diff (or update the PR description/scope accordingly) to keep review and release notes accurate.
| body, err := buildRequestData(data) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to encode request data: %w", err) | ||
| } |
There was a problem hiding this comment.
Put/Patch now call buildRequestData(), but buildRequestData currently hard-codes "HTTP POST" in its encode error message. This will produce confusing errors for PUT/PATCH requests; consider making the error message method-agnostic (or pass the HTTP verb into buildRequestData and include it in the message).
| func Test_impl_put_request(t *testing.T) { | ||
| base, err := compiler.NewBaseEnv() | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, base) | ||
|
|
||
| ctx := Context{&contextImpl{ | ||
| client: testClient{ | ||
| doFunc: func(req *http.Request) (*http.Response, error) { | ||
| assert.Equal(t, req.URL.String(), "http://localhost:8080") | ||
| assert.Equal(t, req.Method, "PUT") | ||
|
|
||
| var data any | ||
| err := json.NewDecoder(req.Body).Decode(&data) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, data.(map[string]any)["key"], "value") | ||
| assert.Equal(t, data.(map[string]any)["foo"], float64(2)) | ||
|
|
||
| return &http.Response{StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader(`{"body": "updated"}`))}, nil | ||
| }, | ||
| }, | ||
| }} | ||
|
|
||
| env, err := base.Extend( | ||
| Lib(&ctx, version.MajorMinor(1, 18)), | ||
| ) | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, env) | ||
| ast, issues := env.Compile(`http.Put("http://localhost:8080", { "key": dyn("value"), "foo": dyn(2) })`) | ||
| fmt.Println(issues.String()) |
There was a problem hiding this comment.
The newly added PUT/PATCH/DELETE tests only exercise the PascalCase methods (http.Put/http.Patch/http.Delete). Since the library also conditionally registers the camelCase variants for version >= 1.18, please add at least one test per verb that compiles/evals http.put/http.patch/http.delete to ensure those overloads are actually wired correctly.
| ast, issues := env.Compile(`http.Put("http://localhost:8080", { "key": dyn("value"), "foo": dyn(2) })`) | ||
| fmt.Println(issues.String()) | ||
| assert.Nil(t, issues) | ||
| assert.NotNil(t, ast) | ||
| prog, err := env.Program(ast) | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, prog) | ||
|
|
||
| out, _, err := prog.Eval(map[string]any{}) | ||
| assert.NoError(t, err) | ||
| body := out.Value().(map[string]any) | ||
| assert.Equal(t, body["body"], "updated") | ||
| assert.Equal(t, body["statusCode"], http.StatusOK) | ||
| } | ||
|
|
||
| func Test_impl_put_request_with_headers(t *testing.T) { | ||
| base, err := compiler.NewBaseEnv() | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, base) | ||
|
|
||
| ctx := Context{&contextImpl{ | ||
| client: testClient{ | ||
| doFunc: func(req *http.Request) (*http.Response, error) { | ||
| assert.Equal(t, req.URL.String(), "http://localhost:8080") | ||
| assert.Equal(t, req.Method, "PUT") | ||
| assert.Equal(t, req.Header.Get("Authorization"), "Bearer token") | ||
|
|
||
| var data any | ||
| err := json.NewDecoder(req.Body).Decode(&data) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, data.(map[string]any)["key"], "value") | ||
|
|
||
| return &http.Response{StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader(`{"body": "updated"}`))}, nil | ||
| }, | ||
| }, | ||
| }} | ||
|
|
||
| env, err := base.Extend( | ||
| Lib(&ctx, version.MajorMinor(1, 18)), | ||
| ) | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, env) | ||
| ast, issues := env.Compile(`http.Put("http://localhost:8080", {"key": "value"}, {"Authorization": "Bearer token"})`) | ||
| fmt.Println(issues.String()) | ||
| assert.Nil(t, issues) |
There was a problem hiding this comment.
These fmt.Println(issues.String()) statements add noisy output to the test suite and aren't needed for assertions. Consider removing them (or only logging when issues != nil and issues.Err() != nil).
Explanation
This PR adds HTTP DELETE method support to the Kyverno SDK CEL HTTP library. Currently, the library only supports GET, POST operations. The DELETE method is essential for removing resources from remote HTTP endpoints. This enhancement allows users to make DELETE requests with custom headers from within CEL expressions, enabling complete resource lifecycle management in policy expressions, including resource deletion scenarios.
Related issue
Fixes kyverno/kyverno#15472
Dependent on the merge of PR #54, post which we can rebase this branch on main and review
Proposed Changes
The DELETE implementation mirrors the existing POST method pattern to maintain consistency across the codebase. These changes will add support to the delete method (say, for example http.Delete() being used in the CEL expressions)
Checklist
Further Comments