feat: Adding extension server policy handling. #3371

liorokman · 2024-05-11T15:51:14Z

What this PR does / why we need it:
This PR implements extension server policies which can be attached to HTTP listeners.

See the full description in the issue (#2975)

Which issue(s) this PR fixes:
Fixes #2975

codecov · 2024-05-12T09:33:56Z

Codecov Report

Attention: Patch coverage is 56.94444% with 124 lines in your changes are missing coverage. Please review.

Project coverage is 67.13%. Comparing base (c2c9b43) to head (62d1a8a).

Files	Patch %	Lines
internal/gatewayapi/runner/runner.go	22.22%	19 Missing and 2 partials ⚠️
internal/provider/kubernetes/status.go	16.66%	19 Missing and 1 partial ⚠️
internal/gatewayapi/resource.go	0.00%	18 Missing ⚠️
internal/provider/kubernetes/controller.go	63.82%	14 Missing and 3 partials ⚠️
internal/extension/registry/xds_hook.go	0.00%	16 Missing ⚠️
internal/gatewayapi/extensionserverpolicy.go	84.90%	10 Missing and 6 partials ⚠️
internal/provider/kubernetes/status_updater.go	25.00%	8 Missing and 1 partial ⚠️
internal/extension/testutils/hooks.go	62.50%	3 Missing and 3 partials ⚠️
internal/message/types.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3371      +/-   ##
==========================================
- Coverage   67.22%   67.13%   -0.10%     
==========================================
  Files         166      167       +1     
  Lines       19720    19961     +241     
==========================================
+ Hits        13257    13400     +143     
- Misses       5507     5590      +83     
- Partials      956      971      +15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

guydc · 2024-05-16T16:54:58Z

cc @AliceProxy

liorokman · 2024-05-19T13:57:26Z

/retest

liorokman · 2024-05-19T14:10:53Z

/retest

liorokman · 2024-05-19T16:14:10Z

/retest

liorokman · 2024-05-22T14:03:23Z

@AliceProxy @arkodg @guydc PTAL

arkodg · 2024-05-22T22:28:09Z

internal/gatewayapi/resource.go

@@ -56,6 +56,7 @@ type Resources struct {
 	SecurityPolicies       []*egv1a1.SecurityPolicy       `json:"securityPolicies,omitempty" yaml:"securityPolicies,omitempty"`
 	BackendTLSPolicies     []*gwapiv1a3.BackendTLSPolicy  `json:"backendTLSPolicies,omitempty" yaml:"backendTLSPolicies,omitempty"`
 	EnvoyExtensionPolicies []*egv1a1.EnvoyExtensionPolicy `json:"envoyExtensionPolicies,omitempty" yaml:"envoyExtensionPolicies,omitempty"`
+	ExtServerPolicies      []unstructured.Unstructured    `json:"extServerPolicies,omitempty" yaml:"extServerPolicies,omitempty"`


Suggested change

ExtServerPolicies []unstructured.Unstructured `json:"extServerPolicies,omitempty" yaml:"extServerPolicies,omitempty"`

ExtensionServerPolicies []unstructured.Unstructured `json:"extensionServerPolicies,omitempty" yaml:"extServerPolicies,omitempty"`

arkodg · 2024-05-22T22:30:57Z

internal/gatewayapi/extensionserverpolicy.go

+		for _, currTarget := range targetRefs {
+			if currTarget.Kind != KindGateway {
+				// TODO: mark this targetRef as an error
+				t.Logger.Sugar().Errorf("extension policy %s doesn't target a Gateway", policy.GetName())


we've tried to keep the gateway-api lib as minimal as possible, so it can be reused again in other places in the future, can we instead collect errs here and return and let the runner log them ?

arkodg · 2024-05-22T22:31:57Z

internal/gatewayapi/extensionserverpolicy.go

+	"github.com/envoyproxy/gateway/internal/utils"
+)
+
+func (t *Translator) ProcessExtensionServerPolicies(policies []unstructured.Unstructured,


I dont see this being used anywhere

It is being used in translator.go.

arkodg · 2024-05-22T22:33:30Z

internal/utils/field/field.go

@@ -12,6 +12,22 @@ import (
 	"unicode"
 )

+func SetMapValues(input map[string]any, fieldName string, fieldValue any) {


can we reuse SetValue ?

I don't think so. SetValue assumes that the input is a struct, this function assumes that the input is a map[string]any.

It's possible to force SetValue to do this, but I don't think that this would make the code clearer.

arkodg · 2024-05-22T22:35:38Z

internal/provider/kubernetes/status.go

+							panic(err)
+						}
+						tCopy := t.DeepCopy()
+						tCopy.Object["status"] = *val


lets make sure the field matches the type gwapiv1a2.PolicyStatus

This is already validated by the compiler.

The val object is provided by the update parameter to the function invoked by the HandleSubscription function. It is a generic type, and it is narrowed to gwapiv1a2.PolicyStatus in line 407:

https://github.com/envoyproxy/gateway/pull/3371/files/cc736cb4086b88cfdfe6db20766a36e847aa4cae#diff-d017a6c31bddedd24ec2acb631ee77e2aab14a461ad17ee416f6563c0253f413R407

arkodg · 2024-05-22T22:36:19Z

internal/provider/kubernetes/controller.go

+	mergeGateways     sets.Set[string]
+	resources         *message.ProviderResources
+	extGVKs           []schema.GroupVersionKind
+	extServerPolicies []schema.GroupVersionKind


why not reuse extGVKs here ?

Like @AliceProxy wrote, keeping it separate captures the fact that these GVKs are used for different purposes in different places.

Maybe I should rename extGVKs to extRouteFilters ?

AliceProxy

No complaints from me. I 👍'd a few of Arko's suggestion, but I don't think this should really break any existing extensions other than needing to update the function signatures when they pull the latest package import with these changes.

internal/gatewayapi/extensionserverpolicy.go

+	"github.com/envoyproxy/gateway/internal/utils"
+)
+
+func (t *Translator) ProcessExtensionServerPolicies(policies []unstructured.Unstructured,


internal/provider/kubernetes/controller.go

+	mergeGateways     sets.Set[string]
+	resources         *message.ProviderResources
+	extGVKs           []schema.GroupVersionKind
+	extServerPolicies []schema.GroupVersionKind


liorokman · 2024-05-23T09:52:22Z

/retest

liorokman · 2024-05-23T10:11:49Z

/retest

arkodg · 2024-05-23T21:06:28Z

internal/gatewayapi/translator.go

@@ -101,12 +102,15 @@ type Translator struct {

 	// Namespace is the namespace that Envoy Gateway runs in.
 	Namespace string
+
+	Logger logging.Logger


still seeing the Logger and LoggedErrors being defined here
can we follow the approach taken in the gatewayapi lib where we have a top level errs field e.g.

gateway/internal/gatewayapi/backendtrafficpolicy.go

Line 299 in cffaf27

err, errs error

, in this case it would be in the Translate function and we'd append the errors returned from individual functions e.g.

gateway/internal/gatewayapi/backendtrafficpolicy.go

Line 321 in cffaf27

errs = errors.Join(errs, err)

The difference is that in BackendTrafficPolicy the errors are aggregated into a single error and then stored in the policy status.

In this policy, the decision was not to update the policy status section for errors, which means that errors would need to be propagated all the way to the runner in order to be logged. This is a pretty large structural change.

I will remove the logger, but I think keeping the LoggedErrors approach is better.

Signed-off-by: Lior Okman <lior.okman@sap.com>

runner Signed-off-by: Lior Okman <lior.okman@sap.com>

Signed-off-by: Lior Okman <lior.okman@sap.com>

liorokman · 2024-05-25T14:31:24Z

/retest

liorokman requested a review from a team as a code owner May 11, 2024 15:51

liorokman marked this pull request as draft May 11, 2024 15:51

liorokman force-pushed the listener-hook-context branch from 754eb7b to ee207be Compare May 11, 2024 16:13

liorokman force-pushed the listener-hook-context branch 2 times, most recently from e227366 to bc120d8 Compare May 15, 2024 08:50

liorokman marked this pull request as ready for review May 16, 2024 05:09

guydc requested a review from AliceProxy May 16, 2024 16:55

liorokman force-pushed the listener-hook-context branch from d5d5bd9 to 7ba5af2 Compare May 22, 2024 11:02

arkodg reviewed May 22, 2024

View reviewed changes

AliceProxy previously approved these changes May 22, 2024

View reviewed changes

This was referenced May 23, 2024

Add a post translation hook for non-HTTP listener #2673

Open

Set unique name for FilterChain in xDS listener's FilterChains #3461

Open

liorokman dismissed AliceProxy’s stale review via 079425f May 23, 2024 09:34

liorokman force-pushed the listener-hook-context branch from 079425f to 486aa61 Compare May 23, 2024 09:46

arkodg reviewed May 23, 2024

View reviewed changes

guydc mentioned this pull request May 24, 2024

E2E: Extension Server #3464

Open

liorokman force-pushed the listener-hook-context branch from cca4bda to cd72c97 Compare May 25, 2024 09:18

zirain mentioned this pull request May 25, 2024

chore: fix expired cert #3477

Closed

liorokman added 14 commits May 25, 2024 15:07

Adding extension server policy handling.

55f9fbc

Signed-off-by: Lior Okman <lior.okman@sap.com>

added missing license header

84f94a6

Signed-off-by: Lior Okman <lior.okman@sap.com>

Make the linter happy

ac765e1

Signed-off-by: Lior Okman <lior.okman@sap.com>

Make spell-checking linter satisfied

86dc289

Signed-off-by: Lior Okman <lior.okman@sap.com>

Added some clarifying comments, fixed watchable key deletion in the

939d936

runner Signed-off-by: Lior Okman <lior.okman@sap.com>

Added tests

af01094

Signed-off-by: Lior Okman <lior.okman@sap.com>

Add tests

7c2e6ef

Signed-off-by: Lior Okman <lior.okman@sap.com>

Finishing the rebase to the latest main.

2d17939

Signed-off-by: Lior Okman <lior.okman@sap.com>

Added an XDS level unit test.

3e29b34

Signed-off-by: Lior Okman <lior.okman@sap.com>

Made the GCI linter happy

6c47094

Signed-off-by: Lior Okman <lior.okman@sap.com>

Some more unit tests.

4acf8c0

Signed-off-by: Lior Okman <lior.okman@sap.com>

Resolved comments provided in the review.

06698f2

Signed-off-by: Lior Okman <lior.okman@sap.com>

Regenerate deepcopies.

660188d

Signed-off-by: Lior Okman <lior.okman@sap.com>

Removed the logger from the Translator type.

62d1a8a

Signed-off-by: Lior Okman <lior.okman@sap.com>

liorokman force-pushed the listener-hook-context branch from cd72c97 to 62d1a8a Compare May 25, 2024 12:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adding extension server policy handling. #3371

feat: Adding extension server policy handling. #3371

liorokman commented May 11, 2024

codecov bot commented May 12, 2024 •

edited

guydc commented May 16, 2024

liorokman commented May 19, 2024

liorokman commented May 19, 2024

liorokman commented May 19, 2024

liorokman commented May 22, 2024

arkodg May 22, 2024

liorokman May 23, 2024

arkodg May 22, 2024

liorokman May 23, 2024

arkodg May 22, 2024

This comment was marked as duplicate.

liorokman May 23, 2024

arkodg May 22, 2024

liorokman May 23, 2024

arkodg May 22, 2024

liorokman May 23, 2024

arkodg May 22, 2024

This comment was marked as duplicate.

liorokman May 23, 2024

AliceProxy left a comment

This comment was marked as duplicate.

This comment was marked as duplicate.

liorokman commented May 23, 2024

liorokman commented May 23, 2024

arkodg May 23, 2024

liorokman May 25, 2024

liorokman commented May 25, 2024

	ExtServerPolicies []unstructured.Unstructured `json:"extServerPolicies,omitempty" yaml:"extServerPolicies,omitempty"`
	ExtensionServerPolicies []unstructured.Unstructured `json:"extensionServerPolicies,omitempty" yaml:"extServerPolicies,omitempty"`

feat: Adding extension server policy handling. #3371

Are you sure you want to change the base?

feat: Adding extension server policy handling. #3371

Conversation

liorokman commented May 11, 2024

codecov bot commented May 12, 2024 • edited

Codecov Report

guydc commented May 16, 2024

liorokman commented May 19, 2024

liorokman commented May 19, 2024

liorokman commented May 19, 2024

liorokman commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

Choose a reason for hiding this comment

AliceProxy left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

This comment was marked as duplicate.

liorokman commented May 23, 2024

liorokman commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liorokman commented May 25, 2024

codecov bot commented May 12, 2024 •

edited