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/10/02 18:04:29 UTC
[GitHub] [trafficcontrol] ocket8888 opened a new pull request #5094: Fix creating a ds with non-existent topology causing ISE
ocket8888 opened a new pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094
## What does this PR (Pull Request) do?
- [x] This PR fixes #4738
Fixes an issue where creating a Delivery Service with a non-existent Topology caused an Internal Server Error. It now instead returns a 409 Conflict response with an error stating that the Topology does not exist.
## Which Traffic Control components are affected by this PR?
- Traffic Ops
No docs changes because it's just a bug fix.
## What is the best way to verify this PR?
Try to create a Delivery Service with a non-existent Topology - easiest is probably just an empty string: `""` - and observe that the returned error is in the client error range with an appropriate alert.
## If this is a bug fix, what versions of Traffic Control are affected?
- master
## The following criteria are ALL met by this PR
- [x] This PR includes tests OR I have explained why tests are unnecessary
- [x] This PR includes documentation OR I have explained why documentation is unnecessary
- [x] This PR includes an update to CHANGELOG.md OR such an update is not necessary
- [x] This PR includes any and all required license headers
- [x] This PR ensures that database migration sequence is correct OR this PR does not include a database migration
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY**
----------------------------------------------------------------
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] rawlinp merged pull request #5094: Fix creating a ds with non-existent topology causing ISE
Posted by GitBox <gi...@apache.org>.
rawlinp merged pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094
----------------------------------------------------------------
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 a change in pull request #5094: Fix creating a ds with non-existent topology causing ISE
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094#discussion_r500595716
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -266,6 +266,14 @@ func createV30(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds tc.D
deepCachingType = ds.DeepCachingType.String() // necessary, because DeepCachingType's default needs to insert the string, not "", and Query doesn't call .String().
}
+ if ds.Topology != nil {
Review comment:
The Validate chain doesn't allow reporting system errors, so that first error check would be swallowed.
----------------------------------------------------------------
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] rawlinp commented on a change in pull request #5094: Fix creating a ds with non-existent topology causing ISE
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094#discussion_r500604334
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -266,6 +266,14 @@ func createV30(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds tc.D
deepCachingType = ds.DeepCachingType.String() // necessary, because DeepCachingType's default needs to insert the string, not "", and Query doesn't call .String().
}
+ if ds.Topology != nil {
Review comment:
Well, well, well... should we add a `// TODO: ...` for that? And in the meantime, can we refactor the check into a shared function that is used in create() as well as update()?
----------------------------------------------------------------
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 pull request #5094: Fix creating a ds with non-existent topology causing ISE
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094#issuecomment-704437484
> PUT already had a test for updating with an invalid topology - but it didn't check the status code. Now it does.
Is there a commit you meant to push for 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 a change in pull request #5094: Fix creating a ds with non-existent topology causing ISE
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094#discussion_r499916218
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -266,6 +266,14 @@ func createV30(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds tc.D
deepCachingType = ds.DeepCachingType.String() // necessary, because DeepCachingType's default needs to insert the string, not "", and Query doesn't call .String().
}
+ if ds.Topology != nil {
+ if ok, err := dbhelpers.TopologyExistsString(tx, *ds.Topology); err != nil {
+ return nil, http.StatusInternalServerError, nil, fmt.Errorf("checking topology existence: %v", err)
+ } else if !ok {
+ return nil, http.StatusConflict, fmt.Errorf("no such Topology '%s'", *ds.Topology), nil
Review comment:
400 seems solid
----------------------------------------------------------------
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 a change in pull request #5094: Fix creating a ds with non-existent topology causing ISE
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094#discussion_r499915105
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -266,6 +266,14 @@ func createV30(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds tc.D
deepCachingType = ds.DeepCachingType.String() // necessary, because DeepCachingType's default needs to insert the string, not "", and Query doesn't call .String().
}
+ if ds.Topology != nil {
+ if ok, err := dbhelpers.TopologyExistsString(tx, *ds.Topology); err != nil {
+ return nil, http.StatusInternalServerError, nil, fmt.Errorf("checking topology existence: %v", err)
+ } else if !ok {
+ return nil, http.StatusConflict, fmt.Errorf("no such Topology '%s'", *ds.Topology), nil
Review comment:
The example with the PUT request is just that - an example. Conditional requests aren't the only use allowed for `409 Conflict` - however, [RFC7231](https://tools.ietf.org/html/rfc7231#section-6.5.8) (which obsoletes 2616) *does* state that it should be used when the conflicted state is that of the "*target* resource", which makes it feel inappropriate. The wording on [MDN's `409 Conflict` page](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/409) is a bit more permissive:
> "The HTTP `409 Conflict` response status code indicates a request conflict with current state of the server."
which is very different that the state of the target resource. That's where my definition of 409 was coming from, but it seems inaccurate.
So, what, 400 then?
----------------------------------------------------------------
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 a change in pull request #5094: Fix creating a ds with non-existent topology causing ISE
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094#discussion_r499314091
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -266,6 +266,14 @@ func createV30(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds tc.D
deepCachingType = ds.DeepCachingType.String() // necessary, because DeepCachingType's default needs to insert the string, not "", and Query doesn't call .String().
}
+ if ds.Topology != nil {
+ if ok, err := dbhelpers.TopologyExistsString(tx, *ds.Topology); err != nil {
+ return nil, http.StatusInternalServerError, nil, fmt.Errorf("checking topology existence: %v", err)
+ } else if !ok {
+ return nil, http.StatusConflict, fmt.Errorf("no such Topology '%s'", *ds.Topology), nil
Review comment:
409 Status Conflict doesn't really fit here since the error does not necessarily stem from an issue relating to state.
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -812,6 +812,14 @@ func GetCacheGroupNameFromID(tx *sql.Tx, id int) (tc.CacheGroupName, bool, error
return tc.CacheGroupName(name), true, nil
}
+// TopologyExistsString is a convenience function for checking if a Topology exists
+// when its name is an actual string instead of a github.com/apache/trafficcontrol/lib/go-tc.TopologyName
+func TopologyExistsString(tx *sql.Tx, name string) (bool, error) {
+ return TopologyExists(tx, tc.TopologyName(name))
+}
Review comment:
I would rather `TopologyExists()` take a string than add an additional `TopologyExistsString()`. `TopologyExists()` was only used 1 place before this PR, so adding a typecast to `string` in the calling function should be pretty easy
----------------------------------------------------------------
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 a change in pull request #5094: Fix creating a ds with non-existent topology causing ISE
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094#discussion_r499910607
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -266,6 +266,14 @@ func createV30(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds tc.D
deepCachingType = ds.DeepCachingType.String() // necessary, because DeepCachingType's default needs to insert the string, not "", and Query doesn't call .String().
}
+ if ds.Topology != nil {
+ if ok, err := dbhelpers.TopologyExistsString(tx, *ds.Topology); err != nil {
+ return nil, http.StatusInternalServerError, nil, fmt.Errorf("checking topology existence: %v", err)
+ } else if !ok {
+ return nil, http.StatusConflict, fmt.Errorf("no such Topology '%s'", *ds.Topology), nil
Review comment:
Where is the conflict? [From RFC 2616](https://tools.ietf.org/html/rfc2616#section-10.4.10):
> The request could not be completed due to a conflict with the current state of the resource.
Also:
> Conflicts are most likely to occur in response to a PUT request. For example, if versioning were being used and the entity being PUT included changes to a resource which conflict with those made by an earlier (third-party) request, the server might use the 409 response to indicate that it can't complete the request. In this case, the response entity would likely contain a list of the differences between the two versions in a format defined by the response Content-Type.
---
> In this case the request payload would be considered acceptable if the state of the server were to change but nothing about the request itself changes - _or_ if the request changes to be acceptable given the server's state.
We have no reason to believe that any such topology ever did exist in the past, so a case in which the server happens to create a topology that is named just the right thing for our request to succeed is improbable and cannot be chalked up to state.
----------------------------------------------------------------
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 pull request #5094: Fix creating a ds with non-existent topology causing ISE
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094#issuecomment-703814851
> This PR should also include an assertion that fails before this patch but succeeds with it.
Whoops, forgot to add a test. Done.
----------------------------------------------------------------
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 a change in pull request #5094: Fix creating a ds with non-existent topology causing ISE
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094#discussion_r499789732
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -812,6 +812,14 @@ func GetCacheGroupNameFromID(tx *sql.Tx, id int) (tc.CacheGroupName, bool, error
return tc.CacheGroupName(name), true, nil
}
+// TopologyExistsString is a convenience function for checking if a Topology exists
+// when its name is an actual string instead of a github.com/apache/trafficcontrol/lib/go-tc.TopologyName
+func TopologyExistsString(tx *sql.Tx, name string) (bool, error) {
+ return TopologyExists(tx, tc.TopologyName(name))
+}
Review comment:
I would prefer that too, but I didn't check the number of usages. I assumed it had a few more than it apparently does, and didn't want to bother with it. Thanks for checking that out for me.
----------------------------------------------------------------
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 edited a comment on pull request #5094: Fix creating a ds with non-existent topology causing ISE
Posted by GitBox <gi...@apache.org>.
zrhoffman edited a comment on pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094#issuecomment-704462516
453d5ca083 changes a test for a client steering type delivery service which cannot have topologies assigned to it anyway, so PUT still needs to be covered in tests.
```json
{
"alerts": [
{
"text": "invalid request: type fields: 'topology' steering deliveryservice types cannot be assigned to a topology",
"level": "error"
}
]
}
```
----------------------------------------------------------------
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 pull request #5094: Fix creating a ds with non-existent topology causing ISE
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094#issuecomment-703948141
The test should check PUT too
----------------------------------------------------------------
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 pull request #5094: Fix creating a ds with non-existent topology causing ISE
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094#issuecomment-704359164
PUT already had a test for updating with an invalid topology - but it didn't check the status code. Now it does.
----------------------------------------------------------------
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] rawlinp commented on a change in pull request #5094: Fix creating a ds with non-existent topology causing ISE
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094#discussion_r500409138
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -266,6 +266,14 @@ func createV30(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds tc.D
deepCachingType = ds.DeepCachingType.String() // necessary, because DeepCachingType's default needs to insert the string, not "", and Query doesn't call .String().
}
+ if ds.Topology != nil {
Review comment:
These checks seem like they belong in the `Validate` chain rather than here.
----------------------------------------------------------------
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 a change in pull request #5094: Fix creating a ds with non-existent topology causing ISE
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094#discussion_r499791438
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -266,6 +266,14 @@ func createV30(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds tc.D
deepCachingType = ds.DeepCachingType.String() // necessary, because DeepCachingType's default needs to insert the string, not "", and Query doesn't call .String().
}
+ if ds.Topology != nil {
+ if ok, err := dbhelpers.TopologyExistsString(tx, *ds.Topology); err != nil {
+ return nil, http.StatusInternalServerError, nil, fmt.Errorf("checking topology existence: %v", err)
+ } else if !ok {
+ return nil, http.StatusConflict, fmt.Errorf("no such Topology '%s'", *ds.Topology), nil
Review comment:
Sure it does. The state of the database is such that the request is invalid - because no such topology exists. 404 would be inappropriate in this case IMO because that typically implies that the request URI itself was not found. In this case the request payload would be considered acceptable if the state of the server were to change but nothing about the request itself changes - _or_ if the request changes to be acceptable given the server's state.
----------------------------------------------------------------
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 pull request #5094: Fix creating a ds with non-existent topology causing ISE
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094#issuecomment-704462516
453d5ca083 changes a test for a client steering type delivery service which cannot have topologies assigned to it anyway. The bug does not occur on that delivery service type. So, PUT still needs to be covered in tests.
```json
{
"alerts": [
{
"text": "invalid request: type fields: 'topology' steering deliveryservice types cannot be assigned to a topology",
"level": "error"
}
]
}
```
----------------------------------------------------------------
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 a change in pull request #5094: Fix creating a ds with non-existent topology causing ISE
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5094:
URL: https://github.com/apache/trafficcontrol/pull/5094#discussion_r500609119
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -266,6 +266,14 @@ func createV30(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, ds tc.D
deepCachingType = ds.DeepCachingType.String() // necessary, because DeepCachingType's default needs to insert the string, not "", and Query doesn't call .String().
}
+ if ds.Topology != nil {
Review comment:
> ...in the meantime, can we refactor the check into a shared function that is used in create() as well as update()?
Yeah, for sure.
> ... should we add a // TODO: ... for that?
It'll mean changing the `github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api.ParseValidator` interface, but I think that'd be a worthwhile change. So: yes, and that's where I'll put it.
----------------------------------------------------------------
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