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

connectivity: test accessing NodePort from outside with L7 policy #1547

Merged
merged 1 commit into from
May 2, 2023

Conversation

jschwinger233
Copy link
Member

@jschwinger233 jschwinger233 commented Apr 27, 2023

This test case covers cilium/cilium#21954.

A new policy echo-ingress-l7-policy-from-anywhere is added to allow HTTP GET / on echo pods from outside.

Use cilium connectivity test --test north-south-loadbalancing --datapath to run this test.

Signed-off-by: Zhichuan Liang <gray.isovalent.com>

@jschwinger233 jschwinger233 changed the title connectivity: add test for accessing NodePort from outside with L7 policy connectivity: test accessing NodePort from outside with L7 policy Apr 27, 2023
@jschwinger233 jschwinger233 temporarily deployed to ci April 27, 2023 09:18 — with GitHub Actions Inactive
This test case covers cilium/cilium#21954.

A new policy `echo-ingress-l7-policy-from-anywhere` is added to allow
HTTP GET / on echo pods from outside.

Use `cilium connectivity test --test north-south-loadbalancing --datapath`
to run this test.

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
@jschwinger233 jschwinger233 temporarily deployed to ci April 27, 2023 10:32 — with GitHub Actions Inactive
@jschwinger233 jschwinger233 marked this pull request as ready for review April 28, 2023 03:46
@jschwinger233 jschwinger233 requested a review from a team as a code owner April 28, 2023 03:46
@jschwinger233
Copy link
Member Author

Please note cilium can't pass the new added test case until cilium/cilium#24882 is merged.

@brb
Copy link
Member

brb commented Apr 28, 2023

Please note cilium can't pass the new added test case until cilium/cilium#24882 is merged.

Nice! I assume you ran the tests locally with the fix?

@jschwinger233
Copy link
Member Author

I assume you ran the tests locally with the fix?

Yes!

@brb brb added dont-merge/blocked Another PR must be merged before this one. area/CI Continuous Integration testing issue or flake labels Apr 28, 2023
@brb
Copy link
Member

brb commented Apr 28, 2023

Added dont-merge/blocked until cilium/cilium#24882 has landed. Otherwise, the ci-datapath will be broken, as it's still using the main branch of cilium-cli.

@pchaigno pchaigno removed the dont-merge/blocked Another PR must be merged before this one. label May 2, 2023
tommyp1ckles added a commit to tommyp1ckles/cilium that referenced this pull request May 2, 2023
Changes in c1a0dba may have introduced flakes for this set of tests
when attempting to reach NodePort from outside the cluster.

A replacement test is being added to cilium cli connectivity tests:

cilium/cilium-cli#1547

Quarantine this test for now until we can remove it.

Addresses: cilium#25119

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 2, 2023
@pchaigno pchaigno merged commit 947dfb6 into cilium:main May 2, 2023
12 checks passed
tommyp1ckles added a commit to cilium/cilium that referenced this pull request May 4, 2023
Changes in c1a0dba may have introduced flakes for this set of tests
when attempting to reach NodePort from outside the cluster.

A replacement test is being added to cilium cli connectivity tests:

cilium/cilium-cli#1547

Quarantine this test for now until we can remove it.

Addresses: #25119

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
jschwinger233 added a commit to jschwinger233/cilium that referenced this pull request May 26, 2023
The test case was introduced to cover issue cilium#21954, but it turned out
the test is buggy and caused a number of CI flakes (cilium#25119).
Consequently, PR cilium#25236 put the test case under quarantine.

This commit removes that problematic test, as the target scenario has
been covered by connectivity test in cilium-cli
(cilium/cilium-cli#1547).

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
brb pushed a commit to cilium/cilium that referenced this pull request May 26, 2023
The test case was introduced to cover issue #21954, but it turned out
the test is buggy and caused a number of CI flakes (#25119).
Consequently, PR #25236 put the test case under quarantine.

This commit removes that problematic test, as the target scenario has
been covered by connectivity test in cilium-cli
(cilium/cilium-cli#1547).

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
sayboras pushed a commit to sayboras/cilium that referenced this pull request May 29, 2023
[ upstream commit eb5bf06 ]

The test case was introduced to cover issue cilium#21954, but it turned out
the test is buggy and caused a number of CI flakes (cilium#25119).
Consequently, PR cilium#25236 put the test case under quarantine.

This commit removes that problematic test, as the target scenario has
been covered by connectivity test in cilium-cli
(cilium/cilium-cli#1547).

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
julianwiedmann pushed a commit to cilium/cilium that referenced this pull request Jun 2, 2023
[ upstream commit eb5bf06 ]

The test case was introduced to cover issue #21954, but it turned out
the test is buggy and caused a number of CI flakes (#25119).
Consequently, PR #25236 put the test case under quarantine.

This commit removes that problematic test, as the target scenario has
been covered by connectivity test in cilium-cli
(cilium/cilium-cli#1547).

Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI Continuous Integration testing issue or flake ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants