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

[release/v0.5] Cherry-pick fixes needed for v0.5 #1749

Merged
merged 7 commits into from
Aug 2, 2023

Conversation

arkodg and others added 4 commits July 28, 2023 15:25
…ating infinite reconciliation loop (envoyproxy#1594)

* fix: envoy proxy resource apply bug.

Signed-off-by: qicz <[email protected]>

* update pointer.

Signed-off-by: qicz <[email protected]>

* add comment

Signed-off-by: qicz <[email protected]>

* update cm cmp logic.

Signed-off-by: qicz <[email protected]>

* fix lint

Signed-off-by: qicz <[email protected]>

* add probe field default value.

Signed-off-by: qicz <[email protected]>

* fix uts

Signed-off-by: qicz <[email protected]>

* align probe

Signed-off-by: qicz <[email protected]>

* optimize deploy compare logic

Signed-off-by: qicz <[email protected]>

* add compare deploy uts

Signed-off-by: qicz <[email protected]>

* rm cm binarydata cmp

Signed-off-by: qicz <[email protected]>

* rm deploy cmp logic

Signed-off-by: qicz <[email protected]>

* fix ut

Signed-off-by: qicz <[email protected]>

* fix lint

Signed-off-by: qicz <[email protected]>

---------

Signed-off-by: qicz <[email protected]>
Signed-off-by: qi <[email protected]>
(cherry picked from commit 9ba9103)
* 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 050394ba..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: envoyproxy#1715

Signed-off-by: Arko Dasgupta <[email protected]>
(cherry picked from commit 5b72451)
container port

Signed-off-by: zirain <[email protected]>
(cherry picked from commit 4bba03a)
@arkodg arkodg requested a review from a team as a code owner August 2, 2023 17:15
arkodg added 2 commits August 2, 2023 10:18
* Add user docs for EnvoyPatchPolicy

Relates to envoyproxy#24

Signed-off-by: Arko Dasgupta <[email protected]>

* nits

Signed-off-by: Arko Dasgupta <[email protected]>

* wrap up

Signed-off-by: Arko Dasgupta <[email protected]>

* lint

Signed-off-by: Arko Dasgupta <[email protected]>

* address comments && fix config

Signed-off-by: Arko Dasgupta <[email protected]>

---------

Signed-off-by: Arko Dasgupta <[email protected]>
(cherry picked from commit 27b0939)
* 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 <[email protected]>
(cherry picked from commit a7784c5)
@arkodg arkodg added this to the 0.5.0 milestone Aug 2, 2023
@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Merging #1749 (d52084f) into release/v0.5 (c303835) will increase coverage by 0.00%.
The diff coverage is 63.26%.

@@              Coverage Diff              @@
##           release/v0.5    #1749   +/-   ##
=============================================
  Coverage         64.79%   64.80%           
=============================================
  Files                83       83           
  Lines             11912    11938   +26     
=============================================
+ Hits               7718     7736   +18     
- Misses             3711     3719    +8     
  Partials            483      483           
Files Changed Coverage Δ
internal/gatewayapi/runner/runner.go 25.49% <0.00%> (-0.51%) ⬇️
internal/provider/kubernetes/controller.go 47.97% <0.00%> (-0.47%) ⬇️
internal/status/envoypatchpolicy.go 0.00% <0.00%> (ø)
internal/xds/server/runner/runner.go 29.80% <0.00%> (-0.29%) ⬇️
internal/gatewayapi/envoypatchpolicy.go 69.51% <100.00%> (ø)
internal/gatewayapi/route.go 88.36% <100.00%> (ø)
...ternal/infrastructure/kubernetes/proxy/resource.go 93.46% <100.00%> (+0.45%) ⬆️
...frastructure/kubernetes/proxy/resource_provider.go 86.39% <100.00%> (+0.16%) ⬆️
...al/infrastructure/kubernetes/ratelimit/resource.go 96.21% <100.00%> (+0.04%) ⬆️
...tructure/kubernetes/ratelimit/resource_provider.go 97.16% <100.00%> (+0.04%) ⬆️

... and 1 file with indirect coverage changes

@arkodg arkodg merged commit a498365 into envoyproxy:release/v0.5 Aug 2, 2023
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.

4 participants