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: egress HTTP and policy w/ toFQDNs #251

Merged
merged 7 commits into from
Jul 19, 2021

Conversation

ti-mo
Copy link
Contributor

@ti-mo ti-mo commented May 27, 2021

Best reviewed per commit.

  • Added test client-egress-l7 for HTTP filtering with and without toFQDNs. Only client Pod 'other' is allowed to generate HTTP traffic.
  • Some fixups to the yaml parser and error reporting.
  • Run PodToWorld calls to google.com from all client Pods.
  • Execute connectivity.Run() inside its own goroutine.

@ti-mo ti-mo requested review from a team as code owners May 27, 2021 15:43
@ti-mo ti-mo requested a review from twpayne May 27, 2021 15:43
@ti-mo ti-mo temporarily deployed to ci May 27, 2021 15:43 Inactive
@ti-mo ti-mo temporarily deployed to ci May 27, 2021 16:15 Inactive
@ti-mo ti-mo temporarily deployed to ci May 27, 2021 19:29 Inactive
@ti-mo ti-mo mentioned this pull request May 27, 2021
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM to me overall, I left a few comments to be addressed

internal/cli/cmd/connectivity.go Outdated Show resolved Hide resolved
connectivity/suite.go Show resolved Hide resolved
connectivity/check/action.go Outdated Show resolved Hide resolved
@ti-mo
Copy link
Contributor Author

ti-mo commented May 31, 2021

Converted to draft as it's currently blocked on cilium/cilium#16376 and L7 ingress policy test needs to be added.

@ti-mo ti-mo temporarily deployed to ci May 31, 2021 16:28 Inactive
@ti-mo ti-mo temporarily deployed to ci June 2, 2021 09:55 Inactive
Commenting out a block starting with --- would incorrectly split the document
on a commented-out string. This only splits when the --- appears at the start
of a line.

Use Fatalf() for strings containing formatting directive.

Signed-off-by: Timo Beckers <timo@isovalent.com>
Without this, a call to Test.Fatal() inside e.g. WithPolicy() would terminate
the main goroutine, hanging the program.

This also sets the correct shell return code by returning an error if the
inner goroutine was terminated.

Don't defer cancel() as this prints the 'interrupt received' message
on successful test runs as well.

Signed-off-by: Timo Beckers <timo@isovalent.com>
Before, a random client Pod was selected, leading to a degree of randomness
that cannot easily be accounted for in verdict overrides. In reality, the
check was often executed from the same client Pod, defeating the point of the
test.

This patch executes all three curls from all available client Pods.

Signed-off-by: Timo Beckers <timo@isovalent.com>
It would be useful to see what the curl/ping command output looks like in CI,
where debug logging is not enabled.

Stdout is only displayed when the command unexpectedly succeeded,
stderr only on unexpected failure.

Signed-off-by: Timo Beckers <timo@isovalent.com>
…licy

- Add test client-egress-l7
- Apply egress DNS policy to populate FQDN cache
- Add policy client-egress-l7-http to allow echo:8080/ and cilium.io:80/
- Update client-egress-only-dns to run with DNS introspection enabled

Currently running pod-to-pod and pod-to-world under client-egress-l7.

Signed-off-by: Timo Beckers <timo@isovalent.com>
…licy

- Added test echo-ingress-l7
- Apply policy echo-ingress-l7-http

L7 drop matching is currently not supported, so drop on L3 instead.

Signed-off-by: Timo Beckers <timo@isovalent.com>
Signed-off-by: Timo Beckers <timo@isovalent.com>
@ti-mo ti-mo temporarily deployed to ci July 13, 2021 13:36 Inactive
@ti-mo ti-mo marked this pull request as ready for review July 13, 2021 13:40
Copy link
Contributor

@twpayne twpayne left a comment

Choose a reason for hiding this comment

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

Very nice!

// Execute connectivity.Run() in its own goroutine, it might call Fatal()
// and end the goroutine without returning.
go func() {
defer func() { done <- struct{}{} }()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: close(done) allows multiple goroutines to block on done. Not needed here of course.

@ti-mo ti-mo added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 16, 2021
@tklauser tklauser merged commit ecaaeb8 into master Jul 19, 2021
@tklauser tklauser deleted the tb/http-policy-coverage branch July 19, 2021 14:11
nbusseneau added a commit that referenced this pull request Jul 21, 2021
Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
nbusseneau added a commit that referenced this pull request Jul 22, 2021
Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
nbusseneau added a commit that referenced this pull request Jul 22, 2021
Revert: 7b044f5
Revert: c061048
Revert: 4401032
Revert: ecaaeb8

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
nbusseneau added a commit that referenced this pull request Jul 22, 2021
Following merge of #251, testing on EKS in tunnel mode is now reliably
broken due to an issue in upstream: cilium/cilium#16975

This type of failure was already happening before #251, but the PR made
it very evident as before we were only running checks from a few random
pods (which would sometimes work) and are now running checks from all
pods, drastically increasing the chance of hitting at least one failure.

Since the test setup is not to blame and this is an actual issue with
Cilium in tunnel momde, we disable the failing tests until upstream
issue is fixed.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
michi-covalent pushed a commit that referenced this pull request Jul 22, 2021
Following merge of #251, testing on EKS in tunnel mode is now reliably
broken due to an issue in upstream: cilium/cilium#16975

This type of failure was already happening before #251, but the PR made
it very evident as before we were only running checks from a few random
pods (which would sometimes work) and are now running checks from all
pods, drastically increasing the chance of hitting at least one failure.

Since the test setup is not to blame and this is an actual issue with
Cilium in tunnel momde, we disable the failing tests until upstream
issue is fixed.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
aditighag pushed a commit to aditighag/cilium-cli that referenced this pull request Apr 21, 2023
Following merge of cilium#251, testing on EKS in tunnel mode is now reliably
broken due to an issue in upstream: cilium/cilium#16975

This type of failure was already happening before cilium#251, but the PR made
it very evident as before we were only running checks from a few random
pods (which would sometimes work) and are now running checks from all
pods, drastically increasing the chance of hitting at least one failure.

Since the test setup is not to blame and this is an actual issue with
Cilium in tunnel momde, we disable the failing tests until upstream
issue is fixed.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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