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

Breaking change introduced #408

Closed
rsdmike opened this issue Jul 15, 2020 · 2 comments · Fixed by #409
Closed

Breaking change introduced #408

rsdmike opened this issue Jul 15, 2020 · 2 comments · Fixed by #409
Assignees
Labels
bug Something isn't working
Milestone

Comments

@rsdmike
Copy link
Member

rsdmike commented Jul 15, 2020

In PR #406

addr = fmt.Sprintf("%v:%d", webserver.Config.Service.Host, webserver.Config.Service.Port)

is NOT backwards compatible with the SDK and actually against best practices per golang documentation https://golang.org/pkg/net/#Listen.

The address can use a host name, but this is not recommended, because it will create a listener for at most one of the host's IP addresses

This is different behavior introduced from the introduction of the app-functions-sdk-go to the current release:

It should be:

p := fmt.Sprintf(":%d", webserver.Config.Service.Port)

@rsdmike rsdmike added the bug Something isn't working label Jul 15, 2020
@lenny-goodell
Copy link
Member

Agree that defaulting it to Host when setting is blank breaks backwards compatibility for the App Services. Don't agree it would always be blank. It should use what ever the new setting values is and by default that will be blank if the setting isn't in the config.

Having it be blank by default circumvents the Kong proxy only access for snaps. Proper non-breaking fix is this only without check for blank and defaulting to Host value and setting that value to localhost in the ASC config TOML files. @rsdmike , agree?

	addr := fmt.Sprintf("%v:%d", webserver.Config.Service.ServerBindAddr, webserver.Config.Service.Port)

@charles-knox-intel
Copy link
Contributor

I think @lenny-intel's code snippet allows for proper backwards compatibility and still allows for users to set ServerBindAddr. I will make a PR with the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants