Skip to content

Support Listener and ClusterLoadAssignment in property-override#17497

Merged
erichaberkorn merged 2 commits into
mainfrom
zalimeni/net-3447-support-listener-clusterloadassignment-prop-override
May 29, 2023
Merged

Support Listener and ClusterLoadAssignment in property-override#17497
erichaberkorn merged 2 commits into
mainfrom
zalimeni/net-3447-support-listener-clusterloadassignment-prop-override

Conversation

@zalimeni
Copy link
Copy Markdown
Member

@zalimeni zalimeni commented May 26, 2023

Add support for patching Listener and ClusterLoadAssignment resources via the builtin property-override extension.

Changes for each resource split across two commits for easier review.

I skipped a separate changelog entry since this is an enhancement to a feature already introduced in the next release.

Note that several golden files used by new tests were previously committed with the bulk of property-override extension work; they're now in use with this change.

Description

  • Add support for patching resources
  • Refactor existing listener patch code in BasicEnvoyExtender to simplify addition of resource support
  • Add golden tests

PR Checklist

  • updated test coverage
  • external facing docs updated - N/A
  • appropriate backport labels added
  • not a security concern

Add support for patching `Listener` resources via the builtin
`property-override` extension.

Refactor existing listener patch code in `BasicEnvoyExtender` to
simplify addition of resource support.
@zalimeni zalimeni added theme/envoy/xds Related to Envoy support pr/no-metrics-test pr/no-backport pr/no-changelog PR does not need a corresponding .changelog entry pr/no-docs PR does not include docs and should not trigger reminder for cherrypicking them. labels May 26, 2023
Comment on lines 351 to 362
Copy link
Copy Markdown
Member Author

@zalimeni zalimeni May 26, 2023

Choose a reason for hiding this comment

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

The golden files for these are already in main 😅

Comment on lines 37 to 39
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Outbound patch 1/2

Comment on lines 74 to 76
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Outbound patch 2/2

Comment on lines 122 to 124
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Inbound patch

Comment on lines 64 to 66
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We don't have a use case for list-order operations on this resource, so only singular patch op is supported

Comment on lines 172 to 238
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This diff is odd-looking due to similar-shaped code being introduced below code that was edited; the red here is the modified end of patchClusters above, and the green is the tail end of the new patchClusterLoadAssignments.

Copy link
Copy Markdown
Member Author

@zalimeni zalimeni May 26, 2023

Choose a reason for hiding this comment

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

I switched to a single loop over listeners here and changed the filter chain fns below to handle individual listener values; this made introducing common listener patch code simpler. This didn't reduce any runtime iteration in practice, but I think it could aid similar changes in the future.

I also renamed those fns and updated their error messages to make it a bit more obvious what they're doing, since all of them deal exclusively w/ filter chains.

Add support for patching `ClusterLoadAssignment` resources via the
builtin `property-override` extension.
@zalimeni zalimeni force-pushed the zalimeni/net-3447-support-listener-clusterloadassignment-prop-override branch from 1e9c56e to a921bd1 Compare May 26, 2023 22:44
@zalimeni zalimeni requested review from cthain and erichaberkorn May 26, 2023 22:59
@zalimeni zalimeni marked this pull request as ready for review May 26, 2023 22:59
// This allows extensions to operate on a collection of listeners.
// For extensions that implement both PatchListener and PatchListeners,
// PatchListeners is always called first with the entire collection of listeners.
// Then PatchListeners is called for each individual listener.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Then PatchListeners is called for each individual listener.
// Then PatchListener is called for each individual listener.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'll make this change in a follow up PR

@erichaberkorn erichaberkorn merged commit e1df0f2 into main May 29, 2023
@erichaberkorn erichaberkorn deleted the zalimeni/net-3447-support-listener-clusterloadassignment-prop-override branch May 29, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr/no-backport pr/no-changelog PR does not need a corresponding .changelog entry pr/no-docs PR does not include docs and should not trigger reminder for cherrypicking them. pr/no-metrics-test theme/envoy/xds Related to Envoy support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants