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

listener options merge what translator generates #10578

Merged
merged 18 commits into from
Feb 18, 2025
Merged

listener options merge what translator generates #10578

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
554a788
listener options merge what translator generates
stevenctl Jan 21, 2025
5e0ff50
fix negative test case
stevenctl Jan 21, 2025
167e30c
changelog
stevenctl Jan 21, 2025
f3064df
Merge branch 'main' into merge-listener-opts
stevenctl Jan 22, 2025
b563ba7
Fix changelog
stevenctl Jan 22, 2025
703e0e6
Merge branch 'main' into merge-listener-opts
stevenctl Feb 10, 2025
0fec0e4
move changelog
stevenctl Feb 11, 2025
d178c64
use same merge semantics of solo APIs
stevenctl Feb 11, 2025
d648f20
Merge branch 'main' into merge-listener-opts
stevenctl Feb 11, 2025
c1d627d
changelog move
stevenctl Feb 11, 2025
99d1da1
Merge branch 'main' into merge-listener-opts
stevenctl Feb 12, 2025
47ccf80
changelog keeps moving
stevenctl Feb 12, 2025
8a847f5
Merge branch 'main' into merge-listener-opts
stevenctl Feb 17, 2025
d32fbba
changelog changelog
stevenctl Feb 17, 2025
0c60f14
Merge branch 'main' into merge-listener-opts
stevenctl Feb 17, 2025
cb16770
Merge branch 'main' into merge-listener-opts
stevenctl Feb 17, 2025
caa3a7c
Merge branch 'main' into merge-listener-opts
stevenctl Feb 18, 2025
2f1990d
a changelog in motion stays in motion
stevenctl Feb 18, 2025
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
8 changes: 8 additions & 0 deletions changelog/v1.19.0-beta6/merge-listener-options.yaml
View file
Open in desktop
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.
8 changes: 7 additions & 1 deletion projects/gateway2/translator/plugins/listeneroptions/listener_options_plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
lisquery "github.com/solo-io/gloo/projects/gateway2/translator/plugins/listeneroptions/query"
v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1"

"google.golang.org/protobuf/proto"

"sigs.k8s.io/controller-runtime/pkg/client"
)

Expand Down Expand Up @@ -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 {
Copy link

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

// ShallowMergeRouteOptions merges the top-level fields of src into dst.
. I think it would make sense to co-locate all our merge code to avoid the implementations drifting, if possible. What do you think?

Copy link
Author

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.

Copy link
Author

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

proto.Merge(outListener.GetOptions(), optToUse.Spec.GetOptions())
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

changed it to be consistent with other merging logic

} else {
outListener.Options = optToUse.Spec.GetOptions()
}

return nil
}
16 changes: 12 additions & 4 deletions projects/gateway2/translator/plugins/listeneroptions/listener_options_plugin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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())
})
})

Expand All @@ -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())
Copy link

Choose a reason for hiding this comment

The 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)?

Copy link
Author

Choose a reason for hiding this comment

The 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 outputListener.GetOptions() == expectedOptions encodes this.

I can rewrite the tests to be super explicit if we want that.

})
})
})

})

func attachedListenerOption() *solokubev1.ListenerOption {
Expand All @@ -144,6 +151,7 @@ func attachedListenerOption() *solokubev1.ListenerOption {
},
}
}

func attachedListenerOptionWithSectionName() *solokubev1.ListenerOption {
listOpt := attachedListenerOption()
listOpt.Spec.TargetRefs[0].SectionName = &wrapperspb.StringValue{
Expand Down