Skip to content

e2e & misc fixes for EnvoyPatchPolicy#1738

Merged
arkodg merged 9 commits intoenvoyproxy:mainfrom
arkodg:epp-e2e
Aug 2, 2023
Merged

e2e & misc fixes for EnvoyPatchPolicy#1738
arkodg merged 9 commits intoenvoyproxy:mainfrom
arkodg:epp-e2e

Conversation

@arkodg
Copy link
Copy Markdown
Contributor

@arkodg arkodg commented Aug 1, 2023

misc fixes:

  • Only add EnvoyPatchPolicy to provider resources if enabled in
    EnvoyGateway API
  • Add omitempty tag to priority field to make it optional

e2e

  • Use LocalReplyConfig to return a custom status code 406 when there is no valid route match

Relates to #24

@arkodg arkodg requested a review from a team as a code owner August 1, 2023 20:49
@arkodg arkodg added this to the 0.5.0 milestone Aug 1, 2023
@arkodg arkodg mentioned this pull request Aug 1, 2023
6 tasks
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 1, 2023

Codecov Report

Merging #1738 (0bac038) into main (b4d4e2f) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1738      +/-   ##
==========================================
- Coverage   64.96%   64.94%   -0.02%     
==========================================
  Files          84       84              
  Lines       12010    12014       +4     
==========================================
+ Hits         7802     7803       +1     
- Misses       3720     3723       +3     
  Partials      488      488              
Files Changed Coverage Δ
internal/provider/kubernetes/controller.go 49.74% <0.00%> (+0.67%) ⬆️
internal/status/envoypatchpolicy.go 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

@arkodg arkodg changed the title Add E2E for EnvoyPatchPolicy e2e & misc fixes for EnvoyPatchPolicy Aug 1, 2023
@arkodg arkodg marked this pull request as draft August 1, 2023 22:30
Arko Dasgupta added 9 commits August 1, 2023 22:17
* Use LocalReplyConfig to return a custom
status code `418` when there is no valid route match

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
* Only add EnvoyPatchPolicy to provider resources if enabled in
  EnvoyGateway API

* Add `omitempty` tag to `priority` field to make it optional

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
(cherry picked from commit 93c92b9)
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg marked this pull request as ready for review August 2, 2023 05:36
@arkodg arkodg requested review from a team, chauhanshubham, qicz and zhaohuabing and removed request for a team August 2, 2023 05:36
// int32.max has the lowest priority.
// Defaults to 0.
Priority int32 `json:"priority"`
Priority int32 `json:"priority,omitempty"`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this's OK to me, but I recall the api guide said should be a pointer?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

its still required internally, omitempty allows the user to skip setting it when creating the resource and use the default value / 0 instead

type: Kubernetes
gateway:
controllerName: gateway.envoyproxy.io/gatewayclass-controller
extensionApis:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this seems not related to an redisexample, can you address some comment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is the yaml used in e2e to update the startup config, I can start renaming and breaking things in separate files, would you prefer that ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes, we can do that later

@arkodg arkodg requested review from a team and Alice-Lilith August 2, 2023 05:43
@arkodg arkodg merged commit a7784c5 into envoyproxy:main Aug 2, 2023
@arkodg arkodg deleted the epp-e2e branch August 2, 2023 05:51
arkodg added a commit to arkodg/gateway that referenced this pull request Aug 2, 2023
* Add E2E for EnvoyPatchPolicy

* Use LocalReplyConfig to return a custom
status code `406` when there is no valid route match

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
(cherry picked from commit a7784c5)
arkodg added a commit that referenced this pull request Aug 2, 2023
* refactor: set defaults in Deployment, else k8s sets them for you, creating infinite reconciliation loop (#1594)

* fix: envoy proxy resource apply bug.

Signed-off-by: qicz <qiczzhu@gmail.com>

* update pointer.

Signed-off-by: qicz <qiczzhu@gmail.com>

* add comment

Signed-off-by: qicz <qiczzhu@gmail.com>

* update cm cmp logic.

Signed-off-by: qicz <qiczzhu@gmail.com>

* fix lint

Signed-off-by: qicz <qiczzhu@gmail.com>

* add probe field default value.

Signed-off-by: qicz <qiczzhu@gmail.com>

* fix uts

Signed-off-by: qicz <qiczzhu@gmail.com>

* align probe

Signed-off-by: qicz <qiczzhu@gmail.com>

* optimize deploy compare logic

Signed-off-by: qicz <qiczzhu@gmail.com>

* add compare deploy uts

Signed-off-by: qicz <qiczzhu@gmail.com>

* rm cm binarydata cmp

Signed-off-by: qicz <qiczzhu@gmail.com>

* rm deploy cmp logic

Signed-off-by: qicz <qiczzhu@gmail.com>

* fix ut

Signed-off-by: qicz <qiczzhu@gmail.com>

* fix lint

Signed-off-by: qicz <qiczzhu@gmail.com>

---------

Signed-off-by: qicz <qiczzhu@gmail.com>
Signed-off-by: qi <qiczzhu@gmail.com>
(cherry picked from commit 9ba9103)

* DeepCopy resources that require status updates (#1723)

* Was seeing constant churn between provider runner publishing resources
and gateway-api runner receiving them.

* Tried to debug it by printing the o/p of `cmp.Diff` between current
  and previous values
```
diff --git a/internal/gatewayapi/runner/runner.go b/internal/gatewayapi/runner/runner.go
index 050394b..50d09f6f 100644
--- a/internal/gatewayapi/runner/runner.go
+++ b/internal/gatewayapi/runner/runner.go
@@ -8,6 +8,7 @@ package runner
 import (
        "context"

+       "github.com/google/go-cmp/cmp"
        "k8s.io/apimachinery/pkg/runtime/schema"
        "sigs.k8s.io/gateway-api/apis/v1beta1"
        "sigs.k8s.io/yaml"
@@ -49,6 +50,7 @@ func (r *Runner) Start(ctx context.Context) error {
 }

 func (r *Runner) subscribeAndTranslate(ctx context.Context) {
+       prev := &gatewayapi.Resources{}
        message.HandleSubscription(r.ProviderResources.GatewayAPIResources.Subscribe(ctx),
                func(update message.Update[string, *gatewayapi.Resources]) {
                        val := update.Value
@@ -56,6 +58,9 @@ func (r *Runner) subscribeAndTranslate(ctx context.Context) {
                        if update.Delete || val == nil {
                                return
                        }
+                       diff := cmp.Diff(prev, val)
+                       r.Logger.WithValues("output", "diff").Info(diff)
+                       prev = val.DeepCopy()

                        // Translate and publish IRs.
                        t := &gatewayapi.Translator{
```

Here's the o/p and its empty
```
2023-07-27T23:55:29.795Z	INFO	gateway-api	runner/runner.go:62		{"runner": "gateway-api", "output": "diff"}
```

* Using a DeepCopy for resources that were updating the `Status`
  subresource seems to have solved the issue, which implies that
  watchable doesnt like clients to mutate the value, even though they
  are meant to be a `DeepCopy`

Fixes: #1715

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
(cherry picked from commit 5b72451)

* observability: add container port for metrics (#1736)

container port

Signed-off-by: zirain <zirain2009@gmail.com>
(cherry picked from commit 4bba03a)

* docs: Add user docs for EnvoyPatchPolicy (#1733)

* Add user docs for EnvoyPatchPolicy

Relates to #24

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* nits

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* wrap up

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* lint

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

* address comments && fix config

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

---------

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
(cherry picked from commit 27b0939)

* e2e & misc fixes for EnvoyPatchPolicy (#1738)

* Add E2E for EnvoyPatchPolicy

* Use LocalReplyConfig to return a custom
status code `406` when there is no valid route match

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
(cherry picked from commit a7784c5)

---------

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Co-authored-by: qi <qiczzhu@gmail.com>
Co-authored-by: zirain <zirain2009@gmail.com>
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.

3 participants