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

Rewrite /capabilities to Go #3870

Open
wants to merge 18 commits into
base: master
from

Conversation

@ocket8888
Copy link
Contributor

ocket8888 commented Aug 14, 2019

What does this PR (Pull Request) do?

Rewrites the /capabilities endpoint to Go. Also adds handlers for the PUT and DELETE request methods.

Which Traffic Control components are affected by this PR?

  • Documentation
  • Traffic Control Client (Python and Go)
  • Traffic Ops
  • Traffic Portal

What is the best way to verify this PR?

Run the associated tests in traffic_ops/testing/api/v14.

The following criteria are ALL met by this PR

  • This PR includes tests
  • This PR includes documentation
  • 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 does not include a database migration
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY
@asfgit

This comment has been minimized.

Copy link

asfgit commented Aug 14, 2019

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

@rawlinp

This comment has been minimized.

Copy link
Contributor

rawlinp commented Aug 14, 2019

So, I have a number of questions about capabilities:

  1. what is the difference between a capability and an api_capability?
  2. do we need both?
  3. do we really need the ability to CRUD capabilities in general? It seems like capabilities are statically defined in terms of API endpoints/"things you can do". Why do we need the ability to dynamically CRUD those things?

It seems like "roles" are what really actually to be dynamic -- not capabilities. If you have a user you want to give a limited set of capabilities, you create a new role, add in the capabilities you want to give them, and assign the new role to the user.

It doesn't make sense to create a new capability via the API or update or delete existing capabilities. IMO it really only makes sense to be able to read the statically defined capabilities, so that you have the full set to choose from.

@ocket8888

This comment has been minimized.

Copy link
Contributor Author

ocket8888 commented Aug 15, 2019

  1. api_capability links an API route to a particular capability.
  2. Yes and no. We need both concepts, but the routes governed by a capability could probably be dealt with under the /capabilities route itself.
  3. new capabilities can exist because arbitrary endpoints can be added via extensions. You can also group api routes under a one or more capabilities in arbitrary ways. Not sure that second one is a good reason, but it is a true one.

In any case, such deprecations probably need to go to the mailing list for discussion. I don't disagree that this could be handled better - possibly by requiring extensions to define a governing capability? - but I do want to just get the route into Go, since deprecation takes a full major release cycle anyway.

@asfgit

This comment has been minimized.

Copy link

asfgit commented Aug 15, 2019

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

@asfgit

This comment has been minimized.

Copy link

asfgit commented Aug 15, 2019

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

@rawlinp

This comment has been minimized.

Copy link
Contributor

rawlinp commented Aug 15, 2019

Ah, I see now that capability.name is used as a FK in the api_capabilities table. If you update the name of a capability does it automatically update all the FK references in the api_capabilities table? That was one concern I had about name-based keys in general. If it doesn't auto-update, then it seems like we should make capability.name an immutable column.

Good point about the arbitrary endpoints via extensions. I don't really like the idea of making all the built-in capabilities mutable though. It has all the same issues as making built-in types mutable today. Maybe we need the concept of extension-specific capabilities, and if extensions want to take advantage of roles/capabilities, they should load their extension-specific capabilities into the DB at startup time. Do extensions have access to TODB? I can't tell from looking through the traffic_ops_golang/plugin dir. Either way, it seems like we wouldn't want the ability to create/update/delete any kind of capabilities dynamically -- only the ability to list current capabilities that we could assign to a custom role.

@ocket8888 ocket8888 force-pushed the ocket8888:capabilities-go branch from 8b3b6b8 to 46a5fec Aug 15, 2019
@rob05c

This comment has been minimized.

Copy link
Member

rob05c commented Aug 15, 2019

If you update the name of a capability does it automatically update all the FK references in the api_capabilities table?

No, but it should.

https://github.com/apache/trafficcontrol/blob/master/traffic_ops/app/db/create_tables.sql#L3243

We should add a migration to change fk_capability to ON UPDATE CASCADE.

@asfgit

This comment has been minimized.

Copy link

asfgit commented Aug 15, 2019

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

@ocket8888

This comment has been minimized.

Copy link
Contributor Author

ocket8888 commented Aug 15, 2019

"If you update the name of a capability does it automatically update all the FK references in the api_capabilities table?"

It can, depends if the foreign key constraint was defined with ON UPDATE CASCADE. Which it should have been. But looking at traffic_ops/app/db/create_tables.sql it was not. Adding that clause is simple enough - and should be done, IMO -, but it's a database migration, so I'm not sure it's proper to include it in what's already a pretty expansive PR.

Extensions certainly have access to the database at runtime, but idk if they do at startup.

@ocket8888

This comment has been minimized.

Copy link
Contributor Author

ocket8888 commented Aug 15, 2019

Oh, rob05c beat me to it. But also of note while we're looking, the Role/Capability relationship has the same problem.

@rob05c

This comment has been minimized.

Copy link
Member

rob05c commented Aug 15, 2019

Do extensions have access to TODB?

Yes. TO Plugins can access anything the main TO app can. If a hook can't access something that's needed, it can be added to the Data struct for that hook, e.g. OnRequestData.

idk if they do at startup.

Same goes for startup, needed data (like the db or tx) can be added to the StartupData struct.

@rawlinp

This comment has been minimized.

Copy link
Contributor

rawlinp commented Aug 15, 2019

Well, if we make the built-in capabilities immutable the ON UPDATE CASCADE is unnecessary. Since TO plugins can access the TODB at startup, I think that would be the "proper" way of supporting capabilities for arbitrary extension routes. An extension route shouldn't require an operator to manually add its capabilities via the API. The extension should load them itself on startup time, and they should be immutable from an API perspective.

Since capabilities aren't really used in the code yet, I think we have some freedom to design them properly before we get stuck in the rut of having to support unnecessary API endpoints.

@ocket8888

This comment has been minimized.

Copy link
Contributor Author

ocket8888 commented Aug 15, 2019

The best we could do in that direction is return an error without breaking the "API version promise". And actually, that might qualify; not sure.

@asfgit

This comment has been minimized.

Copy link

asfgit commented Aug 15, 2019

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

@ocket8888 ocket8888 marked this pull request as ready for review Aug 15, 2019
@asfgit

This comment has been minimized.

Copy link

asfgit commented Aug 15, 2019

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

@asfgit

This comment has been minimized.

Copy link

asfgit commented Aug 15, 2019

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

@ocket8888 ocket8888 force-pushed the ocket8888:capabilities-go branch from d464a1b to f0d013a Aug 19, 2019
@asfgit

This comment has been minimized.

Copy link

asfgit commented Aug 19, 2019

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

@ocket8888 ocket8888 referenced this pull request Aug 19, 2019
6 of 6 tasks complete
@ocket8888

This comment has been minimized.

Copy link
Contributor Author

ocket8888 commented Oct 14, 2019

retest this please

@asf-ci

This comment has been minimized.

Copy link

asf-ci commented Oct 14, 2019

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

@ocket8888

This comment has been minimized.

Copy link
Contributor Author

ocket8888 commented Oct 14, 2019

retest this please

@asf-ci

This comment has been minimized.

Copy link

asf-ci commented Oct 14, 2019

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

@ocket8888 ocket8888 force-pushed the ocket8888:capabilities-go branch from 994bef6 to 0cef8c7 Oct 14, 2019
@asf-ci

This comment has been minimized.

Copy link

asf-ci commented Oct 14, 2019

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

@ocket8888 ocket8888 force-pushed the ocket8888:capabilities-go branch from 0cef8c7 to f19bf48 Oct 15, 2019
@asf-ci

This comment has been minimized.

Copy link

asf-ci commented Oct 15, 2019

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

@ocket8888

This comment has been minimized.

Copy link
Contributor Author

ocket8888 commented Oct 15, 2019

retest this please

@asf-ci

This comment has been minimized.

Copy link

asf-ci commented Oct 15, 2019

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

@ocket8888 ocket8888 force-pushed the ocket8888:capabilities-go branch from f19bf48 to 1721157 Oct 21, 2019
@asf-ci

This comment has been minimized.

Copy link

asf-ci commented Oct 21, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.