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/07/13 18:14:51 UTC
[GitHub] [trafficcontrol] ericholguin opened a new pull request, #6959: Make role name mutable
ericholguin opened a new pull request, #6959:
URL: https://github.com/apache/trafficcontrol/pull/6959
<!--
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: #ISSUE
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.
-->
Related: #6955 #6925
This PR:
- Makes a role's name mutable.
- Fixes internal server error's that were occurring when a user would create or update a role with an already existing name.
- Matches TP validation by not allowing a user to create a role with spaces
- Increases test coverage for roles to match these changes.
<!-- **^ 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 Integration Tests
## 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. -->
- [ ] 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] ericholguin commented on a diff in pull request #6959: Make role name mutable
Posted by GitBox <gi...@apache.org>.
ericholguin commented on code in PR #6959:
URL: https://github.com/apache/trafficcontrol/pull/6959#discussion_r921267514
##########
lib/go-tc/roles.go:
##########
@@ -59,8 +59,9 @@ type RoleV40 struct {
// Validate will validate and make sure all that the fields in the supplied RoleV4 struct are semantically correct.
func (role RoleV4) Validate() error {
+ noSpaces := validation.NewStringRule(tovalidate.NoSpaces, "cannot contain spaces")
errs := validation.Errors{
- "name": validation.Validate(role.Name, validation.Required),
+ "name": validation.Validate(role.Name, validation.Required, noSpaces),
Review Comment:
Should that validation also be removed from TP?
--
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 #6959: Make role name mutable
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on PR #6959:
URL: https://github.com/apache/trafficcontrol/pull/6959#issuecomment-1184804075
@ocket8888 would this be considered a regression bug?
--
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 #6959: Make role name mutable
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on PR #6959:
URL: https://github.com/apache/trafficcontrol/pull/6959#issuecomment-1184837799
Since it affects 3.x I suppose so; intention doesn't matter because that version was supposed to be frozen.
--
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 #6959: Make role name mutable
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6959:
URL: https://github.com/apache/trafficcontrol/pull/6959#discussion_r922303770
##########
traffic_ops/testing/api/v3/roles_test.go:
##########
@@ -266,13 +377,18 @@ func CreateTestRoles(t *testing.T) {
}
func DeleteTestRoles(t *testing.T) {
- for _, r := range testData.Roles {
- roleID := GetRoleID(t, *r.Name)()
- _, _, _, err := TOSession.DeleteRoleByID(roleID)
- assert.NoError(t, err, "Expected no error while deleting role %s, but got %v", *r.Name, err)
+ roles, _, _, err := TOSession.GetRolesWithHdr(nil)
+ assert.NoError(t, err, "Cannot get Roles: %v", err)
+ for _, role := range roles {
+ // Don't delete active roles created by test setup
+ if *role.Name == "admin" || *role.Name == "disallowed" || *role.Name == "operations" || *role.Name == "portal" || *role.Name == "read-only" || *role.Name == "steering" || *role.Name == "federation" {
+ continue
+ }
+ _, _, _, err := TOSession.DeleteRoleByID(*role.ID)
+ assert.NoError(t, err, "Expected no error while deleting role %s, but got %v", role.Name, err)
// Retrieve the Role to see if it got deleted
- getRole, _, _, err := TOSession.GetRoleByIDWithHdr(roleID, nil)
- assert.NoError(t, err, "Error getting Role '%s' after deletion: %v", *r.Name, err)
- assert.Equal(t, 0, len(getRole), "Expected Role '%s' to be deleted, but it was found in Traffic Ops", *r.Name)
+ getRole, _, _, err := TOSession.GetRoleByIDWithHdr(*role.ID, nil)
+ assert.NoError(t, err, "Error getting Role '%s' after deletion: %v", role.Name, err)
+ assert.Equal(t, 0, len(getRole), "Expected Role '%s' to be deleted, but it was found in Traffic Ops", role.Name)
Review Comment:
It looks like using your new `assert.*` utilities is fooling `go vet`. These are formatting a `*string` (`role.Name`) with `%s`, which will fail to produce anything useful at runtime.
##########
traffic_ops/testing/api/v3/roles_test.go:
##########
@@ -266,13 +377,18 @@ func CreateTestRoles(t *testing.T) {
}
func DeleteTestRoles(t *testing.T) {
- for _, r := range testData.Roles {
- roleID := GetRoleID(t, *r.Name)()
- _, _, _, err := TOSession.DeleteRoleByID(roleID)
- assert.NoError(t, err, "Expected no error while deleting role %s, but got %v", *r.Name, err)
+ roles, _, _, err := TOSession.GetRolesWithHdr(nil)
+ assert.NoError(t, err, "Cannot get Roles: %v", err)
+ for _, role := range roles {
+ // Don't delete active roles created by test setup
+ if *role.Name == "admin" || *role.Name == "disallowed" || *role.Name == "operations" || *role.Name == "portal" || *role.Name == "read-only" || *role.Name == "steering" || *role.Name == "federation" {
Review Comment:
We should check for `nil` Name here so that if TO is broken the tests still clean up as many Roles as possible (and also probably `nil` ID since the same problem will occur below with that field).
##########
traffic_ops/traffic_ops_golang/role/roles.go:
##########
@@ -259,6 +259,13 @@ func (role *TORole) Update(h http.Header) (error, error, int) {
}
func (role *TORole) Delete() (error, error, int) {
+
+ if ok, err := dbhelpers.RoleExists(role.ReqInfo.Tx.Tx, *role.ID); err != nil {
+ return nil, errors.New("verifying Role exists: " + err.Error()), http.StatusInternalServerError
Review Comment:
same as above
##########
traffic_ops/traffic_ops_golang/role/roles.go:
##########
@@ -224,6 +217,13 @@ func (role *TORole) Read(h http.Header, useIMS bool) ([]interface{}, error, erro
}
func (role *TORole) Update(h http.Header) (error, error, int) {
+
+ if ok, err := dbhelpers.RoleExists(role.ReqInfo.Tx.Tx, *role.ID); err != nil {
+ return nil, errors.New("verifying Role exists: " + err.Error()), http.StatusInternalServerError
Review Comment:
Creating an error from an underlying error should use `fmt.Errorf`
##########
traffic_ops/testing/api/v4/roles_test.go:
##########
@@ -239,14 +338,20 @@ func CreateTestRoles(t *testing.T) {
}
func DeleteTestRoles(t *testing.T) {
- for _, r := range testData.Roles {
- _, _, err := TOSession.DeleteRole(r.Name, client.NewRequestOptions())
- assert.NoError(t, err, "Expected no error while deleting role %s, but got %v", r.Name, err)
+ roles, _, err := TOSession.GetRoles(client.RequestOptions{})
+ assert.NoError(t, err, "Cannot get Roles: %v - alerts: %+v", err, roles.Alerts)
+ for _, role := range roles.Response {
+ // Don't delete active roles created by test setup
+ if role.Name == "admin" || role.Name == "disallowed" || role.Name == "operations" || role.Name == "portal" || role.Name == "read-only" || role.Name == "steering" || role.Name == "federation" {
+ continue
+ }
+ _, _, err := TOSession.DeleteRole(role.Name, client.NewRequestOptions())
+ assert.NoError(t, err, "Expected no error while deleting role %s, but got %v", role.Name, err)
// Retrieve the Role to see if it got deleted
opts := client.NewRequestOptions()
- opts.QueryParameters.Set("name", r.Name)
+ opts.QueryParameters.Set("name", role.Name)
getRole, _, err := TOSession.GetRoles(opts)
- assert.NoError(t, err, "Error getting Role '%s' after deletion: %v - alerts: %+v", r.Name, err, getRole.Alerts)
- assert.Equal(t, 0, len(getRole.Response), "Expected Role '%s' to be deleted, but it was found in Traffic Ops", r.Name)
+ assert.NoError(t, err, "Error getting Role '%s' after deletion: %v - alerts: %+v", role.Name, err, getRole.Alerts)
+ assert.Equal(t, 0, len(getRole.Response), "Expected Role '%s' to be deleted, but it was found in Traffic Ops", role.Name)
Review Comment:
same comments apply 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.
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 #6959: Make role name mutable
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6959:
URL: https://github.com/apache/trafficcontrol/pull/6959#discussion_r922345327
##########
traffic_ops/testing/api/v4/roles_test.go:
##########
@@ -239,14 +338,20 @@ func CreateTestRoles(t *testing.T) {
}
func DeleteTestRoles(t *testing.T) {
- for _, r := range testData.Roles {
- _, _, err := TOSession.DeleteRole(r.Name, client.NewRequestOptions())
- assert.NoError(t, err, "Expected no error while deleting role %s, but got %v", r.Name, err)
+ roles, _, err := TOSession.GetRoles(client.RequestOptions{})
+ assert.NoError(t, err, "Cannot get Roles: %v - alerts: %+v", err, roles.Alerts)
+ for _, role := range roles.Response {
+ // Don't delete active roles created by test setup
+ if role.Name == "admin" || role.Name == "disallowed" || role.Name == "operations" || role.Name == "portal" || role.Name == "read-only" || role.Name == "steering" || role.Name == "federation" {
+ continue
+ }
+ _, _, err := TOSession.DeleteRole(role.Name, client.NewRequestOptions())
+ assert.NoError(t, err, "Expected no error while deleting role %s, but got %v", role.Name, err)
// Retrieve the Role to see if it got deleted
opts := client.NewRequestOptions()
- opts.QueryParameters.Set("name", r.Name)
+ opts.QueryParameters.Set("name", role.Name)
getRole, _, err := TOSession.GetRoles(opts)
- assert.NoError(t, err, "Error getting Role '%s' after deletion: %v - alerts: %+v", r.Name, err, getRole.Alerts)
- assert.Equal(t, 0, len(getRole.Response), "Expected Role '%s' to be deleted, but it was found in Traffic Ops", r.Name)
+ assert.NoError(t, err, "Error getting Role '%s' after deletion: %v - alerts: %+v", role.Name, err, getRole.Alerts)
+ assert.Equal(t, 0, len(getRole.Response), "Expected Role '%s' to be deleted, but it was found in Traffic Ops", role.Name)
Review Comment:
Ohhh. Good. I should really know that, since I'm quite sure I did that, and specifically so that you wouldn't have to do those checks I just asked for.
--
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 #6959: Make role name mutable
Posted by GitBox <gi...@apache.org>.
ocket8888 merged PR #6959:
URL: https://github.com/apache/trafficcontrol/pull/6959
--
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 #6959: Make role name mutable
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6959:
URL: https://github.com/apache/trafficcontrol/pull/6959#discussion_r922335623
##########
traffic_ops/testing/api/v4/roles_test.go:
##########
@@ -239,14 +338,20 @@ func CreateTestRoles(t *testing.T) {
}
func DeleteTestRoles(t *testing.T) {
- for _, r := range testData.Roles {
- _, _, err := TOSession.DeleteRole(r.Name, client.NewRequestOptions())
- assert.NoError(t, err, "Expected no error while deleting role %s, but got %v", r.Name, err)
+ roles, _, err := TOSession.GetRoles(client.RequestOptions{})
+ assert.NoError(t, err, "Cannot get Roles: %v - alerts: %+v", err, roles.Alerts)
+ for _, role := range roles.Response {
+ // Don't delete active roles created by test setup
+ if role.Name == "admin" || role.Name == "disallowed" || role.Name == "operations" || role.Name == "portal" || role.Name == "read-only" || role.Name == "steering" || role.Name == "federation" {
+ continue
+ }
+ _, _, err := TOSession.DeleteRole(role.Name, client.NewRequestOptions())
+ assert.NoError(t, err, "Expected no error while deleting role %s, but got %v", role.Name, err)
// Retrieve the Role to see if it got deleted
opts := client.NewRequestOptions()
- opts.QueryParameters.Set("name", r.Name)
+ opts.QueryParameters.Set("name", role.Name)
getRole, _, err := TOSession.GetRoles(opts)
- assert.NoError(t, err, "Error getting Role '%s' after deletion: %v - alerts: %+v", r.Name, err, getRole.Alerts)
- assert.Equal(t, 0, len(getRole.Response), "Expected Role '%s' to be deleted, but it was found in Traffic Ops", r.Name)
+ assert.NoError(t, err, "Error getting Role '%s' after deletion: %v - alerts: %+v", role.Name, err, getRole.Alerts)
+ assert.Equal(t, 0, len(getRole.Response), "Expected Role '%s' to be deleted, but it was found in Traffic Ops", role.Name)
Review Comment:
v4 test still has the same issues that you just addressed in v3
--
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] ericholguin commented on a diff in pull request #6959: Make role name mutable
Posted by GitBox <gi...@apache.org>.
ericholguin commented on code in PR #6959:
URL: https://github.com/apache/trafficcontrol/pull/6959#discussion_r922336778
##########
traffic_ops/testing/api/v4/roles_test.go:
##########
@@ -239,14 +338,20 @@ func CreateTestRoles(t *testing.T) {
}
func DeleteTestRoles(t *testing.T) {
- for _, r := range testData.Roles {
- _, _, err := TOSession.DeleteRole(r.Name, client.NewRequestOptions())
- assert.NoError(t, err, "Expected no error while deleting role %s, but got %v", r.Name, err)
+ roles, _, err := TOSession.GetRoles(client.RequestOptions{})
+ assert.NoError(t, err, "Cannot get Roles: %v - alerts: %+v", err, roles.Alerts)
+ for _, role := range roles.Response {
+ // Don't delete active roles created by test setup
+ if role.Name == "admin" || role.Name == "disallowed" || role.Name == "operations" || role.Name == "portal" || role.Name == "read-only" || role.Name == "steering" || role.Name == "federation" {
+ continue
+ }
+ _, _, err := TOSession.DeleteRole(role.Name, client.NewRequestOptions())
+ assert.NoError(t, err, "Expected no error while deleting role %s, but got %v", role.Name, err)
// Retrieve the Role to see if it got deleted
opts := client.NewRequestOptions()
- opts.QueryParameters.Set("name", r.Name)
+ opts.QueryParameters.Set("name", role.Name)
getRole, _, err := TOSession.GetRoles(opts)
- assert.NoError(t, err, "Error getting Role '%s' after deletion: %v - alerts: %+v", r.Name, err, getRole.Alerts)
- assert.Equal(t, 0, len(getRole.Response), "Expected Role '%s' to be deleted, but it was found in Traffic Ops", r.Name)
+ assert.NoError(t, err, "Error getting Role '%s' after deletion: %v - alerts: %+v", role.Name, err, getRole.Alerts)
+ assert.Equal(t, 0, len(getRole.Response), "Expected Role '%s' to be deleted, but it was found in Traffic Ops", role.Name)
Review Comment:
v4 doesnt use pointers
--
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 #6959: Make role name mutable
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6959:
URL: https://github.com/apache/trafficcontrol/pull/6959#discussion_r921279385
##########
lib/go-tc/roles.go:
##########
@@ -59,8 +59,9 @@ type RoleV40 struct {
// Validate will validate and make sure all that the fields in the supplied RoleV4 struct are semantically correct.
func (role RoleV4) Validate() error {
+ noSpaces := validation.NewStringRule(tovalidate.NoSpaces, "cannot contain spaces")
errs := validation.Errors{
- "name": validation.Validate(role.Name, validation.Required),
+ "name": validation.Validate(role.Name, validation.Required, noSpaces),
Review Comment:
Yes, TP should not be enforcing fake constraints
--
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 #6959: Make role name mutable
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6959:
URL: https://github.com/apache/trafficcontrol/pull/6959#discussion_r920737548
##########
lib/go-tc/roles.go:
##########
@@ -59,8 +59,9 @@ type RoleV40 struct {
// Validate will validate and make sure all that the fields in the supplied RoleV4 struct are semantically correct.
func (role RoleV4) Validate() error {
+ noSpaces := validation.NewStringRule(tovalidate.NoSpaces, "cannot contain spaces")
errs := validation.Errors{
- "name": validation.Validate(role.Name, validation.Required),
+ "name": validation.Validate(role.Name, validation.Required, noSpaces),
Review Comment:
Restricting names to not contain spaces is a breaking API change, and I don't think it's necessary to fix this bug.
--
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