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.

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

Improvements to ProxyProtocol testing and implementation #51769

Merged
merged 7 commits into from
Jul 1, 2024

Conversation

howardjohn
Copy link
Member

@howardjohn howardjohn commented Jun 27, 2024

  • Add PROXY support to Echo. This avoids the need to spin up a whole new
    gateway deployment just for one test.
  • Use the new support to make our PROXY test simpler. Now its just
    "apply DR; send traffic"
  • This exposed a bug in the implementation; fixed that

The original motivation for this is I am expanding testing of PROXY in
another PR (wrt to waypoints).

This needed some changes to avoid re-using the TransportSocket for HBONE, which results in mutating shared state. this is broken today in a fatal way, its just not tested enough to trigger it until now.

* Add PROXY support to Echo. This avoids the need to spin up a whole new
  gateway deployment just for one test.
* Use the new support to make our PROXY test simpler. Now its just
  "apply DR; send traffic"
* This exposed a bug in the implementation; fixed that

The original motivation for this is I am expanding testing of PROXY in
another PR (wrt to waypoints).
@howardjohn howardjohn added the release-notes-none Indicates a PR that does not require release notes. label Jun 27, 2024
@howardjohn howardjohn requested review from a team as code owners June 27, 2024 15:11
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 27, 2024
@howardjohn howardjohn changed the title test/echo proxy protocol Improvements to ProxyProtocol testing and implementation Jun 27, 2024
pkg/test/echo/server/endpoint/http.go Outdated Show resolved Hide resolved
pilot/pkg/networking/core/cluster_tls.go Show resolved Hide resolved
pilot/pkg/networking/core/cluster_tls.go Show resolved Hide resolved
@@ -472,6 +473,19 @@ func (cb *ClusterBuilder) applyUpstreamProxyProtocol(
return
}
c := opts.mutable

// No existing transport; wrap RawBuffer.
if c.cluster.TransportSocket == nil && len(c.cluster.TransportSocketMatches) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

How could this happend, either TransportSocket or TransportSocketMatches is set before calling this function

Copy link
Member Author

Choose a reason for hiding this comment

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

Not if tls is disabled, then there is neither

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a unit test case covering this

Copy link
Member Author

@howardjohn howardjohn Jun 28, 2024

Choose a reason for hiding this comment

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

and the new integration test also covers this

@hzxuzhonghu hzxuzhonghu added the do-not-merge/hold Block automatic merging of a PR. label Jun 28, 2024
@howardjohn
Copy link
Member Author

/retest

@hzxuzhonghu hzxuzhonghu removed the do-not-merge/hold Block automatic merging of a PR. label Jun 28, 2024
@howardjohn howardjohn assigned dhawton and unassigned stevenctl Jun 28, 2024
@howardjohn
Copy link
Member Author

/test integ-ambient-ipv6_istio

@istio-testing istio-testing merged commit cbb6a27 into istio:master Jul 1, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes-none Indicates a PR that does not require release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants