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

netcat integration test #149

Merged
merged 5 commits into from
May 25, 2020
Merged

netcat integration test #149

merged 5 commits into from
May 25, 2020

Conversation

FR4NK-W
Copy link
Contributor

@FR4NK-W FR4NK-W commented May 19, 2020

Add integration test for scion-netcat

Catch panic (and fail test, when SplitHostPort is called on a SCION address)
Add workaround to not trigger SplitHostPort in quic-go use by scion-netcat


This change is Reviewable

@FR4NK-W FR4NK-W force-pushed the FR4NK-W/netcat_integration branch from 38d3877 to 404eac6 Compare May 19, 2020 06:26
@FR4NK-W FR4NK-W force-pushed the FR4NK-W/netcat_integration branch from 404eac6 to efbad07 Compare May 19, 2020 06:33
Fixes issues with quic-go trying to parse the SCION address
Copy link
Member

@juagargi juagargi left a comment

Choose a reason for hiding this comment

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

:lgtm: just a non blocking comment/request

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @FR4NK-W)


netcat/netcat_integration_test.go, line 47 at r1 (raw file):

	// Server
	serverPort := "1234"
	serverArgs := []string{"-l", serverPort}

it would be great to test also -k, accept a new connection after closing, as it failed a couple of times in the past.


pkg/appnet/appquic/appquic.go, line 137 at r1 (raw file):

func mangleSCIONAddr(raddr *snet.UDPAddr) string {
	// Turn this into [IA,IP]:port format.
	// Same mangling as for the host part in MangleSCIONAddrURL github.com/netsec-ethz/scion-apps/pkg/shttp/transport.go

it is indeed the same. Could we make it one function for all uses?

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @FR4NK-W)


netcat/netcat_integration_test.go, line 108 at r1 (raw file):

	}

	// UDP tests

Make this a separate test function


netcat/netcat_integration_test.go, line 152 at r1 (raw file):

}

func wrapperCommand(tmpDir string, inputSource string, command string) (wrapperCmd string, err error){

OK for now, but once we need another one of these, we should make running more complex command lines a feature of the integration test framework.


pkg/appnet/appquic/appquic.go, line 71 at r1 (raw file):

	if tlsConf.ServerName == "" {
		tlsConf.ServerName = mangleSCIONAddr(raddr)
	}

The default should be the remote string, which can be a host name.
This should be moved to DialAddr.
As far as I understand, it should be enough to just mangle the host parameter passed to quic.Dial (in DialAddr) -- the tlsConf.ServerName does not need to be touched.


pkg/appnet/appquic/appquic.go, line 105 at r1 (raw file):

	if tlsConf.ServerName == "" {
		tlsConf.ServerName = mangleSCIONAddr(raddr)
	}

Ditto

Copy link
Contributor Author

@FR4NK-W FR4NK-W left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 7 files reviewed, 5 unresolved discussions (waiting on @juagargi and @matzf)


netcat/netcat_integration_test.go, line 47 at r1 (raw file):

Previously, juagargi (Juan A. García Pardo Giménez de los Galanes) wrote…

it would be great to test also -k, accept a new connection after closing, as it failed a couple of times in the past.

Seems to require -c in some cases, maybe in the next PR when we cleanup the wrapping.


netcat/netcat_integration_test.go, line 108 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Make this a separate test function

Done.


netcat/netcat_integration_test.go, line 152 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

OK for now, but once we need another one of these, we should make running more complex command lines a feature of the integration test framework.

Done.


pkg/appnet/appquic/appquic.go, line 71 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

The default should be the remote string, which can be a host name.
This should be moved to DialAddr.
As far as I understand, it should be enough to just mangle the host parameter passed to quic.Dial (in DialAddr) -- the tlsConf.ServerName does not need to be touched.

You are absolutely right.
Done.


pkg/appnet/appquic/appquic.go, line 105 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Ditto

Done.


pkg/appnet/appquic/appquic.go, line 137 at r1 (raw file):

Previously, juagargi (Juan A. García Pardo Giménez de los Galanes) wrote…

it is indeed the same. Could we make it one function for all uses?

We cannot import mangleSCIONAddr directly even if we made it public from github.com/netsec-ethz/scion-apps/pkg/shttp/ because of circular imports.
Also the parameter types are different. But the other way around works.
Moved those address mangling functions to appnet/hosts.go, but I don't like the string typedness of it.

Copy link
Member

@juagargi juagargi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 5 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @matzf)

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@FR4NK-W FR4NK-W merged commit e598e8a into master May 25, 2020
@matzf matzf deleted the FR4NK-W/netcat_integration branch August 11, 2020 15:53
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