-
Notifications
You must be signed in to change notification settings - Fork 330
fix/e2e/loadbalancer: added nlb preventing binding on privileged ports #1153
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
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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 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-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. 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-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 |
/retest-required |
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.
This PR is ready for review. I just added minimum changes to improve readiness. Looks like the failures in the last run was CI infra flake. Checking again in the next round. Hi @kmala , would you mind taking a look here? Thanks! |
/lgtm |
Assigning the github recommendations, PTAL?: |
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, making tests to fail in distributions which restricts that configuration.
The issue details #1150
Furthermore, this also makes the test cases more generically, including two more 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.
There is a discussion here with some options.
Does this PR introduce a user-facing change?: