Skip to content

Add some basic proxy protocol tests #42962

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

Jump to bottom
Merged
merged 11 commits into from
Jan 31, 2023
Merged

Add some basic proxy protocol tests #42962

merged 11 commits into from
Jan 31, 2023

Conversation

bleggett
Copy link
Contributor

@bleggett bleggett commented Jan 24, 2023 •
edited
Loading

PROXY protocol support in Istio is currently marked as experimental and lacks any integration tests - this PR adds a few simple integration tests to the existing gateway topology suite in an effort to move it forward to Alpha status.

Sibling PR for doc update: istio/istio.io#12510

  • Really, Istio isn't very involved with PROXY protocol support - it's an Envoy config.
  • PROXY protocol support is realistically only for proxying TCP traffic from a L4 loadbalancer thru a Gateway to a backend service - using PROXY protocol for L7 traffic is a bad idea, as this sibiling doc update attempts to make clear.
  • Currently only PROXY protocol V1 support is tested - this is all the major cloud L4s support ATM anyway.
  • Pulls in github.com/pires/go-proxyproto as an integration test rig dependency - this lib is reasonably well tested and maintained and using it seemed better than implementing a L4 header library from scratch in the test rig.

@istio-policy-bot istio-policy-bot added area/networking area/test and release release-notes-none Indicates a PR that does not require release notes. labels Jan 24, 2023
@istio-policy-bot
Copy link

😊 Welcome @bleggett! This is either your first contribution to the Istio istio repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@google-cla
Copy link

google-cla bot commented Jan 24, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test labels Jan 24, 2023
@istio-testing
Copy link
Collaborator

Hi @bleggett. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

howardjohn
howardjohn reviewed Jan 24, 2023
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

skimmed 75% of it, seems reasonable. will finish review tomorrow. Thanks!

@kyessenov
Copy link
Contributor

Is there a corresponding API proposal? We can't support EnvoyFilter long term.

@bleggett
Add basic integration tests for PROXY protocol and gateway
c8c4b28 
Update protobufs

Add dep

Derp

Consider actually using TCP

Hmm

Fix this

Missed one

Add log message

Logging

Remove these

Testing

Missed this

gawd

Hmm

Fix this

Testing

Ugh

Whitespace

Testing

Invalid component

Testing

Fix logging

Testt

Add alt case

Hmm

Fix

Fix 2

Fix routing

Actually fix

Fix this

Don't cleanup gateways

Aight

I'm a goofus

Tidy

Testing

Test

Hmm

Test

Fix this

Hmmm

Oh

Fix this

Fix this

test

Hmm

Test

Hmm

Hmm

Cleanup

Revert "Cleanup"

This reverts commit 1f933b720c504dc82cdb5683e1e08e6d464f5f1a.

Fix this

Better comment

Fix comment

Automator: update proxy@master in istio/istio@master (istio#42865)

Automator: update common-files@master in istio/istio@master (istio#42879)

Automator: update istio/client-go@master dependency in istio/istio@master (istio#42880)

improve wasm ecds comments (istio#42883)

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Change no server certificate verify as warning level (istio#42876)

* Change no server certificate verify as warning level

* update

Use latest helm dependency (istio#42891)

Automator: update proxy@master in istio/istio@master (istio#42898)

Automator: update common-files@master in istio/istio@master (istio#42900)

Automator: update istio/client-go@master dependency in istio/istio@master (istio#42901)

Automator: update proxy@master in istio/istio@master (istio#42907)

Cleanup unused metadata (istio#42903)

Update BASE_VERSION to master-2023-01-20T04-29-21 (istio#42913)

Add Locality info to debug interface (istio#42884)

Switching revision in IstioOperator istio-gateway cache fix (istio#42529)

* switching revision in IstioOperator istio-gateway cache fix

Signed-off-by: Shubham Chauhan <shubham@tetrate.io>

* include all components

Signed-off-by: Shubham Chauhan <shubham@tetrate.io>

Signed-off-by: Shubham Chauhan <shubham@tetrate.io>

Automator: update proxy@master in istio/istio@master (istio#42923)

Automator: update istio/client-go@master dependency in istio/istio@master (istio#42926)

add e2e for MetricDefinition (istio#42563) (istio#42924)

* add e2e for MetricDefinition

* fix lint

* send http traffic only

* fix prom query

Co-authored-by: zirain <hejianpeng2@huawei.com>

tf: ensure Check is always set (istio#42908)

* tf: ensure Check is always set

Avoid cases where we do not test anything

* Always set

* a few more

* fix

Initial messing about for PROXY protocol tests

Fix this

Automator: update proxy@master in istio/istio@master (istio#42865)

Automator: update common-files@master in istio/istio@master (istio#42879)

Automator: update istio/client-go@master dependency in istio/istio@master (istio#42880)

improve wasm ecds comments (istio#42883)

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Change no server certificate verify as warning level (istio#42876)

* Change no server certificate verify as warning level

* update

Use latest helm dependency (istio#42891)

Automator: update proxy@master in istio/istio@master (istio#42898)

Automator: update common-files@master in istio/istio@master (istio#42900)

Automator: update istio/client-go@master dependency in istio/istio@master (istio#42901)

Automator: update proxy@master in istio/istio@master (istio#42907)

Cleanup unused metadata (istio#42903)

Update BASE_VERSION to master-2023-01-20T04-29-21 (istio#42913)

Add Locality info to debug interface (istio#42884)

Switching revision in IstioOperator istio-gateway cache fix (istio#42529)

* switching revision in IstioOperator istio-gateway cache fix

Signed-off-by: Shubham Chauhan <shubham@tetrate.io>

* include all components

Signed-off-by: Shubham Chauhan <shubham@tetrate.io>

Signed-off-by: Shubham Chauhan <shubham@tetrate.io>

Automator: update proxy@master in istio/istio@master (istio#42923)

Automator: update istio/client-go@master dependency in istio/istio@master (istio#42926)

add e2e for MetricDefinition (istio#42563) (istio#42924)

* add e2e for MetricDefinition

* fix lint

* send http traffic only

* fix prom query

Co-authored-by: zirain <hejianpeng2@huawei.com>

tf: ensure Check is always set (istio#42908)

* tf: ensure Check is always set

Avoid cases where we do not test anything

* Always set

* a few more

* fix

wasm: Send nack when there is not well-formed resource messages (istio#42928)

* Send nack when there is an unmarshal error.

Change-Id: I8143ba8bc8c292ce2de5abda0a4f0c70c111bf80

* change if structure to have more readability

Change-Id: I076929d14dd1312b3717fe992352520223997354

* add unit tests

Change-Id: I31831ea1ba87c99ffc0451a7f3a2cb02acc14d91

Automator: update proxy@master in istio/istio@master (istio#42931)

Automator: update go dependencies@ in istio/istio@master (istio#42932)

Fix TestDeltaEDS flake (istio#42925)

improve appsv1 package import statement (istio#42946)

Signed-off-by: xin.li <xin.li@daocloud.io>

Signed-off-by: xin.li <xin.li@daocloud.io>

Use the slices package for contains check (istio#42947)

Automator: update common-files@master in istio/istio@master (istio#42948)

reducing leaseduration, renewdeadline and retryperiod to shorten test time (istio#42920)

add patch name to envoy filter crash log line (istio#42935)

* add patch name to envoy filter crash log line

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

* log line change

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

use authn filter constant every where (istio#42936)

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

Automator: update istio/client-go@master dependency in istio/istio@master (istio#42949)

Automator: update proxy@master in istio/istio@master (istio#42950)

Automator: update proxy@master in istio/istio@master (istio#42953)

Update master branch from 1.17 to 1.18 (istio#42875)

* Update master branch from 1.17 to 1.18

* Remove older envoy filters

* Fixup some version errors

Update default k8s for test and buildkit (istio#42952)

Use helm repo for integration tests instead of tarballs (istio#42864)

* Use helm repo for integration tests instead of tarballs

Signed-off-by: Faseela K <faseela.k@est.tech>

* refactor

Signed-off-by: Faseela K <faseela.k@est.tech>

* configuration option for helm repo

Signed-off-by: Faseela K <faseela.k@est.tech>

* fix lint failure

Signed-off-by: Faseela K <faseela.k@est.tech>

* add --repo to individual commands

Signed-off-by: Faseela K <faseela.k@est.tech>

Signed-off-by: Faseela K <faseela.k@est.tech>

Automator: update common-files@master in istio/istio@master (istio#42957)
@bleggett bleggett force-pushed the add-proxy-protocol-tests branch from 3b36f16 to c8c4b28 Compare January 24, 2023 00:50
@bleggett bleggett requested a review from a team as a code owner January 24, 2023 00:50
@bleggett bleggett requested a review from a team January 24, 2023 00:50
@bleggett bleggett requested review from a team as code owners January 24, 2023 00:50
@istio-testing istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 24, 2023
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 24, 2023
@istio-testing istio-testing added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 24, 2023
@howardjohn
Copy link
Member

IMO there is no feature status. We have a feature - Envoyfitler - which is permanently alpha and unstable. We have examples of using it (https://github.com/istio/istio/wiki/EnvoyFilter-Samples); Proxy protocol is one example. Its reasonable to have tests for it. We should not call "proxy protocol" a feature of Istio with a stability gaurantee

@linsun
Copy link
Member

linsun commented Jan 24, 2023

IMO there is no feature status. We have a feature - Envoyfitler - which is permanently alpha and unstable. We have examples of using it (https://github.com/istio/istio/wiki/EnvoyFilter-Samples); Proxy protocol is one example. Its reasonable to have tests for it. We should not call "proxy protocol" a feature of Istio with a stability gaurantee

That is reasonable too, as long as we don't emphasize it is experimental in our docs which is misleading.

@bleggett
Copy link
Contributor Author

bleggett commented Jan 24, 2023 •
edited
Loading

@linsun @howardjohn Sounds good - absent objections I will remove the existing experimental tag in the sibling PR istio/istio.io#12510 and simply indicate that this is an EnvoyFilter and not an Istio feature.

PROXY protocol is not in any preexisting feature matrix I can find anyway.

@howardjohn
Copy link
Member

Forgot to mention - having a test is still useful regardless

bleggett added a commit to bleggett/istio.io that referenced this pull request Jan 24, 2023
@bleggett
As per discuss in istio/istio#42962 (comment)
fc8b46c
howardjohn
howardjohn reviewed Jan 24, 2023
View reviewed changes
@bleggett bleggett requested a review from howardjohn January 24, 2023 17:37
@bleggett
Copy link
Contributor Author

It's unclear to me why the arm64 suites are failing specifically.

@howardjohn
Copy link
Member

howardjohn commented Jan 24, 2023 via email

istio-testing pushed a commit to istio/istio.io that referenced this pull request Jan 24, 2023
@bleggett @frankbu
Add more context to the PROXY protocol gateway config section (#12510)
a204008 
* Add more context

* Update content/en/docs/ops/configuration/traffic-management/network-topologies/index.md

Co-authored-by: Frank Budinsky <frankb@ca.ibm.com>

* Update content/en/docs/ops/configuration/traffic-management/network-topologies/index.md

Co-authored-by: Frank Budinsky <frankb@ca.ibm.com>

* Update content/en/docs/ops/configuration/traffic-management/network-topologies/index.md

Co-authored-by: Frank Budinsky <frankb@ca.ibm.com>

* Update content/en/docs/ops/configuration/traffic-management/network-topologies/index.md

Co-authored-by: Frank Budinsky <frankb@ca.ibm.com>

* As per discuss in istio/istio#42962 (comment)

* Lint fix

Co-authored-by: Frank Budinsky <frankb@ca.ibm.com>
@bleggett
Copy link
Contributor Author

bleggett commented Jan 25, 2023 •
edited
Loading

Should be good, just need a review approval - doc PR istio/istio.io/#12510 already merged as per discussion here.

@bleggett bleggett removed request for a team and howardjohn January 31, 2023 15:26
@bleggett
Copy link
Contributor Author

Because of a botched initial diff several other people got sucked into this as reviewers - apologies.

@bleggett bleggett requested a review from howardjohn January 31, 2023 15:40
linsun
linsun reviewed Jan 31, 2023
View reviewed changes
@bleggett @linsun
Update tests/integration/pilot/common/routing.go
18eec72 
Co-authored-by: Lin Sun <lin.sun@solo.io>
@bleggett bleggett requested a review from linsun January 31, 2023 16:17
linsun
linsun approved these changes Jan 31, 2023
View reviewed changes
Copy link
Member

@linsun linsun left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @bleggett for addressing all comments from various reviewers.

@istio-testing istio-testing merged commit a306e4c into istio:master Jan 31, 2023
@bleggett bleggett deleted the add-proxy-protocol-tests branch January 31, 2023 18:14
justedennnnn pushed a commit to justedennnnn/istio.io that referenced this pull request Jun 29, 2023
@bleggett @frankbu @justedennnnn
Add more context to the PROXY protocol gateway config section (istio#…
1aeece8 
…12510)

* Add more context

* Update content/en/docs/ops/configuration/traffic-management/network-topologies/index.md

Co-authored-by: Frank Budinsky <frankb@ca.ibm.com>

* Update content/en/docs/ops/configuration/traffic-management/network-topologies/index.md

Co-authored-by: Frank Budinsky <frankb@ca.ibm.com>

* Update content/en/docs/ops/configuration/traffic-management/network-topologies/index.md

Co-authored-by: Frank Budinsky <frankb@ca.ibm.com>

* Update content/en/docs/ops/configuration/traffic-management/network-topologies/index.md

Co-authored-by: Frank Budinsky <frankb@ca.ibm.com>

* As per discuss in istio/istio#42962 (comment)

* Lint fix

Co-authored-by: Frank Budinsky <frankb@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardjohn howardjohn howardjohn approved these changes

@linsun linsun linsun approved these changes

@costinm costinm Awaiting requested review from costinm costinm is a code owner

@stevenctl stevenctl Awaiting requested review from stevenctl stevenctl is a code owner

@ericvn ericvn Awaiting requested review from ericvn

@hzxuzhonghu hzxuzhonghu Awaiting requested review from hzxuzhonghu hzxuzhonghu is a code owner

Assignees
No one assigned
Labels
area/networking area/test and release ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. release-notes-none Indicates a PR that does not require release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@bleggett @istio-policy-bot @istio-testing @kyessenov @ericvn @costinm @linsun @howardjohn