You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/08/05 21:25:07 UTC
[GitHub] [trafficcontrol] dpham692 opened a new issue #4935: Internal Server Error when perform a /GET update_status
dpham692 opened a new issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935
<!--
************ STOP!! ************
If this issue identifies a security vulnerability, DO NOT submit it! Instead, contact
the Apache Software Foundation Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://www.apache.org/security/ regarding vulnerability disclosure.
- For *SUPPORT QUESTIONS*, use the
[Traffic Control slack channels](https://traffic-control-cdn.slack.com) or [Traffic Control mailing lists](http://trafficcontrol.apache.org/mailing_lists/).
- Before submitting, please **SEARCH GITHUB** for a similar issue or PR. -->
## I'm submitting a ...
<!-- delete all those that don't apply -->
<!--- security vulnerability (STOP!! - see above)-->
- [x] bug report
- new feature / enhancement request
- improvement request (usability, performance, tech debt, etc.)
- other <!--(Please do not submit support requests here - see above)-->
## Traffic Control components affected ...
<!-- delete all those that don't apply -->
- CDN in a Box
- Documentation
- Grove
- Traffic Control Client
- Traffic Monitor
- [x] Traffic Ops
- Traffic Ops ORT
- [x] Traffic Portal
- Traffic Router
- Traffic Stats
- Traffic Vault
- unknown
## Current behavior:
<!-- Describe how the bug manifests / how the current features are insufficient. -->
1. Create 2 CacheGroup with Servers.
2. Create a Topology using those two cachegroup as parent and child relationship
3. Perform a /GET `/api/3.0/servers/hostName/update_status` with Server that in cachegroup child.
4. The API will return Internal Server Error.
```
{
"alerts": [
{
"text": "Internal Server Error",
"level": "error"
}
]
}
```
For example:
Do a GET
https://{{TO_BASE_URL}}/api/3.0/servers/testingServerSZUIO/update_status
## Expected / new behavior:
<!-- Describe what the behavior would be without the bug / how the feature would improve Traffic Control -->
It should return correct response like 200 with parent_pending = true
## Minimal reproduction of the problem with instructions:
<!--
If the current behavior is a bug or you can illustrate your feature request better with an example,
please provide the *STEPS TO REPRODUCE* and include the applicable TC version.
-->
## Anything else:
<!-- e.g. stacktraces, related issues, suggestions how to fix -->
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on issue #4935: Internal Server Error when perform a /GET update_status on a non-unique hostname
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935#issuecomment-669594171
@zrhoffman @rob05c - in this scenario, would it make sense to set upd_pend=1 for BOTH servers IF 2 servers (for example) share a hostname?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 closed issue #4935: Internal Server Error when perform a /GET update_status on a non-unique hostname
Posted by GitBox <gi...@apache.org>.
mitchell852 closed issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on issue #4935: Internal Server Error when perform a /GET update_status on a non-unique hostname
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935#issuecomment-669607245
> So, at one point hostname was unique and like @​rob05c said the uniqueness was changed to hostname + profile. I believe this was done to support MSO delivery services in some way but i forget exactly why.
@mitchell852 If the only reason we dropped unique hostnames was to support MSOs, origins should be the only server type that can have non-unique hostnames. We can adjust the constraint for that and re-add the constraint, right?
> in this scenario, would it make sense to set upd_pend=1 for BOTH servers IF 2 servers (for example) share a hostname?
Because we queue updates by server ID, that endpoint thankfully will not have this issue.
---
> But with regard to the parent pending, I'm not 100% sure but looking at the SQL, I think it's probably implicitly returning if parents of either server are pending. But that's not where the error is.
@rob05c That's correct. But since we have no preference for which of those 2 servers with that hostname we get, that is undefined behavior.
> The error is in `WHERE s.cachegroup != (SELECT s.cachegroup FROM server s WHERE s.host_name = $4)`, which I don't fully understand.
Our set of cachegroups initially contains the cachegroup we want to filter out as the base result of the query's recursive CTE. We could filter it out within the base result itself, but that would double the size of the base result SQL. From this discussion, it's probably still worth it.
> Changing it to `WHERE s.cachegroup NOT IN` would make it execute successfully, and might have that effect. @zrhoffman ?
Depends on what you mean by "successfully". If the unintended server's cachegroup parents the intended server's cachegroup in the topology and updates are queued on the unintended server's cachegroup, we would still get inaccurate results. But maybe that is an infrequent edge case. Without the constraint, I'm not confident that a server's hostname is enough info to produce a well-defined result.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c edited a comment on issue #4935: Internal Server Error when perform a /GET update_status
Posted by GitBox <gi...@apache.org>.
rob05c edited a comment on issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935#issuecomment-669571341
And maybe put reworking the hostname endpoints to include ports (and changing the DB constraint from host+profile to host+port) on the agenda for the TO/API meetings. Definitely an issue that doesn't get as much attention as it needs
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on issue #4935: Internal Server Error when perform a /GET update_status
Posted by GitBox <gi...@apache.org>.
rob05c commented on issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935#issuecomment-669546358
Ugh. No, the current DB constraint is `{hostname, profile}`. IMO it should be `{hostname, port}`, but that's a different discussion.
We have a ton of endpoints based on host, that only mostly work if you have multiple hosts. This is one of them. The "right" solution is to include the port/profile in the query, but the endpoint doesn't have that info.
We really need to rework all the hostname endpoints in the API (IDs doesn't help, because things like ORT only know their hostname in the first place).
But for now, IMO the best short-term answer is to just assume this host is unique, and make the query `fetch first 1 rows only`. And if it's not, and they differ, an arbitrary value succeeds. Just like every other endpoint like this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on issue #4935: Internal Server Error when perform a /GET update_status on a non-unique hostname
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935#issuecomment-669649681
#4936 returns accurate results for duplicate hostnames for `GET /servers/{host_name}/update_status` (as with previous API versions, the response array will have 2 elements in that case) . Also, using `NULL` as the initial cachegroup made filtering the initial cachegroups later unnecessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on issue #4935: Internal Server Error when perform a /GET update_status
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935#issuecomment-669553423
Ok, we can include a comment like
```sql
/* TODO: Remove "FETCH FIRST 1 ROW ONLY" once the endpoint is changed to take both a hostname and port parameter. */
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c edited a comment on issue #4935: Internal Server Error when perform a /GET update_status
Posted by GitBox <gi...@apache.org>.
rob05c edited a comment on issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935#issuecomment-669546358
>So hostnames don't need to be unique?
Ugh. No, the current DB constraint is `{hostname, profile}`. IMO it should be `{hostname, port}`, but that's a different discussion.
We have a ton of endpoints based on host, that only mostly work if you have multiple hosts. This is one of them. The "right" solution is to include the port/profile in the query, but the endpoint doesn't have that info.
We really need to rework all the hostname endpoints in the API (ID doesn't help, because things like ORT only know their hostname in the first place).
But for now, IMO the best short-term answer is to just assume this host is unique, and make the query `fetch first 1 rows only`. And if it's not, and they differ, an arbitrary value succeeds. Just like every other endpoint like this.
Maybe with a comment, to fix it when the endpoint is fixed to be host+port?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 edited a comment on issue #4935: Internal Server Error when perform a /GET update_status on a non-unique hostname
Posted by GitBox <gi...@apache.org>.
mitchell852 edited a comment on issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935#issuecomment-669594171
@zrhoffman @rob05c - in this scenario, would it make sense to set upd_pend=1 for BOTH servers IF 2 servers (for example) share a hostname? rather than simply picking the 1st one in the result....which might be the wrong one :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on issue #4935: Internal Server Error when perform a /GET update_status on a non-unique hostname
Posted by GitBox <gi...@apache.org>.
rob05c commented on issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935#issuecomment-669597373
This is getting, not setting. Returning the pending status for multiple servers would be a fundamental change to the API, and is likely to break clients.
But with regard to the parent pending, I'm not 100% sure but looking at the SQL, I think it's probably implicitly returning if parents of either server are pending. But that's not where the error is.
The error is in `WHERE s.cachegroup != (SELECT s.cachegroup FROM server s WHERE s.host_name = $4)`, which I don't fully understand. Changing it to `WHERE s.cachegroup NOT IN` would make it execute successfully, and might have that effect. @zrhoffman ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on issue #4935: Internal Server Error when perform a /GET update_status on a non-unique hostname
Posted by GitBox <gi...@apache.org>.
rob05c commented on issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935#issuecomment-669609664
>If the only reason we dropped unique hostnames was to support MSOs, origins should be the only server type that can have non-unique hostnames. We can adjust the constraint for that and re-add the constraint, right?
That's not the only reason. That might have been the triggering event, but there are many reasons to support multiple hostnames. It's valid to run a CDN, especialy a small one, with multiple components on the same machine. It's also extremely useful, especially for testing and development, to run multiple caches on the same host on different ports.
Fundamentally, hostname does not define a unique server on the internet, host+port does. In fact, it's really FQDN+port (that hasn't been a significant concern in the past, but we should consider including the domain in the key as well).
Supporting multiple services in TC on the same host is a very valuable feature we should improve, not remove.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c commented on issue #4935: Internal Server Error when perform a /GET update_status
Posted by GitBox <gi...@apache.org>.
rob05c commented on issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935#issuecomment-669571341
And maybe put reworking the hostname endpoints to include ports (and changing the DB constraint from host+profile to host+port) on the agenda for the TO/API meetings? Definitely an issue that doesn't get as much attention as it needs
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on issue #4935: Internal Server Error when perform a /GET update_status
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935#issuecomment-669592803
@dpham692 - can you change the title of this to `Internal Server Error when perform a /GET update_status on a non-unique hostname`
So, at one point hostname was unique and like @rob05c said the uniqueness was changed to hostname + profile. I believe this was done to support MSO delivery services in some way but i forget exactly why. (MSO is the root of much of our bad tech debt imo). I created a bug for this a long time ago - #1407
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on issue #4935: Internal Server Error when perform a /GET update_status on a non-unique hostname
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935#issuecomment-669629636
It should also be noted that the transformation is not trivial. This endpoint is only one of many places - including throughout all of Traffic Monitor and probably all of Traffic Router - where hostnames for servers are assumed to be unique.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c edited a comment on issue #4935: Internal Server Error when perform a /GET update_status
Posted by GitBox <gi...@apache.org>.
rob05c edited a comment on issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935#issuecomment-669546358
>So hostnames don't need to be unique?
Ugh. No, the current DB constraint is `{hostname, profile}`. IMO it should be `{hostname, port}`, but that's a different discussion.
We have a ton of endpoints based on host, that only mostly work if you have multiple hosts. This is one of them. The "right" solution is to include the port/profile in the query, but the endpoint doesn't have that info.
We really need to rework all the hostname endpoints in the API (ID doesn't help, because things like ORT only know their hostname in the first place).
But for now, IMO the best short-term answer is to just assume this host is unique, and make the query `fetch first 1 rows only`. And if it's not, and they differ, an arbitrary value succeeds. Just like every other endpoint like this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rob05c edited a comment on issue #4935: Internal Server Error when perform a /GET update_status
Posted by GitBox <gi...@apache.org>.
rob05c edited a comment on issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935#issuecomment-669546358
>So hostnames don't need to be unique?
Ugh. No, the current DB constraint is `{hostname, profile}`. IMO it should be `{hostname, port}`, but that's a different discussion.
We have a ton of endpoints based on host, that only mostly work if you have multiple hosts. This is one of them. The "right" solution is to include the port/profile in the query, but the endpoint doesn't have that info.
We really need to rework all the hostname endpoints in the API (IDs doesn't help, because things like ORT only know their hostname in the first place).
But for now, IMO the best short-term answer is to just assume this host is unique, and make the query `fetch first 1 rows only`. And if it's not, and they differ, an arbitrary value succeeds. Just like every other endpoint like this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on issue #4935: Internal Server Error when perform a /GET update_status
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on issue #4935:
URL: https://github.com/apache/trafficcontrol/issues/4935#issuecomment-669542938
Steps to reproduce: Create 2 servers with the same hostname and go to `/api/3.0/servers/{host_name}/update_status`.
Seeing this in the TO logs in CDN-in-a-Box:
```go
INFO: routing.go:249: 2020-08-05T21:54:13.543144535Z: GET /api/3.0/servers/edge/update_status? handling (reqid 39848)
DEBUG: plugin.go:190: 2020-08-05T21:54:13.543168115Z: DEBUG plugins.OnRequest calling 0 plugins
ERROR: servers_update_status.go:126: 2020-08-05T21:54:13.546664335Z: could not execute query: pq: more than one row returned by a subquery used as an expression
ERROR: api.go:201: 2020-08-05T21:54:13.546794479Z: 172.16.239.11:59424 database access error
DEBUG: api.go:206: 2020-08-05T21:54:13.54682155Z: Internal Server Error
```
[traffic_ops/traffic_ops_golang/server/servers_update_status.go#L81](https://github.com/apache/trafficcontrol/blob/732c3f8e1c/traffic_ops/traffic_ops_golang/server/servers_update_status.go#L81), the line with the subquery in question:
```sql
WHERE s.cachegroup != (SELECT s.cachegroup FROM server s WHERE s.host_name = $4)
```
So hostnames don't need to be unique?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org