-
Notifications
You must be signed in to change notification settings - Fork 7
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
listener options merge what translator generates #10578
Changes from 17 commits
554a788
5e0ff50
167e30c
f3064df
b563ba7
703e0e6
0fec0e4
d178c64
d648f20
c1d627d
99d1da1
47ccf80
8a847f5
d32fbba
0c60f14
cb16770
caa3a7c
2f1990d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
changelog: | ||
- type: NON_USER_FACING | ||
issueLink: https://github.com/solo-io/solo-projects/issues/7300 | ||
resolvesIssue: true | ||
description: >- | ||
If we generate any ListenerOptions from a Gateway to Proxy | ||
translator, they will no longer be overridden by user | ||
ListenerOptions. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ import ( | |
"github.com/solo-io/gloo/projects/gateway2/translator/testutils" | ||
"github.com/solo-io/gloo/projects/gateway2/wellknown" | ||
v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1" | ||
"github.com/solo-io/gloo/projects/gloo/pkg/api/v1/options/proxy_protocol" | ||
corev1 "github.com/solo-io/skv2/pkg/api/core.skv2.solo.io/v1" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"sigs.k8s.io/controller-runtime/pkg/client" | ||
|
@@ -48,12 +49,19 @@ var _ = Describe("ListenerOptions Plugin", func() { | |
}, | ||
} | ||
|
||
outputListener = &v1.Listener{} | ||
outputListener = &v1.Listener{ | ||
Options: &v1.ListenerOptions{ | ||
ProxyProtocol: &proxy_protocol.ProxyProtocol{}, | ||
}, | ||
} | ||
|
||
expectedOptions = &v1.ListenerOptions{ | ||
// from config | ||
PerConnectionBufferLimitBytes: &wrapperspb.UInt32Value{ | ||
Value: uint32(419), | ||
}, | ||
// base | ||
ProxyProtocol: &proxy_protocol.ProxyProtocol{}, | ||
} | ||
}) | ||
JustBeforeEach(func() { | ||
|
@@ -102,7 +110,7 @@ var _ = Describe("ListenerOptions Plugin", func() { | |
It("does not add buffer limit", func() { | ||
err := plugin.ApplyListenerPlugin(ctx, listenerCtx, outputListener) | ||
Expect(err).ToNot(HaveOccurred()) | ||
Expect(outputListener.GetOptions()).To(BeNil()) | ||
Expect(outputListener.GetOptions().GetPerConnectionBufferLimitBytes()).To(BeNil()) | ||
}) | ||
}) | ||
|
||
|
@@ -114,11 +122,10 @@ var _ = Describe("ListenerOptions Plugin", func() { | |
It("does not add buffer limit", func() { | ||
err := plugin.ApplyListenerPlugin(ctx, listenerCtx, outputListener) | ||
Expect(err).ToNot(HaveOccurred()) | ||
Expect(outputListener.GetOptions()).To(BeNil()) | ||
Expect(outputListener.GetOptions().GetPerConnectionBufferLimitBytes()).To(BeNil()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a test that shows the merging, (if there isn't already one)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's not super explicit; We now have ProxyProtocol set on the base options, expect I can rewrite the tests to be super explicit if we want that. |
||
}) | ||
}) | ||
}) | ||
|
||
}) | ||
|
||
func attachedListenerOption() *solokubev1.ListenerOption { | ||
|
@@ -144,6 +151,7 @@ func attachedListenerOption() *solokubev1.ListenerOption { | |
}, | ||
} | ||
} | ||
|
||
func attachedListenerOptionWithSectionName() *solokubev1.ListenerOption { | ||
listOpt := attachedListenerOption() | ||
listOpt.Spec.TargetRefs[0].SectionName = &wrapperspb.StringValue{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have some very similar code for our other policies defined here:
gloo/projects/gloo/pkg/utils/merge.go
Line 83 in d4fa277
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I could write a manual one.. but I think the proto merge semantics make sense here. ListenerOptions has lists and maps, i'd imagine we want those to merge although the specific bug I care about only needs the top level fields to be merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay updated it to do this