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

The API routing does not feel RESTful #98

Closed
thibaultcha opened this issue Mar 25, 2015 · 14 comments
Closed

The API routing does not feel RESTful #98

thibaultcha opened this issue Mar 25, 2015 · 14 comments
Assignees
Milestone

Comments

@thibaultcha
Copy link
Member

Writing the Getting Started guide raised a concern about the structure of our routes. Having to explain:

Nope

# Create an API:
$ curl -i -X POST \
  --url http://localhost:8001/apis/ \
  --data 'name=mockbin&target_url=http://mockbin.com/&public_dns=mockbin.com'
HTTP/1.1 201 Created
...

# Add a plugin to it:
# Make sure the api_id parameter matches the one of mockbin created earlier
$ curl -i -X POST \
  --url http://localhost:8001/plugins/ \
  --data 'name=headerauth&api_id=<api_id>&value.header_names=apikey'
HTTP/1.1 201 Created
...

Feels wrong. Having to pass the api_id as a form parameter does not feel REST. It would be nicer to do:

Yep

# Create an API:
$ curl -i -X POST \
  --url http://localhost:8001/apis/ \
  --data 'name=mockbin&target_url=http://mockbin.com/&public_dns=mockbin.com'
HTTP/1.1 201 Created
...

# Add a plugin to it:
$ curl -i -X POST \
  --url http://localhost:8001/apis/mockbin/plugins/ \
  --data 'name=headerauth&value.header_names=apikey'
HTTP/1.1 201 Created
...

Here the route became: /apis/{api_name}/plugins/. Since plugins can also be attached to applications, do we have to create the applications/{id}/apis/{api_id/api_name}/plugins/ route? Probably.

Same for applications and accounts:

Nope

# Create an account
$ curl -i -X POST \ 
  --url http://localhost:8001/accounts/
  --data ''
HTTP/1.1 201 Created
...

# And an application
# Make sure the given account_id matches the freshly created account
$ curl -i -X POST \
  --url http://localhost:8001/applications/
  --data 'public_key=123456&account_id=<account_id>'
HTTP/1.1 201 Created
...

Becomes:

Yep

# Create an account
$ curl -i -X POST \ 
  --url http://localhost:8001/accounts/
  --data ''
HTTP/1.1 201 Created
...

# And an application
$ curl -i -X POST \
  --url http://localhost:8001/accounts/{account_id}/applications/
  --data 'public_key=123456'
HTTP/1.1 201 Created
...

New route: /accounts/{account_id/provider_id}/applications/.

@subnetmarco
Copy link
Member

We would still need to provide generic endpoints like /plugins/ or /applications/: for example, in your examples above how would you retrieve an application by ID without knowing the owner account (or by public_key, or secret_key) ?

We need to learn what are the use-cases we want to support (does it make sense to search an application by ID in the first place?), then we need to find the right mix of simplicity and RESTfulness without necessarily complicating the user experience.

@thibaultcha
Copy link
Member Author

Yes of course, /plugins/ and others would stay.

@thibaultcha thibaultcha modified the milestone: 1.0.0 Mar 27, 2015
@subnetmarco subnetmarco modified the milestones: 0.3.0, 0.2.0 Apr 22, 2015
@wilmoore
Copy link

While at it, it would be nice to also consider accepting parameters via Content-Type: application/json vs. Content-Type: application/x-www-form-urlencoded.

@nijikokun
Copy link
Contributor

@thibaultcha You are passing the identifiers twice (once in uri, and another in the payload) in your yep examples, which I think you meant the opposite, otherwise there would be no benefit.

@thibaultcha thibaultcha self-assigned this May 14, 2015
@thibaultcha
Copy link
Member Author

@wilmoore This is now done since #236! The API supports PATCH, PUT, POST all in JSON or form-encoded.

@wilmoore
Copy link

@thibaultcha 👍

@DavidTPate
Copy link

👍

@thibaultcha
Copy link
Member Author

So, among many other things, the API now has an improved support for the plugins_configurations (plugins enabled on some APIs, they contain the plugin config for that API).

It means:

GET PUT POST
/apis/{:name | :id}/plugins/

GET PATCH DELETE
/apis/{:name | :id}/plugins/{:name | :id}

Does it still make sense to support the old route:

GET PUT POST
/plugins_configurations/

GET PATCH DELETE
/plugins_configurations/:id

or not? One use case I can see for GET on /plugins_configurations/ is for listing all plugins enabled across all the APIs, but that would be all.

@ahmadnassri
Copy link
Contributor

@thibaultcha i would keep support for old route in the next release, and remove it in the following release, just for backward compatibility.

as for all plugins, I see that as a special endpoint, perhaps simply GET /plugins

@subnetmarco
Copy link
Member

If we use /apis/{:name | :id}/plugins/ as opposed to /apis/{:name | :id}/plugins_configurations/ then also the general route should be called /plugins instead of /plugins_configurations if we decide to keep it.

@ahmadnassri
Copy link
Contributor

@thefosk perhaps the general route is only a GET for listing, and keep all the actual plugin interactions (PUT, DELETE, POST, etc ...) per API route.

@thibaultcha
Copy link
Member Author

The reasoning we had is that in the context of apis/:api/plugins/, we know we are talking about configuration of a plugin on top of an API. Where plugins_configurations was named like this in the first place because we wanted to make it clear what it was about.

thibaultcha added a commit that referenced this issue May 22, 2015
- keyauth and basicauth plugins now have their APIs updated and their
  route feels more RESTful too, they are accessible via
  /consumers/:consumer/keyauth
- A migration was needed to be able to retrieve them per consumer.
- Slightly better file structure

Related to #98
@thibaultcha
Copy link
Member Author

PR opened with some massive changes: #257

thibaultcha added a commit that referenced this issue May 22, 2015
- keyauth and basicauth plugins now have their APIs updated and their
  route feels more RESTful too, they are accessible via
  /consumers/:consumer/keyauth
- A migration was needed to be able to retrieve them per consumer.
- Slightly better file structure

Related to #98
thibaultcha added a commit that referenced this issue May 26, 2015
- keyauth and basicauth plugins now have their APIs updated and their
  route feels more RESTful too, they are accessible via
  /consumers/:consumer/keyauth
- A migration was needed to be able to retrieve them per consumer.
- Slightly better file structure

Related to #98
thibaultcha added a commit that referenced this issue May 26, 2015
- keyauth and basicauth plugins now have their APIs updated and their
  route feels more RESTful too, they are accessible via
  /consumers/:consumer/keyauth
- A migration was needed to be able to retrieve them per consumer.
- Slightly better file structure

Related to #98
thibaultcha added a commit that referenced this issue May 28, 2015
- keyauth and basicauth plugins now have their APIs updated and their
  route feels more RESTful too, they are accessible via
  /consumers/:consumer/keyauth
- A migration was needed to be able to retrieve them per consumer.
- Slightly better file structure

Related to #98
thibaultcha added a commit that referenced this issue May 28, 2015
- keyauth and basicauth plugins now have their APIs updated and their
  route feels more RESTful too, they are accessible via
  /consumers/:consumer/keyauth
- A migration was needed to be able to retrieve them per consumer.
- Slightly better file structure

Related to #98
@thibaultcha
Copy link
Member Author

Closing this now that #257 has been merged. Please report any potential problem in a new issue!

ctranxuan pushed a commit to streamdataio/kong that referenced this issue Aug 25, 2015
- keyauth and basicauth plugins now have their APIs updated and their
  route feels more RESTful too, they are accessible via
  /consumers/:consumer/keyauth
- A migration was needed to be able to retrieve them per consumer.
- Slightly better file structure

Related to Kong#98


Former-commit-id: 4c0ff2dc64a7dcb607472fe164c0b97ee5b9bd87
fffonion added a commit that referenced this issue Jun 2, 2021
- Fix exporter to attach subsystem label to memory stats
  [#118](Kong/kong-plugin-prometheus#118)
- Expose dataplane status on control plane, new metrics `data_plane_last_seen`,
  `data_plane_config_hash` and `data_plane_version_compatible` are added.
  [#98](Kong/kong-plugin-prometheus#98)
dndx pushed a commit that referenced this issue Jun 2, 2021
- Fix exporter to attach subsystem label to memory stats
  [#118](Kong/kong-plugin-prometheus#118)
- Expose dataplane status on control plane, new metrics `data_plane_last_seen`,
  `data_plane_config_hash` and `data_plane_version_compatible` are added.
  [#98](Kong/kong-plugin-prometheus#98)
gszr pushed a commit that referenced this issue Jun 10, 2021
This PR adds a series of metrics to expose connected Data Plane metrics on Control Plane
side, including `data_plane_last_seen`, `data_plane_config_hash` and `data_plane_version_compatible`.
gszr pushed a commit that referenced this issue Aug 6, 2021
This PR adds a series of metrics to expose connected Data Plane metrics on Control Plane
side, including `data_plane_last_seen`, `data_plane_config_hash` and `data_plane_version_compatible`.
gszr pushed a commit that referenced this issue Aug 18, 2021
* chore(session) bump lua-resty-session from 3.3 to 3.5

### Summary

## [3.5] - 2020-05-22
### Fixed
- Fix `session:hide()` to not clear non-session request cookies that it
  seemed to do in some cases as reported by @altexy who also provided
  initial fix with #100. Thank you!

## [3.4] - 2020-05-08
### Fixed
- Fix session_cookie_maxsize - error attempt to compare string with number,
  fixes #98, thank you @vavra5

### Changed
- More robust and uniform configuration parsing

* chore(session) release 2.4.1

### Summary

- bump lua-resty-session from 3.3 to 3.5
javierguerragiraldez pushed a commit that referenced this issue Sep 3, 2021
also loosen Penlight constraints
fffonion added a commit that referenced this issue Dec 6, 2022
<a name="0.10.1"></a>
## [0.10.1] - 2022-12-06
### bug fixes
- **zerossl:** concatenate response body as string instead of table ([#98](fffonion/lua-resty-acme#98)) [986b1db](fffonion/lua-resty-acme@986b1db)


<a name="0.10.0"></a>
## [0.10.0] - 2022-11-18
### features
- **autossl:** expose function to get cert from LRU cache ([#96](fffonion/lua-resty-acme#96)) [6135d0e](fffonion/lua-resty-acme@6135d0e)
- **autossl:** better cache handling in blocking mode [40f5d2d](fffonion/lua-resty-acme@40f5d2d)
- **autossl:** fix behavior change in non blocking mode [aa484cc](fffonion/lua-resty-acme@aa484cc)
- **autossl:** move chains set condition back inside the main loop [b83a535](fffonion/lua-resty-acme@b83a535)
- **autossl:** add blocking mode [5a623a5](fffonion/lua-resty-acme@5a623a5)
flrgh pushed a commit that referenced this issue Dec 8, 2022
<a name="0.10.1"></a>
## [0.10.1] - 2022-12-06
### bug fixes
- **zerossl:** concatenate response body as string instead of table ([#98](fffonion/lua-resty-acme#98)) [986b1db](fffonion/lua-resty-acme@986b1db)


<a name="0.10.0"></a>
## [0.10.0] - 2022-11-18
### features
- **autossl:** expose function to get cert from LRU cache ([#96](fffonion/lua-resty-acme#96)) [6135d0e](fffonion/lua-resty-acme@6135d0e)
- **autossl:** better cache handling in blocking mode [40f5d2d](fffonion/lua-resty-acme@40f5d2d)
- **autossl:** fix behavior change in non blocking mode [aa484cc](fffonion/lua-resty-acme@aa484cc)
- **autossl:** move chains set condition back inside the main loop [b83a535](fffonion/lua-resty-acme@b83a535)
- **autossl:** add blocking mode [5a623a5](fffonion/lua-resty-acme@5a623a5)
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

6 participants