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

Test framework .Call fails due to port mismatch #13091

Closed
howardjohn opened this issue Apr 5, 2019 · 6 comments
Closed

Test framework .Call fails due to port mismatch #13091

howardjohn opened this issue Apr 5, 2019 · 6 comments
Assignees
Milestone

Comments

@howardjohn
Copy link
Member


func ExternalRequest(expectedKnownPort []string, t *testing.T) {
	framework.
		NewTest(t).
		//RequiresEnvironment(environment.Kube).
		Run(func(ctx framework.TestContext) {
			g := galley.NewOrFail(t, ctx, galley.Config{})
			pilotInstance := pilot.NewOrFail(t, ctx, pilot.Config{Galley: g})

			instance := apps.NewOrFail(ctx, t, apps.Config{
				Pilot: pilotInstance,
				AppParams: []apps.AppParam{
					{"client"},
					{"dest"},
				},
			})


			client := instance.GetAppOrFail("client", t)
			dest := instance.GetAppOrFail("dest", t)

			for _, ep := range dest.EndpointsForProtocol(model.ProtocolHTTP) {
				resp, err := client.Call(ep, apps.AppCallOptions{})
				codes := make([]string, 0, len(resp))
				for _, r := range resp {
					codes = append(codes, r.Code)
				}
				if !reflect.DeepEqual(codes, expectedKnownPort) {
					log.Errorf("got codes %v expected %v for known port\n", codes, expectedKnownPort)
				}
				if err != nil {
					t.Error(err)
				}
			}
		})

This fails with:

    helper.go:63: unexpected port: 8080 (expected 80)
    helper.go:63: unexpected port: 80 (expected 8080)

I think this is because we have:

  - port: 80
    targetPort: 8080
    name: http
  - port: 8080
    targetPort: 80
    name: http-two

in the Service.

https://github.com/istio/istio/blob/master/pkg/test/framework/components/apps/kube.go#L619-L621

@howardjohn
Copy link
Member Author

I think we can just remove the check, unless there is some context I am missing why we have it

@nmittler
Copy link
Contributor

nmittler commented Apr 9, 2019

@howardjohn I think this should be resolved in #13175. The echo component has been re-written to support both native and kube. It also has the separation of servicePort and instancePort (i.e. TargetPort). Would you mind trying it out and confirming?

@ozevren
Copy link
Contributor

ozevren commented Apr 18, 2019

@howardjohn did you get a chance to look at @nmittler's recommendation?

nmittler added a commit to nmittler/istio that referenced this issue Apr 18, 2019
Was using the service port, but should have been using the target port.

Fixes istio#13091
@nmittler
Copy link
Contributor

I think this should fix it: #13443

@howardjohn
Copy link
Member Author

Fixed

@nmittler
Copy link
Contributor

@howardjohn what PR fixed this?

@rlenglet rlenglet modified the milestones: 1.4, 1.3, 1.2 Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants