Astarte: update /version endpoint #893

osmanhadzic · 2024-01-22T14:35:20Z

Provide third-party applications with information about the version of Astarte microservices they are interacting with, enabling them to adapt their behavior based on the underlying service version.

Standardization of routers for the Astarte API endpoints to have a uniform Phoenix pipeline structure.

Add a /version endpoint to each microservice API, allowing third-party applications to query and retrieve the microservice version using the GET verb. Authentication is not required for accessing this endpoint.

Standardize code for Astarte APIs by refactoring the router for endpoints. Remove plugs from controllers and incorporate them into the router pipeline.

By improving code readability in the app, astarte will expose information about the microservice version to third-party applications without authorization.

(#890)

codecov · 2024-01-22T14:38:39Z

Codecov Report

Attention: Patch coverage is 78.94737% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 68.56%. Comparing base (965ed38) to head (ee23ce3).

Files	Patch %	Lines
...engine_api/lib/astarte_appengine_api_web/router.ex	78.57%	6 Missing ⚠️
...ing_api/lib/astarte_housekeeping_api_web/router.ex	0.00%	1 Missing ⚠️
..._pairing_api/lib/astarte_pairing_api_web/router.ex	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #893      +/-   ##
==========================================
+ Coverage   68.08%   68.56%   +0.48%     
==========================================
  Files         274      226      -48     
  Lines        7093     6553     -540     
==========================================
- Hits         4829     4493     -336     
+ Misses       2264     2060     -204

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

apps/astarte_appengine_api/config/config.exs

apps/astarte_appengine_api/lib/astarte_appengine_api_web/endpoint.ex

apps/astarte_appengine_api/lib/astarte_appengine_api_web/plug/microservice_version_plug.ex

apps/astarte_appengine_api/lib/astarte_appengine_api_web/endpoint.ex

Annopaolo

Could you please also update commit and PR description to reflect latest changes?

apps/astarte_appengine_api/lib/astarte_appengine_api_web/router.ex

apps/astarte_pairing_api/lib/astarte_pairing_api_web/router.ex

Annopaolo

Some nitpicking on commits:

Pay attention to typos in the title: /verison --> /version
It would be better to keep consistency in commit titles.
If you prefer, you can have a different pipeline standardization commit for each app, but then it would be better to mention the app in the title rather than keeping it generic (as you already did for AppEngine and RealmManagement API). Otherwise, if you want, you can put all pipeline standardization commits together: in this case, it is ok to say that the commit scope includes all apps.

apps/astarte_appengine_api/lib/astarte_appengine_api_web/router.ex

Annopaolo

More nitpicking:

Commits 706f43a and 28ad443 should refer to pairing API rather than be generic, as all others
Commit ff0e150 has a typo in the title
The PR title is now outdated, as it is not just AppEngine that is changed

Finally, a new line should be added to CHANGELOG.md stating that the new /version endpoint was added. Refer to Keep a Changelog for the format

apps/astarte_appengine_api/lib/astarte_appengine_api_web/router.ex

Annopaolo

@davidebriani's point was to use nested scopes, instead of nested pipelines. At each nesting level in scopes, it would be possible to add different plugs.
This could be clearer as it would allow to segregate the different sections of the API without adding new names to keep in mind (i.e. pipeline names).

I left a comment to show an example of a scope that could be nested

apps/astarte_appengine_api/lib/astarte_appengine_api_web/router.ex

apps/astarte_pairing_api/lib/astarte_pairing_api_web/router.ex

Annopaolo · 2024-02-20T16:56:04Z

apps/astarte_pairing_api/lib/astarte_pairing_api_web/router.ex

    post "/agent/devices", AgentController, :create
    delete "/agent/devices/:device_id", AgentController, :delete


The Astarte.Pairing.APIWeb.Plug.AuthorizePath plug is missing (see https://github.com/astarte-platform/astarte/pull/893/files#diff-7bdaad5dbdd7c2ef3cc43ee2eaa46cc23afd1f6e6ae3ef955e274188665cc0c2L27), it should be included

I created :api_device and :api pipeline to separate. DeviceController does not require using Astarte.Pairing.APIWeb.Plug.AuthorizePath . I create a new pipeline only for DeviceController to separate from other controllers. @Annopaolo
:api_device does not have anything with pipeline :api

Both "/agent/devices" and "/agent/devices/:device_id" should have the Astarte.Pairing.APIWeb.Plug.AuthorizePath plug, that is currently missing (it has been removed by this PR, see https://github.com/astarte-platform/astarte/pull/893/files/cc196eee9bb629d3ef0863faba74d6718c8d1f41#diff-7bdaad5dbdd7c2ef3cc43ee2eaa46cc23afd1f6e6ae3ef955e274188665cc0c2L27-L28). Unless it is added back, those two endpoints are broken

Annopaolo

Also, an AppEngine API route seems to be missing (see CI run):

(Phoenix.Router.NoRouteError) no route found for GET /v1/autotestrealm/devices-by-alias/device_a/interfaces/com.test.LCDMonitor/time/to (Astarte.AppEngine.APIWeb.Router)

Add a new scope, '/version', in the router with the HTTP GET method endpoint for the version controller. The scope does not require any type of authorization and does not have any effect on other endpoints in the API. (astarte-platform#890) Signed-off-by: Osman Hadzic <[email protected]>

Annopaolo

Almost there!

apps/astarte_pairing_api/lib/astarte_pairing_api_web/router.ex

Annopaolo

Apart from two minor details, LGTM

Annopaolo · 2024-03-11T10:24:57Z

apps/astarte_pairing_api/lib/astarte_pairing_api_web/router.ex

+    scope "/version" do
+      pipe_through :api
+
+      get "/", VersionController, :show
+    end


Since this scope is inside another scope that is piped through :device_api, the "/version" call will pass through the LogHwId plug, but there is no id to log when calling /version

Annopaolo · 2024-03-11T10:36:52Z

apps/astarte_pairing_api/lib/astarte_pairing_api_web/router.ex

+    scope "/agent" do
+      pipe_through :api
+
+      post "/devices", AgentController, :create
+      delete "/devices/:device_id", AgentController, :delete
+    end


Since this scope is inside another scope that is piped through :device_api, the "/agent" call will pass through the LogHwId plug, but there is no id to log when calling /agent

I made changes.

apps/astarte_appengine_api/lib/astarte_appengine_api_web/router.ex

apps/astarte_pairing_api/lib/astarte_pairing_api_web/router.ex

`astarte_appengine_api` components did not have a uniform Phoenix pipeline structure: some plugs were specified inside the pipeline, and some inside controllers. Most of the plugs specified inside controllers were replicated (e.g. `Astarte.AppEngine.APIWeb.Plug.AuthorizePath` in `astarte_appengine_api`). Refactor exiting code: rationalize the structure of the pipeline by factoring out replicated plugs from controllers and including them in the pipelines. This leads to some replication for scope (seee.g apps/astarte_pairing_api/lib/astarte_pairing_api_web/router.ex), but Phoenix allows for duplicated scopes as long as endpoints are different. The API of the components is not changed. Signed-off-by: Osman Hadzic <[email protected]>

Add a new scope, '/version', in the router with the HTTP GET method endpoint for the version controller. The scope does not require any type of authorization and does not have any effect on other endpoints in the API. Signed-off-by: Osman Hadzic <[email protected]>

`astarte_pairing_api` components did not have a uniform Phoenix pipeline structure: some plugs were specified inside the pipeline, and some inside controllers. Most of the plugs specified inside controllers were replicated (e.g. `Astarte.Pairing.APIWeb.Plug.AuthorizePath` in `astarte_pairing_api`). Refactor exiting code: rationalize the structure of the pipeline by factoring out replicated plugs from controllers and including them in the pipelines. This leads to some replication for scope (seee.g apps/astarte_pairing_api/lib/astarte_pairing_api_web/router.ex), but Phoenix allows for duplicated scopes as long as endpoints are different. The API of the components is not changed. Signed-off-by: Osman Hadzic <[email protected]>

Add a new scope, '/version', in the router with the HTTP GET method endpoint for the version controller. The scope does not require any type of authorization and does not have any effect on other endpoints in the API. (astarte-platform#890) Signed-off-by: Osman Hadzic <[email protected]>

`astarte_realm_management_api` components did not have a uniform Phoenix pipeline structure: some plugs were specified inside the pipeline and some inside controllers. Most of the plugs specified inside controllers were replicated (e.g.`Astarte.RealmManagement.APIWeb.Plug.AuthorizePath` in `astarte_realm_management_api`). Refactor existing code: rationalize the structure of the pipeline by factoring out replicated plugs from controllers and included them in the pipelines. This leads to some replication of the scope (see e.g.apps/astarte_realm_management_api/lib/astarte_pairing_api_web /router.ex), but Phoenix allows for duplicated scopes as long as endpoints are different. The API of the components is not changed. Signed-off-by: Osman Hadzic <[email protected]>

Add a new scope, '/version', in the router with the HTTP GET method endpoint for the version controller. The scope does not require any type of authorization and does not have any effect on other endpoints in the API. (astarte-platform#890) Signed-off-by: Osman Hadzic <[email protected]>

Signed-off-by: Osman Hadzic <[email protected]>

osmanhadzic force-pushed the astarte_api_version branch from 847f994 to 02dd022 Compare January 23, 2024 12:18

Annopaolo requested changes Jan 23, 2024

View reviewed changes

osmanhadzic force-pushed the astarte_api_version branch from 02dd022 to 1f9a76c Compare January 24, 2024 08:51

osmanhadzic requested a review from Annopaolo January 24, 2024 08:51

Annopaolo requested changes Jan 24, 2024

View reviewed changes

apps/astarte_appengine_api/lib/astarte_appengine_api_web/plug/microservice_version_plug.ex Outdated Show resolved Hide resolved

apps/astarte_appengine_api/lib/astarte_appengine_api_web/endpoint.ex Outdated Show resolved Hide resolved

osmanhadzic force-pushed the astarte_api_version branch from 1f9a76c to 4dcb336 Compare January 24, 2024 13:25

osmanhadzic closed this Jan 24, 2024

osmanhadzic force-pushed the astarte_api_version branch from 4dcb336 to d12bab6 Compare January 24, 2024 15:14

osmanhadzic reopened this Jan 24, 2024

Annopaolo requested changes Jan 24, 2024

View reviewed changes

apps/astarte_appengine_api/lib/astarte_appengine_api_web/router.ex Show resolved Hide resolved

osmanhadzic force-pushed the astarte_api_version branch from 93f5d87 to 4ba0016 Compare January 25, 2024 07:45

Annopaolo reviewed Jan 25, 2024

View reviewed changes

apps/astarte_pairing_api/lib/astarte_pairing_api_web/router.ex Show resolved Hide resolved

osmanhadzic force-pushed the astarte_api_version branch 3 times, most recently from 444583d to 0dcb387 Compare January 25, 2024 14:24

osmanhadzic requested a review from Annopaolo January 25, 2024 15:41

osmanhadzic force-pushed the astarte_api_version branch from 0dcb387 to 9ee380e Compare January 26, 2024 08:05

Annopaolo requested changes Jan 26, 2024

View reviewed changes

apps/astarte_appengine_api/lib/astarte_appengine_api_web/router.ex Outdated Show resolved Hide resolved

osmanhadzic force-pushed the astarte_api_version branch from 9ee380e to 1cb07f0 Compare January 29, 2024 14:29

osmanhadzic requested a review from Annopaolo January 29, 2024 15:05

Annopaolo requested changes Jan 29, 2024

View reviewed changes

apps/astarte_appengine_api/lib/astarte_appengine_api_web/router.ex Outdated Show resolved Hide resolved

apps/astarte_appengine_api/lib/astarte_appengine_api_web/router.ex Outdated Show resolved Hide resolved

Annopaolo added the API This issue or pull request is about API (e.g. unclear API, new API, API change, deprecation) label Jan 29, 2024

osmanhadzic changed the title ~~AppEngine: update /version endpoint~~ Astarte: update /version endpoint Jan 30, 2024

osmanhadzic force-pushed the astarte_api_version branch from 1cb07f0 to aa84119 Compare January 30, 2024 12:58

davidebriani requested changes Jan 30, 2024

View reviewed changes

apps/astarte_appengine_api/lib/astarte_appengine_api_web/router.ex Outdated Show resolved Hide resolved

osmanhadzic force-pushed the astarte_api_version branch from aa84119 to 138acf8 Compare February 6, 2024 09:18

osmanhadzic requested a review from davidebriani February 6, 2024 09:18

Annopaolo reviewed Feb 6, 2024

View reviewed changes

apps/astarte_appengine_api/lib/astarte_appengine_api_web/router.ex Outdated Show resolved Hide resolved

osmanhadzic force-pushed the astarte_api_version branch from 138acf8 to d266243 Compare February 8, 2024 15:24

osmanhadzic requested a review from davidebriani February 14, 2024 07:44

osmanhadzic marked this pull request as ready for review February 19, 2024 08:02

osmanhadzic force-pushed the astarte_api_version branch from 96eb6a7 to cc196ee Compare February 20, 2024 16:01

Annopaolo requested changes Feb 20, 2024

View reviewed changes

osmanhadzic force-pushed the astarte_api_version branch 2 times, most recently from 35dbde4 to 2474849 Compare March 6, 2024 11:54

osmanhadzic force-pushed the astarte_api_version branch from 2474849 to 736eb8b Compare March 6, 2024 13:36

osmanhadzic requested a review from Annopaolo March 6, 2024 13:47

Annopaolo requested changes Mar 8, 2024

View reviewed changes

apps/astarte_pairing_api/lib/astarte_pairing_api_web/router.ex Show resolved Hide resolved

osmanhadzic requested a review from Annopaolo March 11, 2024 08:03

Annopaolo requested changes Mar 11, 2024

View reviewed changes

osmanhadzic force-pushed the astarte_api_version branch from 736eb8b to 5501d85 Compare March 11, 2024 15:16

osmanhadzic requested a review from Annopaolo March 11, 2024 15:18

Annopaolo approved these changes Mar 11, 2024

View reviewed changes

davidebriani requested changes Mar 11, 2024

View reviewed changes

osmanhadzic added 7 commits March 12, 2024 16:03

Update CHANGELOG.md

ee23ce3

Signed-off-by: Osman Hadzic <[email protected]>

osmanhadzic force-pushed the astarte_api_version branch from 5501d85 to ee23ce3 Compare March 12, 2024 15:03

osmanhadzic requested a review from davidebriani March 12, 2024 15:12

davidebriani approved these changes Mar 12, 2024

View reviewed changes

Annopaolo approved these changes Mar 12, 2024

View reviewed changes

Annopaolo merged commit c17ee3a into astarte-platform:master Mar 12, 2024
29 checks passed

Annopaolo mentioned this pull request Mar 28, 2024

Astarte microservices should expose a version endpoint #890

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Astarte: update /version endpoint #893

Astarte: update /version endpoint #893

osmanhadzic commented Jan 22, 2024 •

edited

Loading

codecov bot commented Jan 22, 2024 •

edited

Loading

Annopaolo left a comment

Annopaolo left a comment

Annopaolo left a comment

Annopaolo left a comment •

edited

Loading

Annopaolo Feb 20, 2024

osmanhadzic Feb 21, 2024

Annopaolo Feb 21, 2024

Annopaolo left a comment

Annopaolo left a comment

Annopaolo left a comment

Annopaolo Mar 11, 2024

Annopaolo Mar 11, 2024

osmanhadzic Mar 11, 2024

		post "/agent/devices", AgentController, :create
		delete "/agent/devices/:device_id", AgentController, :delete

Astarte: update /version endpoint #893

Astarte: update /version endpoint #893

Conversation

osmanhadzic commented Jan 22, 2024 • edited Loading

codecov bot commented Jan 22, 2024 • edited Loading

Codecov Report

Annopaolo left a comment

Choose a reason for hiding this comment

Annopaolo left a comment

Choose a reason for hiding this comment

Annopaolo left a comment

Choose a reason for hiding this comment

Annopaolo left a comment • edited Loading

Choose a reason for hiding this comment

Annopaolo Feb 20, 2024

Choose a reason for hiding this comment

osmanhadzic Feb 21, 2024

Choose a reason for hiding this comment

Annopaolo Feb 21, 2024

Choose a reason for hiding this comment

Annopaolo left a comment

Choose a reason for hiding this comment

Annopaolo left a comment

Choose a reason for hiding this comment

Annopaolo left a comment

Choose a reason for hiding this comment

Annopaolo Mar 11, 2024

Choose a reason for hiding this comment

Annopaolo Mar 11, 2024

Choose a reason for hiding this comment

osmanhadzic Mar 11, 2024

Choose a reason for hiding this comment

osmanhadzic commented Jan 22, 2024 •

edited

Loading

codecov bot commented Jan 22, 2024 •

edited

Loading

Annopaolo left a comment •

edited

Loading