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

Clean up bindConfigServices function #25

Open
MaxwellBoecker opened this issue Sep 7, 2023 · 2 comments
Open

Clean up bindConfigServices function #25

MaxwellBoecker opened this issue Sep 7, 2023 · 2 comments

Comments

@MaxwellBoecker
Copy link

Description:
The current implementation of the bindConfigServices function could create outputs and/or errors that would be confusing to debug.

Issue:
Looking at the https://github.com/taubyte/dreamland/blob/main/cli/new/helpers.go file, it seems that if the conditional check on line 78 fails, then we will end up with a port of 0 and a sub of "" for any given service. Apparently it is possible to use port 0 to bind to a random available port, however this could cause problems with the use of TCP as 0 is a reserved port not intended to be used. In any case, a sub of "" is not in the list of ValidSubBinds found in the file https://github.com/taubyte/dreamland/blob/main/cli/common/vars.go. We see sub being validated on line 86, but not if the conditional check on line 78 fails. So this seems like a contradiction of logic: likely it should be validated both times.

Also, if it is possible for any service to bind to port 0, I would expect the conditional on line 112 to fail from time to time, in which case we would get an error for two services having duplicate port bindings. And the reason they are duplicate is just that they ended up taking on a default value after the conditional on line 78 failed. This gives the appearance that the code is written assuming the conditional on 78 will not fail.

Possible Solution:
It would likely be better to save the confusion by just creating a requirement that the result of strings.Split(bind, "@") on line 63 have a length == 2, instead of letting the inputs fall all the way through to the validation of duplicate ports to fail. This could save some debugging efforts insofar as we would not have to read all the times that port gets initialized, reassigned, or accessed on the binds map.

@samyfodil
Copy link
Contributor

port==0 means that dreamland will randomly assign a port.

Can you provide a snippet of what you think the solution should be?

@MaxwellBoecker
Copy link
Author

The simplest solution if we only wanted to accept binds which had a length of 2 when split would be to just change the conditional on line 64 in https://github.com/taubyte/dreamland/blob/main/cli/new/helpers.go:

if len(_def) != 2 {
	return nil, fmt.Errorf("processing bindings for `%s` failed", bind)
}

However, if the application has been designed to accept the value 0 and use it as a signifier to assign a random port to the service during runtime, the better solution would probably be to allow multiple bindings to port 0. This is assuming that we wouldn't cause some kind of failure due to a lack of spare random ports to assign. In order to allow multiple bindings to port 0, we could modify the condition on line 112 to read like this:

if ok && port != 0 { // proceed with error handling here}

These are just the bare minimum simple ways to achieve the desired functionality. Though the code in this file is in need of a refactor which is probably equally as important.
It could easily be broken into several functions at the least. Perhaps separate structs each with their own set of methods? Likely some similar logic is being used elsewhere in the repository and could even benefit from using some of the methods we would extract here, should we decide to expose them.

This might be a nice first pass for a refactor of the bindConfigServices function if we extracted three methods, each representing what I think the function is doing at a high level:

func bindConfigServices(_binds []string, _services []string) (map[string]ServiceConfig, error) {
	binds, err := parseServiceBindings(_binds, _services)
	if err != nil {
		return nil, err
	}

	err = validateNoPortConflicts(binds)
	if err != nil {
		return nil, err
	}

	return createConfig(binds, _services), nil
}

We could imagine each of the functions encapsulating a chunk of the logic. parseServiceBindings would encapsulate the logic from lines 57-105, validateNoPortConflicts would encapsulate the logic from lines 108-123 and createConfig would encapsulate the logic from lines 128-145. Naturally, these functions would need to be broken down further

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

No branches or pull requests

2 participants