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 2022/05/09 23:35:37 UTC
[GitHub] [trafficcontrol] srijeet0406 opened a new pull request, #6817: Prevent modifying and/or deleting reserved statuses
srijeet0406 opened a new pull request, #6817:
URL: https://github.com/apache/trafficcontrol/pull/6817
<!--
Thank you for contributing! Please be sure to read our contribution guidelines: https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md
If this closes or relates to an existing issue, please reference it using one of the following:
Closes: #6291
Related: #ISSUE
If this PR fixes a security vulnerability, DO NOT submit! Instead, contact
the Apache Traffic Control Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://apache.org/security regarding vulnerability disclosure.
-->
<!-- **^ Add meaningful description above** --><hr/>
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this PR.
Feel free to add the name of a tool or script that is affected but not on the list.
-->
- Traffic Ops
## What is the best way to verify this PR?
<!-- Please include here ALL the steps necessary to test your PR.
If your PR has tests (and most should), provide the steps needed to run the tests.
If not, please provide step-by-step instructions to test the PR manually and explain why your PR does not need tests. -->
Run TO and TP locally.
Try to modify/ delete one of the following statuses. You should get an error while doing so.
Reserved status names:
`PRE_PROD, ONLINE, OFFLINE, ADMIN_DOWN, REPORTED`
## If this is a bugfix, which Traffic Control versions contained the bug?
<!-- Delete this section if the PR is not a bugfix, or if the bug is only in the master branch.
Examples:
- 5.1.2
- 5.1.3 (RC1)
-->
- master
## PR submission checklist
- [x] This PR has tests <!-- If not, please delete this text and explain why this PR does not need tests. -->
- [ ] This PR has documentation <!-- If not, please delete this text and explain why this PR does not need documentation. -->
- [x] This PR has a CHANGELOG.md entry <!-- A fix for a bug from an ATC release, an improvement, or a new feature should have a changelog entry. -->
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
<!--
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.
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 #6817: Prevent modifying and/or deleting reserved statuses
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on PR #6817:
URL: https://github.com/apache/trafficcontrol/pull/6817#issuecomment-1122624306
> just curious. is "PRE_PROD" a reserved status? i.e. is there code that looks specifically for PRE_PROD and performs certain actions based on that status? is this status seeded? i was kind of under the impression Comcast added this status to support an internal process.
I added it in there for two reasons:
1. The issue mentions it.
2. Our seeds file adds this status as part of the seeding process, so I assumed it was reserved.
That said, I can remove it if we think that it shouldn't be reserved. Not sure if/ when it got to be a reserved status.
--
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] mitchell852 commented on pull request #6817: Prevent modifying and/or deleting reserved statuses
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on PR #6817:
URL: https://github.com/apache/trafficcontrol/pull/6817#issuecomment-1122717854
> Comcast may have done that, but it's seeded into everyone's TODB on installation
ok well is guess since it's "seeded" it should be immutable
--
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 #6817: Prevent modifying and/or deleting reserved statuses
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on PR #6817:
URL: https://github.com/apache/trafficcontrol/pull/6817#issuecomment-1122651460
> just curious. is "PRE_PROD" a reserved status? i.e. is there code that looks specifically for PRE_PROD and performs certain actions based on that status? is this status seeded? i was kind of under the impression Comcast added this status to support an internal process.
Comcast may have done that, but it's seeded into TODB on installation. There is no code in ATC that checks for and acts explicitly on that status. It has no non-semantic meaning.
--
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] mitchell852 commented on pull request #6817: Prevent modifying and/or deleting reserved statuses
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on PR #6817:
URL: https://github.com/apache/trafficcontrol/pull/6817#issuecomment-1122517650
just curious. is "PRE_PROD" a reserved name? i.e. is there code that looks specifically for PRE_PROD and performs certain actions based on that status? is this status seeded? i was kind of under the impression Comcast added this status to support an internal process.
--
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 diff in pull request #6817: Prevent modifying and/or deleting reserved statuses
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on code in PR #6817:
URL: https://github.com/apache/trafficcontrol/pull/6817#discussion_r871839450
##########
cache-config/testing/ort-tests/tcdata/statuses.go:
##########
@@ -17,30 +17,14 @@ package tcdata
Review Comment:
You're right, I've removed the file 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] ocket8888 commented on a diff in pull request #6817: Prevent modifying and/or deleting reserved statuses
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6817:
URL: https://github.com/apache/trafficcontrol/pull/6817#discussion_r869458440
##########
cache-config/testing/ort-tests/tcdata/statuses.go:
##########
@@ -19,6 +19,14 @@ import (
"testing"
)
+var statusNameMap = map[string]bool{
Review Comment:
I see this map repeated in 5 different places in this PR. I think the reason you're doing that is because it's mutable so exposing it as an exported member of `lib/go-tc` could be problematic. But functions are immutable, so you could just have one function in the lib that does this instead of repeating it 5 times:
```go
// IsReservedStatus checks if the given status name is reserved; i.e. may not be
// deleted or modified.
func IsReservedStatus(status string) bool {
switch (status) {
case CacheStatusAdminDown:
fallthrough
case CacheStatusOffline:
fallthrough
case CacheStatusOnline:
fallthrough
case CacheStatusPreProd:
fallthrough
case CacheStatusReported:
return true
}
return false
}
```
##########
cache-config/testing/ort-tests/tcdata/statuses.go:
##########
@@ -46,17 +54,43 @@ func (r *TCData) DeleteTestStatuses(t *testing.T) {
respStatus := resp[0]
delResp, _, err := TOSession.DeleteStatusByID(respStatus.ID)
- if err != nil {
- t.Errorf("cannot DELETE Status by name: %v - %v", err, delResp)
- }
+ if _, ok := statusNameMap[*status.Name]; !ok {
+ if err != nil {
+ t.Errorf("cannot DELETE Status by name: %v - %v", err, delResp)
+ }
- // Retrieve the Status to see if it got deleted
- types, _, err := TOSession.GetStatusByName(*status.Name)
- if err != nil {
- t.Errorf("error deleting Status name: %v", err)
- }
- if len(types) > 0 {
- t.Errorf("expected Status name: %s to be deleted", *status.Name)
+ // Retrieve the Status to see if it got deleted
+ types, _, err := TOSession.GetStatusByName(*status.Name)
+ if err != nil {
+ t.Errorf("error deleting Status name: %v", err)
+ }
+ if len(types) > 0 {
+ t.Errorf("expected Status name: %s to be deleted", *status.Name)
+ }
+ } else {
+ if err == nil {
+ t.Errorf("expected an error while trying to delete a reserved status, but got nothing")
+ }
}
}
+ r.ForceDeleteStatuses(t)
+}
+
+// ForceDeleteStatuses forcibly deletes the statuses from the db.
+func (r *TCData) ForceDeleteStatuses(t *testing.T) {
Review Comment:
Instead of doing this, we should just treat the statuses as immutable, which they now are, and not create an invalid Traffic Ops instance by deleting them. That means removing the Statuses which already exist in every valid Traffic Ops instance from the testing data "fixtures".
##########
traffic_ops/app/db/migrations/2022050916074300_add_reserved_statuses.down.sql:
##########
@@ -0,0 +1,19 @@
+/*
+ * 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.
+ */
+
+DELETE FROM public.status WHERE name IN ('ONLINE', 'OFFLINE', 'REPORTED', 'ADMIN_DOWN', 'PRE_PROD');
Review Comment:
This will break existing TO installations. If you have data unchanged from seeds.sql and then try to upgrade with this migration, then downgrade back, first of all it will probably fail to roll back because there will almost certainly be servers using at least one of these statuses, but then also your data will not be left in the same state as it was pre-upgrade.
This file should be blank. In the worst case scenario, that would mean downgrading creates statuses that need to exist for ATC to work, so there's only benefit.
##########
traffic_ops/testing/api/v2/statuses_test.go:
##########
@@ -42,35 +50,44 @@ func CreateTestStatuses(t *testing.T) {
func UpdateTestStatuses(t *testing.T) {
- firstStatus := testData.Statuses[0]
- if firstStatus.Name == nil {
- t.Fatal("cannot update test statuses: first test data status must have a name")
- }
-
- // Retrieve the Status by name so we can get the id for the Update
- resp, _, err := TOSession.GetStatusByName(*firstStatus.Name)
- if err != nil {
- t.Errorf("cannot GET Status by name: %v - %v", firstStatus.Name, err)
- }
- remoteStatus := resp[0]
- expectedStatusDesc := "new description"
- remoteStatus.Description = expectedStatusDesc
- var alert tc.Alerts
- alert, _, err = TOSession.UpdateStatusByID(remoteStatus.ID, remoteStatus)
- if err != nil {
- t.Errorf("cannot UPDATE Status by id: %v - %v", err, alert)
- }
-
- // Retrieve the Status to check Status name got updated
- resp, _, err = TOSession.GetStatusByID(remoteStatus.ID)
- if err != nil {
- t.Errorf("cannot GET Status by ID: %v - %v", firstStatus.Description, err)
+ if len(testData.Statuses) < 1 {
+ t.Fatal("Need at least one Status to test updating a Status")
}
- respStatus := resp[0]
- if respStatus.Description != expectedStatusDesc {
- t.Errorf("results do not match actual: %s, expected: %s", respStatus.Name, expectedStatusDesc)
+ for _, status := range testData.Statuses {
+ if status.Name == nil {
+ t.Fatal("cannot update test statuses: test data status must have a name")
+ }
+ // Retrieve the Status by name so we can get the id for the Update
+ resp, _, err := TOSession.GetStatusByName(*status.Name)
+ if err != nil {
+ t.Errorf("cannot GET Status by name: %v - %v", status.Name, err)
+ }
+ remoteStatus := resp[0]
+ expectedStatusDesc := "new description"
+ remoteStatus.Description = expectedStatusDesc
+ var alert tc.Alerts
+ alert, _, err = TOSession.UpdateStatusByID(remoteStatus.ID, remoteStatus)
+
+ if _, ok := statusNameMap[*status.Name]; ok {
+ if err == nil {
+ t.Errorf("expected an error about while updating a reserved status, but got nothing")
+ }
+ } else {
+ if err != nil {
+ t.Errorf("cannot UPDATE Status by id: %v - %v", err, alert)
+ }
+
+ // Retrieve the Status to check Status name got updated
+ resp, _, err = TOSession.GetStatusByID(remoteStatus.ID)
+ if err != nil {
+ t.Errorf("cannot GET Status by ID: %v - %v", status.Description, err)
Review Comment:
Same as above; `status.Description` is a `*string`.
##########
cache-config/testing/ort-tests/tcdata/statuses.go:
##########
@@ -46,17 +54,43 @@ func (r *TCData) DeleteTestStatuses(t *testing.T) {
respStatus := resp[0]
delResp, _, err := TOSession.DeleteStatusByID(respStatus.ID)
- if err != nil {
- t.Errorf("cannot DELETE Status by name: %v - %v", err, delResp)
- }
+ if _, ok := statusNameMap[*status.Name]; !ok {
+ if err != nil {
+ t.Errorf("cannot DELETE Status by name: %v - %v", err, delResp)
+ }
- // Retrieve the Status to see if it got deleted
- types, _, err := TOSession.GetStatusByName(*status.Name)
- if err != nil {
- t.Errorf("error deleting Status name: %v", err)
- }
- if len(types) > 0 {
- t.Errorf("expected Status name: %s to be deleted", *status.Name)
+ // Retrieve the Status to see if it got deleted
+ types, _, err := TOSession.GetStatusByName(*status.Name)
+ if err != nil {
+ t.Errorf("error deleting Status name: %v", err)
+ }
+ if len(types) > 0 {
+ t.Errorf("expected Status name: %s to be deleted", *status.Name)
+ }
+ } else {
+ if err == nil {
+ t.Errorf("expected an error while trying to delete a reserved status, but got nothing")
+ }
}
Review Comment:
nit:
```go
...
} else {
if condition {
...
}
}
```
is equivalent to the shorter
```go
...
} else if condition {
...
}
##########
traffic_ops/traffic_ops_golang/status/statuses.go:
##########
@@ -119,9 +127,29 @@ func (st *TOStatus) Read(h http.Header, useIMS bool) ([]interface{}, error, erro
return readVals, nil, nil, errCode, maxTime
}
-func (st *TOStatus) Update(h http.Header) (error, error, int) { return api.GenericUpdate(h, st) }
-func (st *TOStatus) Create() (error, error, int) { return api.GenericCreate(st) }
-func (st *TOStatus) Delete() (error, error, int) { return api.GenericDelete(st) }
+func (st *TOStatus) Update(h http.Header) (error, error, int) {
+ var statusName string
+ err := st.APIInfo().Tx.QueryRow(`SELECT name from status WHERE id = $1`, *st.ID).Scan(&statusName)
+ if err != nil {
+ return nil, fmt.Errorf("error querying status name from ID: %v", err), http.StatusInternalServerError
Review Comment:
errors constructed from errors should wrap with `%w`.
##########
traffic_ops/testing/api/v2/statuses_test.go:
##########
@@ -42,35 +50,44 @@ func CreateTestStatuses(t *testing.T) {
func UpdateTestStatuses(t *testing.T) {
- firstStatus := testData.Statuses[0]
- if firstStatus.Name == nil {
- t.Fatal("cannot update test statuses: first test data status must have a name")
- }
-
- // Retrieve the Status by name so we can get the id for the Update
- resp, _, err := TOSession.GetStatusByName(*firstStatus.Name)
- if err != nil {
- t.Errorf("cannot GET Status by name: %v - %v", firstStatus.Name, err)
- }
- remoteStatus := resp[0]
- expectedStatusDesc := "new description"
- remoteStatus.Description = expectedStatusDesc
- var alert tc.Alerts
- alert, _, err = TOSession.UpdateStatusByID(remoteStatus.ID, remoteStatus)
- if err != nil {
- t.Errorf("cannot UPDATE Status by id: %v - %v", err, alert)
- }
-
- // Retrieve the Status to check Status name got updated
- resp, _, err = TOSession.GetStatusByID(remoteStatus.ID)
- if err != nil {
- t.Errorf("cannot GET Status by ID: %v - %v", firstStatus.Description, err)
+ if len(testData.Statuses) < 1 {
+ t.Fatal("Need at least one Status to test updating a Status")
}
- respStatus := resp[0]
- if respStatus.Description != expectedStatusDesc {
- t.Errorf("results do not match actual: %s, expected: %s", respStatus.Name, expectedStatusDesc)
+ for _, status := range testData.Statuses {
+ if status.Name == nil {
+ t.Fatal("cannot update test statuses: test data status must have a name")
+ }
+ // Retrieve the Status by name so we can get the id for the Update
+ resp, _, err := TOSession.GetStatusByName(*status.Name)
+ if err != nil {
+ t.Errorf("cannot GET Status by name: %v - %v", status.Name, err)
Review Comment:
`status.Name` is a pointer to a string, so using `%v` to format it is going to not be very helpful since [`%v` formats `*string`s as memory addresses](https://go.dev/play/p/YEGLp3CQe7x). This is why types should always use the most specific formatting parameter they can; if you use `%s` here it will fail `go vet` and thus the test won't build, which lets you know immediately that it's not printing what it should be.
##########
traffic_ops/testing/api/v2/statuses_test.go:
##########
@@ -106,17 +123,43 @@ func DeleteTestStatuses(t *testing.T) {
respStatus := resp[0]
delResp, _, err := TOSession.DeleteStatusByID(respStatus.ID)
- if err != nil {
- t.Errorf("cannot DELETE Status by name: %v - %v", err, delResp)
+ if _, ok := statusNameMap[*status.Name]; !ok {
+ if err != nil {
+ t.Errorf("cannot DELETE Status by name: %v - %v", err, delResp)
+ }
+
+ // Retrieve the Status to see if it got deleted
+ types, _, err := TOSession.GetStatusByName(*status.Name)
+ if err != nil {
+ t.Errorf("error deleting Status name: %s", err.Error())
Review Comment:
nit: `.Error()` is redundant because `%s` formats errors as strings by calling their `Error` method.
--
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 merged pull request #6817: Prevent modifying and/or deleting reserved statuses
Posted by GitBox <gi...@apache.org>.
ocket8888 merged PR #6817:
URL: https://github.com/apache/trafficcontrol/pull/6817
--
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 a diff in pull request #6817: Prevent modifying and/or deleting reserved statuses
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6817:
URL: https://github.com/apache/trafficcontrol/pull/6817#discussion_r871838405
##########
cache-config/testing/ort-tests/tcdata/statuses.go:
##########
@@ -17,30 +17,14 @@ package tcdata
Review Comment:
Are the two functions in this file ever called anymore with all of the removals you did? I wouldn't think so, but I could be wrong.
--
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 a diff in pull request #6817: Prevent modifying and/or deleting reserved statuses
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6817:
URL: https://github.com/apache/trafficcontrol/pull/6817#discussion_r870660393
##########
traffic_ops/testing/api/v4/statuses_test.go:
##########
@@ -142,13 +145,26 @@ func GetTestStatusesIMS(t *testing.T) {
}
func CreateTestStatuses(t *testing.T) {
+ response, _, err := TOSession.GetStatuses(client.NewRequestOptions())
+ if err != nil {
+ t.Errorf("could not get statuses: %v", err)
+ }
+ statusNameMap := make(map[string]bool, 0)
+ for _, r := range response.Response {
+ statusNameMap[r.Name] = true
+ }
+
for _, status := range testData.Statuses {
- resp, _, err := TOSession.CreateStatus(status, client.RequestOptions{})
- if err != nil {
- t.Errorf("could not create Status: %v - alerts: %+v", err, resp.Alerts)
+ if status.Name != nil {
+ if _, ok := statusNameMap[*status.Name]; !ok {
+ resp, _, err := TOSession.CreateStatus(status, client.NewRequestOptions())
+ t.Log("Response: ", resp)
+ if err != nil {
+ t.Errorf("could not create Status: %v - alerts: %+v", err, resp.Alerts)
+ }
+ }
Review Comment:
You wouldn't need to build this map and do these checks if you just remove standard statuses from the testing fixture data.
##########
traffic_ops/traffic_ops_golang/status/statuses.go:
##########
@@ -40,6 +40,14 @@ type TOStatus struct {
tc.StatusNullable
}
+var statusNameMap = map[tc.CacheStatus]bool{
+ tc.CacheStatusAdminDown: true,
+ tc.CacheStatusOnline: true,
+ tc.CacheStatusOffline: true,
+ tc.CacheStatusReported: true,
+ tc.CacheStatusPreProd: true,
+}
+
Review Comment:
This map is not used
##########
lib/go-tc/statuses.go:
##########
@@ -70,3 +70,19 @@ type StatusNullable struct {
LastUpdated *TimeNoMod `json:"lastUpdated" db:"last_updated"`
Name *string `json:"name" db:"name"`
}
+
+func IsReservedStatus(status string) bool {
Review Comment:
This function could use a GoDoc.
--
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