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 2021/06/24 17:13:50 UTC
[GitHub] [trafficcontrol] srijeet0406 opened a new pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
srijeet0406 opened a new pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974
<!--
************ STOP!! ************
If this Pull Request is intended to fix 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.
-->
## What does this PR (Pull Request) do?
<!-- Explain the changes you made here. If this fixes an Issue, identify it by
replacing the text in the checkbox item with the Issue number e.g.
- [x] This PR fixes #9001 OR is not related to any Issue
^ This will automatically close Issue number 9001 when the Pull Request is
merged (The '#' is important).
Be sure you check the box properly, see the "The following criteria are ALL
met by this PR" section for details.
-->
- [x] This PR is not related to any Issue <!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this
Pull Request. Also, feel free to add the name of a tool or script that is
affected but not on the list.
Additionally, if this Pull Request does NOT affect documentation, please
explain why documentation is not required. -->
- Traffic Ops
- CI tests
## What is the best way to verify this PR?
<!-- Please include here ALL the steps necessary to test your Pull Request. If
it includes tests (and most should), outline here the steps needed to run the
tests. If not, lay out the manual testing procedure and please explain why
tests are unnecessary for this Pull Request. -->
Make sure all the unit tests and api tests pass.
Manual testing:
Run TO and TP locally.
Grab a hard lock for any cdn for a particular user.
Now, login to TP as another user, and try to modify the CDN, or CDN related components. These operations should be forbidden.
Log out and log back in to TP as the first user and try the same operations. This time they should all work correctly.
## If this is a bug fix, what versions of Traffic Control are affected?
<!-- If this PR fixes a bug, please list here all of the affected versions - to
the best of your knowledge. It's also pretty helpful to include a commit hash
of where 'master' is at the time this PR is opened (if it affects master),
because what 'master' means will change over time. For example, if this PR
fixes a bug that's present in master (at commit hash '1df853c8'), in v4.0.0,
and in the current 4.0.1 Release candidate (e.g. RC1), then this list would
look like:
- master (1df853c8)
- 4.0.0
- 4.0.1 (RC1)
If you don't know what other versions might have this bug, AND don't know how
to find the commit hash of 'master', then feel free to leave this section
blank (or, preferably, delete it entirely).
-->
- master
## The following criteria are ALL met by this PR
<!-- Check the boxes to signify that the associated statement is true. To
"check a box", replace the space inside of the square brackets with an 'x'.
e.g.
- [ x] <- Wrong
- [x ] <- Wrong
- [] <- Wrong
- [*] <- Wrong
- [x] <- Correct!
-->
- [x] This PR includes tests
- [x] This PR does not include documentation
- [x] This PR does not include an update to CHANGELOG.md
- [x] This PR includes any and all required license headers
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
## Additional Information
<!-- If you would like to include any additional information on the PR for
potential reviewers please put it here.
Some examples of this would be:
- Before and after screenshots/gifs of the Traffic Portal if it is affected
- Links to other dependent Pull Requests
- References to relevant context (e.g. new/updates to dependent libraries,
mailing list records, blueprints)
Feel free to leave this section blank (or, preferably, delete it entirely).
-->
<!--
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] rawlinp commented on a change in pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#discussion_r665805796
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -242,52 +289,59 @@ func CheckIfCurrentUserCanModifyCDNWithID(tx *sql.Tx, cdnID int64, user string)
} else if !ok {
return errors.New("CDN not found"), nil, http.StatusNotFound
}
- query := `SELECT username, soft FROM cdn_lock WHERE cdn=$1`
+ return CheckIfCurrentUserCanModifyCDN(tx, string(cdnName), user)
+}
+
+// CheckIfCurrentUserCanModifyCachegroup checks if the current user has the lock on the cdns that are associated with the provided cachegroup ID.
+// This will succeed if no other user has a hard lock on any of the CDNs that relate to the cachegroup in question.
+func CheckIfCurrentUserCanModifyCachegroup(tx *sql.Tx, cachegroupID int, user string) (error, error, int) {
+ query := `SELECT username, cdn, soft FROM cdn_lock WHERE cdn IN (SELECT name FROM cdn WHERE id IN (SELECT cdn_id FROM server WHERE cachegroup = ($1)))`
var userName string
+ var cdn string
var soft bool
- rows, err := tx.Query(query, string(cdnName))
+ rows, err := tx.Query(query, cachegroupID)
if err != nil {
if errors.Is(err, sql.ErrNoRows) {
return nil, nil, http.StatusOK
}
- return nil, errors.New("querying cdn_lock for user " + user + " and cdn " + string(cdnName) + ": " + err.Error()), http.StatusInternalServerError
+ return nil, errors.New("querying cdn_lock for user " + user + " and cachegroup ID " + strconv.Itoa(cachegroupID) + ": " + err.Error()), http.StatusInternalServerError
}
defer rows.Close()
for rows.Next() {
- err = rows.Scan(&userName, &soft)
+ err = rows.Scan(&userName, &cdn, &soft)
if err != nil {
- return nil, errors.New("scanning cdn_lock for user " + user + " and cdn " + string(cdnName) + ": " + err.Error()), http.StatusInternalServerError
+ return nil, errors.New("scanning cdn_lock for user " + user + " and cachegroup ID " + strconv.Itoa(cachegroupID) + ": " + err.Error()), http.StatusInternalServerError
}
}
if userName != "" && user != userName && !soft {
Review comment:
Is this check supposed to be in the loop above?
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -242,52 +289,59 @@ func CheckIfCurrentUserCanModifyCDNWithID(tx *sql.Tx, cdnID int64, user string)
} else if !ok {
return errors.New("CDN not found"), nil, http.StatusNotFound
}
- query := `SELECT username, soft FROM cdn_lock WHERE cdn=$1`
+ return CheckIfCurrentUserCanModifyCDN(tx, string(cdnName), user)
+}
+
+// CheckIfCurrentUserCanModifyCachegroup checks if the current user has the lock on the cdns that are associated with the provided cachegroup ID.
+// This will succeed if no other user has a hard lock on any of the CDNs that relate to the cachegroup in question.
+func CheckIfCurrentUserCanModifyCachegroup(tx *sql.Tx, cachegroupID int, user string) (error, error, int) {
+ query := `SELECT username, cdn, soft FROM cdn_lock WHERE cdn IN (SELECT name FROM cdn WHERE id IN (SELECT cdn_id FROM server WHERE cachegroup = ($1)))`
var userName string
+ var cdn string
var soft bool
- rows, err := tx.Query(query, string(cdnName))
+ rows, err := tx.Query(query, cachegroupID)
if err != nil {
if errors.Is(err, sql.ErrNoRows) {
return nil, nil, http.StatusOK
}
- return nil, errors.New("querying cdn_lock for user " + user + " and cdn " + string(cdnName) + ": " + err.Error()), http.StatusInternalServerError
+ return nil, errors.New("querying cdn_lock for user " + user + " and cachegroup ID " + strconv.Itoa(cachegroupID) + ": " + err.Error()), http.StatusInternalServerError
}
defer rows.Close()
for rows.Next() {
- err = rows.Scan(&userName, &soft)
+ err = rows.Scan(&userName, &cdn, &soft)
if err != nil {
- return nil, errors.New("scanning cdn_lock for user " + user + " and cdn " + string(cdnName) + ": " + err.Error()), http.StatusInternalServerError
+ return nil, errors.New("scanning cdn_lock for user " + user + " and cachegroup ID " + strconv.Itoa(cachegroupID) + ": " + err.Error()), http.StatusInternalServerError
}
}
if userName != "" && user != userName && !soft {
- return errors.New("user " + userName + " currently has a hard lock on cdn " + string(cdnName)), nil, http.StatusForbidden
+ return errors.New("user " + userName + " currently has a hard lock on cdn " + cdn), nil, http.StatusForbidden
}
return nil, nil, http.StatusOK
}
-// CheckIfCurrentUserCanModifyCachegroup checks if the current user has the lock on the cdns that are associated with the provided cachegroup ID.
-// This will succeed if no other user has a hard lock on any of the CDNs that relate to the cachegroup in question.
-func CheckIfCurrentUserCanModifyCachegroup(tx *sql.Tx, cachegroupID int, user string) (error, error, int) {
- query := `SELECT username, cdn, soft FROM cdn_lock WHERE cdn IN (SELECT name FROM cdn WHERE id IN (SELECT cdn_id FROM server WHERE cachegroup = ($1)))`
+// CheckIfCurrentUserCanModifyCachegroups checks if the current user has the lock on the cdns that are associated with the provided cachegroup IDs.
+// This will succeed if no other user has a hard lock on any of the CDNs that relate to the cachegroups in question.
+func CheckIfCurrentUserCanModifyCachegroups(tx *sql.Tx, cachegroupID []int, user string) (error, error, int) {
Review comment:
nit: `cachegroupID` -> `cachegroupIDs` since it's a slice
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 edited a comment on pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
ocket8888 edited a comment on pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#issuecomment-875696607
Do you have a list anywhere of the endpoints this affects? The blueprint didn't include one iirc because it was just a bit too pedantic for a blueprint, but I think it'd be nice to know which endpoints are "lock-able" - would also maybe help reviewers verify that everything that _should_ be locked _is_.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#issuecomment-876778973
Seems like this TO API v4 test might have spurious failures:
```
--- FAIL: TestCacheGroupParameters (1.99s)
```
However, the test output doesn't seem to indicate why it actually failed. FWIW I'm removing that v4 test in https://github.com/apache/trafficcontrol/pull/5993, so I'm not going to worry about 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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#issuecomment-876565689
> I'd be very hesitant to make this a perfect long-term solution when we may not need it for long anyways.
> [T]his was never meant to be the long term solution
> Makes sense that you don't want to overhaul the entire PR at this point.
Good points, agreed that this PR is not the place to add a middleware.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#discussion_r658184979
##########
File path: traffic_ops/traffic_ops_golang/cachegroup/cachegroups.go
##########
@@ -611,12 +611,23 @@ func (cg *TOCacheGroup) Update(h http.Header) (error, error, int) {
return userErr, sysErr, errCode
}
+ cdns, err := dbhelpers.GetCDNsForCachegroup(cg.ReqInfo.Tx.Tx, cg.ID)
+ if err != nil {
+ return nil, err, http.StatusInternalServerError
+ }
+ for cdn, _ := range cdns {
+ userErr, sysErr, statusCode := dbhelpers.CheckIfCurrentUserCanModifyCDN(cg.ReqInfo.Tx.Tx, cdn, cg.ReqInfo.User.UserName)
Review comment:
Should be fixed now.
--
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] srijeet0406 commented on a change in pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#discussion_r666313931
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -242,52 +289,59 @@ func CheckIfCurrentUserCanModifyCDNWithID(tx *sql.Tx, cdnID int64, user string)
} else if !ok {
return errors.New("CDN not found"), nil, http.StatusNotFound
}
- query := `SELECT username, soft FROM cdn_lock WHERE cdn=$1`
+ return CheckIfCurrentUserCanModifyCDN(tx, string(cdnName), user)
+}
+
+// CheckIfCurrentUserCanModifyCachegroup checks if the current user has the lock on the cdns that are associated with the provided cachegroup ID.
+// This will succeed if no other user has a hard lock on any of the CDNs that relate to the cachegroup in question.
+func CheckIfCurrentUserCanModifyCachegroup(tx *sql.Tx, cachegroupID int, user string) (error, error, int) {
+ query := `SELECT username, cdn, soft FROM cdn_lock WHERE cdn IN (SELECT name FROM cdn WHERE id IN (SELECT cdn_id FROM server WHERE cachegroup = ($1)))`
var userName string
+ var cdn string
var soft bool
- rows, err := tx.Query(query, string(cdnName))
+ rows, err := tx.Query(query, cachegroupID)
if err != nil {
if errors.Is(err, sql.ErrNoRows) {
return nil, nil, http.StatusOK
}
- return nil, errors.New("querying cdn_lock for user " + user + " and cdn " + string(cdnName) + ": " + err.Error()), http.StatusInternalServerError
+ return nil, errors.New("querying cdn_lock for user " + user + " and cachegroup ID " + strconv.Itoa(cachegroupID) + ": " + err.Error()), http.StatusInternalServerError
}
defer rows.Close()
for rows.Next() {
- err = rows.Scan(&userName, &soft)
+ err = rows.Scan(&userName, &cdn, &soft)
if err != nil {
- return nil, errors.New("scanning cdn_lock for user " + user + " and cdn " + string(cdnName) + ": " + err.Error()), http.StatusInternalServerError
+ return nil, errors.New("scanning cdn_lock for user " + user + " and cachegroup ID " + strconv.Itoa(cachegroupID) + ": " + err.Error()), http.StatusInternalServerError
}
}
if userName != "" && user != userName && !soft {
Review comment:
Yes, because if any of the scans for the locks fails, we'd want to exit.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#issuecomment-876008125
So, I agree that we should try to reduce duplication as much as possible, but I'm not sure that changing the entire direction of this PR to use middleware is really appropriate for this. When I think of middleware, I think of things that generally apply to all routes (authorization, setting response headers, gzipping responses, etc.). These locking checks are somewhat specialized per route and depend on what is being modified. Somehow the middleware would have to know which CDNs were being modified, then check those against the current CDN locks. That means the route would have to set the CDN list in the request context (probably), and the middleware would have to get those out of the request context (after the request has been "handled successfully" as far as the handler knows) to run the check (which might fail). So where we're getting the CDNs and where we're checking the CDNs would be two totally different areas of the code (which isn't great IMO).
Maybe there is another way to do it better with middleware, but that is just the idea that came to mind. It just doesn't seem like a good fit. On the plus side, CDN locking should be unnecessary once we complete the plan for DS and server snapshots, so we should be able to deprecate and remove CDN locks once we get to that point. But in the meantime, they will tide us over.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp merged pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
rawlinp merged pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#issuecomment-876539328
I agree that code duplication should be minimized, but as Rawlin specified, this was never meant to be the long term solution, so holding up this last PR(and the project and the 6.0 release) for a major refactor isn't super beneficial IMO. I'm also trying to keep the CDN locks simple without adding a bunch more database tables and such, in accordance with the blueprint. Additionally, while speaking with Dave and the rest of the team, this was supposed to be the "first pass" at CDN locks. We can keep improving it in the next iterations (until it is replaced) and I'd be happy to add a ticket to take care of this in a future HIP sprint. Does that sound good to everyone?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#issuecomment-875847845
> Do you have a list anywhere of the endpoints this affects? The blueprint didn't include one iirc because it was just a bit too pedantic for a blueprint, but I think it'd be nice to know which endpoints are "lock-able" - would also maybe help reviewers verify that everything that _should_ be locked _is_.
Yeah, you're right. I didn't include a list of endpoints in the blueprint, but I'll list them out here:
POST/ PUT/ DELETE endpoints for the following:
CDNs (not Post, because the CDN needs to be present for a user to be able to lock it)
Servers
Cachegroups (while assigning and unassigning servers)
Cachegroup parameters
Topologies (while adding or deleting cachegroups)
DeliveryServices
Delivery Service related cert or key renewal/ creation/ deletion
Delivery Service Servers
Delivery Service Regexes
Federations
Origins
Profiles
Profile Parameters
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#issuecomment-875941865
Yeah, I was gonna suggest maybe adding a field to a Route structure, but it's much the same as a middleware. Would be many more lines of change than a middleware, 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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#discussion_r658141805
##########
File path: traffic_ops/traffic_ops_golang/cachegroup/cachegroups.go
##########
@@ -611,12 +611,23 @@ func (cg *TOCacheGroup) Update(h http.Header) (error, error, int) {
return userErr, sysErr, errCode
}
+ cdns, err := dbhelpers.GetCDNsForCachegroup(cg.ReqInfo.Tx.Tx, cg.ID)
+ if err != nil {
+ return nil, err, http.StatusInternalServerError
+ }
+ for cdn, _ := range cdns {
+ userErr, sysErr, statusCode := dbhelpers.CheckIfCurrentUserCanModifyCDN(cg.ReqInfo.Tx.Tx, cdn, cg.ReqInfo.User.UserName)
Review comment:
Just perusing here, but we generally shouldn't make DB calls in a loop like this. It's better to make a _single_ DB query that gets all the stuff we need to check in memory and check everything in memory at once, as opposed to making a DB query per CDN or per delivery service. Especially for things as numerous as delivery services, that can really slow things down and increase load on the DB.
--
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 #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#discussion_r666374080
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -242,52 +289,59 @@ func CheckIfCurrentUserCanModifyCDNWithID(tx *sql.Tx, cdnID int64, user string)
} else if !ok {
return errors.New("CDN not found"), nil, http.StatusNotFound
}
- query := `SELECT username, soft FROM cdn_lock WHERE cdn=$1`
+ return CheckIfCurrentUserCanModifyCDN(tx, string(cdnName), user)
+}
+
+// CheckIfCurrentUserCanModifyCachegroup checks if the current user has the lock on the cdns that are associated with the provided cachegroup ID.
+// This will succeed if no other user has a hard lock on any of the CDNs that relate to the cachegroup in question.
+func CheckIfCurrentUserCanModifyCachegroup(tx *sql.Tx, cachegroupID int, user string) (error, error, int) {
+ query := `SELECT username, cdn, soft FROM cdn_lock WHERE cdn IN (SELECT name FROM cdn WHERE id IN (SELECT cdn_id FROM server WHERE cachegroup = ($1)))`
var userName string
+ var cdn string
var soft bool
- rows, err := tx.Query(query, string(cdnName))
+ rows, err := tx.Query(query, cachegroupID)
if err != nil {
if errors.Is(err, sql.ErrNoRows) {
return nil, nil, http.StatusOK
}
- return nil, errors.New("querying cdn_lock for user " + user + " and cdn " + string(cdnName) + ": " + err.Error()), http.StatusInternalServerError
+ return nil, errors.New("querying cdn_lock for user " + user + " and cachegroup ID " + strconv.Itoa(cachegroupID) + ": " + err.Error()), http.StatusInternalServerError
}
defer rows.Close()
for rows.Next() {
- err = rows.Scan(&userName, &soft)
+ err = rows.Scan(&userName, &cdn, &soft)
if err != nil {
- return nil, errors.New("scanning cdn_lock for user " + user + " and cdn " + string(cdnName) + ": " + err.Error()), http.StatusInternalServerError
+ return nil, errors.New("scanning cdn_lock for user " + user + " and cachegroup ID " + strconv.Itoa(cachegroupID) + ": " + err.Error()), http.StatusInternalServerError
}
}
if userName != "" && user != userName && !soft {
Review comment:
Ok, this was marked resolved, but this check wasn't moved into the loop above
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#issuecomment-875696607
Do you have a list anywhere of the endpoints this affects? The blueprint didn't include one because it was just a bit too pedantic for a blueprint, but I think it'd be nice to know which endpoints are "lock-able" - would also maybe help reviewers verify that everything that _should_ be locked _is_.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#issuecomment-876508823
> maybe it would be more maintainable long-term than calling CheckIfCurrentUserCanModifyCDN() from each affected handler
I will refer back to my previous comment:
> On the plus side, CDN locking should be unnecessary once we complete the plan for DS and server snapshots, so we should be able to deprecate and remove CDN locks once we get to that point. But in the meantime, they will tide us over.
Long-term, I see CDN locking going away in favor of DS and server snapshots. Those are actually on our roadmap for Q4, so I'd be very hesitant to make this a perfect long-term solution when we may not need it for long anyways.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#issuecomment-876547175
I don't ultimately care how it gets done, just a suggestion. Makes sense that you don't want to overhaul the entire PR at this point.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#discussion_r661823511
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -207,6 +207,91 @@ func BuildWhereAndOrderByAndPagination(parameters map[string]string, queryParams
return whereClause, orderBy, paginationClause, queryValues, errs
}
+// CheckIfCurrentUserCanModifyCDN checks if the current user has the lock on the cdn that the requested operation is to be performed on.
+// This will succeed if the either there is no lock by any user on the CDN, or if the current user has the lock on the CDN.
+func CheckIfCurrentUserCanModifyCDN(tx *sql.Tx, cdn, user string) (error, error, int) {
+ query := `SELECT username, soft FROM cdn_lock WHERE cdn=$1`
+ var userName string
+ var soft bool
+ rows, err := tx.Query(query, cdn)
+ if err != nil {
+ if errors.Is(err, sql.ErrNoRows) {
+ return nil, nil, http.StatusOK
+ }
+ return nil, errors.New("querying cdn_lock for user " + user + " and cdn " + cdn + ": " + err.Error()), http.StatusInternalServerError
+ }
+ defer rows.Close()
+ for rows.Next() {
+ err = rows.Scan(&userName, &soft)
+ if err != nil {
+ return nil, errors.New("scanning cdn_lock for user " + user + " and cdn " + cdn + ": " + err.Error()), http.StatusInternalServerError
+ }
+ }
+ if userName != "" && user != userName && !soft {
+ return errors.New("user " + userName + " currently has a hard lock on cdn " + cdn), nil, http.StatusForbidden
+ }
+ return nil, nil, http.StatusOK
+}
+
+// CheckIfCurrentUserCanModifyCDNWithID checks if the current user has the lock on the cdn (identified by ID) that the requested operation is to be performed on.
+// This will succeed if the either there is no lock by any user on the CDN, or if the current user has the lock on the CDN.
+func CheckIfCurrentUserCanModifyCDNWithID(tx *sql.Tx, cdnID int64, user string) (error, error, int) {
+ cdnName, ok, err := GetCDNNameFromID(tx, cdnID)
+ if err != nil {
+ return nil, err, http.StatusInternalServerError
+ } else if !ok {
+ return errors.New("CDN not found"), nil, http.StatusNotFound
+ }
+ query := `SELECT username, soft FROM cdn_lock WHERE cdn=$1`
Review comment:
Now that we have the `cdnName` here, can we just call `CheckIfCurrentUserCanModifyCDN`?
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -1257,3 +1342,82 @@ func CheckTopologyOrgServerCGInDSCG(tx *sql.Tx, cdnIds []int, dsTopology string,
}
return nil, nil, http.StatusOK
}
+
+// GetCDNNameFromDSID returns the associated CDN name with the delivery service ID that is provided.
+func GetCDNNameFromDSID(tx *sql.Tx, dsID int) (string, error) {
+ if dsID == -1 {
+ return "", nil
+ }
+ cdn := ""
+ if err := tx.QueryRow(`SELECT name FROM cdn WHERE id = (SELECT cdn_id FROM deliveryservice WHERE id = $1)`, dsID).Scan(&cdn); err != nil {
+ return cdn, fmt.Errorf("querying CDN for deliveryservice with ID '%v': %v", dsID, err)
+ }
+ return cdn, nil
+}
+
+// GetCDNNameFromDSXMLID returns the associated CDN name with the delivery service XML ID that is provided.
+func GetCDNNameFromDSXMLID(tx *sql.Tx, xmlID string) (string, error) {
Review comment:
This is really similar to `GetDSIDAndCDNFromName` except that it also returns the DS ID. Could that be reused instead and just ignore the DS ID if we don't need it?
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -1257,3 +1342,82 @@ func CheckTopologyOrgServerCGInDSCG(tx *sql.Tx, cdnIds []int, dsTopology string,
}
return nil, nil, http.StatusOK
}
+
+// GetCDNNameFromDSID returns the associated CDN name with the delivery service ID that is provided.
+func GetCDNNameFromDSID(tx *sql.Tx, dsID int) (string, error) {
Review comment:
This is really similar to `GetDSNameAndCDNFromID` except that it also returns the DS XMLID. Could that be reused instead and just ignore the XMLID if we don't need it?
##########
File path: traffic_ops/traffic_ops_golang/cachegroup/cachegroups.go
##########
@@ -611,6 +611,11 @@ func (cg *TOCacheGroup) Update(h http.Header) (error, error, int) {
return userErr, sysErr, errCode
}
+ // CheckIfCurrentUserCanModifyCachegroup
Review comment:
this comment seems unnecessary
##########
File path: traffic_ops/traffic_ops_golang/cachegroup/cachegroups.go
##########
@@ -716,6 +721,11 @@ func (cg *TOCacheGroup) Delete() (error, error, int) {
return nil, errors.New("cachegroup delete: getting coord: " + err.Error()), http.StatusInternalServerError
}
+ // CheckIfCurrentUserCanModifyCachegroup
Review comment:
this comment seems unnecessary
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -1257,3 +1342,82 @@ func CheckTopologyOrgServerCGInDSCG(tx *sql.Tx, cdnIds []int, dsTopology string,
}
return nil, nil, http.StatusOK
}
+
+// GetCDNNameFromDSID returns the associated CDN name with the delivery service ID that is provided.
+func GetCDNNameFromDSID(tx *sql.Tx, dsID int) (string, error) {
+ if dsID == -1 {
+ return "", nil
+ }
+ cdn := ""
+ if err := tx.QueryRow(`SELECT name FROM cdn WHERE id = (SELECT cdn_id FROM deliveryservice WHERE id = $1)`, dsID).Scan(&cdn); err != nil {
+ return cdn, fmt.Errorf("querying CDN for deliveryservice with ID '%v': %v", dsID, err)
+ }
+ return cdn, nil
+}
+
+// GetCDNNameFromDSXMLID returns the associated CDN name with the delivery service XML ID that is provided.
+func GetCDNNameFromDSXMLID(tx *sql.Tx, xmlID string) (string, error) {
+ cdn := ""
+ if err := tx.QueryRow(`SELECT name FROM cdn WHERE id = (SELECT cdn_id FROM deliveryservice WHERE xml_id = $1)`, xmlID).Scan(&cdn); err != nil {
+ return cdn, fmt.Errorf("querying CDN for deliveryservice with XML ID '%v': %v", xmlID, err)
+ }
+ return cdn, nil
+}
+
+// GetCDNNameFromProfileID returns the cdn name for the provided profile ID.
+func GetCDNNameFromProfileID(tx *sql.Tx, id int) (tc.CDNName, error) {
+ name := ""
+ if err := tx.QueryRow(`SELECT name FROM cdn WHERE id = (SELECT cdn FROM profile WHERE id = $1)`, id).Scan(&name); err != nil {
+ return "", errors.New("querying CDN name from profile ID: " + err.Error())
+ }
+ return tc.CDNName(name), nil
+}
+
+// GetCDNNameFromProfileName returns the cdn name for the provided profile name.
+func GetCDNNameFromProfileName(tx *sql.Tx, profileName string) (tc.CDNName, error) {
+ name := ""
+ if err := tx.QueryRow(`SELECT name FROM cdn WHERE id = (SELECT cdn FROM profile WHERE name = $1)`, profileName).Scan(&name); err != nil {
+ return "", errors.New("querying CDN name from profile name: " + err.Error())
+ }
+ return tc.CDNName(name), nil
+}
+
+// GetServerIDsFromCachegroupNames returns a list of servers IDs for a list of cachegroup IDs.
+func GetServerIDsFromCachegroupNames(tx *sql.Tx, cgID []string) ([]int64, error) {
+ var serverIDs []int64
+ var serverID int64
+ query := `SELECT server.id FROM server JOIN cachegroup cg ON cg.id = server.cachegroup where cg.name = ANY($1)`
+ rows, err := tx.Query(query, pq.Array(cgID))
+ if err != nil {
+ return serverIDs, errors.New("getting server IDs from cachegroup names : " + err.Error())
+ }
+ defer rows.Close()
Review comment:
we could call `log.Close(rows, "some context")` here to log an error if one occurs
##########
File path: traffic_ops/traffic_ops_golang/cachegroup/dspost.go
##########
@@ -84,6 +84,19 @@ func DSPostHandlerV40(w http.ResponseWriter, r *http.Request) {
return
}
+ for _, dsID := range req.DeliveryServices {
+ cdn, err := dbhelpers.GetCDNNameFromDSID(inf.Tx.Tx, dsID)
+ if err != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("cachegroup deliveryservice update: getting CDN from DS ID "+err.Error()))
+ return
+ }
+ // CheckIfCurrentUserCanModifyCDN
Review comment:
this comment seems unnecessary
##########
File path: traffic_ops/traffic_ops_golang/cachegroupparameter/parameters.go
##########
@@ -311,6 +319,12 @@ func AddCacheGroupParameters(w http.ResponseWriter, r *http.Request) {
}
for _, p := range params {
+ // CheckIfCurrentUserCanModifyCachegroup
Review comment:
this comment seems unnecessary
##########
File path: traffic_ops/traffic_ops_golang/cachegroupparameter/parameters.go
##########
@@ -207,6 +207,11 @@ func (cgparam *TOCacheGroupParameter) Delete() (error, error, int) {
return fmt.Errorf("parameter %v does not exist", *cgparam.ID), nil, http.StatusNotFound
}
+ // CheckIfCurrentUserCanModifyCachegroup
Review comment:
this comment seems unnecessary
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/acme_renew.go
##########
@@ -45,6 +46,10 @@ func RenewAcmeCertificate(w http.ResponseWriter, r *http.Request) {
return
}
defer inf.Close()
+ if inf.User == nil {
Review comment:
nit: these `inf.User == nil` checks are unnecessary because `api.NewInfo` would return an error if it was unable to get a user, and we always check that error
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/acme.go
##########
@@ -265,6 +278,14 @@ func GenerateLetsEncryptCertificates(w http.ResponseWriter, r *http.Request) {
return
}
+ // CheckIfCurrentUserCanModifyCDN
Review comment:
this comment seems unnecessary
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -1167,6 +1185,26 @@ func (ds *TODeliveryService) Delete() (error, error, int) {
}
ds.XMLID = &xmlID
+ if ds.CDNID != nil {
+ userErr, sysErr, errCode := dbhelpers.CheckIfCurrentUserCanModifyCDNWithID(ds.APIInfo().Tx.Tx, int64(*ds.CDNID), ds.APIInfo().User.UserName)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, errCode
+ }
+ } else if ds.CDNName != nil {
+ userErr, sysErr, errCode := dbhelpers.CheckIfCurrentUserCanModifyCDN(ds.APIInfo().Tx.Tx, *ds.CDNName, ds.APIInfo().User.UserName)
+ if userErr != nil || sysErr != nil {
+ return userErr, sysErr, errCode
+ }
+ } else {
+ cdnName, err := dbhelpers.GetCDNNameFromDSID(ds.ReqInfo.Tx.Tx, *ds.ID)
+ if err != nil {
+ return fmt.Errorf("couldn't get cdn name for DS: %v", err), nil, http.StatusBadRequest
Review comment:
I think this would be a `sysErr` instead of a `userErr` since at this point we know the DS exists so the error would most likely be a system error
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -729,6 +736,17 @@ func UpdateV40(w http.ResponseWriter, r *http.Request) {
return
}
ds.ID = &id
+ cdn, err := dbhelpers.GetCDNNameFromDSID(inf.Tx.Tx, id)
+ if err != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("deliveryservice update: getting CDN from DS ID "+err.Error()))
+ return
+ }
+ // CheckIfCurrentUserCanModifyCDN
Review comment:
this comment seems unnecessary
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/keys.go
##########
@@ -76,7 +81,17 @@ func AddSSLKeys(w http.ResponseWriter, r *http.Request) {
api.HandleErr(w, r, inf.Tx.Tx, http.StatusNotFound, errors.New("no DS with name "+*req.DeliveryService), nil)
return
}
-
+ cdn, err := dbhelpers.GetCDNNameFromDSID(inf.Tx.Tx, dsID)
+ if err != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("deliveryservice.AddSSLKeys: getting CDN from DS ID "+err.Error()))
+ return
+ }
+ // CheckIfCurrentUserCanModifyCDN
Review comment:
this comment seems unnecessary
##########
File path: traffic_ops/traffic_ops_golang/servercheck/servercheck.go
##########
@@ -131,6 +131,16 @@ func CreateUpdateServercheck(w http.ResponseWriter, r *http.Request) {
return
}
+ cdnName, err := dbhelpers.GetCDNNameFromServerID(inf.Tx.Tx, int64(id))
Review comment:
`servercheck` probably doesn't need to be lock-safe since it's primarily just for monitoring and doesn't really affect CDN operation
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/autorenewcerts.go
##########
@@ -100,6 +105,17 @@ func renewCertificates(w http.ResponseWriter, r *http.Request, deprecated bool)
api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
return
}
+ cdn, err := dbhelpers.GetCDNNameFromDSXMLID(inf.Tx.Tx, ds.XmlId)
Review comment:
I'm pretty sure this will cause an error because there are still unread rows in the current transaction that we're using here. We have to fully read the rows before being able to reuse the same transaction for another query.
Also, similar to my earlier comment, we wouldn't want to run a DB query for every single DS and cert -- rather, we should run a query to get all the distinct CDN names of the DSes here then pass that result to another function which runs a query to check all the CDNs at once.
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -1257,3 +1342,82 @@ func CheckTopologyOrgServerCGInDSCG(tx *sql.Tx, cdnIds []int, dsTopology string,
}
return nil, nil, http.StatusOK
}
+
+// GetCDNNameFromDSID returns the associated CDN name with the delivery service ID that is provided.
+func GetCDNNameFromDSID(tx *sql.Tx, dsID int) (string, error) {
+ if dsID == -1 {
+ return "", nil
+ }
+ cdn := ""
+ if err := tx.QueryRow(`SELECT name FROM cdn WHERE id = (SELECT cdn_id FROM deliveryservice WHERE id = $1)`, dsID).Scan(&cdn); err != nil {
+ return cdn, fmt.Errorf("querying CDN for deliveryservice with ID '%v': %v", dsID, err)
+ }
+ return cdn, nil
+}
+
+// GetCDNNameFromDSXMLID returns the associated CDN name with the delivery service XML ID that is provided.
+func GetCDNNameFromDSXMLID(tx *sql.Tx, xmlID string) (string, error) {
+ cdn := ""
+ if err := tx.QueryRow(`SELECT name FROM cdn WHERE id = (SELECT cdn_id FROM deliveryservice WHERE xml_id = $1)`, xmlID).Scan(&cdn); err != nil {
+ return cdn, fmt.Errorf("querying CDN for deliveryservice with XML ID '%v': %v", xmlID, err)
+ }
+ return cdn, nil
+}
+
+// GetCDNNameFromProfileID returns the cdn name for the provided profile ID.
+func GetCDNNameFromProfileID(tx *sql.Tx, id int) (tc.CDNName, error) {
+ name := ""
+ if err := tx.QueryRow(`SELECT name FROM cdn WHERE id = (SELECT cdn FROM profile WHERE id = $1)`, id).Scan(&name); err != nil {
+ return "", errors.New("querying CDN name from profile ID: " + err.Error())
+ }
+ return tc.CDNName(name), nil
+}
+
+// GetCDNNameFromProfileName returns the cdn name for the provided profile name.
+func GetCDNNameFromProfileName(tx *sql.Tx, profileName string) (tc.CDNName, error) {
+ name := ""
+ if err := tx.QueryRow(`SELECT name FROM cdn WHERE id = (SELECT cdn FROM profile WHERE name = $1)`, profileName).Scan(&name); err != nil {
+ return "", errors.New("querying CDN name from profile name: " + err.Error())
+ }
+ return tc.CDNName(name), nil
+}
+
+// GetServerIDsFromCachegroupNames returns a list of servers IDs for a list of cachegroup IDs.
+func GetServerIDsFromCachegroupNames(tx *sql.Tx, cgID []string) ([]int64, error) {
+ var serverIDs []int64
+ var serverID int64
+ query := `SELECT server.id FROM server JOIN cachegroup cg ON cg.id = server.cachegroup where cg.name = ANY($1)`
+ rows, err := tx.Query(query, pq.Array(cgID))
+ if err != nil {
+ return serverIDs, errors.New("getting server IDs from cachegroup names : " + err.Error())
+ }
+ defer rows.Close()
+ for rows.Next() {
+ err = rows.Scan(&serverID)
+ if err != nil {
+ return serverIDs, errors.New("scanning server ID : " + err.Error())
+ }
+ serverIDs = append(serverIDs, serverID)
+ }
+ return serverIDs, nil
+}
+
+// GetCDNNamesFromServerIds returns a list of cdn names for a list of server IDs.
+func GetCDNNamesFromServerIds(tx *sql.Tx, serverIds []int64) ([]string, error) {
+ var cdns []string
+ cdn := ""
+ query := `SELECT DISTINCT(name) FROM cdn JOIN server ON cdn.id = server.cdn_id WHERE server.id = ANY($1)`
+ rows, err := tx.Query(query, pq.Array(serverIds))
+ if err != nil {
+ return cdns, errors.New("getting cdn name for server : " + err.Error())
+ }
+ defer rows.Close()
Review comment:
we could call `log.Close(rows, "some context")` here to log an error if one occurs
##########
File path: traffic_ops/traffic_ops_golang/cachegroupparameter/parameters.go
##########
@@ -311,6 +319,12 @@ func AddCacheGroupParameters(w http.ResponseWriter, r *http.Request) {
}
for _, p := range params {
+ // CheckIfCurrentUserCanModifyCachegroup
+ userErr, sysErr, errCode := dbhelpers.CheckIfCurrentUserCanModifyCachegroup(inf.Tx.Tx, *p.CacheGroup, inf.User.UserName)
Review comment:
similar to my earlier comment, we should try to make one query to check all the given cachegroups at once, rather than looping over each parameter in the request
##########
File path: traffic_ops/traffic_ops_golang/cachegroup/dspost.go
##########
@@ -84,6 +84,19 @@ func DSPostHandlerV40(w http.ResponseWriter, r *http.Request) {
return
}
+ for _, dsID := range req.DeliveryServices {
+ cdn, err := dbhelpers.GetCDNNameFromDSID(inf.Tx.Tx, dsID)
Review comment:
We should really avoid making DB queries in a loop like this because it scales linearly with the number of delivery services given in the request. Can we make one query to get all the distinct DS CDN names, then run a 2nd query to check all the CDNs at once?
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/acme_renew.go
##########
@@ -56,10 +61,22 @@ func RenewAcmeCertificate(w http.ResponseWriter, r *http.Request) {
return
}
+ cdn, err := dbhelpers.GetCDNNameFromDSXMLID(inf.Tx.Tx, xmlID)
+ if err != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("renew acme certificate: getting CDN from DS XML ID "+err.Error()))
+ return
+ }
+ // CheckIfCurrentUserCanModifyCDN
Review comment:
comment seems unnecessary
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/acme.go
##########
@@ -205,10 +209,19 @@ func GenerateAcmeCertificates(w http.ResponseWriter, r *http.Request) {
api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("delivery service not in cdn"), nil)
return
}
+ // CheckIfCurrentUserCanModifyCDN
Review comment:
this comment seems unnecessary
##########
File path: traffic_ops/traffic_ops_golang/federations/ds.go
##########
@@ -63,6 +63,18 @@ func PostDSes(w http.ResponseWriter, r *http.Request) {
api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("A federation must have at least one delivery service assigned"), nil)
return
}
+ for _, dsID := range post.DSIDs {
+ cdnName, err := dbhelpers.GetCDNNameFromDSID(inf.Tx.Tx, dsID)
Review comment:
ditto earlier comments about running DB queries in a loop
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/autorenewcerts.go
##########
@@ -100,6 +105,17 @@ func renewCertificates(w http.ResponseWriter, r *http.Request, deprecated bool)
api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, err)
return
}
+ cdn, err := dbhelpers.GetCDNNameFromDSXMLID(inf.Tx.Tx, ds.XmlId)
+ if err != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("renew acme certificate: getting CDN from DS XML ID "+err.Error()))
+ return
+ }
+ // CheckIfCurrentUserCanModifyCDN
Review comment:
this comment seems unnecessary
##########
File path: traffic_ops/traffic_ops_golang/profileparameter/postparameterprofile.go
##########
@@ -45,6 +46,20 @@ func PostParamProfile(w http.ResponseWriter, r *http.Request) {
api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("parse error: "+err.Error()), nil)
return
}
+ if paramProfile.ProfileIDs != nil {
+ for _, profileID := range *paramProfile.ProfileIDs {
+ cdnName, err := dbhelpers.GetCDNNameFromProfileID(inf.Tx.Tx, int(profileID))
Review comment:
ditto earlier comments about running DB queries in a loop
##########
File path: traffic_ops/traffic_ops_golang/topology/topologies.go
##########
@@ -806,3 +821,22 @@ func selectMaxLastUpdatedQuery(where string) string {
` UNION ALL
select max(last_updated) as ti from last_deleted l where l.table_name='topology') as res`
}
+
+func (topology *TOTopology) checkIfTopologyCanBeAlteredByCurrentUser() (error, error, int) {
+ cachegroups := topology.getCachegroupNames()
+ serverIDs, err := dbhelpers.GetServerIDsFromCachegroupNames(topology.ReqInfo.Tx.Tx, cachegroups)
+ if err != nil {
+ return nil, err, http.StatusInternalServerError
+ }
+ cdns, err := dbhelpers.GetCDNNamesFromServerIds(topology.ReqInfo.Tx.Tx, serverIDs)
+ if err != nil {
+ return nil, err, http.StatusInternalServerError
+ }
+ for _, cdn := range cdns {
+ userErr, sysErr, statusCode := dbhelpers.CheckIfCurrentUserCanModifyCDN(topology.ReqInfo.Tx.Tx, cdn, topology.ReqInfo.User.UserName)
Review comment:
ditto earlier comments about making DB queries in a loop
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/keys.go
##########
@@ -29,6 +29,7 @@ import (
"encoding/pem"
"errors"
"fmt"
+ "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
Review comment:
nit: I just noticed that this import is out of order in most of these changes
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#discussion_r666384723
##########
File path: traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go
##########
@@ -242,52 +289,59 @@ func CheckIfCurrentUserCanModifyCDNWithID(tx *sql.Tx, cdnID int64, user string)
} else if !ok {
return errors.New("CDN not found"), nil, http.StatusNotFound
}
- query := `SELECT username, soft FROM cdn_lock WHERE cdn=$1`
+ return CheckIfCurrentUserCanModifyCDN(tx, string(cdnName), user)
+}
+
+// CheckIfCurrentUserCanModifyCachegroup checks if the current user has the lock on the cdns that are associated with the provided cachegroup ID.
+// This will succeed if no other user has a hard lock on any of the CDNs that relate to the cachegroup in question.
+func CheckIfCurrentUserCanModifyCachegroup(tx *sql.Tx, cachegroupID int, user string) (error, error, int) {
+ query := `SELECT username, cdn, soft FROM cdn_lock WHERE cdn IN (SELECT name FROM cdn WHERE id IN (SELECT cdn_id FROM server WHERE cachegroup = ($1)))`
var userName string
+ var cdn string
var soft bool
- rows, err := tx.Query(query, string(cdnName))
+ rows, err := tx.Query(query, cachegroupID)
if err != nil {
if errors.Is(err, sql.ErrNoRows) {
return nil, nil, http.StatusOK
}
- return nil, errors.New("querying cdn_lock for user " + user + " and cdn " + string(cdnName) + ": " + err.Error()), http.StatusInternalServerError
+ return nil, errors.New("querying cdn_lock for user " + user + " and cachegroup ID " + strconv.Itoa(cachegroupID) + ": " + err.Error()), http.StatusInternalServerError
}
defer rows.Close()
for rows.Next() {
- err = rows.Scan(&userName, &soft)
+ err = rows.Scan(&userName, &cdn, &soft)
if err != nil {
- return nil, errors.New("scanning cdn_lock for user " + user + " and cdn " + string(cdnName) + ": " + err.Error()), http.StatusInternalServerError
+ return nil, errors.New("scanning cdn_lock for user " + user + " and cachegroup ID " + strconv.Itoa(cachegroupID) + ": " + err.Error()), http.StatusInternalServerError
}
}
if userName != "" && user != userName && !soft {
Review comment:
Whoops, I thought you were referring to the previous error check. The other check should be inside the loop. Fixed now.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on pull request #5974: Every endpoint that modifies a CDN, or components related to a CDN, needs to be "lock safe"
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5974:
URL: https://github.com/apache/trafficcontrol/pull/5974#issuecomment-876112001
> So where we're getting the CDNs and where we're checking the CDNs would be two totally different areas of the code (which isn't great IMO).
Since all "lock-safe" endpoints are either POST, PUT, or DELETE, the parameter identifying the resource will be almost always be unique to that resource. There could be an additional table like `locks_by_route` with 3 columns:
* `resource_key`
* `route`
* `cdn_is_locked`
and a CDN Locks middleware would be able to check if a given Route is already locked with respect to a given resource key without needing to derive which CDN that resource uses.
This almost always means needing to know in advance which parameters to use for `resource_key` (as far as I know, this would be primary key in all cases except endpoints implementing `OptionsDeleter.DeleteKeyOptions()`), but there could be an additional table indicating which DB tables correspond to which TO Routes.
In the case of resources implementing `OptionsDeleter.DeleteKeyOptions()` (Topologies and Regions only), there would need to be a row for each option returned by ``DeleteKeyOptions()` for that resource type.
Since this approach would involve setting up infrastructure in Traffic Ops that we don't currently have, it wouldn't necessarily immediately save implementation time, though maybe it would be more maintainable long-term than calling `CheckIfCurrentUserCanModifyCDN()` from each affected handler.
> The middleware would have to get those out of the request context (after the request has been "handled successfully" as far as the handler knows) to run the check (which might fail).
If there is a CDN Locks middleware on some Routes, it would go at the end of the middlewares list (after `middleware.WrapPanicRecover()`. Since it is at the end of the middleware list, and since we'd be passing a custom middlewares list for each such route, calling `h(w, r)` from the CDN Locks middleware would always call the Route handler, meaning that if the CDN Locks check fails, calling `h(w, r)` could just be skipped.
This PR aside, middlewares are very underused in Traffic Ops (we actually have 0 endpoints that use non-default middlewares, `Route.Middlewares[]` is nil or empty for all Routes listed in `routes.go`), so hopefully someone does make use of them at some point.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org