Skip to content

Commit

Permalink
HTTP controller API
Browse files Browse the repository at this point in the history
  • Loading branch information
vk committed Oct 6, 2021
1 parent e089718 commit 2dee681
Show file tree
Hide file tree
Showing 7 changed files with 1,481 additions and 5 deletions.
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,5 @@ ergw.config
data.ergw*
log.ergw*
apps/ergw_core/priv*
data
ergw@*
23 changes: 18 additions & 5 deletions apps/ergw/src/ergw_config.erl
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
-compile({no_auto_import,[put/2]}).

%% API
-export([load/0, apply/1, serialize_config/1]).
-export([load/0, apply/1, serialize_config/1, load_part_of_config/1]).
-export([ergw_core_init/2]).

This comment has been minimized.

Copy link
@ebengt

ebengt Oct 6, 2021

Contributor

I would appreciate an explanation about why this is its own export. Why not add the function to the existing export?

This comment has been minimized.

Copy link
@vkatsuba

vkatsuba Oct 6, 2021

Contributor

Yep make sense, will update it. Thanks.


-ifdef(TEST).
-export([config_meta/0,
Expand Down Expand Up @@ -45,6 +46,16 @@
%%% API
%%%===================================================================

load_part_of_config(Config) when is_map(Config) ->
load_typespecs(),
do([error_m ||
load_schemas(),
validate_config_with_schema(Config),
return(coerce_config(Config))
]);
load_part_of_config(Config) ->
erlang:error(badarg, [Config]).

This comment has been minimized.

Copy link
@RoadRunnr

RoadRunnr Oct 6, 2021

Member

I really hate this construct. The config does not need to be identical to the HTTP API structures and even when it reuses parts, applying those parts should not require a full config validation.

This comment has been minimized.

Copy link
@javiermtorres

javiermtorres Oct 6, 2021

IMHO the idea should be to reuse what's already in place as much as possible to have a link from k8s to dynamic config ASAP. Then we can polish the internal comm.

This comment has been minimized.

Copy link
@RoadRunnr

RoadRunnr Oct 6, 2021

Member

Reuse is good, but this CAN NOT impact code quality. Not matter how fast you want to move this.

This comment has been minimized.

Copy link
@javiermtorres

javiermtorres Oct 6, 2021

This is a prototype. If it's good enough, let's move forward. There will always be corrections later on, no matter how closely we watch things. If you have a concrete proposal for right now, we can do it; otherwise you can have some more time to think of something and we can change it later on.

This comment has been minimized.

Copy link
@vkatsuba

vkatsuba Oct 6, 2021

Contributor

the main idea is apply as part as full config, I suppose we need just rename this function because name is confusing

This comment has been minimized.

Copy link
@RoadRunnr

RoadRunnr Oct 6, 2021

Member

This is a PR for merging into master, it doesn't have a [wip] tag or anything like that. It should therefore not merge stuff that it is not ready for master.
Especially since this such a small thing and doing it right is not hard at all.

This comment has been minimized.

Copy link
@javiermtorres

javiermtorres Oct 6, 2021

Then what is your suggestion? We can definitely use a feature/smc_cluster branch and merge from master to keep it aligned, for example, instead of merging with master now.

This comment has been minimized.

Copy link
@RoadRunnr

RoadRunnr Oct 6, 2021

Member

more like rebasing on top of master from time to time. It's not as if we are doing major change in the ergw app right now. Merging from master would create a non-linear history and we try hard to avoid that.

This comment has been minimized.

Copy link
@vkatsuba

vkatsuba Oct 6, 2021

Contributor

Maybe I miss something but if user will want to set full config JSON by REST API, the format of config is the same if we will put it into erGW before run. As I understand the function should be ready for validate/load all config values.

load() ->
load_typespecs(),
do([error_m ||
Expand Down Expand Up @@ -129,8 +140,9 @@ ergw_aaa_init(apps, #{apps := Apps0}) ->
Apps = ergw_aaa_config:validate_options(fun ergw_aaa_config:validate_app/2, Apps0, []),
maps:map(fun ergw_aaa:add_application/2, Apps),
Apps;
ergw_aaa_init(_, _) ->
ok.
ergw_aaa_init(K, _) ->
?LOG(warning, "The key ~p is missed in config of erGW-AAA", [K]),
{error, unhandled}.

ergw_sbi_client_init(Opts) ->
ergw_sbi_client_config:validate_options(fun ergw_sbi_client_config:validate_option/2, Opts).
Expand Down Expand Up @@ -185,8 +197,9 @@ ergw_core_init(proxy_map, #{proxy_map := Map}) ->
ok = ergw_core:setopts(proxy_map, Map);
ergw_core_init(http_api, #{http_api := Opts}) ->
ergw_http_api:init(Opts);
ergw_core_init(_K, _) ->
ok.
ergw_core_init(K, _) ->
?LOG(warning, "The key ~p is missed in config of erGW", [K]),
{error, unhandled}.

ergw_charging_init(rules, #{rules := Rules}) ->
maps:map(fun ergw_core:add_charging_rule/2, Rules);
Expand Down
2 changes: 2 additions & 0 deletions apps/ergw/src/ergw_http_api.erl
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ start_http_listener(#{enabled := true} = Opts) ->
{"/status/[...]", http_status_handler, []},
%% 5G SBI APIs
{"/sbi/nbsf-management/v1/pcfBindings", sbi_nbsf_handler, []},
%% HTTP controller
{"/api/v1/controller", http_controller_handler, []},

This comment has been minimized.

Copy link
@ebengt

ebengt Oct 6, 2021

Contributor

If possible it would be nice to have the path defined in one place, instead of hard coded in two places (here and http_controller_handler.erl)

This comment has been minimized.

Copy link
@vkatsuba

vkatsuba Oct 6, 2021

Contributor

Used same approach and code style. Keep in one place, means that we will need create *.hrl file and use macros in all places or use persistent terms, this refactoring is a make sense but this is out of scope of current PR.

This comment has been minimized.

Copy link
@RoadRunnr

RoadRunnr Oct 6, 2021

Member

might be better to use /api/v1/controller/... instead or something similar.

The full path checking is replicated in some other handlers as well. A review/change of those places should be done.

%% serves static files for swagger UI
{"/api/v1/spec/ui", swagger_ui_handler, []},
{"/api/v1/spec/ui/[...]", cowboy_static, {priv_dir, ergw_core, "static"}}]}
Expand Down
163 changes: 163 additions & 0 deletions apps/ergw/src/http_controller_handler.erl
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
%% Copyright 2021, Travelping GmbH <info@travelping.com>

%% This program is free software; you can redistribute it and/or
%% modify it under the terms of the GNU General Public License
%% as published by the Free Software Foundation; either version
%% 2 of the License, or (at your option) any later version.

-module(http_controller_handler).

-behavior(cowboy_rest).

-export([init/2, content_types_provided/2, handle_request_json/2,
allowed_methods/2, delete_resource/2, content_types_accepted/2]).

-ignore_xref([handle_request_json/2]).

-include_lib("kernel/include/logger.hrl").

-define(POST, <<"POST">>).

This comment has been minimized.

Copy link
@RoadRunnr

RoadRunnr Oct 6, 2021

Member

I'm not sure that this is really needed.

-define(HTTP_200, 200).
-define(HTTP_400, 400).

This comment has been minimized.

Copy link
@RoadRunnr

RoadRunnr Oct 6, 2021

Member

this is really not needed, HTTP status codes are so well known that having defines for them is superfluous. Especially when the name of the define contains the number as well.

-define(CONTENT_TYPE_PROBLEM_JSON, #{<<"content-type">> => "application/problem+json"}).

init(Req0, State) ->
case cowboy_req:version(Req0) of
'HTTP/2' ->
{cowboy_rest, Req0, State};
_ ->
Body = jsx:encode(#{
title => <<"HTTP/2 is mandatory.">>,
status => 505,
cause => <<"UNSUPPORTED_HTTP_VERSION">>
}),
Req = cowboy_req:reply(505, ?CONTENT_TYPE_PROBLEM_JSON, Body, Req0),
{ok, Req, done}
end.

allowed_methods(Req, State) ->
{[?POST], Req, State}.

content_types_provided(Req, State) ->
{[{<<"application/json">>, handle_request_json}], Req, State}.

content_types_accepted(Req, State) ->
{[{'*', handle_request_json}], Req, State}.

delete_resource(Req, State) ->
Path = cowboy_req:path(Req),
Method = cowboy_req:method(Req),
handle_request(Method, Path, Req, State).

handle_request_json(Req, State) ->
Path = cowboy_req:path(Req),
Method = cowboy_req:method(Req),
handle_request(Method, Path, Req, State).

%%%===================================================================
%%% Handler of request
%%%===================================================================

handle_request(?POST, <<"/api/v1/controller">>, Req0, State) ->
{ok, Body, Req} = read_body(Req0),
case validate_json_req(Body) of
{ok, Response} ->
reply(?HTTP_200, Req, jsx:encode(Response), State);
{error, invalid_json} ->
Response = rfc7807(<<"Invalid JSON">>, <<"INVALID_JSON">>, []),
reply(?HTTP_400, Req, Response, State);
{error, InvalidParams} ->
Response = rfc7807(<<"Invalid JSON params">>, <<"INVALID_JSON_PARAM">>, InvalidParams),
reply(?HTTP_400, Req, Response, State)
end.

%%%===================================================================
%%% Helper functions
%%%===================================================================

validate_json_req(JsonBin) ->
case json_to_map(JsonBin) of
{ok, Map} ->
apply_config(Map);
Error ->
Error
end.

% Jesse errors: https://github.com/for-GET/jesse/blob/1.5.6/src/jesse_error.erl#L40
apply_config(Map) ->
case catch ergw_config:load_part_of_config(Map) of
{ok, Config} ->
_ = maps:fold(fun add_config_part/3, #{}, Config),
% @TODO for response collect all keys of config what was successfully applied
{ok, #{type => <<"success">>}};
{error, [_|_] = Error} = Reason ->
?LOG(warning, "~p", [Reason]),
Params = build_error_params(Error),
{error, Params};
Error ->
?LOG(warning, "Unhandled error ~p~nfor config ~p", [Error, Map]),
{error, []}
end.

build_error_params(Data) ->
build_error_params(Data, []).

This comment has been minimized.

Copy link
@ebengt

ebengt Oct 6, 2021

Contributor

What is the reason to not use lists:map/2 here?

This comment has been minimized.

Copy link
@vkatsuba

vkatsuba Oct 6, 2021

Contributor

More flexible for me use functions.

This comment has been minimized.

Copy link
@javiermtorres

javiermtorres Oct 6, 2021

Yes, I agree. Using the map function is cleaner.

This comment has been minimized.

Copy link
@vkatsuba

vkatsuba Oct 6, 2021

Contributor

Again when we will need to update it, we will remove map function from here, no make sense to use it here.


build_error_params([], Acc) ->
Acc;
build_error_params([{Type, Schema, Error, Data, Path}|T], Acc) ->
I = #{
type => atom_to_binary(Type),
schema => Schema,
error => atom_to_binary(Error),
data => Data,
path => Path
},
build_error_params(T, [I|Acc]).

add_config_part(K, V, Acc) ->
Result = ergw_config:ergw_core_init(K, #{K => V}),
?LOG(info, "The ~p added with result ~p", [K, Result]),
maps:merge(Acc, Result).

json_to_map(JsonBin) ->
case catch jsx:decode(JsonBin) of
#{} = Map ->
{ok, Map};
_ ->
{error, invalid_json}
end.

read_body(Req) ->
read_body(Req, <<>>).

read_body(Req0, Acc) ->
case cowboy_req:read_body(Req0) of
{ok, Data, Req} ->
{ok, <<Acc/binary, Data/binary>>, Req};
{more, Data, Req} ->
read_body(Req, <<Acc/binary, Data/binary>>)
end.

reply(StatusCode, Req0, Body, State) ->
Req = case StatusCode of
?HTTP_200 ->
cowboy_req:reply(StatusCode, #{}, Body, Req0);
?HTTP_400 ->
cowboy_req:reply(StatusCode, ?CONTENT_TYPE_PROBLEM_JSON, Body, Req0)
end,
{stop, Req, State}.

%% https://datatracker.ietf.org/doc/html/rfc7807
rfc7807(Title, Cause, []) ->
jsx:encode(#{
title => Title,
status => ?HTTP_400,
cause => Cause
});
rfc7807(Title, Cause, InvalidParams) ->
jsx:encode(#{
title => Title,
status => ?HTTP_400,
cause => Cause,
'invalid-params' => InvalidParams
}).

1 comment on commit 2dee681

@ebengt
Copy link
Contributor

@ebengt ebengt commented on 2dee681 Oct 6, 2021

Choose a reason for hiding this comment

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

Looks good.

Please sign in to comment.