Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

tests(e2e): add egress test #1832

Merged
merged 5 commits into from
Oct 16, 2020
Merged

tests(e2e): add egress test #1832

merged 5 commits into from
Oct 16, 2020

Conversation

nojnhuh
Copy link
Contributor

@nojnhuh nojnhuh commented Oct 14, 2020

Description: This test will issue requests against edition.cnn.com and github.com both with egress enabled, ensure the requests are successful, disable egress, issue the same requests, and ensure those requests fail.

Affected area:

  • New Functionality [ ]
  • Documentation [ ]
  • Install [ ]
  • Control Plane [ ]
  • CLI Tool [ ]
  • Certificate Management [ ]
  • Networking [ ]
  • Metrics [ ]
  • SMI Policy [ ]
  • Security [ ]
  • Tests [X]
  • CI System [ ]
  • Performance [ ]
  • Other [ ]

Please answer the following questions with yes/no.

  • Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution?
    No

@nojnhuh nojnhuh requested a review from a team as a code owner October 14, 2020 18:40
snehachhabria
snehachhabria previously approved these changes Oct 14, 2020
Comment on lines +47 to +48
"edition.cnn.com",
"github.com",
Copy link
Member

Choose a reason for hiding this comment

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

We should be testing both HTTP and HTTPS traffic with egress.
Could you document what is being tested in the Describe or Context container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shashankram Can you clarify what you mean by "what is being tested?"

Copy link
Member

Choose a reason for hiding this comment

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

With egress there are different things that can be tested - HTTP egress, HTTPS egress etc.
It would be nice to document what is being tested as a part of the Describe and Context container messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ginkgo's way would be to have the test separated by It()s, so that each seaction represents a part of the test. We are not yet there in current impl, but we should file an issue to rework the tests into sections.

Copy link
Member

Choose a reason for hiding this comment

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

You don't need an It() for every test, depends on what is being tested. Just a Describe and Context are more than enough for many tests.

@codecov-io
Copy link

codecov-io commented Oct 15, 2020

Codecov Report

Merging #1832 into main will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1832      +/-   ##
==========================================
- Coverage   59.09%   59.07%   -0.02%     
==========================================
  Files         125      125              
  Lines        5171     5171              
==========================================
- Hits         3056     3055       -1     
- Misses       2112     2113       +1     
  Partials        3        3              
Impacted Files Coverage Δ
pkg/certificate/rotor/rotor.go 84.00% <0.00%> (-4.00%) ⬇️
pkg/utils/mtls.go 100.00% <0.00%> (ø)
pkg/health/health.go 20.51% <0.00%> (ø)
pkg/debugger/envoy.go 0.00% <0.00%> (ø)
pkg/certificate/file.go 52.17% <0.00%> (ø)
pkg/injector/webhook.go 73.48% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9393ef...d4183a6. Read the comment docs.

shashankram
shashankram previously approved these changes Oct 15, 2020
"http://",
"https://",
}
egressTests := []string{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
egressTests := []string{
egressURLs := []string{

@nojnhuh nojnhuh merged commit 122cb13 into openservicemesh:main Oct 16, 2020
@nojnhuh nojnhuh deleted the e2e-egress branch October 16, 2020 21:28
draychev pushed a commit to draychev/osm that referenced this pull request Oct 28, 2020
* tests(e2e): add egress test

* check http and https

* log urls for requests after disabling egress

* address comments
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants