You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2022/05/11 18:30:43 UTC
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #6763: Refactor CDNs tests
ocket8888 commented on code in PR #6763:
URL: https://github.com/apache/trafficcontrol/pull/6763#discussion_r870617058
##########
traffic_ops/testing/api/v3/cdns_test.go:
##########
@@ -16,192 +16,192 @@ package v3
*/
import (
+ "encoding/json"
"net/http"
+ "net/url"
"sort"
"testing"
"time"
"github.com/apache/trafficcontrol/lib/go-rfc"
tc "github.com/apache/trafficcontrol/lib/go-tc"
+ "github.com/apache/trafficcontrol/traffic_ops/testing/api/assert"
+ "github.com/apache/trafficcontrol/traffic_ops/testing/api/utils"
+ "github.com/apache/trafficcontrol/traffic_ops/toclientlib"
)
func TestCDNs(t *testing.T) {
WithObjs(t, []TCObj{CDNs, Parameters}, func() {
- GetTestCDNsIMS(t)
- currentTime := time.Now().UTC().Add(-5 * time.Second)
- time := currentTime.Format(time.RFC1123)
- var header http.Header
- header = make(map[string][]string)
- header.Set(rfc.IfModifiedSince, time)
- header.Set(rfc.IfUnmodifiedSince, time)
- SortTestCDNs(t)
- UpdateTestCDNs(t)
- UpdateTestCDNsWithHeaders(t, header)
- header = make(map[string][]string)
- etag := rfc.ETag(currentTime)
- header.Set(rfc.IfMatch, etag)
- UpdateTestCDNsWithHeaders(t, header)
- GetTestCDNs(t)
- GetTestCDNsIMSAfterChange(t, header)
- })
-}
-
-func UpdateTestCDNsWithHeaders(t *testing.T, header http.Header) {
- firstCDN := testData.CDNs[0]
- // Retrieve the CDN by name so we can get the id for the Update
- resp, _, err := TOSession.GetCDNByNameWithHdr(firstCDN.Name, header)
- if err != nil {
- t.Errorf("cannot GET CDN by name: '%s', %v", firstCDN.Name, err)
- }
- if len(resp) > 0 {
- remoteCDN := resp[0]
- remoteCDN.DomainName = "domain2"
- _, reqInf, err := TOSession.UpdateCDNByIDWithHdr(remoteCDN.ID, remoteCDN, header)
- if err == nil {
- t.Errorf("Expected error about Precondition Failed, got none")
- }
- if reqInf.StatusCode != http.StatusPreconditionFailed {
- t.Errorf("Expected status code 412, got %v", reqInf.StatusCode)
- }
- }
-}
-
-func GetTestCDNsIMSAfterChange(t *testing.T, header http.Header) {
- for _, cdn := range testData.CDNs {
- _, reqInf, err := TOSession.GetCDNByNameWithHdr(cdn.Name, header)
- if err != nil {
- t.Fatalf("Expected no error, but got %v", err.Error())
- }
- if reqInf.StatusCode != http.StatusOK {
- t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
- }
- }
- currentTime := time.Now().UTC()
- currentTime = currentTime.Add(1 * time.Second)
- timeStr := currentTime.Format(time.RFC1123)
- header.Set(rfc.IfModifiedSince, timeStr)
- for _, cdn := range testData.CDNs {
- _, reqInf, err := TOSession.GetCDNByNameWithHdr(cdn.Name, header)
- if err != nil {
- t.Fatalf("Expected no error, but got %v", err.Error())
+ currentTime := time.Now().UTC().Add(-15 * time.Second)
+ currentTimeRFC := currentTime.Format(time.RFC1123)
+ tomorrow := currentTime.AddDate(0, 0, 1).Format(time.RFC1123)
+
+ methodTests := utils.V3TestCase{
+ "GET": {
+ "NOT MODIFIED when NO CHANGES made": {
+ ClientSession: TOSession, RequestHeaders: http.Header{rfc.IfModifiedSince: {tomorrow}},
+ Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusNotModified)),
+ },
+ "OK when VALID request": {
+ ClientSession: TOSession, Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK),
+ utils.ResponseLengthGreaterOrEqual(1), validateCDNSort()),
+ },
+ "OK when VALID NAME parameter": {
+ ClientSession: TOSession, RequestParams: url.Values{"name": {"cdn1"}},
+ Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK), utils.ResponseHasLength(1),
+ validateCDNFields(map[string]interface{}{"Name": "cdn1"})),
+ },
+ },
+ "PUT": {
+ "OK when VALID request": {
+ EndpointId: GetCDNID(t, "cdn1"), ClientSession: TOSession,
+ RequestBody: map[string]interface{}{
+ "dnssecEnabled": false,
+ "domainName": "newDomain",
+ "name": "cdn1",
+ },
+ Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK)),
+ },
+ "PRECONDITION FAILED when updating with IF-UNMODIFIED-SINCE Headers": {
+ EndpointId: GetCDNID(t, "cdn1"), ClientSession: TOSession,
+ RequestHeaders: http.Header{rfc.IfUnmodifiedSince: {currentTimeRFC}},
+ RequestBody: map[string]interface{}{
+ "dnssecEnabled": false,
+ "domainName": "newDomain",
+ "name": "cdn1",
+ },
+ Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusPreconditionFailed)),
+ },
+ "PRECONDITION FAILED when updating with IFMATCH ETAG Header": {
+ EndpointId: GetCDNID(t, "cdn1"), ClientSession: TOSession,
+ RequestHeaders: http.Header{rfc.IfMatch: {rfc.ETag(currentTime)}},
+ RequestBody: map[string]interface{}{
+ "dnssecEnabled": false,
+ "domainName": "newDomain",
+ "name": "cdn1",
+ },
+ Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusPreconditionFailed)),
+ },
+ },
+ "GET AFTER CHANGES": {
+ "OK when CHANGES made": {
+ ClientSession: TOSession,
+ RequestHeaders: http.Header{rfc.IfModifiedSince: {currentTimeRFC}},
+ Expectations: utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusOK)),
+ },
+ },
}
- if reqInf.StatusCode != http.StatusNotModified {
- t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
+ for method, testCases := range methodTests {
+ t.Run(method, func(t *testing.T) {
+ for name, testCase := range testCases {
+ cdn := tc.CDN{}
+
+ if testCase.RequestBody != nil {
+ dat, err := json.Marshal(testCase.RequestBody)
+ assert.NoError(t, err, "Error occurred when marshalling request body: %v", err)
+ err = json.Unmarshal(dat, &cdn)
+ assert.NoError(t, err, "Error occurred when unmarshalling request body: %v", err)
+ }
+
+ switch method {
+ case "GET", "GET AFTER CHANGES":
+ t.Run(name, func(t *testing.T) {
+ if name == "OK when VALID NAME parameter" {
+ resp, reqInf, err := testCase.ClientSession.GetCDNByNameWithHdr(testCase.RequestParams["name"][0], testCase.RequestHeaders)
+ for _, check := range testCase.Expectations {
+ check(t, reqInf, resp, tc.Alerts{}, err)
+ }
+ } else {
+ resp, reqInf, err := testCase.ClientSession.GetCDNsWithHdr(testCase.RequestHeaders)
+ for _, check := range testCase.Expectations {
+ check(t, reqInf, resp, tc.Alerts{}, err)
+ }
+ }
+ })
+ case "PUT":
+ t.Run(name, func(t *testing.T) {
+ alerts, reqInf, err := testCase.ClientSession.UpdateCDNByIDWithHdr(testCase.EndpointId(), cdn, testCase.RequestHeaders)
+ for _, check := range testCase.Expectations {
+ check(t, reqInf, nil, alerts, err)
+ }
+ })
+ case "DELETE":
+ t.Run(name, func(t *testing.T) {
+ alerts, reqInf, err := testCase.ClientSession.DeleteCDNByID(testCase.EndpointId())
+ for _, check := range testCase.Expectations {
+ check(t, reqInf, nil, alerts, err)
+ }
+ })
+ }
+ }
+ })
}
- }
+ })
}
-func GetTestCDNsIMS(t *testing.T) {
- var header http.Header
- header = make(map[string][]string)
- for _, cdn := range testData.CDNs {
- futureTime := time.Now().AddDate(0, 0, 1)
- time := futureTime.Format(time.RFC1123)
- header.Set(rfc.IfModifiedSince, time)
- _, reqInf, err := TOSession.GetCDNByNameWithHdr(cdn.Name, header)
- if err != nil {
- t.Fatalf("Expected no error, but got %v", err.Error())
- }
- if reqInf.StatusCode != http.StatusNotModified {
- t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
+func validateCDNFields(expectedResp map[string]interface{}) utils.CkReqFunc {
+ return func(t *testing.T, _ toclientlib.ReqInf, resp interface{}, _ tc.Alerts, _ error) {
+ cdnResp := resp.([]tc.CDN)
+ for field, expected := range expectedResp {
+ for _, cdn := range cdnResp {
+ switch field {
+ case "Name":
+ assert.Equal(t, expected, cdn.Name, "Expected Name to be %v, but got %v", expected, cdn.Name)
+ case "DomainName":
+ assert.Equal(t, expected, cdn.DomainName, "Expected DomainName to be %v, but got %v", expected, cdn.DomainName)
+ case "DNSSECEnabled":
+ assert.Equal(t, expected, cdn.DNSSECEnabled, "Expected DNSSECEnabled to be %v, but got %v", expected, cdn.DNSSECEnabled)
+ default:
+ t.Errorf("Expected field: %v, does not exist in response", field)
+ }
+ }
}
}
}
-func CreateTestCDNs(t *testing.T) {
+func validateCDNSort() utils.CkReqFunc {
+ return func(t *testing.T, _ toclientlib.ReqInf, resp interface{}, alerts tc.Alerts, _ error) {
+ var sortedList []string
+ cdnResp := resp.([]tc.CDN)
- for _, cdn := range testData.CDNs {
- resp, _, err := TOSession.CreateCDN(cdn)
- t.Log("Response: ", resp)
- if err != nil {
- t.Errorf("could not CREATE cdns: %v", err)
+ for _, cdn := range cdnResp {
+ sortedList = append(sortedList, cdn.Name)
}
- }
-
-}
-
-func SortTestCDNs(t *testing.T) {
- var header http.Header
- var sortedList []string
- resp, _, err := TOSession.GetCDNsWithHdr(header)
- if err != nil {
- t.Fatalf("Expected no error, but got %v", err.Error())
- }
- for i, _ := range resp {
- sortedList = append(sortedList, resp[i].Name)
- }
- res := sort.SliceIsSorted(sortedList, func(p, q int) bool {
- return sortedList[p] < sortedList[q]
- })
- if res != true {
- t.Errorf("list is not sorted by their names: %v", sortedList)
+ res := sort.SliceIsSorted(sortedList, func(p, q int) bool {
Review Comment:
I realize you didn't write this originally, but this is re-implementing a `sort` package function (`StringsAreSorted`), so as long as we're making improvements to this test suite, it'd be nice if we could stop re-implementing standard library functions.
##########
traffic_ops/testing/api/v4/cdn_dnsseckeys_test.go:
##########
@@ -0,0 +1,153 @@
+package v4
+
+/*
+
+ Licensed 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.
+*/
+
+import (
+ "net/http"
+ "reflect"
+ "strconv"
+ "strings"
+ "testing"
+
+ tc "github.com/apache/trafficcontrol/lib/go-tc"
+ "github.com/apache/trafficcontrol/lib/go-util"
+ "github.com/apache/trafficcontrol/traffic_ops/testing/api/assert"
+ client "github.com/apache/trafficcontrol/traffic_ops/v4-client"
+)
+
+func TestCDNsDNSSEC(t *testing.T) {
+ if !includeSystemTests {
+ t.Skip()
+ }
+ WithObjs(t, []TCObj{CDNs, Types, Tenants, Users, Parameters, Profiles, Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers, Topologies, ServerCapabilities, ServiceCategories, DeliveryServices}, func() {
+ GenerateDNSSECKeys(t)
+ RefreshDNSSECKeys(t) // NOTE: testing refresh last (while no keys exist) because it's asynchronous and might affect other tests
+ })
+}
+
+func RefreshDNSSECKeys(t *testing.T) {
+ resp, reqInf, err := TOSession.RefreshDNSSECKeys(client.RequestOptions{})
+ assert.NoError(t, err, "Unable to refresh DNSSEC keys: %v - alerts: %+v", err, resp.Alerts)
+ assert.Equal(t, reqInf.StatusCode, http.StatusAccepted, "Refreshing DNSSEC keys - Expected: status code %d, Actual: %d", http.StatusAccepted, reqInf.StatusCode)
+
+ loc := reqInf.RespHeaders.Get("Location")
+ if loc == "" {
+ t.Errorf("Refreshing DNSSEC keys - Expected: non-empty 'Location' response header, Actual: empty")
Review Comment:
Should this be `Fatal` (`f` not necessary with no formatting being done) ? The steps that follow can't possibly succeed if this point is reached.
##########
traffic_ops/testing/api/v4/cdn_dnsseckeys_test.go:
##########
@@ -0,0 +1,153 @@
+package v4
+
+/*
+
+ Licensed 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.
+*/
+
+import (
+ "net/http"
+ "reflect"
+ "strconv"
+ "strings"
+ "testing"
+
+ tc "github.com/apache/trafficcontrol/lib/go-tc"
+ "github.com/apache/trafficcontrol/lib/go-util"
+ "github.com/apache/trafficcontrol/traffic_ops/testing/api/assert"
+ client "github.com/apache/trafficcontrol/traffic_ops/v4-client"
+)
+
+func TestCDNsDNSSEC(t *testing.T) {
+ if !includeSystemTests {
+ t.Skip()
+ }
+ WithObjs(t, []TCObj{CDNs, Types, Tenants, Users, Parameters, Profiles, Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers, Topologies, ServerCapabilities, ServiceCategories, DeliveryServices}, func() {
+ GenerateDNSSECKeys(t)
+ RefreshDNSSECKeys(t) // NOTE: testing refresh last (while no keys exist) because it's asynchronous and might affect other tests
Review Comment:
These should be run with `t.Run` so that if the first one fails the second one runs, and so that any failure messages are more specific as to what failed.
##########
traffic_ops/testing/api/v4/cdn_dnsseckeys_test.go:
##########
@@ -0,0 +1,153 @@
+package v4
+
+/*
+
+ Licensed 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.
+*/
+
+import (
+ "net/http"
+ "reflect"
+ "strconv"
+ "strings"
+ "testing"
+
+ tc "github.com/apache/trafficcontrol/lib/go-tc"
+ "github.com/apache/trafficcontrol/lib/go-util"
+ "github.com/apache/trafficcontrol/traffic_ops/testing/api/assert"
+ client "github.com/apache/trafficcontrol/traffic_ops/v4-client"
+)
+
+func TestCDNsDNSSEC(t *testing.T) {
+ if !includeSystemTests {
+ t.Skip()
+ }
+ WithObjs(t, []TCObj{CDNs, Types, Tenants, Users, Parameters, Profiles, Statuses, Divisions, Regions, PhysLocations, CacheGroups, Servers, Topologies, ServerCapabilities, ServiceCategories, DeliveryServices}, func() {
+ GenerateDNSSECKeys(t)
+ RefreshDNSSECKeys(t) // NOTE: testing refresh last (while no keys exist) because it's asynchronous and might affect other tests
+ })
+}
+
+func RefreshDNSSECKeys(t *testing.T) {
+ resp, reqInf, err := TOSession.RefreshDNSSECKeys(client.RequestOptions{})
+ assert.NoError(t, err, "Unable to refresh DNSSEC keys: %v - alerts: %+v", err, resp.Alerts)
+ assert.Equal(t, reqInf.StatusCode, http.StatusAccepted, "Refreshing DNSSEC keys - Expected: status code %d, Actual: %d", http.StatusAccepted, reqInf.StatusCode)
+
+ loc := reqInf.RespHeaders.Get("Location")
+ if loc == "" {
+ t.Errorf("Refreshing DNSSEC keys - Expected: non-empty 'Location' response header, Actual: empty")
+ }
+ asyncID, err := strconv.Atoi(strings.Split(loc, "/")[4])
+ assert.NoError(t, err, "Parsing async_status ID from 'Location' response header - Expected: no error, Actual: %v", err)
+
+ status, _, err := TOSession.GetAsyncStatus(asyncID, client.RequestOptions{})
+ assert.NoError(t, err, "Getting async status id %d - Expected: no error, Actual: %v", asyncID, err)
+ assert.NotNil(t, status.Response.Message, "Getting async status for DNSSEC refresh job - Expected: non-nil message, Actual: nil")
+}
+
+func GenerateDNSSECKeys(t *testing.T) {
+ assert.GreaterOrEqual(t, len(testData.CDNs), 1, "Need at least one CDN to test updating CDNs")
+ firstCDN := testData.CDNs[0]
+ opts := client.NewRequestOptions()
+ opts.QueryParameters.Set("name", firstCDN.Name)
+ cdns, _, err := TOSession.GetCDNs(opts)
+ assert.NoError(t, err, "Unexpected error getting CDNs filtered by name '%s': %v - alerts: %+v", firstCDN.Name, err, cdns.Alerts)
+ assert.Equal(t, len(cdns.Response), 1, "Expected exactly one CDN named '%s' to exist, found: %d", firstCDN.Name, len(cdns.Response))
+
+ cdn := cdns.Response[0]
+
+ ttl := util.JSONIntStr(60)
+ req := tc.CDNDNSSECGenerateReq{
+ Key: util.StrPtr(firstCDN.Name),
+ TTL: &ttl,
+ KSKExpirationDays: &ttl,
+ ZSKExpirationDays: &ttl,
+ }
+ resp, _, err := TOSession.GenerateCDNDNSSECKeys(req, client.RequestOptions{})
+ assert.RequireNoError(t, err, "Unexpected error generating CDN DNSSEC keys: %v - alerts: %+v", err, resp.Alerts)
+
+ res, _, err := TOSession.GetCDNDNSSECKeys(firstCDN.Name, client.RequestOptions{})
+ assert.RequireNoError(t, err, "Unexpected error getting CDN DNSSEC keys: %v - alerts: %+v", err, res.Alerts)
+
+ if _, ok := res.Response[firstCDN.Name]; !ok {
+ t.Errorf("getting CDN DNSSEC keys - expected: key %s, actual: missing", firstCDN.Name)
+ }
+ originalKeys := res.Response
+
+ resp, _, err = TOSession.GenerateCDNDNSSECKeys(req, client.RequestOptions{})
+ assert.RequireNoError(t, err, "Unexpected error generating CDN DNSSEC keys: %v - alerts: %+v", err, resp.Alerts)
+
+ res, _, err = TOSession.GetCDNDNSSECKeys(firstCDN.Name, client.RequestOptions{})
+ assert.RequireNoError(t, err, "Unexpected error getting CDN DNSSEC keys: %v - alerts: %+v", err, res.Alerts)
+
+ newKeys := res.Response
+
+ if reflect.DeepEqual(originalKeys, newKeys) {
+ t.Errorf("Generating CDN DNSSEC keys - expected: original keys to differ from new keys, actual: they are the same")
+ }
+
+ kskReq := tc.CDNGenerateKSKReq{
+ ExpirationDays: util.Uint64Ptr(30),
+ }
+ originalKSK := newKeys
+ resp, _, err = TOSession.GenerateCDNDNSSECKSK(firstCDN.Name, kskReq, client.RequestOptions{})
+ assert.NoError(t, err, "Unexpected error generating DNSSEC KSK: %v - alerts: %+v", err, resp.Alerts)
+
+ res, _, err = TOSession.GetCDNDNSSECKeys(firstCDN.Name, client.RequestOptions{})
+ assert.NoError(t, err, "Unexpected error getting CDN DNSSEC keys: %v - alerts: %+v", err, res.Alerts)
+
+ newKSK := res.Response
+ if reflect.DeepEqual(originalKSK[firstCDN.Name].KSK, newKSK[firstCDN.Name].KSK) {
Review Comment:
This line will panic if `firstCDN.Name` isn't in the `newKSK` and/or `originalKSK` map(s), should check that first like you did for `originalKeys`.
--
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