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.

Sign up for GitHub

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

Default ProxyType for builtin extensions #17657

Merged
merged 2 commits into from
Jun 12, 2023
Merged

Default ProxyType for builtin extensions #17657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
131d234
default proxy type to `"connect-proxy"` for rate limit and lua extens…
cthain Jun 12, 2023
1adc487
default `ProxyType` for prop override extension
cthain Jun 12, 2023
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
5 changes: 4 additions & 1 deletion agent/envoyextensions/builtin/http/localratelimit/ratelimit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ func (r *ratelimit) fromArguments(args map[string]interface{}) error {
if err := mapstructure.Decode(args, r); err != nil {
return fmt.Errorf("error decoding extension arguments: %v", err)
}
if r.ProxyType == "" {
r.ProxyType = string(api.ServiceKindConnectProxy)
}
return r.validate()
}

Expand Down Expand Up @@ -188,7 +191,7 @@ func (r ratelimit) PatchFilter(p extensioncommon.FilterPayload) (*envoy_listener
}

func validateProxyType(t string) error {
if t != "connect-proxy" {
if t != string(api.ServiceKindConnectProxy) {
return fmt.Errorf("unexpected ProxyType %q", t)
}

Expand Down
24 changes: 24 additions & 0 deletions agent/envoyextensions/builtin/http/localratelimit/ratelimit_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,30 @@ func TestConstructor(t *testing.T) {
expectedErrMsg: "cannot parse 'FilterEnforced', -1 overflows uint",
ok: false,
},
"invalid proxy type": {
arguments: makeArguments(map[string]interface{}{
"ProxyType": "invalid",
"FillInterval": 30,
"MaxTokens": 20,
"TokensPerFill": 5,
}),
expectedErrMsg: `unexpected ProxyType "invalid"`,
ok: false,
},
"default proxy type": {
arguments: makeArguments(map[string]interface{}{
"FillInterval": 30,
"MaxTokens": 20,
"TokensPerFill": 5,
}),
expected: ratelimit{
ProxyType: "connect-proxy",
MaxTokens: intPointer(20),
FillInterval: intPointer(30),
TokensPerFill: intPointer(5),
},
ok: true,
},
"valid everything": {
arguments: makeArguments(map[string]interface{}{
"ProxyType": "connect-proxy",
Expand Down
5 changes: 4 additions & 1 deletion agent/envoyextensions/builtin/lua/lua.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ func (l *lua) fromArguments(args map[string]interface{}) error {
if err := mapstructure.Decode(args, l); err != nil {
return fmt.Errorf("error decoding extension arguments: %v", err)
}
if l.ProxyType == "" {
l.ProxyType = string(api.ServiceKindConnectProxy)
}
return l.validate()
}

Expand All @@ -53,7 +56,7 @@ func (l *lua) validate() error {
if l.Script == "" {
resultErr = multierror.Append(resultErr, fmt.Errorf("missing Script value"))
}
if l.ProxyType != "connect-proxy" {
if l.ProxyType != string(api.ServiceKindConnectProxy) {
resultErr = multierror.Append(resultErr, fmt.Errorf("unexpected ProxyType %q", l.ProxyType))
}
if l.Listener != "inbound" && l.Listener != "outbound" {
Expand Down
9 changes: 9 additions & 0 deletions agent/envoyextensions/builtin/lua/lua_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,15 @@ func TestConstructor(t *testing.T) {
arguments: makeArguments(map[string]interface{}{"Listener": "invalid"}),
ok: false,
},
"default proxy type": {
arguments: makeArguments(map[string]interface{}{"ProxyType": ""}),
expected: lua{
ProxyType: "connect-proxy",
Listener: "inbound",
Script: "lua-script",
},
ok: true,
},
"valid everything": {
arguments: makeArguments(map[string]interface{}{}),
expected: lua{
Expand Down
3 changes: 3 additions & 0 deletions agent/envoyextensions/builtin/property-override/property_override.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,9 @@ func (p *propertyOverride) validate() error {
}
}

if p.ProxyType == "" {
p.ProxyType = api.ServiceKindConnectProxy
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put this here, instead of in the constructor, for the prop override extension because it is more consistent with how normalization is handled in the other validate funcs.

if err := validProxyTypes.CheckRequired(string(p.ProxyType), "ProxyType"); err != nil {
resultErr = multierror.Append(resultErr, err)
}
Expand Down
2 changes: 1 addition & 1 deletion agent/envoyextensions/builtin/property-override/property_override_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ func TestConstructor(t *testing.T) {
// enforces expected behavior until we do. Multi-member slices should be unaffected
// by WeakDecode as it is a more-permissive version of the default behavior.
"single value Patches decoded as map construction succeeds": {
arguments: makeArguments(map[string]any{"Patches": makePatch(map[string]any{})}),
arguments: makeArguments(map[string]any{"Patches": makePatch(map[string]any{}), "ProxyType": nil}),
expected: validTestCase(OpAdd, extensioncommon.TrafficDirectionOutbound, ResourceTypeRoute).expected,
ok: true,
},
Expand Down