srijeet0406 commented on code in PR #7065:
URL: https://github.com/apache/trafficcontrol/pull/7065#discussion_r972184353
##########
traffic_ops/testing/api/v4/serverupdatestatus_test.go:
##########
@@ -29,6 +29,119 @@ import (
client "github.com/apache/trafficcontrol/traffic_ops/v4-client"
)
+func TestServerUpdateApplyTimeLocked(t *testing.T) {
+ WithObjs(t, []TCObj{CDNs, Types, Tenants, Parameters, Profiles,
Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers,
ServiceCategories, Topologies, DeliveryServices}, func() {
+ opts := client.NewRequestOptions()
+ opts.QueryParameters.Set("hostName", "atlanta-edge-01")
+ resp, _, err := TOSession.GetServers(opts)
+ if err != nil {
+ t.Fatalf("cannot get server by hostname: %v", err)
+ }
+ if len(resp.Response) != 1 {
+ t.Fatalf("Expected a server named 'atlanta-edge-01' to
exist")
+ }
+ testServer := resp.Response[0]
+
+ testVals := func(configApply, revalApply *time.Time) {
+ resp, _, err := TOSession.GetServers(opts)
+ if err != nil {
+ t.Errorf("cannot get Server by name '%s': %v -
alerts: %+v", *testServer.HostName, err, resp.Alerts)
+ } else if len(resp.Response) != 1 {
+ t.Fatalf("GET Server expected 1, actual %v",
len(resp.Response))
+ }
+
+ beforeServer := resp.Response[0]
+
+ // Ensure baseline
+ if beforeServer.UpdPending == nil {
+ t.Fatalf("Server '%s' had nil UpdPending before
update status change", *testServer.HostName)
Review Comment:
nit, these errors should begin with a lowercase letter.
##########
traffic_ops/traffic_ops_golang/server/update.go:
##########
@@ -296,10 +296,19 @@ func UpdateHandlerV4(w http.ResponseWriter, r
*http.Request) {
api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError,
nil, err)
return
}
- userErr, sysErr, statusCode :=
dbhelpers.CheckIfCurrentUserHasCdnLock(inf.Tx.Tx, string(cdnName),
inf.User.UserName)
- if userErr != nil || sysErr != nil {
- api.HandleErr(w, r, inf.Tx.Tx, statusCode, userErr, sysErr)
- return
+
+ _, hasConfigUpdatedBoolParam := inf.Params["updated"]
+ _, hasRevalUpdatedBoolParam := inf.Params["reval_updated"]
+ _, hasConfigApplyTimeParam := inf.Params["config_apply_time"]
+ _, hasRevalidateApplyTimeParam := inf.Params["revalidate_apply_time"]
+ // Allow `apply_time` changes when the CDN is locked, but not `updated`
+ canIgnoreLock := (hasConfigApplyTimeParam ||
hasRevalidateApplyTimeParam) && !hasConfigUpdatedBoolParam &&
!hasRevalUpdatedBoolParam
+ if !canIgnoreLock {
+ userDoesntHaveLockErr, sysErr, statusCode :=
dbhelpers.CheckIfCurrentUserHasCdnLock(inf.Tx.Tx, string(cdnName),
inf.User.UserName)
+ if sysErr != nil || (userDoesntHaveLockErr != nil &&
!canIgnoreLock) {
Review Comment:
You don't need the `canIgnoreLock` check here again.
##########
traffic_ops/testing/api/v4/serverupdatestatus_test.go:
##########
@@ -29,6 +29,119 @@ import (
client "github.com/apache/trafficcontrol/traffic_ops/v4-client"
)
+func TestServerUpdateApplyTimeLocked(t *testing.T) {
+ WithObjs(t, []TCObj{CDNs, Types, Tenants, Parameters, Profiles,
Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers,
ServiceCategories, Topologies, DeliveryServices}, func() {
+ opts := client.NewRequestOptions()
+ opts.QueryParameters.Set("hostName", "atlanta-edge-01")
+ resp, _, err := TOSession.GetServers(opts)
+ if err != nil {
+ t.Fatalf("cannot get server by hostname: %v", err)
+ }
+ if len(resp.Response) != 1 {
+ t.Fatalf("Expected a server named 'atlanta-edge-01' to
exist")
+ }
+ testServer := resp.Response[0]
+
+ testVals := func(configApply, revalApply *time.Time) {
+ resp, _, err := TOSession.GetServers(opts)
+ if err != nil {
+ t.Errorf("cannot get Server by name '%s': %v -
alerts: %+v", *testServer.HostName, err, resp.Alerts)
+ } else if len(resp.Response) != 1 {
+ t.Fatalf("GET Server expected 1, actual %v",
len(resp.Response))
+ }
+
+ beforeServer := resp.Response[0]
+
+ // Ensure baseline
+ if beforeServer.UpdPending == nil {
+ t.Fatalf("Server '%s' had nil UpdPending before
update status change", *testServer.HostName)
+ }
+ if beforeServer.RevalPending == nil {
+ t.Fatalf("Server '%s' had nil RevalPending
before update status change", *testServer.HostName)
+ }
+ if beforeServer.ConfigUpdateTime == nil {
+ t.Fatalf("Server '%s' had nil ConfigUpdateTime
before update status change", *testServer.HostName)
+ }
+ if beforeServer.ConfigApplyTime == nil {
+ t.Fatalf("Server '%s' had nil ConfigApplyTime
before update status change", *testServer.HostName)
+ }
+ if beforeServer.RevalUpdateTime == nil {
+ t.Fatalf("Server '%s' had nil RevalUpdateTime
before update status change", *testServer.HostName)
+ }
+ if beforeServer.RevalApplyTime == nil {
+ t.Fatalf("Server '%s' had nil RevalApplyTime
before update status change", *testServer.HostName)
+ }
+
+ _, _, err = TOSession.CreateCDNLock(tc.CDNLock{CDN:
*testServer.CDNName, Soft: util.BoolPtr(false)}, client.NewRequestOptions())
+ if err != nil {
+ t.Errorf("cannont acquire lock on the Server
'%s' CDN '%s: %v", *testServer.HostName, *testServer.CDNName, err)
+ }
+
+ opsSession, _, err :=
client.LoginWithAgent(Config.TrafficOps.URL, Config.TrafficOps.Users.Operations,
+ Config.TrafficOps.UserPassword, true,
"to-api-v4-locks-ops",
+ true,
time.Second*time.Duration(Config.Default.Session.TimeoutInSecs))
+ if err != nil {
+ t.Errorf("cannot login as '%s' user: %v",
Config.TrafficOps.Users.Operations, err)
Review Comment:
Should this be a `fatal` error? Because if we can't login using the
`Operations` user, we can't possibly check the status update, right?
##########
traffic_ops/testing/api/v4/serverupdatestatus_test.go:
##########
@@ -29,6 +29,119 @@ import (
client "github.com/apache/trafficcontrol/traffic_ops/v4-client"
)
+func TestServerUpdateApplyTimeLocked(t *testing.T) {
+ WithObjs(t, []TCObj{CDNs, Types, Tenants, Parameters, Profiles,
Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers,
ServiceCategories, Topologies, DeliveryServices}, func() {
+ opts := client.NewRequestOptions()
+ opts.QueryParameters.Set("hostName", "atlanta-edge-01")
+ resp, _, err := TOSession.GetServers(opts)
+ if err != nil {
+ t.Fatalf("cannot get server by hostname: %v", err)
+ }
+ if len(resp.Response) != 1 {
+ t.Fatalf("Expected a server named 'atlanta-edge-01' to
exist")
+ }
+ testServer := resp.Response[0]
+
+ testVals := func(configApply, revalApply *time.Time) {
+ resp, _, err := TOSession.GetServers(opts)
+ if err != nil {
+ t.Errorf("cannot get Server by name '%s': %v -
alerts: %+v", *testServer.HostName, err, resp.Alerts)
+ } else if len(resp.Response) != 1 {
+ t.Fatalf("GET Server expected 1, actual %v",
len(resp.Response))
+ }
+
+ beforeServer := resp.Response[0]
+
+ // Ensure baseline
+ if beforeServer.UpdPending == nil {
+ t.Fatalf("Server '%s' had nil UpdPending before
update status change", *testServer.HostName)
+ }
+ if beforeServer.RevalPending == nil {
+ t.Fatalf("Server '%s' had nil RevalPending
before update status change", *testServer.HostName)
+ }
+ if beforeServer.ConfigUpdateTime == nil {
+ t.Fatalf("Server '%s' had nil ConfigUpdateTime
before update status change", *testServer.HostName)
+ }
+ if beforeServer.ConfigApplyTime == nil {
+ t.Fatalf("Server '%s' had nil ConfigApplyTime
before update status change", *testServer.HostName)
+ }
+ if beforeServer.RevalUpdateTime == nil {
+ t.Fatalf("Server '%s' had nil RevalUpdateTime
before update status change", *testServer.HostName)
+ }
+ if beforeServer.RevalApplyTime == nil {
+ t.Fatalf("Server '%s' had nil RevalApplyTime
before update status change", *testServer.HostName)
+ }
+
+ _, _, err = TOSession.CreateCDNLock(tc.CDNLock{CDN:
*testServer.CDNName, Soft: util.BoolPtr(false)}, client.NewRequestOptions())
+ if err != nil {
+ t.Errorf("cannont acquire lock on the Server
'%s' CDN '%s: %v", *testServer.HostName, *testServer.CDNName, err)
+ }
+
+ opsSession, _, err :=
client.LoginWithAgent(Config.TrafficOps.URL, Config.TrafficOps.Users.Operations,
+ Config.TrafficOps.UserPassword, true,
"to-api-v4-locks-ops",
+ true,
time.Second*time.Duration(Config.Default.Session.TimeoutInSecs))
+ if err != nil {
+ t.Errorf("cannot login as '%s' user: %v",
Config.TrafficOps.Users.Operations, err)
+ }
+
+ badQueryOpts := client.NewRequestOptions()
+ badQueryOpts.QueryParameters.Set("updated", "true")
+ if _, _, err :=
opsSession.SetUpdateServerStatusTimes(*testServer.HostName, configApply,
revalApply, badQueryOpts); err == nil {
+ t.Fatalf("expected SetUpdateServerStatusTimes
to not allow updates due to `updated` field")
+ }
+
+ badQueryOpts = client.NewRequestOptions()
+ badQueryOpts.QueryParameters.Set("reval_updated",
"true")
+ if _, _, err :=
opsSession.SetUpdateServerStatusTimes(*testServer.HostName, configApply,
revalApply, badQueryOpts); err == nil {
+ t.Fatalf("expected SetUpdateServerStatusTimes
to not allow updates due to `reval_updated` field")
+ }
+
+ if alerts, _, err :=
opsSession.SetUpdateServerStatusTimes(*testServer.HostName, configApply,
revalApply, client.RequestOptions{}); err != nil {
+ t.Fatalf("SetUpdateServerStatusTimes error.
expected: nil, actual: %v - alerts: %+v", err, alerts.Alerts)
+ }
+
+ resp, _, err = opsSession.GetServers(opts)
+ if err != nil {
+ t.Errorf("cannot GET Server by name '%s': %v -
alerts: %+v", *testServer.HostName, err, resp.Alerts)
Review Comment:
This should be `fatal`, to prevent a panic in the following section, if
`err` is not `nil` and `resp` is `nil`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
