Fix TO Go DSS GET endpoints to check tenancy#2979
Conversation
rob05c
added
bug
|
I'm labelling this |
|
Refer to this link for build results (access rights to CI server needed): |
|
This was fixed in #3081 - closing. |
|
Ah, no it wasn't, my mistake. That PR fixed one endpoint, but not the other. |
rob05c
force-pushed
the
to-go-dss-read-tenancy
branch
from
d3ab377 to
6053db1
Compare
|
Refer to this link for build results (access rights to CI server needed): |
|
@ocket8888 is going to review this PR |
ocket8888
left a comment
ocket8888
left a comment
There was a problem hiding this comment.
Seems to work perfectly
| } | ||
|
|
||
| query, err := selectQuery(orderby, strconv.Itoa(limit), strconv.Itoa(offset)) | ||
| tenancyEnabled, err := tenant.IsTenancyEnabledTx(tx.Tx) |
There was a problem hiding this comment.
late to the party, but tenancy is always enabled by default now so this check is unnecessary
There was a problem hiding this comment.
But can it be turned off?
There was a problem hiding this comment.
I think we decided to do that, but I don't think we actually did. The code still checks use_tenancy everywhere, and I think it's considered false if it doesn't exist.
I think this because the API Tests until recently broke horribly if you created that Parameter, suggesting Tenancy is disabled by default.
Also, I'm not opposed to making Tenancy "always on," but I do kind of think we should enable Capabilities at the same time (#2791). Tenancy isn't really useful without Capabilities to limit what users can do, and it could be deceptive, a CDN operator might think tenancy limits an admin/ops user, when it doesn't.
There was a problem hiding this comment.
This has been my understanding, that tenancy is enabled by default, but still optional - though I have heard that the decision to make it not optional has been made (and not actually done yet).
There was a problem hiding this comment.
It can be "turned off" by setting the parameter to false or deleting the parameter, but it is not really "turned off" at that point because we already have various places in the code that assume tenancy is always enabled (so they don't check the parameter). So "turning it off" doesn't completely turn it off, which is why we should really be preventing the deletion/update of the use_tenancy parameter to "turn off tenancy", since that really just leaves the system in an inconsistent state. Some things will check the tenancy parameter, whereas others already assume tenancy is always enabled.
We attempted to delete the use_tenancy parameter from the system but realized that there is too much existing code that depends on that parameter being there. The plan was to "fix" the existing code that checks the parameter by removing the parameter check and just assume that tenancy is always enabled. This was all discussed on the mailing list already; the decision to have it always enabled has already been made. Steps have already been made to enable it by default, but the existing code that checks the use_tenancy parameter has not been cleaned up yet.
tldr; tenancy should always be considered enabled, and existing code that checks the use_tenancy parameter needs to be fixed. Once all existing tenancy checks have been "fixed", the parameter can be deleted.
There was a problem hiding this comment.
I definitely agree that should all be done, and the sooner the better. But until the use_tenancy parameter can actually be removed so that tenancy is a hard requirement of ATC it just seems safer to keep checking.
There was a problem hiding this comment.
But until the
use_tenancyparameter can actually be removed so that tenancy is a hard requirement of ATC it just seems safer to keep checking.
Then we should do our best to remove all the usages of the use_tenancy parameter from the codebase. I think the usage is fairly self-contained within helper functions, so maybe we can just change the helper functions to just return true. Then we can remove all the usages of the now-useless helper functions to clean up the code.
5 participants
What does this PR do?
Fixes #2978
Which TC components are affected by this PR?
What is the best way to verify this PR?
With a Traffic Ops system and database set up with many delivery services, servers, and deliveryservice-server assignments:
Change your user's tenant to a tenant with fewer delivery services assigned, query the DSS read endpoints (
deliveryservices/{xml_id}/serversand/deliveryserviceserver, verify only the DSes assigned to your user are returned.Create a new tenant, with no delivery services assigned, change your user to that tenant, verify no DSes are returned.
Change your user to the root tenant, verify all delivery service server mappings are returned.
Check all that apply