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

Ingress: clean nginx, traefik and ambassador tests #19

Merged
merged 9 commits into from
Aug 25, 2020

Conversation

mayankshah1607
Copy link
Contributor

@mayankshah1607 mayankshah1607 commented Aug 7, 2020

A cleaner, refactored version of #12

This PR includes:

  • tackle traefik, nginx and ambassador ingress controllers, as gloo and contour have a slightly different testing process (which will be tackled in a separate PR).
  • use controller YAML URL instead of storing it locally
  • port-forward on the ingress controller pod instead of relying on external IP
  • make test structure a lot simpler

I did not directly make changes to #12 so that gloo and contour tests are not lost because of the refactoring process.

Signed-off-by: Mayank Shah mayankshah1614@gmail.com

Copy link

@ihcsim ihcsim left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks! Left some questions and minor suggestions.


var cmd *exec.Cmd
if tc.ingressName != "ambassador" {
cmd, err = beginPortForward(tc.namespace, "deploy/"+tc.controllerDeployName)
Copy link

Choose a reason for hiding this comment

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

Any reasons why we don't want to use the svc of the controller here? E.g., nginx-ingress has the ingress-nginx-controller service, and traefik has traefik-ingress-controller.

"github.com/onsi/gomega"
)

func pingEmojivoto(ip string) error {
req, err := http.NewRequest("GET", fmt.Sprintf("http://%s", ip), nil)
func pingIngress() error {
Copy link

Choose a reason for hiding this comment

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

NIt: Consider renaming this function, because in the context of network tests, ping means ICMP to me. How about just httpGet(url string)?

@mayankshah1607
Copy link
Contributor Author

Thanks @ihcsim ! I have made the changes. :)

@ihcsim
Copy link

ihcsim commented Aug 20, 2020

This one should be ready to be merged.

Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>
Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>
Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>
- Update port-forward to use service
- rename `pingIngress` to `httpGet`

Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>
Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>
Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>
Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>
Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>
Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>
Copy link
Contributor

@Pothulapati Pothulapati left a comment

Choose a reason for hiding this comment

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

LGTM! and works well!

{"name":"  tap: creating sample application","status":"passed","meta":{"path":"linkerd2-conformance|report.xml|Linkerd2 conformance tests"}}
{"name":"  tap: can tap a deployment","status":"passed","meta":{"path":"linkerd2-conformance|report.xml|Linkerd2 conformance tests"}}
{"name":"  tap: cannot tap a disabled deployment","status":"passed","meta":{"path":"linkerd2-conformance|report.xml|Linkerd2 conformance tests"}}
{"name":"  tap: can tap a service call","status":"passed","meta":{"path":"linkerd2-conformance|report.xml|Linkerd2 conformance tests"}}
{"name":"  tap: can tap a pod","status":"passed","meta":{"path":"linkerd2-conformance|report.xml|Linkerd2 conformance tests"}}
{"name":"  tap: can filter tap events by method","status":"passed","meta":{"path":"linkerd2-conformance|report.xml|Linkerd2 conformance tests"}}
{"name":"  tap: can filter tap events by authority","status":"passed","meta":{"path":"linkerd2-conformance|report.xml|Linkerd2 conformance tests"}}
{"name":"  ingress:  nginx: should work with Linkerd","status":"passed","meta":{"path":"linkerd2-conformance|report.xml|Linkerd2 conformance tests"}}
{"name":"  ingress:  nginx: should delete resources created for testing","status":"passed","meta":{"path":"linkerd2-conformance|report.xml|Linkerd2 conformance tests"}}
{"name":"  ingress:  traefik: should work with Linkerd","status":"passed","meta":{"path":"linkerd2-conformance|report.xml|Linkerd2 conformance tests"}}
{"name":"  ingress:  traefik: should delete resources created for testing","status":"passed","meta":{"path":"linkerd2-conformance|report.xml|Linkerd2 conformance tests"}}
{"name":"  ingress:  ambassador: should work with Linkerd","status":"passed","meta":{"path":"linkerd2-conformance|report.xml|Linkerd2 conformance tests"}}
{"name":"  ingress:  ambassador: should delete resources created for testing","status":"passed","meta":{"path":"linkerd2-conformance|report.xml|Linkerd2 conformance tests"}

@Pothulapati Pothulapati merged commit feb226e into linkerd:main Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants