Add server capabilities API #3966

rawlinp · 2019-10-08T16:00:46Z

What does this PR (Pull Request) do?

Add a new API for creating, reading, and deleting (not updating) server capabilities:

POST /api/1.4/server_capabilities
GET /api/1.4/server_capabilities[?name=<name>]
DELETE /api/1.4/server_capabilities?name=<name>

expected POST body:
{"name": "bar"}

expected POST response:
{
  "alerts": [
    {
      "text": "server capability was created.",
      "level": "success"
    }
  ],
  "response": {
    "name": "bar",
    "lastUpdated": "2019-10-07 22:10:00+00"
  }
}

expected GET response (can use ?name=... qparam to filter by name):
{
  "response": [
    {
      "name": "bar",
      "lastUpdated": "2019-10-07 20:38:24+00"
    }
  ]
}

expected DELETE response:
{
  "alerts": [
    {
      "text": "server capability was deleted.",
      "level": "success"
    }
  ]
}

This PR is not related to any Issue

Which Traffic Control components are affected by this PR?

Traffic Control Client (Go)
Traffic Monitor
Traffic Ops

Documentation will be done in a follow-up PR.

What is the best way to verify this PR?

Read the added TO API tests to verify they're sufficient, then run the TO API tests.

The following criteria are ALL met by this PR

This PR includes tests OR I have explained why tests are unnecessary
This PR includes documentation OR I have explained why documentation is unnecessary
This PR includes an update to CHANGELOG.md OR such an update is not necessary
This PR includes any and all required license headers
This PR ensures that database migration sequence is correct OR this PR does not include a database migration
This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

asf-ci · 2019-10-08T16:27:08Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4418/

asf-ci · 2019-10-08T16:31:33Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4419/

traffic_ops/traffic_ops_golang/servercapability/servercapability.go

traffic_ops/client/servercapability.go

asf-ci · 2019-10-08T16:58:35Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4420/

ocket8888 · 2019-10-08T17:21:00Z

Was this new endpoint brought to the mailing list's attention? Not that I think anyone will have a problem with it, just seems like something important enough to bother with.

rawlinp · 2019-10-08T17:48:57Z

I think Rob is working on a blueprint.

asf-ci · 2019-10-08T17:50:45Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4422/

mhoppa

Aside from the blueprint/mailing list discussion. The changes look good. API tests work locally for me and manually testing of the API looks good.

mitchell852 · 2019-10-08T20:23:54Z

Since there are no docs with this PR, can you add to your PR description what that routes are and the expected request payloads for each? Or simply an example?

rawlinp · 2019-10-08T20:39:22Z

Updated the description with routes and expected payloads.

mitchell852 · 2019-10-08T20:47:50Z

Updated the description with routes and expected payloads.

GET /server_capabilities[?name=]
POST /server_capabilities
DELETE /server_capabilities?name=

right?

rawlinp · 2019-10-08T21:10:07Z

right, updated to include that

rob05c

Looks great! Tested, works as expected. API tests pass, unit tests pass.

All my comments are nitpicks; additional PRs for them would be great, but not necessary.

But, this shouldn't be merged into master until there's consensus on the list.

Also, this either needs docs added to this PR, or a Github Issue to make docs for it, before merging.

traffic_ops/client/servercapability.go

traffic_ops/testing/api/v14/servercapabilities_test.go

traffic_ops/traffic_ops_golang/servercapability/servercapability.go

rawlinp · 2019-10-08T22:18:12Z

Opened placeholder draft PR for adding documentation: #3971

mitchell852 · 2019-10-09T15:05:54Z

without reading thru the code, does DELETE validate that the "server capability" is:

a. not being used by any server and
b. is not required by any ds

with some sort of alert error like "server capability failed. server capability used by X servers/delivery services"

sorry, i should just read the code :)

rob05c · 2019-10-09T15:36:58Z

without reading thru the code, does DELETE validate that the "server capability" is:

a. not being used by any server and
b. is not required by any ds

Server-Capabilities and DS-Required-Capabilities don't exist in this PR, that'll have to be added in a subsequent PR.

But, I'd vote we don't add custom Go logic to handle that - rather, if we make them proper Foreign Keys that don't CASCADE DELETE, the DB will automatically prevent it. Then, all we have to do is parse the Postgres error to return a useful message to the user (we already have a func for that api.ParseDBError

mhoppa · 2019-10-09T15:39:30Z

without reading thru the code, does DELETE validate that the "server capability" is:

a. not being used by any server and
b. is not required by any ds

Server-Capabilities and DS-Required-Capabilities don't exist in this PR, that'll have to be added in a subsequent PR.

But, I'd vote we don't add custom Go logic to handle that - rather, if we make them proper Foreign Keys that don't CASCADE DELETE, the DB will automatically prevent it. Then, all we have to do is parse the Postgres error to return a useful message to the user (we already have a func for that api.ParseDBError

FWIW Did add that constraint to my PR per @rawlinp feedback https://github.com/apache/trafficcontrol/pull/3964/files#diff-7d3547a278b38f437ab01966666c94a8R26

mitchell852 · 2019-10-09T15:45:52Z

i don't suppose you happened to add entries to seeds.sql for creating a "capability" (i'm talking about a user capability here) like the following:

server-capabilities-read (for the GET)
server-capabilities-write (for the POST/DELETE) - attached just to admin role?

seems a bit silly since we don't use "user capabilities" yet but i feel like it will make our lives easier (if/when roles/capabilities is every enabled) if every time we add new api endpoints we keep the "user capabilities" current.

rawlinp · 2019-10-09T16:11:05Z

@mitchell852 I didn't add seeds.sql entries yet. @mhoppa probably needs those for his API endpoints as well. Should POST/DELETE only be admin? Right now I have it set to Operations.

mitchell852 · 2019-10-09T16:56:37Z

Should POST/DELETE only be admin? Right now I have it set to Operations.

probably a question for @rob05c

asf-ci · 2019-10-09T18:07:50Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4435/

rob05c · 2019-10-09T22:53:22Z

Should POST/DELETE only be admin? Right now I have it set to Operations.

I think Ops is right. Admin vs Ops is somewhat nebulous, but I think in general, Operations can do pretty much everything except see password and security stuff. So I think Operations is right for these endpoints.

mitchell852 · 2019-10-10T14:00:39Z

Are there any restrictions on server capability name? I.e. no spaces, alphanumeric only, etc...

mhoppa · 2019-10-10T14:07:00Z

traffic_ops/client/servercapability.go

+
+// GetServerCapability returns the given server capability by name.
+func (to *Session) GetServerCapability(name string) (*tc.ServerCapability, ReqInf, error) {
+	url := fmt.Sprintf("%s?name=%s", APIServerCapabilities, name)


Non blocking as we dont do it anywhere but for here and below, on query parameters, we should use the net/url package to build them out so they are correctly url encoded.

Good call. I'll add that into this one.

rob05c · 2019-10-10T14:11:27Z

Are there any restrictions on server capability name? I.e. no spaces, alphanumeric only, etc...

The design reqs don't, I was originally thinking we'd let people put anything, but now that I think more about it, I think we should limit it to URI path chars, I.e. digit/alpha/_/-. That lets us put it in a path or query easier later, and I can't imagine why someone would need any other chars.

Someone might want, for example, Chinese characters for their names. And then, we could extend the feature to require/do punycode/urlencoding of high Unicode in paths. But that's adding a good bit of work to this feature, and it's much easier to allow more characters in the future without breaking people, than it is to remove things previously allowed.

Thoughts? If no one objects, I'll put that in the blueprint (but not hold up this PR for it, we can easily add that in another)

mitchell852 · 2019-10-10T14:13:38Z

Just a thought. What about a GET response like:

{
  "response": [
    {
      "name": "bar",
      "lastUpdated": "2019-10-07 20:38:24+00",
      "servers: [],
      "deliveryServices: []
    }
  ]
}

We do something similar with GET /parameters where we have a []profiles property. It gives me the ability in the UI to show a "server count" and a "delivery service count" which seems like interesting info to the user. But not a big deal if not included. I can get that info calling the join tables directly. It just makes the UI a bit less friendly.

rawlinp · 2019-10-10T16:41:07Z

+1 on restricting the name to alphanumeric/-/_.

Could the "server count" and "ds count" information be answered by just including links to pages which are backed by the server_server_capabilities API? E.g. click into the RAM capability, then click the "show servers" link to see all the servers with this capability or "show DSes" to see all the DSes with this capability.

mitchell852 · 2019-10-10T16:47:27Z

Could the "server count" and "ds count" information be answered by just including links to pages which are backed by the server_server_capabilities API?

yep. i think that's fine.

lbathina · 2019-10-11T03:22:14Z

 "alerts": [
     {
         "text": "no server capability with that id found",
         "level": "error"
     }
 ]

}``` message here needs to be corrected. we are deleting server_capabilities only by name.
2. when spaces are at the start and end of name, they are trimmed and accepted instead of throwing error.
3. api parameters are not validated. for instance, requesting service_capabilities endpoint with param id doesn't throw any error.
4. on PUT which is not defined - may be we should do method not allowed?
5. When a post with no body is made. the following error is provided.

    "alerts": [
        {
            "text": "EOF",
            "level": "error"
        }
    ]
}```
 instead of a meaningful message indicating the missing info.

rob05c · 2019-10-11T03:49:17Z

message here needs to be corrected. we are deleting server_capabilities only by name.

Yep, looks like an artifact of api.GenericDelete. Good catch!

Looks like either api.GenericDelete will have to be changed to something like "no x with that key found," or otherwise extended to know the names of the key(s).

when spaces are at the start and end of name, they are trimmed and accepted instead of throwing error.

Trimming whitespace should be fine. It should be documented, but the extra safety seems like a good thing to me.

api parameters are not validated. for instance, requesting service_capabilities endpoint with param id doesn't throw any error.

The API shouldn't do this. Unknown parameters should be ignored. This follows the Robustness Principle. See
https://tools.ietf.org/html/rfc1122#section-1.2.2
https://en.wikipedia.org/wiki/Robustness_principle

This makes our API easier to use, and less likely to break users, and more forward-compatible. For example, someone could be using a client with a newer version, or newer code within the same version, but knowingly double-checking their filtering params (atstccfg does this); or posting some kind of debug information that helps themselves; or simply making a simple mistake that doesn't need to break things.

I know some other APIs do this. It is a gross violation of the Robustness Principle, and I strongly disapprove. It makes servers brittle and fragile, and complicates integration and forward-compatibility.

mitchell852 · 2019-10-11T15:17:45Z

Trimming whitespace should be fine. It should be documented, but the extra safety seems like a good thing to me.

agree

mitchell852 · 2019-10-11T15:18:04Z

Unknown parameters should be ignored.

unknown query params that is. agree. plus this is how our api has always behaved.

rawlinp · 2019-10-11T15:33:14Z

 "alerts": [
     {
         "text": "no server capability with that id found",
         "level": "error"
     }
 ]
}``` message here needs to be corrected. we are deleting server_capabilities only by name.

Yeah, I noticed that as well and just kind of figured "name" is really the "id" of the resource, so I just left it as it was. I'll change the GenericDelete function to use "key" instead of "id".

when spaces are at the start and end of name, they are trimmed and accepted instead of throwing error.

I'm not intentionally trimming the name, but about to submit a fix that validates the name is alphanumeric/dash/underscore.

api parameters are not validated. for instance, requesting service_capabilities endpoint with param id doesn't throw any error.

I also agree unknown API parameters should be ignored.

on PUT which is not defined - may be we should do method not allowed?

This seems like a problem with our entire API rather than just this PR. We should fix that holistically and separately IMO.

When a post with no body is made. the following error is provided.

    "alerts": [
        {
            "text": "EOF",
            "level": "error"
        }
    ]
}```
 instead of a meaningful message indicating the missing info.

I suspect that is also a larger problem with the majority of our API, since we share the common code for unmarshalling JSON and returning errors across multiple endpoints. We should fix that separately.

asf-ci · 2019-10-11T16:31:08Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4455/

asf-ci · 2019-10-11T16:40:16Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4458/

mitchell852 · 2019-10-11T19:32:35Z

When a post with no body is made. the following error is provided.

    "alerts": [
        {
            "text": "EOF",
            "level": "error"
        }
    ]
}```
 instead of a meaningful message indicating the missing info.

what to create a separate issue for that @lbathina as it sounds like this is a problem with all our POSTs

mitchell852

Everything seems to work as I'd expect.

Rawlin Peters added 4 commits October 4, 2019 13:18

Add server_capability goose migration

46eb2b5

Add server_capabilities API endpoints

23ff788

Add server capabilities methods to TO-Go client

845b9d9

Add TO API tests for server capabilities

c46a7f0

rawlinp added new feature A new feature, capability or behavior Traffic Ops related to Traffic Ops labels Oct 8, 2019

Add CHANGELOG entry for server_capabilities

021fa66

rob05c self-assigned this Oct 8, 2019

Fix godoc comments on server_capabilities structs

bfcf82f

awilliams reviewed Oct 8, 2019

View reviewed changes

traffic_ops/traffic_ops_golang/servercapability/servercapability.go Outdated Show resolved Hide resolved

traffic_ops/traffic_ops_golang/servercapability/servercapability.go Outdated Show resolved Hide resolved

traffic_ops/client/servercapability.go Outdated Show resolved Hide resolved

Addressed review comments

83862fc

mhoppa approved these changes Oct 8, 2019

View reviewed changes

awilliams mentioned this pull request Oct 8, 2019

Add GET/POST/DELETE for server_server_capabilities #3964

Merged

7 tasks

rob05c approved these changes Oct 8, 2019

View reviewed changes

rawlinp mentioned this pull request Oct 8, 2019

Add server_capabilities API docs #3971

Merged

7 tasks

Address PR review comments

e6abc19

mhoppa reviewed Oct 10, 2019

View reviewed changes

Address more PR feedback

efcaab2

Fix godoc comment before someone yells at me

13625ca

mitchell852 mentioned this pull request Oct 11, 2019

CLOSED - TP: Adds Server Capabilities UI #3977

Closed

7 tasks

mitchell852 approved these changes Oct 11, 2019

View reviewed changes

mitchell852 merged commit fda6675 into apache:master Oct 14, 2019

rawlinp deleted the add-server-capabilities branch October 14, 2019 16:10

mitchell852 mentioned this pull request Oct 21, 2019

implement deliveryservice_server_capabilities api endpoint #4006

Closed

7 tasks

Add server capabilities API #3966

Add server capabilities API #3966

Conversation

rawlinp commented Oct 8, 2019 • edited

What does this PR (Pull Request) do?

Which Traffic Control components are affected by this PR?

What is the best way to verify this PR?

The following criteria are ALL met by this PR

asf-ci commented Oct 8, 2019

asf-ci commented Oct 8, 2019

asf-ci commented Oct 8, 2019

ocket8888 commented Oct 8, 2019

rawlinp commented Oct 8, 2019

asf-ci commented Oct 8, 2019

mhoppa left a comment

Choose a reason for hiding this comment

mitchell852 commented Oct 8, 2019 • edited

rawlinp commented Oct 8, 2019

mitchell852 commented Oct 8, 2019

rawlinp commented Oct 8, 2019

rob05c left a comment

Choose a reason for hiding this comment

rawlinp commented Oct 8, 2019

mitchell852 commented Oct 9, 2019

rob05c commented Oct 9, 2019

mhoppa commented Oct 9, 2019

mitchell852 commented Oct 9, 2019 • edited

rawlinp commented Oct 9, 2019

mitchell852 commented Oct 9, 2019

asf-ci commented Oct 9, 2019

rob05c commented Oct 9, 2019

mitchell852 commented Oct 10, 2019

mhoppa Oct 10, 2019

Choose a reason for hiding this comment

rawlinp Oct 10, 2019

Choose a reason for hiding this comment

rob05c commented Oct 10, 2019 • edited

mitchell852 commented Oct 10, 2019 • edited

rawlinp commented Oct 10, 2019

mitchell852 commented Oct 10, 2019

lbathina commented Oct 11, 2019 • edited

rob05c commented Oct 11, 2019 • edited

mitchell852 commented Oct 11, 2019

mitchell852 commented Oct 11, 2019 • edited

rawlinp commented Oct 11, 2019

asf-ci commented Oct 11, 2019

asf-ci commented Oct 11, 2019

mitchell852 commented Oct 11, 2019 • edited

mitchell852 left a comment

Choose a reason for hiding this comment

rawlinp commented Oct 8, 2019 •

edited

mitchell852 commented Oct 8, 2019 •

edited

mitchell852 commented Oct 9, 2019 •

edited

rob05c commented Oct 10, 2019 •

edited

mitchell852 commented Oct 10, 2019 •

edited

lbathina commented Oct 11, 2019 •

edited

rob05c commented Oct 11, 2019 •

edited

mitchell852 commented Oct 11, 2019 •

edited

mitchell852 commented Oct 11, 2019 •

edited