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

e2e & misc fixes for EnvoyPatchPolicy #1738

Merged
merged 9 commits into from
Aug 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/v1alpha1/envoypatchpolicy_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ type EnvoyPatchPolicySpec struct {
// the priority i.e. int32.min has the highest priority and
// int32.max has the lowest priority.
// Defaults to 0.
Priority int32 `json:"priority"`
Priority int32 `json:"priority,omitempty"`
Copy link
Contributor

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
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

}

// EnvoyPatchType specifies the types of Envoy patching mechanisms.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ spec:
- JSONPatch
type: string
required:
- priority
- targetRef
- type
type: object
Expand Down
24 changes: 12 additions & 12 deletions docs/latest/user/envoy-patch-policy.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,19 +86,19 @@ spec:
name: default/eg/http
operation:
op: add
path: "/default_filter_chain/filters/0/typed_config"
path: "/default_filter_chain/filters/0/typed_config/local_reply_config"
value:
local_reply_config:
mappers:
- filter:
status_code_filter:
comparison:
op: EQ
value:
default_value: 404
runtime_key: key_b
body:
inline_string: "could not find what you are looking for"
mappers:
- filter:
status_code_filter:
comparison:
op: EQ
value:
default_value: 404
runtime_key: key_b
status_code: 406
body:
inline_string: "could not find what you are looking for"
EOF
```

Expand Down
2 changes: 2 additions & 0 deletions examples/redis/redis.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ data:
type: Kubernetes
gateway:
controllerName: gateway.envoyproxy.io/gatewayclass-controller
extensionApis:
Copy link
Contributor

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
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
Contributor

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

enableEnvoyPatchPolicy: true
rateLimit:
backend:
type: Redis
Expand Down
23 changes: 13 additions & 10 deletions internal/provider/kubernetes/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,16 +240,19 @@
}

// Add all EnvoyPatchPolicies
envoyPatchPolicies := egv1a1.EnvoyPatchPolicyList{}
if err := r.client.List(ctx, &envoyPatchPolicies); err != nil {
return reconcile.Result{}, fmt.Errorf("error listing envoypatchpolicies: %v", err)
}
for _, policy := range envoyPatchPolicies.Items {
policy := policy
// Discard Status to reduce memory consumption in watchable
// It will be recomputed by the gateway-api layer
policy.Status = egv1a1.EnvoyPatchPolicyStatus{}
resourceTree.EnvoyPatchPolicies = append(resourceTree.EnvoyPatchPolicies, &policy)
if r.envoyGateway.ExtensionAPIs != nil && r.envoyGateway.ExtensionAPIs.EnableEnvoyPatchPolicy {
envoyPatchPolicies := egv1a1.EnvoyPatchPolicyList{}
if err := r.client.List(ctx, &envoyPatchPolicies); err != nil {
return reconcile.Result{}, fmt.Errorf("error listing envoypatchpolicies: %v", err)
}

Check warning on line 247 in internal/provider/kubernetes/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/kubernetes/controller.go#L244-L247

Added lines #L244 - L247 were not covered by tests

for _, policy := range envoyPatchPolicies.Items {
policy := policy
// Discard Status to reduce memory consumption in watchable
// It will be recomputed by the gateway-api layer
policy.Status = egv1a1.EnvoyPatchPolicyStatus{}
resourceTree.EnvoyPatchPolicies = append(resourceTree.EnvoyPatchPolicies, &policy)
}

Check warning on line 255 in internal/provider/kubernetes/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/kubernetes/controller.go#L249-L255

Added lines #L249 - L255 were not covered by tests
}

// For this particular Gateway, and all associated objects, check whether the
Expand Down
3 changes: 3 additions & 0 deletions internal/status/envoypatchpolicy.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
if c.Type == string(egv1a1.PolicyConditionProgrammed) {
return
}
if c.Type == string(gwv1a2.PolicyConditionAccepted) && c.Status == metav1.ConditionFalse {
return
}

Check warning on line 30 in internal/status/envoypatchpolicy.go

View check run for this annotation

Codecov / codecov/patch

internal/status/envoypatchpolicy.go#L28-L30

Added lines #L28 - L30 were not covered by tests
}

cond := newCondition(string(egv1a1.PolicyConditionProgrammed), metav1.ConditionTrue, string(egv1a1.PolicyReasonProgrammed), message, time.Now(), 0)
Expand Down
48 changes: 48 additions & 0 deletions test/e2e/testdata/envoy-patch-policy.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
---
apiVersion: gateway.networking.k8s.io/v1beta1
kind: HTTPRoute
metadata:
name: http-envoy-patch-policy
namespace: gateway-conformance-infra
spec:
parentRefs:
- name: same-namespace
rules:
- backendRefs:
- name: infra-backend-v1
port: 8080
matches:
- path:
type: PathPrefix
value: /foo
---
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: EnvoyPatchPolicy
metadata:
name: custom-response-patch-policy
namespace: gateway-conformance-infra
spec:
targetRef:
group: gateway.networking.k8s.io
kind: Gateway
name: same-namespace
namespace: gateway-conformance-infra
type: JSONPatch
jsonPatches:
- type: "type.googleapis.com/envoy.config.listener.v3.Listener"
name: "gateway-conformance-infra/same-namespace/http"
operation:
op: add
path: "/default_filter_chain/filters/0/typed_config/local_reply_config"
value:
mappers:
- filter:
status_code_filter:
comparison:
op: EQ
value:
default_value: 404
runtime_key: key_b
status_code: 406
body:
inline_string: "not acceptable"
61 changes: 61 additions & 0 deletions test/e2e/tests/envoy-patch-policy.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// Copyright Envoy Gateway Authors
// SPDX-License-Identifier: Apache-2.0
// The full text of the Apache license is available in the LICENSE file at
// the root of the repo.

//go:build e2e
// +build e2e

package tests

import (
"testing"

"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/gateway-api/conformance/utils/http"
"sigs.k8s.io/gateway-api/conformance/utils/kubernetes"
"sigs.k8s.io/gateway-api/conformance/utils/suite"
)

func init() {
ConformanceTests = append(ConformanceTests, EnvoyPatchPolicyTest)
}

var EnvoyPatchPolicyTest = suite.ConformanceTest{
ShortName: "EnvoyPatchPolicy",
Description: "update xds using EnvoyPatchPolicy",
Manifests: []string{"testdata/envoy-patch-policy.yaml"},
Test: func(t *testing.T, suite *suite.ConformanceTestSuite) {
t.Run("envoy patch policy", func(t *testing.T) {
ns := "gateway-conformance-infra"
routeNN := types.NamespacedName{Name: "http-envoy-patch-policy", Namespace: ns}
gwNN := types.NamespacedName{Name: "same-namespace", Namespace: ns}
gwAddr := kubernetes.GatewayAndHTTPRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gwNN), routeNN)
OkResp := http.ExpectedResponse{
Request: http.Request{
Path: "/foo",
},
Response: http.Response{
StatusCode: 200,
},
Namespace: ns,
}

// Send a request to an valid path and expect a successful response
http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, OkResp)

customResp := http.ExpectedResponse{
Request: http.Request{
Path: "/bar",
},
Response: http.Response{
StatusCode: 406,
},
Namespace: ns,
}

// Send a request to an invalid path and expect a custom response
http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, customResp)
})
},
}