feat: API server

[mojtaba-esk]

Closes #19

[smuu]

This is 🤯
Great work! Just a few comments/questions. I need to give the review a second round later.

[smuu]

[question] Is setting latency and jitter at the same endpoint required, or can that be splitted into separate endpoints?

[mojtaba-esk]

Since both are done via tc and cannot be done easily in parallel. It is simpler to have them on one endpoint.
In fact, tc receives a parameter for variable latency along side the fixed latency, so we pass it always if jitter is not set, we simply pass zero there.

[MSevey]

Left some comments on the api package the appear to apply to all files in that package. If they are applicable then please apply throughout, else please help me understand the context more.

[MSevey]

Do you need the else condition? If you inline it, then it because a built in check that the if condition is correct.

Suggested change

	if a.bw == nil {
	a.bw = &netRestrictService{
	service: &bandwidth.Bandwidth{
	Limit: body.Limit,
	},
	logger: a.logger,
	}
	} else {
	bw, ok := a.bw.service.(*bandwidth.Bandwidth)
	if !ok {
	sendJSONError(resp,
	MetaMessage{
	Type: APIMetaMessageTypeError,
	Slug: SlugTypeError,
	Title: "Type cast error",
	Message: "could not cast netRestrictService.service to *bandwidth.Bandwidth",
	},
	http.StatusInternalServerError)
	return
	}
	bw.Limit = body.Limit
	}
	if a.bw == nil {
	a.bw = &netRestrictService{
	service: &bandwidth.Bandwidth{
	Limit: body.Limit,
	},
	logger: a.logger,
	}
	}
	_, ok := a.bw.service.(*bandwidth.Bandwidth)
	if !ok {
	sendJSONError(resp,
	MetaMessage{
	Type: APIMetaMessageTypeError,
	Slug: SlugTypeError,
	Title: "Type cast error",
	Message: "could not cast netRestrictService.service to *bandwidth.Bandwidth",
	},
	http.StatusInternalServerError)
	return
	}

[mojtaba-esk]

The else is there to check if the service object is already assigned, then it just updates the parameter e.g. bandwidth limit because user can change it dynamically.
if we remove the else, we need to create the service every time the parameter is changed which is also possible, but we need to refactor the underlying code.

[MSevey]

if we remove the else, we need to create the service every time the parameter is changed

why?

Current the code checks if the element exists and either initiates it or updates it.

If you remove the else, the only change you'd need to make to the original if is the assignment of the Limit field, since you would do it at the end.

The benefit would be that now, regardless of if the element existed previously, or if it was just initiated, we are type checking and then updating.

[MSevey]

also where did all this code go? I don't see it in the diff anymore?

[mojtaba-esk]

also where did all this code go? I don't see it in the diff anymore?

There was an issue in commits after resolving conflicts the codes are not much different from the main.

Current the code checks if the element exists and either initiates it or updates it.

If we wanna get rid of the else, we do not even need to do a type check.
we just need to write this code:

a.bw = &netRestrictService{
		service: &bandwidth.Bandwidth{
			Limit: body.Limit,
		},
	}

After checking the underlying code found out it is safe to keep the if and else because the netRestrictService object keeps a status boolean ready which makes sure not to start again when the stop has not been called first on an already started service. The stop function calls the cancel function whose reference is stored in the same object.

type netRestrictService struct {
	service xdp.XdpLoader
	cancel  xdp.CancelFunc
	ready   bool
}

if we simply replace it, we will loose the status and the cancel function reference.
It will work on setting the Kernel configs as they will be replaced, but if something like the network interface index changes or when we want to stop the service, we will face wrong file descriptor error because of not getting cancelled causes the underlying XdpObject thinks that there are running services while they do not exist and starting on an unclosed object creates issues.

Ref: https://github.com/celestiaorg/bittwister/blob/mojtaba/api-server/xdp/types.go#L63

We can find a way to get rid of that if by changing the logic, like we create netRestrictService objects when the API server starts and then change the checks. But I am not sure if that adds a lot of value. WDYT?

[mojtaba-esk]

@MSevey I made some changes to the code where we initiate the xdp services with the API server object and simplify the code in this file, there is no large if else statement anymore.