Skip to content

Fix router tests to use the cloud service cluster IP instead of targetting internal router pods #21680

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
openshift-merge-robot merged 1 commit into from
Dec 20, 2018
Merged

Fix router tests to use the cloud service cluster IP instead of targetting internal router pods #21680

openshift-merge-robot merged 1 commit into from
Dec 20, 2018

Conversation

@ramr
Copy link
Contributor

@ramr ramr commented Dec 17, 2018

This breaks when the ingress-router will be changed and will expect the PROXY
protocol (not HTTP[S]) requests. Change to use the external cloud load balancer to talk "http[s]".
PTAL Thx

/cc @openshift/sig-network-edge @ironcladlou

@openshift-ci-robot openshift-ci-robot added sig/network-edge size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 17, 2018
@ramr ramr mentioned this pull request Dec 17, 2018
Merged
@ironcladlou
Copy link
Contributor

ironcladlou commented Dec 17, 2018

Looks like waitForRouterServiceIP() assumes the router under test is exposed by a service named "router", an assumption which does not hold with a 4.0 router (which will have a service like "router-default"). And for the 4.0 service, you would need to connect to status.loadBalancer.ingress[0].hostname.

@ramr
Copy link
Contributor Author

ramr commented Dec 17, 2018

@ironcladlou aah ok didn't realize the reencrypt tests were being skipped. I'll fix this up.

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 17, 2018
@ramr
Fix router tests to use the cloud service cluster IP rather than go
e7892c2 
to target internal router pods. This breaks when the ingress-router will be
changed and will expect the PROXY protocol (not HTTP[S]) requests.
Change to use the external cloud load balancer to talk "http[s]".
@ramr
Copy link
Contributor Author

ramr commented Dec 18, 2018

/retest

1 similar comment
@ramr
Copy link
Contributor Author

ramr commented Dec 18, 2018

/retest

@ramr
Copy link
Contributor Author

ramr commented Dec 18, 2018 •
edited
Loading

@ironcladlou /bump could you PTAL thx
These changes also enable more tests and doubles the test coverage.

Edited comment re: more tests

@ironcladlou
Copy link
Contributor

ironcladlou commented Dec 19, 2018

Awesome, thank you!

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 19, 2018
@openshift-ci-robot
Copy link

openshift-ci-robot commented Dec 19, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, ramr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 19, 2018
@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 19, 2018

/retest

Please review the full test history for this PR and help us cut down flakes.

@ramr
Copy link
Contributor Author

ramr commented Dec 19, 2018

/retest

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 19, 2018

/retest

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 19, 2018

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 19, 2018

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 19, 2018

/retest

Please review the full test history for this PR and help us cut down flakes.

@ramr
Copy link
Contributor Author

ramr commented Dec 19, 2018

/retest

@openshift-bot
Copy link
Contributor

openshift-bot commented Dec 19, 2018

/retest

Please review the full test history for this PR and help us cut down flakes.

@ramr
Copy link
Contributor Author

ramr commented Dec 20, 2018

/test unit

@openshift-merge-robot openshift-merge-robot merged commit ccc42c9 into openshift:master Dec 20, 2018
@ramr ramr deleted the fix-router-tests branch December 20, 2018 06:16
@ramr ramr mentioned this pull request Dec 22, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ironcladlou ironcladlou Awaiting requested review from ironcladlou

Assignees

@ironcladlou ironcladlou

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. sig/network-edge size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ramr @ironcladlou @openshift-ci-robot @openshift-bot @openshift-merge-robot