fix/e2e/loadbalancer: preventing binding on privileged ports, and added nlb cases#1153
Conversation
|
This issue is currently awaiting triage. If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the The DetailsInstructions 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-sigs/prow repository. |
|
Welcome @mtulio! |
|
Hi @mtulio. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
|
|
/ok-to-test |
oops, missed local changes. retesting: /test pull-cloud-provider-aws-e2e |
65ebb0d to
9dc6148
Compare
|
Fixing node label discovery to support different k8s distributions - e.g. CI infra uses |
/test pull-cloud-provider-aws-e2e |
Learning with CI infra here, the node-role tag shows different from reported by get nodes, fixing to lower case and trying again: /test pull-cloud-provider-aws-e2e |
|
/test all |
|
Assigning the github recommendations, PTAL?: |
elmiko
left a comment
There was a problem hiding this comment.
this makes sense to me, i just have a minor nit that might help with triaging.
This change enhance test scenarios by: - supporting more distributions which does not allow pods to bind on privileged ports (default behavior of libjig, see issue - refact tests to allow adding more cases - introduce tests to NLB, including advanced tests to validate the node selector annotation. AWS SDK is added to satisfy this validatoin.
Thanks! LGTM tests. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elmiko, kmala The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
-#1215-#1217-#1214-upstream-release-1.33 Automated cherry pick of #1153: e2e/deps: enhance test scenarios with NLB #1161: e2e/loadbalancer: implement hairpin connection cases #1215: refact: e2e tests documenting hooks and enhance logging/steps #1217: e2e/debug: increase data collection on e2e failures #1214: doc/service: describe supported target group attributes
-#1215-#1217-#1214-upstream-release-1.32 Automated cherry pick of #1153: e2e/deps: enhance test scenarios with NLB #1161: e2e/loadbalancer: implement hairpin connection cases #1215: refact: e2e tests documenting hooks and enhance logging/steps #1217: e2e/debug: increase data collection on e2e failures #1214: doc/service: describe supported target group attributes
-#1215-#1217-#1214-upstream-release-1.31 Automated cherry pick of #1153: e2e/deps: enhance test scenarios with NLB #1161: e2e/loadbalancer: implement hairpin connection cases #1215: refact: e2e tests documenting hooks and enhance logging/steps #1217: e2e/debug: increase data collection on e2e failures #1214: doc/service: describe supported target group attributes
What type of PR is this?
/kind bug
/kind failing-test
/kind flake
What this PR does / why we need it:
This change fixes the e2e loadbalancer by preventing running tests in privileged ports, which was making tests to fail in distributions which restricts that configuration.
The issue details #1150
Furthermore, this also introduce the test cases more generically, including two scenarios not covered yet:
Which issue(s) this PR fixes:
Fixes #1150
Special notes for your reviewer:
The test framework implements mostly the code, but unfortunately the port of the target/pod is tied to the library (jig) - not exported, causing some code duplication to provide one off fix here.
There is a discussion here with some options.
Does this PR introduce a user-facing change?: