Set server XMPPID to hostname if empty #5725

zrhoffman · 2021-04-08T23:04:38Z

What does this PR (Pull Request) do?

This PR fixes PUT /api/3.x/servers with no xmppId (hashId) results in an internal server error #5724 by allowing a Server to be updated when its XMPPID field is empty.

This PR also adds a migration to set the XMPPID to the hostname for each server in the database for which the xmpp_id field is empty.

Which Traffic Control components are affected by this PR?

Traffic Ops

What is the best way to verify this PR?

Run the TO API tests
Running an SQL query directly against the database, set the XMPPID for a server to NULL or an empty string, then try updating it using Traffic Portal

If this is a bug fix, what versions of Traffic Control are affected?

master (4e8e3ba)

The following criteria are ALL met by this PR

This PR includes tests
This PR contains documentation
This PR includes an update to CHANGELOG.md
This PR includes any and all required license headers
This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

traffic_ops/app/db/migrations/2021040819481469_generate_uuid_for_xmpp_id_where_empty.sql

traffic_ops/traffic_ops_golang/server/servers.go

mitchell852 · 2021-04-09T16:54:34Z

there is some incorrect documentation regarding XMPPID. want to fix that too? lots of this:

xmppId
An identifier to be used in XMPP communications with the server - in nearly all cases this will be the same as hostName

a better description may be something like:

xmppId
A poorly named property of a server used by Traffic Router for consistent hashing. On create, this is set to a UUID, however, legacy behaviour would use server hostname if this value was undefined.

^^ Pretty sure I got that wrong. Maybe @rawlinp has a better definition.

traffic_ops/traffic_ops_golang/server/servers.go

rawlinp · 2021-04-09T17:14:45Z

A poorly named property of a server used by Traffic Router for consistent hashing. On create, this is set to a UUID, however, legacy behaviour would use server hostname if this value was undefined.

This is pretty good, but I would add that it's an immutable property.

rimashah25 · 2021-04-09T19:07:48Z

there is some incorrect documentation regarding XMPPID. want to fix that too? lots of this:
xmppId
An identifier to be used in XMPP communications with the server - in nearly all cases this will be the same as hostName

Isn't that part of v1 docs.. soon to go away? I checked the codebase and couldn't find the above mention in any other documentation.

mitchell852 · 2021-04-09T19:48:44Z

there is some incorrect documentation regarding XMPPID. want to fix that too? lots of this:
xmppId
An identifier to be used in XMPP communications with the server - in nearly all cases this will be the same as hostName
Isn't that part of v1 docs.. soon to go away? I checked the codebase and couldn't find the above mention in any other documentation.

you are right @rimashah25 and it looks like it's better defined elsewhere:

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.

zrhoffman · 2021-04-09T20:28:32Z

there is some incorrect documentation regarding XMPPID. want to fix that too? lots of this:
xmppId
An identifier to be used in XMPP communications with the server - in nearly all cases this will be the same as hostName
Isn't that part of v1 docs.. soon to go away? I checked the codebase and couldn't find the above mention in any other documentation.
you are right @rimashah25 and it looks like it's better defined elsewhere:
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.

Using the newer XMPPID definition for API v1 in a20cdf0.

rawlinp

This looks good now, just nitpicking the changelog and title :)

rawlinp · 2021-04-09T20:33:37Z

CHANGELOG.md

@@ -47,6 +47,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - [#5405](https://github.com/apache/trafficcontrol/issues/5405) - Prevent Tenant update from choosing child as new parent
 - [#5384](https://github.com/apache/trafficcontrol/issues/5384) - New grids will now properly remember the current page number.
 - [#5548](https://github.com/apache/trafficcontrol/issues/5548) - Don't return a `403 Forbidden` when the user tries to get servers of a non-existent DS using `GET /servers?dsId={{nonexistent DS ID}}`
+- [#5724](https://github.com/apache/trafficcontrol/issues/5724) - Generate XMPPID on update if the server had none


Since this PR no longer generates UUIDs for XMPPID, we should probably say Set XMPPID to hostname if the server had none, don't error on server update when XMPPID is empty.

Also, would the title would make more sense now as "Set server XMPPID to hostname if empty"?

Updated PR title and updated changelog entry in 04f137b.

rawlinp · 2021-04-09T21:30:49Z

Also if this PR is considered for backporting to a release branch, the backport should not include the DB migration. The code fix alone should be sufficient for fixing the bug.

* Generate XMPPID on server update if it is empty * Hyphenate traffic-ops.yml * Simplify condition * Do not persistently set XMPPID on PUT if it is empty * Remove xmpp_id from SQL query * Include hostname in warning * Use newer definition of XMPPID for API v1 * More accurate changelog entry (cherry picked from commit f599571)

zrhoffman added 2 commits April 8, 2021 13:54

Generate XMPPID on server update if it is empty

0bc6299

Hyphenate traffic-ops.yml

26f7501

zrhoffman added bug something isn't working as intended Traffic Ops related to Traffic Ops labels Apr 8, 2021

rawlinp requested changes Apr 8, 2021

View reviewed changes

traffic_ops/app/db/migrations/2021040819481469_generate_uuid_for_xmpp_id_where_empty.sql Outdated Show resolved Hide resolved

zrhoffman added 2 commits April 8, 2021 17:23

Simplify condition

e151b91

Migration sets XMPPID to hostname if XMPPID is null

0cb43fe

rawlinp requested changes Apr 9, 2021

View reviewed changes

traffic_ops/traffic_ops_golang/server/servers.go Outdated Show resolved Hide resolved

Do not persistently set XMPPID on PUT if it is empty

cb1b997

zrhoffman requested a review from rawlinp April 9, 2021 16:47

rawlinp requested changes Apr 9, 2021

View reviewed changes

traffic_ops/traffic_ops_golang/server/servers.go Outdated Show resolved Hide resolved

traffic_ops/traffic_ops_golang/server/servers.go Outdated Show resolved Hide resolved

zrhoffman added 3 commits April 9, 2021 14:20

Remove xmpp_id from SQL query

68c51e3

Include hostname in warning

034ad01

Use newer definition of XMPPID for API v1

a20cdf0

zrhoffman requested a review from rawlinp April 9, 2021 20:29

rawlinp approved these changes Apr 9, 2021

View reviewed changes

zrhoffman changed the title ~~Generate XMPPID on server update if it is empty~~ Set server XMPPID to hostname if empty Apr 9, 2021

More accurate changelog entry

04f137b

rawlinp merged commit f599571 into apache:master Apr 9, 2021

zrhoffman deleted the generate-xmppid-if-empty branch April 9, 2021 22:04

zrhoffman mentioned this pull request Apr 9, 2021

Set server XMPPID to hostname if empty #5731

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set server XMPPID to hostname if empty #5725

Set server XMPPID to hostname if empty #5725

zrhoffman commented Apr 8, 2021 •

edited

Loading

mitchell852 commented Apr 9, 2021 •

edited

Loading

rawlinp commented Apr 9, 2021

rimashah25 commented Apr 9, 2021 •

edited

Loading

mitchell852 commented Apr 9, 2021

zrhoffman commented Apr 9, 2021

rawlinp left a comment

rawlinp Apr 9, 2021

zrhoffman Apr 9, 2021

rawlinp commented Apr 9, 2021

Set server XMPPID to hostname if empty #5725

Set server XMPPID to hostname if empty #5725

Conversation

zrhoffman commented Apr 8, 2021 • edited Loading

What does this PR (Pull Request) do?

Which Traffic Control components are affected by this PR?

What is the best way to verify this PR?

If this is a bug fix, what versions of Traffic Control are affected?

The following criteria are ALL met by this PR

mitchell852 commented Apr 9, 2021 • edited Loading

rawlinp commented Apr 9, 2021

rimashah25 commented Apr 9, 2021 • edited Loading

mitchell852 commented Apr 9, 2021

zrhoffman commented Apr 9, 2021

rawlinp left a comment

Choose a reason for hiding this comment

rawlinp Apr 9, 2021

Choose a reason for hiding this comment

zrhoffman Apr 9, 2021

Choose a reason for hiding this comment

rawlinp commented Apr 9, 2021

zrhoffman commented Apr 8, 2021 •

edited

Loading

mitchell852 commented Apr 9, 2021 •

edited

Loading

rimashah25 commented Apr 9, 2021 •

edited

Loading