listener options merge what translator generates #10578
Conversation
stevenctl
force-pushed
the
merge-listener-opts
branch
from
d2beba6 to
b563ba7
Compare
|
Issues linked to changelog: |
| @@ -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.
can you add a test that shows the merging, (if there isn't already one)?
There was a problem hiding this comment.
it's not super explicit; We now have ProxyProtocol set on the base options, expect outputListener.GetOptions() == expectedOptions encodes this.
I can rewrite the tests to be super explicit if we want that.
| @@ -47,7 +49,11 @@ func (p *plugin) ApplyListenerPlugin( | |||
| // use the first option (highest in priority) | |||
| // see for more context: https://github.com/solo-io/solo-projects/issues/6313 | |||
| optToUse := attachedOptions[0] | |||
| outListener.Options = optToUse.Spec.GetOptions() | |||
| if outListener.GetOptions() != nil { | |||
| proto.Merge(outListener.GetOptions(), optToUse.Spec.GetOptions()) | |||
There was a problem hiding this comment.
Can we note why we use proto merge here given how it handles unset values?
Weve had a variety of merge semantics in the code base and its hard to know why we choose one over another so would prefer erring on the side of a quick comment on why this merge strategy was chosen
There was a problem hiding this comment.
changed it to be consistent with other merging logic
| @@ -47,7 +49,11 @@ func (p *plugin) ApplyListenerPlugin( | |||
| // use the first option (highest in priority) | |||
| // see for more context: https://github.com/solo-io/solo-projects/issues/6313 | |||
| optToUse := attachedOptions[0] | |||
| outListener.Options = optToUse.Spec.GetOptions() | |||
| if outListener.GetOptions() != nil { | |||
There was a problem hiding this comment.
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.
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.
Description
A translator may generate a Listener with ListenerOptions (it's currently the internal IR).
A user listener opts should not automatically clear unrelated items in ListenerOptions.
This would also apply if another plugin used ListenerOptions.
API changes
ListenerOptions will now act as an overlay to system ones (if the system ones exist).
Code changes
Use proto merge when existing options are on the listener.
Testing steps
Added base options in the unit tests
Notes for reviewers
Open to manually implementing merge logic instead of proto merge
BOT NOTES:
resolves https://github.com/solo-io/solo-projects/issues/7300