You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/07/15 23:58:15 UTC
[GitHub] [trafficcontrol] rimashah25 opened a new pull request #4881: Bugfix/cdn 1973
rimashah25 opened a new pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881
## What does this PR (Pull Request) do?
- [x] This PR fixes #2156
## Which Traffic Control components are affected by this PR?
- Documentation
- Traffic Ops
## What is the best way to verify this PR?
1. Create a server in traffic_portal. Observe the xmpp_id value in db. It should be set to uuid.
2. Edit the hostname of the created server (from step 1)
3. Observe the change of hostname in DB but xmpp_id remains unaffected.
## If this is a bug fix, what versions of Traffic Control are affected?
- master
- 4.1.0
## The following criteria are ALL met by this PR
- [x] This PR includes tests
- [x] This PR includes documentation
- [x] This PR includes an update to CHANGELOG.md
- [x] This PR includes any and all required license headers
- [x] This PR ensures that database migration sequence is correct OR this PR does not include a database migration
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
## Additional Information
A new package, uuid, was added to traffic_ops_golang vendor folder.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-659638360
@rimashah25 - so you probably want to update the api docs that say:
```
xmppId: An identifier to be used in XMPP communications with the server - in nearly all cases this will be the same as hostName
```
to something like:
```
xmppId: A system-generated UUID used to generate a server hash id. This value is set when a server is created.
```
----------------------------------------------------------------
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 #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r458219401
##########
File path: traffic_ops/testing/api/v2/servers_test.go
##########
@@ -130,6 +136,33 @@ func UpdateTestServers(t *testing.T) {
t.Errorf("results do not match actual: %s, expected: %s", respServer.Rack, updatedServerRack)
}
+ //Check change in hostname with no change to xmppid
+ if originalHostname == respServer.HostName && originalXMPIDD == respServer.XMPPID {
+ t.Errorf("HostName didn't change. Expected: #{updatedHostName}, actual: #{originalHostname}")
+ }
+
+ //Check to verify XMPPID never gets updated
+ changeXMPPID := true
+ if changeXMPPID {
Review comment:
Won't this always be true?
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -984,6 +984,11 @@ func Update(w http.ResponseWriter, r *http.Request) {
}
defer inf.Close()
+ //Get original xmppid
+ origSer, _, _, _, _, _ := getServers(r.Header, inf.Params, inf.Tx, inf.User, false)
Review comment:
We should check for the errors here, and only dereference the `origSer` variable in the case when there was no error
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1055,52 +1065,56 @@ func Update(w http.ResponseWriter, r *http.Request) {
return
}
- rows, err := inf.Tx.NamedQuery(updateQuery, server)
- if err != nil {
- userErr, sysErr, errCode = api.ParseDBError(err)
- api.HandleErr(w, r, tx, errCode, userErr, sysErr)
- return
- }
- defer rows.Close()
+ if changeXMPPID {
+ api.WriteRespAlertObj(w, r, tc.ErrorLevel, fmt.Sprintf("server cannot be updated due to requested XMPPID change. XMPIDD is immutable"), nil)
Review comment:
You should return here, after `api.WriteRespAlertObj` because of two reasons:
1.) That way, you dont have to use the `else` block
2.) If you dont return, the `error.log` will complain about response being written twice
##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -321,6 +332,33 @@ func UpdateTestServers(t *testing.T) {
t.Fatalf("Cannot test server type change update; server '%s' had nil type ID", hostName)
}
+ //Check change in hostname with no change to xmppid
+ if originalHostname == *respServer.HostName && originalXMPIDD == *respServer.XMPPID {
+ t.Errorf("HostName didn't change. Expected: #{updatedHostName}, actual: #{originalHostname}")
+ }
+
+ //Check to verify XMPPID never gets updated
+ changeXMPPID := true
Review comment:
Same comment here
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rimashah25 commented on pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-659664368
> @rimashah25 - so you probably want to update the api docs that say:
>
> ```
> xmppId: An identifier to be used in XMPP communications with the server - in nearly all cases this will be the same as hostName
> ```
>
> to something like:
>
> ```
> xmppId: A system-generated UUID used to generate a server hash id for use in traffic router's consistent hashing algorithm. This value is set when a server is created and cannot be changed afterwards.
> ```
>
> or something like that.
@mitchell852: Updated the documentation wrt xmppId
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r455887366
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1224,6 +1228,9 @@ func createV3(inf *api.APIInfo, w http.ResponseWriter, r *http.Request) {
return
}
+ str := uuid.New().String()
Review comment:
so i guess what i'm getting at is this:
```
Create a server in traffic_portal. Observe the xmpp_id value in db. It should be set to uuid.
Edit the hostname of the created server (from step 1)
Observe the change of hostname in DB but xmpp_id remains unaffected.
```
but can i update the xmppId on a server via PUT /servers/{id}?
----------------------------------------------------------------
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] rimashah25 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r455926298
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1224,6 +1228,9 @@ func createV3(inf *api.APIInfo, w http.ResponseWriter, r *http.Request) {
return
}
+ str := uuid.New().String()
Review comment:
Thanks for the feedback, Jeremy. I have updated the PR to handle change in XMPPID, which would be ignored and ensure hashid remains unaffected.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r455887366
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1224,6 +1228,9 @@ func createV3(inf *api.APIInfo, w http.ResponseWriter, r *http.Request) {
return
}
+ str := uuid.New().String()
Review comment:
so i guess what i'm getting at is this:
```
Create a server in traffic_portal. Observe the xmpp_id value in db. It should be set to uuid.
Edit the hostname of the created server (from step 1)
Observe the change of hostname in DB but xmpp_id remains unaffected.
```
but can i update the xmppId (which drives the hashId) on a server via PUT /servers/{id}? because i _think_ that would be bad.
----------------------------------------------------------------
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 removed a comment on pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
srijeet0406 removed a comment on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-660334898
Noticing an issue in the following case:
1.) Create a server
2.) Change the host name
3.) The xmpp_id doesn't change -> so far it looks great
4.) Do a PUT to change the server's xmpp ID with a json payload that has a different xmpp ID than the one in the database
5.) The Change doesn't take place in the DB (which is the expected behavior), however, we get back a message saying that the server was updated with the new xmpp ID. This is what the response looks like:
`
{"alerts":[{"text":"Server updated","level":"success"}],"response":{"cachegroup":"edgeserver","cachegroupId":2,"cdnId":1,"cdnName":"ALL","domainName":"rima2","guid":null,"hostName":"rimatest2","httpsPort":443,"id":10,"iloIpAddress":null,"iloIpGateway":null,"iloIpNetmask":null,"iloPassword":null,"iloUsername":null,"lastUpdated":"2020-07-17 14:51:47-06","mgmtIpAddress":null,"mgmtIpGateway":null,"mgmtIpNetmask":null,"offlineReason":null,"physLocation":"phys2","physLocationId":2,"profile":"TRAFFIC_OPS","profileDesc":"Traffic Ops profile","profileId":3,"rack":null,"revalPending":false,"routerHostName":null,"routerPortName":null,"status":"ONLINE","statusId":2,"tcpPort":80,"type":"EDGE","typeId":11,"updPending":false,"xmppId":"5fa51f60-6606-4c9d-8b60-2d78f96018zf","xmppPasswd":null,"interfaces":[{"ipAddresses":[{"address":"192.23.27.67","gateway":"255.255.245.0","serviceAddress":true},{"address":"123.45.67.89","gateway":"255.255.255.0","serviceAddress":false}],"maxBandwidth":5,"monitor":
false,"mtu":1500,"name":"rima-interface"}]}}
`
The xmpp ID before this update was `5fa51f60-6606-4c9d-8b60-2d78f96018ae` in the database.
This remains the same even after the `PUT` (which is what we want), however, notice how the response says that the server was updated with the new xmpp ID of `5fa51f60-6606-4c9d-8b60-2d78f96018zf`. We do not want this. The user should be notified that the `server couldn't be updated because change of xmpp_id is not allowed`, or something to that effect.
----------------------------------------------------------------
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 #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r456556553
##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -248,6 +249,8 @@ func UpdateTestServers(t *testing.T) {
// Retrieve the server by hostname so we can get the id for the Update
resp, _, err := TOSession.GetServers(¶ms, nil)
+ originalHostname := *resp.Response[0].HostName
Review comment:
Same comment here.
##########
File path: traffic_ops/testing/api/v2/servers_test.go
##########
@@ -101,17 +101,23 @@ func UpdateTestServers(t *testing.T) {
hostName := firstServer.HostName
// Retrieve the server by hostname so we can get the id for the Update
resp, _, err := TOSession.GetServerByHostName(hostName)
+ originalHostname := resp[0].HostName
Review comment:
We should probably do this after we check that the error is nil. In case we dont get back a valid resp, this can throw a seg fault due to dereferencing of a nil ptr.
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -992,6 +992,9 @@ func Update(w http.ResponseWriter, r *http.Request) {
api.HandleErr(w, r, tx, http.StatusBadRequest, err, nil)
return
}
+ if newServer.XMPPID != nil {
+ fmt.Println("Change in xmpp_id was requested but it will be ignored to ensure hashId remains consistent")
Review comment:
Change to `log.Debugln` or something like that
----------------------------------------------------------------
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] rimashah25 commented on pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-662135882
> Since `xmppId` is always no longer the hostname, CDN-in-a-Box won't finish starting up, because for some reason we were [using xmppIds as hostnames](https://github.com/apache/trafficcontrol/blob/8e4a27b38/infrastructure/cdn-in-a-box/traffic_ops/to-access.sh#L335):
>
> ```shell
> current_servers_json=$(to-get 'api/'$TO_API_VERSION'/servers' 2>/dev/null | jq -c -e '[.response[] | .xmppId] | sort')
> ```
>
> Would you please change `.xmppId` there to `.hostName`?
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 edited a comment on pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
mitchell852 edited a comment on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-661321717
> Noticing an issue in the following case:
>
> 1.) Create a server
> 2.) Change the host name
> 3.) The xmpp_id doesn't change -> so far it looks great
> 4.) Do a PUT to change the server's xmpp ID with a json payload that has a different xmpp ID than the one in the database
> 5.) The Change doesn't take place in the DB (which is the expected behavior), however, we get back a message saying that the server was updated with the new xmpp ID. This is what the response looks like:
>
> `{"alerts":[{"text":"Server updated","level":"success"}],"response":{"cachegroup":"edgeserver","cachegroupId":2,"cdnId":1,"cdnName":"ALL","domainName":"rima2","guid":null,"hostName":"rimatest2","httpsPort":443,"id":10,"iloIpAddress":null,"iloIpGateway":null,"iloIpNetmask":null,"iloPassword":null,"iloUsername":null,"lastUpdated":"2020-07-17 14:51:47-06","mgmtIpAddress":null,"mgmtIpGateway":null,"mgmtIpNetmask":null,"offlineReason":null,"physLocation":"phys2","physLocationId":2,"profile":"TRAFFIC_OPS","profileDesc":"Traffic Ops profile","profileId":3,"rack":null,"revalPending":false,"routerHostName":null,"routerPortName":null,"status":"ONLINE","statusId":2,"tcpPort":80,"type":"EDGE","typeId":11,"updPending":false,"xmppId":"5fa51f60-6606-4c9d-8b60-2d78f96018zf","xmppPasswd":null,"interfaces":[{"ipAddresses":[{"address":"192.23.27.67","gateway":"255.255.245.0","serviceAddress":true},{"address":"123.45.67.89","gateway":"255.255.255.0","serviceAddress":false}],"maxBandwidth":5,"monito
r":false,"mtu":1500,"name":"rima-interface"}]}}`
>
> The xmpp ID before this update was `5fa51f60-6606-4c9d-8b60-2d78f96018ae` in the database.
> This remains the same even after the `PUT` (which is what we want), however, notice how the response says that the server was updated with the new xmpp ID of `5fa51f60-6606-4c9d-8b60-2d78f96018zf`. We do not want this. The user should be notified that the `server couldn't be updated because change of xmpp_id is not allowed`, or something to that effect.
I'm wondering if rather than simply ignoring the xmppId in the update query (which I believe is what it is doing) maybe it makes sense to return a 400 bad request if request.xmppId does not equal the xmpp_id found in the database for that server. i.e. if the user is trying to update xmppId, return 400 bad request with an alert of type error with a message like "xmppId cannot be changed".
@ocket8888 - what do you think? xmppId is now immutable. if the user tries to change it on PUT which of the following should happen?
----------------------------------------------------------------
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] rimashah25 commented on pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-661941606
> `{"alerts":[{"text":"Bad Request","level":"error"}]}`
>
> @Rima - can you add message to let the user know what was wrong with the request? i.e. `xmppId is immutable` or something like that?
@mitchell852 Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r456087045
##########
File path: traffic_ops/testing/api/v2/servers_test.go
##########
@@ -101,17 +101,23 @@ func UpdateTestServers(t *testing.T) {
hostName := firstServer.HostName
// Retrieve the server by hostname so we can get the id for the Update
resp, _, err := TOSession.GetServerByHostName(hostName)
+ originalHostname := resp[0].HostName
Review comment:
Test cases to test the following:
1. Change in hostname updated the field hostName but doesn't impact xmppid
2. Also, any change to xmppid, the Update() ignores the change.
Also, the original hostname is retained to ensure all the following test cases pass.
----------------------------------------------------------------
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 pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-660334898
Noticing an issue in the following case:
`
1.) Create a server
2.) Change the host name
3.) The xmpp_id doesn't change -> so far it looks great
4.) Do a PUT to change the server's xmpp ID with a json payload that has a different xmpp ID than the one in the database
5.) The Change doesn't take place in the DB (which is the expected behavior), however, we get back a message saying that the server was updated with the new xmpp ID. This is what the response looks like:
{"alerts":[{"text":"Server updated","level":"success"}],"response":{"cachegroup":"edgeserver","cachegroupId":2,"cdnId":1,"cdnName":"ALL","domainName":"rima2","guid":null,"hostName":"rimatest2","httpsPort":443,"id":10,"iloIpAddress":null,"iloIpGateway":null,"iloIpNetmask":null,"iloPassword":null,"iloUsername":null,"lastUpdated":"2020-07-17 14:51:47-06","mgmtIpAddress":null,"mgmtIpGateway":null,"mgmtIpNetmask":null,"offlineReason":null,"physLocation":"phys2","physLocationId":2,"profile":"TRAFFIC_OPS","profileDesc":"Traffic Ops profile","profileId":3,"rack":null,"revalPending":false,"routerHostName":null,"routerPortName":null,"status":"ONLINE","statusId":2,"tcpPort":80,"type":"EDGE","typeId":11,"updPending":false,"xmppId":"5fa51f60-6606-4c9d-8b60-2d78f96018zf","xmppPasswd":null,"interfaces":[{"ipAddresses":[{"address":"192.23.27.67","gateway":"255.255.245.0","serviceAddress":true},{"address":"123.45.67.89","gateway":"255.255.255.0","serviceAddress":false}],"maxBandwidth":5,"monitor":
false,"mtu":1500,"name":"rima-interface"}]}}
`
The xmpp ID before this update was `5fa51f60-6606-4c9d-8b60-2d78f96018ae` in the database.
This remains the same even after the `PUT` (which is what we want), however, notice how the response says that the server was updated with the new xmpp ID of `5fa51f60-6606-4c9d-8b60-2d78f96018zf`. We do not want this. The user should be notified that the `server couldn't be updated because change of xmpp_id is not allowed`, or something to that effect.
----------------------------------------------------------------
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] rimashah25 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r456088289
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -255,7 +256,6 @@ UPDATE server SET
tcp_port=:tcp_port,
type=:server_type_id,
upd_pending=:upd_pending,
- xmpp_id=:xmpp_id,
Review comment:
Removed xmppid update to avoid impacting hashId
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4881: Bugfix/cdn 1973
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r455880466
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1224,6 +1228,9 @@ func createV3(inf *api.APIInfo, w http.ResponseWriter, r *http.Request) {
return
}
+ str := uuid.New().String()
Review comment:
sorry, i just did a quick glance at the code. what happens on server update (PUT /servers/{id})? can the user change the XMPPID? If so, that seems bad and should be prevented if the XMPPID is driving the hashId value used in TR.
----------------------------------------------------------------
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] rimashah25 edited a comment on pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 edited a comment on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-661941606
> `{"alerts":[{"text":"Bad Request","level":"error"}]}`
>
> @Rima - can you add message to let the user know what was wrong with the request? i.e. `xmppId is immutable` or something like that?
@mitchell852 Done.
`{"alerts":[{"text":"server cannot be updated due to requested XMPPID change. XMPIDD is immutable","level":"error"}],"response":null}`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-661347756
To keep with our API pattern, the fields should only be ignored if we're gonna remove it from responses. Changing it would be an invalid action on a field we admit exists, so it should be a 4XX error.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r456593610
##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -248,6 +249,8 @@ func UpdateTestServers(t *testing.T) {
// Retrieve the server by hostname so we can get the id for the Update
resp, _, err := TOSession.GetServers(¶ms, nil)
+ originalHostname := *resp.Response[0].HostName
Review comment:
Completed the change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-661321717
> Noticing an issue in the following case:
>
> 1.) Create a server
> 2.) Change the host name
> 3.) The xmpp_id doesn't change -> so far it looks great
> 4.) Do a PUT to change the server's xmpp ID with a json payload that has a different xmpp ID than the one in the database
> 5.) The Change doesn't take place in the DB (which is the expected behavior), however, we get back a message saying that the server was updated with the new xmpp ID. This is what the response looks like:
>
> `{"alerts":[{"text":"Server updated","level":"success"}],"response":{"cachegroup":"edgeserver","cachegroupId":2,"cdnId":1,"cdnName":"ALL","domainName":"rima2","guid":null,"hostName":"rimatest2","httpsPort":443,"id":10,"iloIpAddress":null,"iloIpGateway":null,"iloIpNetmask":null,"iloPassword":null,"iloUsername":null,"lastUpdated":"2020-07-17 14:51:47-06","mgmtIpAddress":null,"mgmtIpGateway":null,"mgmtIpNetmask":null,"offlineReason":null,"physLocation":"phys2","physLocationId":2,"profile":"TRAFFIC_OPS","profileDesc":"Traffic Ops profile","profileId":3,"rack":null,"revalPending":false,"routerHostName":null,"routerPortName":null,"status":"ONLINE","statusId":2,"tcpPort":80,"type":"EDGE","typeId":11,"updPending":false,"xmppId":"5fa51f60-6606-4c9d-8b60-2d78f96018zf","xmppPasswd":null,"interfaces":[{"ipAddresses":[{"address":"192.23.27.67","gateway":"255.255.245.0","serviceAddress":true},{"address":"123.45.67.89","gateway":"255.255.255.0","serviceAddress":false}],"maxBandwidth":5,"monito
r":false,"mtu":1500,"name":"rima-interface"}]}}`
>
> The xmpp ID before this update was `5fa51f60-6606-4c9d-8b60-2d78f96018ae` in the database.
> This remains the same even after the `PUT` (which is what we want), however, notice how the response says that the server was updated with the new xmpp ID of `5fa51f60-6606-4c9d-8b60-2d78f96018zf`. We do not want this. The user should be notified that the `server couldn't be updated because change of xmpp_id is not allowed`, or something to that effect.
I'm wondering if rather than simply ignoring the xmppId in the update query (which I believe is what it is doing) maybe it makes sense to return a 400 bad request if request.xmppId does not equal the xmpp_id found in the database. i.e. if the user is trying to update xmppId, return 400 bad request with an alert of type error with a message like "xmppId cannot be changed".
----------------------------------------------------------------
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 edited a comment on pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
srijeet0406 edited a comment on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-660334898
Noticing an issue in the following case:
1.) Create a server
2.) Change the host name
3.) The xmpp_id doesn't change -> so far it looks great
4.) Do a PUT to change the server's xmpp ID with a json payload that has a different xmpp ID than the one in the database
5.) The Change doesn't take place in the DB (which is the expected behavior), however, we get back a message saying that the server was updated with the new xmpp ID. This is what the response looks like:
`
{"alerts":[{"text":"Server updated","level":"success"}],"response":{"cachegroup":"edgeserver","cachegroupId":2,"cdnId":1,"cdnName":"ALL","domainName":"rima2","guid":null,"hostName":"rimatest2","httpsPort":443,"id":10,"iloIpAddress":null,"iloIpGateway":null,"iloIpNetmask":null,"iloPassword":null,"iloUsername":null,"lastUpdated":"2020-07-17 14:51:47-06","mgmtIpAddress":null,"mgmtIpGateway":null,"mgmtIpNetmask":null,"offlineReason":null,"physLocation":"phys2","physLocationId":2,"profile":"TRAFFIC_OPS","profileDesc":"Traffic Ops profile","profileId":3,"rack":null,"revalPending":false,"routerHostName":null,"routerPortName":null,"status":"ONLINE","statusId":2,"tcpPort":80,"type":"EDGE","typeId":11,"updPending":false,"xmppId":"5fa51f60-6606-4c9d-8b60-2d78f96018zf","xmppPasswd":null,"interfaces":[{"ipAddresses":[{"address":"192.23.27.67","gateway":"255.255.245.0","serviceAddress":true},{"address":"123.45.67.89","gateway":"255.255.255.0","serviceAddress":false}],"maxBandwidth":5,"monitor":
false,"mtu":1500,"name":"rima-interface"}]}}
`
The xmpp ID before this update was `5fa51f60-6606-4c9d-8b60-2d78f96018ae` in the database.
This remains the same even after the `PUT` (which is what we want), however, notice how the response says that the server was updated with the new xmpp ID of `5fa51f60-6606-4c9d-8b60-2d78f96018zf`. We do not want this. The user should be notified that the `server couldn't be updated because change of xmpp_id is not allowed`, or something to that effect.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 edited a comment on pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
mitchell852 edited a comment on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-659638360
@rimashah25 - so you probably want to update the api docs that say:
```
xmppId: An identifier to be used in XMPP communications with the server - in nearly all cases this will be the same as hostName
```
to something like:
```
xmppId: A system-generated UUID used to generate a server hash id for use in traffic router's consistent hashing algorithm. This value is set when a server is created and cannot be changed afterwards.
```
or something like that.
----------------------------------------------------------------
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] rimashah25 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r456087045
##########
File path: traffic_ops/testing/api/v2/servers_test.go
##########
@@ -101,17 +101,23 @@ func UpdateTestServers(t *testing.T) {
hostName := firstServer.HostName
// Retrieve the server by hostname so we can get the id for the Update
resp, _, err := TOSession.GetServerByHostName(hostName)
+ originalHostname := resp[0].HostName
Review comment:
Test cases to test the following:
1. Change in hostname updated the field hostName but doesn't impact xmppid
2. Also, any change to xmppid, the Update() ignores the change.
----------------------------------------------------------------
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] rimashah25 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r456088018
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -47,6 +47,7 @@ import (
"github.com/go-ozzo/ozzo-validation"
"github.com/go-ozzo/ozzo-validation/is"
+ "github.com/google/uuid"
Review comment:
Using UUID to set xmppid to a random uuid
----------------------------------------------------------------
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] rimashah25 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r458237275
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -984,6 +984,11 @@ func Update(w http.ResponseWriter, r *http.Request) {
}
defer inf.Close()
+ //Get original xmppid
+ origSer, _, _, _, _, _ := getServers(r.Header, inf.Params, inf.Tx, inf.User, false)
Review comment:
Added error check
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-661893152
`{"alerts":[{"text":"Bad Request","level":"error"}]}`
@rima - can you add message to let the user know what was wrong with the request? i.e. `xmppId is immutable` or something like that?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r455887366
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1224,6 +1228,9 @@ func createV3(inf *api.APIInfo, w http.ResponseWriter, r *http.Request) {
return
}
+ str := uuid.New().String()
Review comment:
so i guess what i'm getting at is this:
```
Create a server in traffic_portal. Observe the xmpp_id value in db. It should be set to uuid.
Edit the hostname of the created server (from step 1)
Observe the change of hostname in DB but xmpp_id remains unaffected.
```
but can i update the xmppId (which drives the hashId) on a server via PUT /servers/{id}?
----------------------------------------------------------------
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] rimashah25 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r456593511
##########
File path: traffic_ops/testing/api/v2/servers_test.go
##########
@@ -101,17 +101,23 @@ func UpdateTestServers(t *testing.T) {
hostName := firstServer.HostName
// Retrieve the server by hostname so we can get the id for the Update
resp, _, err := TOSession.GetServerByHostName(hostName)
+ originalHostname := resp[0].HostName
Review comment:
Sounds good. Changed it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r458405772
##########
File path: docs/source/api/v2/servers.rst
##########
@@ -244,7 +244,7 @@ Request Structure
:typeId: The integral, unique identifier of the 'type' of this server
:updPending: A boolean value which, if ``true``, indicates that the server has updates of some kind pending, typically to be acted upon by Traffic Ops ORT
-:xmppId: An optional identifier to be used in XMPP communications with the server - in nearly all cases this should be the same as ``hostName``
+:xmppId: A system-generated UUID used to generate a server hashId for use in traffic router's consistent hashing algorithm. This value is set when a server is created and cannot be changed afterwards.
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r456596498
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -992,6 +992,9 @@ func Update(w http.ResponseWriter, r *http.Request) {
api.HandleErr(w, r, tx, http.StatusBadRequest, err, nil)
return
}
+ if newServer.XMPPID != nil {
+ fmt.Println("Change in xmpp_id was requested but it will be ignored to ensure hashId remains consistent")
Review comment:
Changed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 merged pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
mitchell852 merged pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881
----------------------------------------------------------------
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] rimashah25 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r458237912
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1055,52 +1065,56 @@ func Update(w http.ResponseWriter, r *http.Request) {
return
}
- rows, err := inf.Tx.NamedQuery(updateQuery, server)
- if err != nil {
- userErr, sysErr, errCode = api.ParseDBError(err)
- api.HandleErr(w, r, tx, errCode, userErr, sysErr)
- return
- }
- defer rows.Close()
+ if changeXMPPID {
+ api.WriteRespAlertObj(w, r, tc.ErrorLevel, fmt.Sprintf("server cannot be updated due to requested XMPPID change. XMPIDD is immutable"), nil)
Review comment:
:) Simplified it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r458237103
##########
File path: traffic_ops/testing/api/v2/servers_test.go
##########
@@ -130,6 +136,33 @@ func UpdateTestServers(t *testing.T) {
t.Errorf("results do not match actual: %s, expected: %s", respServer.Rack, updatedServerRack)
}
+ //Check change in hostname with no change to xmppid
+ if originalHostname == respServer.HostName && originalXMPIDD == respServer.XMPPID {
+ t.Errorf("HostName didn't change. Expected: #{updatedHostName}, actual: #{originalHostname}")
+ }
+
+ //Check to verify XMPPID never gets updated
+ changeXMPPID := true
+ if changeXMPPID {
Review comment:
Changed.
##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -321,6 +332,33 @@ func UpdateTestServers(t *testing.T) {
t.Fatalf("Cannot test server type change update; server '%s' had nil type ID", hostName)
}
+ //Check change in hostname with no change to xmppid
+ if originalHostname == *respServer.HostName && originalXMPIDD == *respServer.XMPPID {
+ t.Errorf("HostName didn't change. Expected: #{updatedHostName}, actual: #{originalHostname}")
+ }
+
+ //Check to verify XMPPID never gets updated
+ changeXMPPID := true
Review comment:
Changed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 edited a comment on pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
mitchell852 edited a comment on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-661321717
> Noticing an issue in the following case:
>
> 1.) Create a server
> 2.) Change the host name
> 3.) The xmpp_id doesn't change -> so far it looks great
> 4.) Do a PUT to change the server's xmpp ID with a json payload that has a different xmpp ID than the one in the database
> 5.) The Change doesn't take place in the DB (which is the expected behavior), however, we get back a message saying that the server was updated with the new xmpp ID. This is what the response looks like:
>
> `{"alerts":[{"text":"Server updated","level":"success"}],"response":{"cachegroup":"edgeserver","cachegroupId":2,"cdnId":1,"cdnName":"ALL","domainName":"rima2","guid":null,"hostName":"rimatest2","httpsPort":443,"id":10,"iloIpAddress":null,"iloIpGateway":null,"iloIpNetmask":null,"iloPassword":null,"iloUsername":null,"lastUpdated":"2020-07-17 14:51:47-06","mgmtIpAddress":null,"mgmtIpGateway":null,"mgmtIpNetmask":null,"offlineReason":null,"physLocation":"phys2","physLocationId":2,"profile":"TRAFFIC_OPS","profileDesc":"Traffic Ops profile","profileId":3,"rack":null,"revalPending":false,"routerHostName":null,"routerPortName":null,"status":"ONLINE","statusId":2,"tcpPort":80,"type":"EDGE","typeId":11,"updPending":false,"xmppId":"5fa51f60-6606-4c9d-8b60-2d78f96018zf","xmppPasswd":null,"interfaces":[{"ipAddresses":[{"address":"192.23.27.67","gateway":"255.255.245.0","serviceAddress":true},{"address":"123.45.67.89","gateway":"255.255.255.0","serviceAddress":false}],"maxBandwidth":5,"monito
r":false,"mtu":1500,"name":"rima-interface"}]}}`
>
> The xmpp ID before this update was `5fa51f60-6606-4c9d-8b60-2d78f96018ae` in the database.
> This remains the same even after the `PUT` (which is what we want), however, notice how the response says that the server was updated with the new xmpp ID of `5fa51f60-6606-4c9d-8b60-2d78f96018zf`. We do not want this. The user should be notified that the `server couldn't be updated because change of xmpp_id is not allowed`, or something to that effect.
I'm wondering if rather than simply ignoring the xmppId in the update query (which I believe is what it is doing) maybe it makes sense to return a 400 bad request if request.xmppId does not equal the xmpp_id found in the database for that server. i.e. if the user is trying to update xmppId, return 400 bad request with an alert of type error with a message like "xmppId cannot be changed".
----------------------------------------------------------------
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] rimashah25 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r458406081
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1055,6 +1069,11 @@ func Update(w http.ResponseWriter, r *http.Request) {
return
}
+ if changeXMPPID {
+ api.WriteRespAlertObj(w, r, tc.ErrorLevel, fmt.Sprintf("server cannot be updated due to requested XMPPID change. XMPIDD is immutable"), nil)
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 edited a comment on pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
mitchell852 edited a comment on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-659638360
@rimashah25 - so you probably want to update the api docs that say:
```
xmppId: An identifier to be used in XMPP communications with the server - in nearly all cases this will be the same as hostName
```
to something like:
```
xmppId: A system-generated UUID used to generate a server hash id. This value is set when a server is created and cannot be changed afterwards.
```
or something like that.
----------------------------------------------------------------
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] rimashah25 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r456087462
##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -248,6 +249,8 @@ func UpdateTestServers(t *testing.T) {
// Retrieve the server by hostname so we can get the id for the Update
resp, _, err := TOSession.GetServers(¶ms, nil)
+ originalHostname := *resp.Response[0].HostName
Review comment:
Test cases to test the following:
1. Change in hostname updated the field hostName but doesn't impact xmppid
2. Also, any change to xmppid, the Update() ignores the change.
Also, the original hostname is retained to ensure all the following test cases pass.
##########
File path: traffic_ops/testing/api/v3/servers_test.go
##########
@@ -248,6 +249,8 @@ func UpdateTestServers(t *testing.T) {
// Retrieve the server by hostname so we can get the id for the Update
resp, _, err := TOSession.GetServers(¶ms, nil)
+ originalHostname := *resp.Response[0].HostName
Review comment:
Test cases to test the following:
1. Change in hostname updated the field hostName but doesn't impact xmppid
2. Also, any change to xmppid, the Update() ignores the change.
Also, the original hostname is retained to ensure all the following test cases pass.
----------------------------------------------------------------
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 edited a comment on pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
srijeet0406 edited a comment on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-660334898
Noticing an issue in the following case:
`
1.) Create a server
2.) Change the host name
3.) The xmpp_id doesn't change -> so far it looks great
4.) Do a PUT to change the server's xmpp ID with a json payload that has a different xmpp ID than the one in the database
5.) The Change doesn't take place in the DB (which is the expected behavior), however, we get back a message saying that the server was updated with the new xmpp ID. This is what the response looks like:`
`
{"alerts":[{"text":"Server updated","level":"success"}],"response":{"cachegroup":"edgeserver","cachegroupId":2,"cdnId":1,"cdnName":"ALL","domainName":"rima2","guid":null,"hostName":"rimatest2","httpsPort":443,"id":10,"iloIpAddress":null,"iloIpGateway":null,"iloIpNetmask":null,"iloPassword":null,"iloUsername":null,"lastUpdated":"2020-07-17 14:51:47-06","mgmtIpAddress":null,"mgmtIpGateway":null,"mgmtIpNetmask":null,"offlineReason":null,"physLocation":"phys2","physLocationId":2,"profile":"TRAFFIC_OPS","profileDesc":"Traffic Ops profile","profileId":3,"rack":null,"revalPending":false,"routerHostName":null,"routerPortName":null,"status":"ONLINE","statusId":2,"tcpPort":80,"type":"EDGE","typeId":11,"updPending":false,"xmppId":"5fa51f60-6606-4c9d-8b60-2d78f96018zf","xmppPasswd":null,"interfaces":[{"ipAddresses":[{"address":"192.23.27.67","gateway":"255.255.245.0","serviceAddress":true},{"address":"123.45.67.89","gateway":"255.255.255.0","serviceAddress":false}],"maxBandwidth":5,"monitor":
false,"mtu":1500,"name":"rima-interface"}]}}
`
The xmpp ID before this update was `5fa51f60-6606-4c9d-8b60-2d78f96018ae` in the database.
This remains the same even after the `PUT` (which is what we want), however, notice how the response says that the server was updated with the new xmpp ID of `5fa51f60-6606-4c9d-8b60-2d78f96018zf`. We do not want this. The user should be notified that the `server couldn't be updated because change of xmpp_id is not allowed`, or something to that effect.
----------------------------------------------------------------
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] rimashah25 commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r458405873
##########
File path: docs/source/api/v2/servers_id.rst
##########
@@ -183,7 +183,7 @@ Response Structure
:type: The name of the 'type' of this server
:typeId: The integral, unique identifier of the 'type' of this server
:updPending: A boolean value which, if ``true``, indicates that the server has updates of some kind pending, typically to be acted upon by Traffic Ops ORT
-:xmppId: An identifier to be used in XMPP communications with the server - in nearly all cases this will be the same as ``hostName``
+:xmppId: A system-generated UUID used to generate a server hashId for use in traffic router's consistent hashing algorithm. This value is set when a server is created and cannot be changed afterwards.
Review comment:
Done
##########
File path: docs/source/api/v3/servers.rst
##########
@@ -287,7 +287,7 @@ Request Structure
:typeId: The integral, unique identifier of the 'type' of this server
:updPending: A boolean value which, if ``true``, indicates that the server has updates of some kind pending, typically to be acted upon by Traffic Ops ORT
-:xmppId: An optional identifier to be used in XMPP communications with the server - in nearly all cases this should be the same as ``hostName``
+:xmppId: A system-generated UUID used to generate a server hashId for use in traffic router's consistent hashing algorithm. This value is set when a server is created and cannot be changed afterwards.
Review comment:
Done
##########
File path: docs/source/api/v3/servers_id.rst
##########
@@ -94,7 +94,7 @@ Request Structure
:typeId: The integral, unique identifier of the 'type' of this server
:updPending: A boolean value which, if ``true``, indicates that the server has updates of some kind pending, typically to be acted upon by Traffic Ops ORT
-:xmppId: An optional identifier to be used in XMPP communications with the server - in nearly all cases this should be the same as ``hostName``
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rimashah25 commented on pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-661438440
> Noticing an issue in the following case:
>
> 1.) Create a server
> 2.) Change the host name
> 3.) The xmpp_id doesn't change -> so far it looks great
> 4.) Do a PUT to change the server's xmpp ID with a json payload that has a different xmpp ID than the one in the database
> 5.) The Change doesn't take place in the DB (which is the expected behavior), however, we get back a message saying that the server was updated with the new xmpp ID. This is what the response looks like:
>
> `{"alerts":[{"text":"Server updated","level":"success"}],"response":{"cachegroup":"edgeserver","cachegroupId":2,"cdnId":1,"cdnName":"ALL","domainName":"rima2","guid":null,"hostName":"rimatest2","httpsPort":443,"id":10,"iloIpAddress":null,"iloIpGateway":null,"iloIpNetmask":null,"iloPassword":null,"iloUsername":null,"lastUpdated":"2020-07-17 14:51:47-06","mgmtIpAddress":null,"mgmtIpGateway":null,"mgmtIpNetmask":null,"offlineReason":null,"physLocation":"phys2","physLocationId":2,"profile":"TRAFFIC_OPS","profileDesc":"Traffic Ops profile","profileId":3,"rack":null,"revalPending":false,"routerHostName":null,"routerPortName":null,"status":"ONLINE","statusId":2,"tcpPort":80,"type":"EDGE","typeId":11,"updPending":false,"xmppId":"5fa51f60-6606-4c9d-8b60-2d78f96018zf","xmppPasswd":null,"interfaces":[{"ipAddresses":[{"address":"192.23.27.67","gateway":"255.255.245.0","serviceAddress":true},{"address":"123.45.67.89","gateway":"255.255.255.0","serviceAddress":false}],"maxBandwidth":5,"monito
r":false,"mtu":1500,"name":"rima-interface"}]}}`
>
> The xmpp ID before this update was `5fa51f60-6606-4c9d-8b60-2d78f96018ae` in the database.
> This remains the same even after the `PUT` (which is what we want), however, notice how the response says that the server was updated with the new xmpp ID of `5fa51f60-6606-4c9d-8b60-2d78f96018zf`. We do not want this. The user should be notified that the `server couldn't be updated because change of xmpp_id is not allowed`, or something to that effect.
@mitchell852 @srijeet0406: I have updated the PR to reflect the changes based on this comment.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 edited a comment on pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
mitchell852 edited a comment on pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#issuecomment-661321717
> Noticing an issue in the following case:
>
> 1.) Create a server
> 2.) Change the host name
> 3.) The xmpp_id doesn't change -> so far it looks great
> 4.) Do a PUT to change the server's xmpp ID with a json payload that has a different xmpp ID than the one in the database
> 5.) The Change doesn't take place in the DB (which is the expected behavior), however, we get back a message saying that the server was updated with the new xmpp ID. This is what the response looks like:
>
> `{"alerts":[{"text":"Server updated","level":"success"}],"response":{"cachegroup":"edgeserver","cachegroupId":2,"cdnId":1,"cdnName":"ALL","domainName":"rima2","guid":null,"hostName":"rimatest2","httpsPort":443,"id":10,"iloIpAddress":null,"iloIpGateway":null,"iloIpNetmask":null,"iloPassword":null,"iloUsername":null,"lastUpdated":"2020-07-17 14:51:47-06","mgmtIpAddress":null,"mgmtIpGateway":null,"mgmtIpNetmask":null,"offlineReason":null,"physLocation":"phys2","physLocationId":2,"profile":"TRAFFIC_OPS","profileDesc":"Traffic Ops profile","profileId":3,"rack":null,"revalPending":false,"routerHostName":null,"routerPortName":null,"status":"ONLINE","statusId":2,"tcpPort":80,"type":"EDGE","typeId":11,"updPending":false,"xmppId":"5fa51f60-6606-4c9d-8b60-2d78f96018zf","xmppPasswd":null,"interfaces":[{"ipAddresses":[{"address":"192.23.27.67","gateway":"255.255.245.0","serviceAddress":true},{"address":"123.45.67.89","gateway":"255.255.255.0","serviceAddress":false}],"maxBandwidth":5,"monito
r":false,"mtu":1500,"name":"rima-interface"}]}}`
>
> The xmpp ID before this update was `5fa51f60-6606-4c9d-8b60-2d78f96018ae` in the database.
> This remains the same even after the `PUT` (which is what we want), however, notice how the response says that the server was updated with the new xmpp ID of `5fa51f60-6606-4c9d-8b60-2d78f96018zf`. We do not want this. The user should be notified that the `server couldn't be updated because change of xmpp_id is not allowed`, or something to that effect.
I'm wondering if rather than simply ignoring the xmppId in the update query (which I believe is what it is doing) maybe it makes sense to return a 400 bad request if request.xmppId does not equal the xmpp_id found in the database for that server. i.e. if the user is trying to update xmppId, return 400 bad request with an alert of type error with a message like "xmppId cannot be changed".
@ocket8888 - what do you think? xmppId is now immutable. if the user tries to change it on PUT which of the following should happen?
- 400 bad request with error alert along the lines of xmppId is immutable or
- simply ignore the xmppId in the update query and continue w/ the update?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4881: Renaming a host in TC, does not impact hash_id
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4881:
URL: https://github.com/apache/trafficcontrol/pull/4881#discussion_r458364707
##########
File path: docs/source/api/v3/servers.rst
##########
@@ -287,7 +287,7 @@ Request Structure
:typeId: The integral, unique identifier of the 'type' of this server
:updPending: A boolean value which, if ``true``, indicates that the server has updates of some kind pending, typically to be acted upon by Traffic Ops ORT
-:xmppId: An optional identifier to be used in XMPP communications with the server - in nearly all cases this should be the same as ``hostName``
+:xmppId: A system-generated UUID used to generate a server hashId for use in traffic router's consistent hashing algorithm. This value is set when a server is created and cannot be changed afterwards.
Review comment:
Same as above, `traffic router` should be capitalized.
##########
File path: docs/source/api/v2/servers.rst
##########
@@ -244,7 +244,7 @@ Request Structure
:typeId: The integral, unique identifier of the 'type' of this server
:updPending: A boolean value which, if ``true``, indicates that the server has updates of some kind pending, typically to be acted upon by Traffic Ops ORT
-:xmppId: An optional identifier to be used in XMPP communications with the server - in nearly all cases this should be the same as ``hostName``
+:xmppId: A system-generated UUID used to generate a server hashId for use in traffic router's consistent hashing algorithm. This value is set when a server is created and cannot be changed afterwards.
Review comment:
`traffic router` should be capitalized.
##########
File path: docs/source/api/v2/servers_id.rst
##########
@@ -183,7 +183,7 @@ Response Structure
:type: The name of the 'type' of this server
:typeId: The integral, unique identifier of the 'type' of this server
:updPending: A boolean value which, if ``true``, indicates that the server has updates of some kind pending, typically to be acted upon by Traffic Ops ORT
-:xmppId: An identifier to be used in XMPP communications with the server - in nearly all cases this will be the same as ``hostName``
+:xmppId: A system-generated UUID used to generate a server hashId for use in traffic router's consistent hashing algorithm. This value is set when a server is created and cannot be changed afterwards.
Review comment:
Same as above, `traffic router` should be capitalized.
##########
File path: docs/source/api/v3/servers_id.rst
##########
@@ -94,7 +94,7 @@ Request Structure
:typeId: The integral, unique identifier of the 'type' of this server
:updPending: A boolean value which, if ``true``, indicates that the server has updates of some kind pending, typically to be acted upon by Traffic Ops ORT
-:xmppId: An optional identifier to be used in XMPP communications with the server - in nearly all cases this should be the same as ``hostName``
Review comment:
Same as above, `traffic router` should be capitalized.
##########
File path: traffic_ops/traffic_ops_golang/server/servers.go
##########
@@ -1055,6 +1069,11 @@ func Update(w http.ResponseWriter, r *http.Request) {
return
}
+ if changeXMPPID {
+ api.WriteRespAlertObj(w, r, tc.ErrorLevel, fmt.Sprintf("server cannot be updated due to requested XMPPID change. XMPIDD is immutable"), nil)
Review comment:
Now that this is using `api.WriteRespAlertObj()`, I get a 200-level response code. Since we're not writing an object in this case, just `api.WriteAlerts()` should work.
----------------------------------------------------------------
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