From b8dc6753500fa9658dd140646cb9ac5f1b73ad77 Mon Sep 17 00:00:00 2001 From: Srijeet Chatterjee Date: Tue, 11 Jul 2023 15:25:48 -0600 Subject: [PATCH] Fix status code and alerts when querying delivery service with no SSL keys --- CHANGELOG.md | 1 + .../api/v5/deliveryservices_keys_test.go | 24 +++++++++++++++++ .../deliveryservice/keys.go | 26 ++++++++++++++++--- .../deliveryservice/sslkeys.go | 4 ++- .../backends/postgres/postgres.go | 2 +- 5 files changed, 51 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index faa26aeb8a..3da5a61a84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). - [#7621](https://github.com/apache/trafficcontrol/pull/7621) *Traffic Ops* Use ID token for OAuth authentication, not Access Token ### Fixed +- [#4393](https://github.com/apache/trafficcontrol/issues/4393) *Traffic Ops* Fixed the error code and alert structure when TO is queried for a delivery service with no ssl keys. - [#7623] (https://github.com/apache/trafficcontrol/pull/7623) *Traffic Ops* Removed TryIfModifiedSinceQuery from servicecategories.go and reused from ims.go - [#7608](https://github.com/apache/trafficcontrol/pull/7608) *Traffic Monitor* Use stats_over_http(plugin.system_stats.timestamp_ms) timestamp field to calculate bandwidth for TM's caches. - [#6318](https://github.com/apache/trafficcontrol/issues/6318) *Docs* Included docs for POST, PUT, DELETE (v3,v4,v5) for statuses and statuses{id} diff --git a/traffic_ops/testing/api/v5/deliveryservices_keys_test.go b/traffic_ops/testing/api/v5/deliveryservices_keys_test.go index 54b0775d5b..7044ded2db 100644 --- a/traffic_ops/testing/api/v5/deliveryservices_keys_test.go +++ b/traffic_ops/testing/api/v5/deliveryservices_keys_test.go @@ -14,6 +14,7 @@ package v5 import ( "encoding/json" + "net/http" "strconv" "strings" "testing" @@ -41,6 +42,7 @@ func TestDeliveryServicesKeys(t *testing.T) { t.Run("Delete URI Signing keys for a Delivery Service", DeleteTestDeliveryServicesURISigningKeys) t.Run("Delete old CDN SSL keys", DeleteCDNOldSSLKeys) t.Run("Create and retrieve SSL keys for a Delivery Service", DeliveryServiceSSLKeys) + t.Run("Retrieve SSL keys for a Delivery Service without keys", DeliveryServiceNoSSLKeys) }) } @@ -234,6 +236,28 @@ func DeleteCDNOldSSLKeys(t *testing.T) { assert.RequireEqual(t, len(newCdnKeys), 1, "Expected 1 ssl keys for CDN %v, got %d instead", cdn.Name, len(newCdnKeys)) } +func DeliveryServiceNoSSLKeys(t *testing.T) { + cdn := createBlankCDN("testDSWithNoSSLKeysCDN", t) + + opts := client.NewRequestOptions() + opts.QueryParameters.Set("name", "HTTP") + types, _, err := TOSession.GetTypes(opts) + assert.RequireNoError(t, err, "Unable to get Types: %v - alerts: %+v", err, types.Alerts) + assert.RequireGreaterOrEqual(t, len(types.Response), 1, "Expected at least one type") + + customDS := getCustomDS(cdn.ID, types.Response[0].ID, "dsNoSSLKeys", "routingName", "https://test.com", "dsNoSSLKeys") + customDS.Protocol = util.Ptr(0) + resp, _, err := TOSession.CreateDeliveryService(customDS, client.RequestOptions{}) + assert.RequireNoError(t, err, "Unexpected error creating a Delivery Service: %v - alerts: %+v", err, resp.Alerts) + + ds := resp.Response + assert.RequireNotNil(t, ds.XMLID, "Traffic Ops returned a representation for a Delivery Service with null or undefined XMLID") + + _, reqInf, err := TOSession.GetDeliveryServiceSSLKeys(ds.XMLID, client.NewRequestOptions()) + assert.RequireNotNil(t, err, "expected error getting ssl keys, but got nothing") + assert.RequireEqual(t, reqInf.StatusCode, http.StatusNotFound) +} + func DeliveryServiceSSLKeys(t *testing.T) { cdn := createBlankCDN("sslkeytransfer", t) diff --git a/traffic_ops/traffic_ops_golang/deliveryservice/keys.go b/traffic_ops/traffic_ops_golang/deliveryservice/keys.go index 2017fa78ce..ad4b8682f9 100644 --- a/traffic_ops/traffic_ops_golang/deliveryservice/keys.go +++ b/traffic_ops/traffic_ops_golang/deliveryservice/keys.go @@ -187,12 +187,27 @@ func GetSSLKeysByXMLID(w http.ResponseWriter, r *http.Request) { return } + var userError error + sc := http.StatusInternalServerError + logAlert := true keyObjV4, err := getSslKeys(inf, r.Context()) if err != nil { - userErr := api.LogErr(r, http.StatusInternalServerError, nil, err) - alerts.AddNewAlert(tc.ErrorLevel, userErr.Error()) - api.WriteAlerts(w, r, http.StatusInternalServerError, alerts) - return + if err == sql.ErrNoRows { + if inf.Version.GreaterThanOrEqualTo(&api.Version{Major: 5, Minor: 0}) { + sc = http.StatusNotFound + userError = api.LogErr(r, sc, errors.New("no ssl keys for XML ID "+xmlID), nil) + } else { + // For versions lesser than 5.0, don't log an alert if the error is ErrNoRows. This is for backward compatibility reasons. + logAlert = false + } + } else { + userError = api.LogErr(r, sc, nil, err) + } + if logAlert { + alerts.AddNewAlert(tc.ErrorLevel, userError.Error()) + api.WriteAlerts(w, r, sc, alerts) + return + } } var keyObj interface{} @@ -216,6 +231,9 @@ func getSslKeys(inf *api.APIInfo, ctx context.Context) (tc.DeliveryServiceSSLKey keyObjFromTv, ok, err := inf.Vault.GetDeliveryServiceSSLKeys(xmlID, version, inf.Tx.Tx, ctx) if err != nil { + if err == sql.ErrNoRows { + return tc.DeliveryServiceSSLKeysV4{}, err + } return tc.DeliveryServiceSSLKeysV4{}, errors.New("getting ssl keys: " + err.Error()) } keyObj := tc.DeliveryServiceSSLKeysV4{} diff --git a/traffic_ops/traffic_ops_golang/deliveryservice/sslkeys.go b/traffic_ops/traffic_ops_golang/deliveryservice/sslkeys.go index f3a133c066..38402e9cf0 100644 --- a/traffic_ops/traffic_ops_golang/deliveryservice/sslkeys.go +++ b/traffic_ops/traffic_ops_golang/deliveryservice/sslkeys.go @@ -122,7 +122,9 @@ func GeneratePlaceholderSelfSignedCert(ds tc.DeliveryServiceV5, inf *api.APIInfo tv := inf.Vault _, ok, err := tv.GetDeliveryServiceSSLKeys(ds.XMLID, "", tx, context) if err != nil { - return fmt.Errorf("getting latest ssl keys for XMLID '%s': %w", ds.XMLID, err), http.StatusInternalServerError + if err != sql.ErrNoRows { + return fmt.Errorf("getting latest ssl keys for XMLID '%s': %w", ds.XMLID, err), http.StatusInternalServerError + } } if ok { return nil, http.StatusOK diff --git a/traffic_ops/traffic_ops_golang/trafficvault/backends/postgres/postgres.go b/traffic_ops/traffic_ops_golang/trafficvault/backends/postgres/postgres.go index b1bdc1b394..1499882d30 100644 --- a/traffic_ops/traffic_ops_golang/trafficvault/backends/postgres/postgres.go +++ b/traffic_ops/traffic_ops_golang/trafficvault/backends/postgres/postgres.go @@ -138,7 +138,7 @@ func (p *Postgres) GetDeliveryServiceSSLKeys(xmlID string, version string, tx *s err = tvTx.QueryRow(query, xmlID, version).Scan(&encryptedSslKeys) if err != nil { if err == sql.ErrNoRows { - return tc.DeliveryServiceSSLKeysV15{}, false, nil + return tc.DeliveryServiceSSLKeysV15{}, false, err } e := checkErrWithContext("Traffic Vault PostgreSQL: executing SELECT SSL Keys query", err, ctx.Err()) return tc.DeliveryServiceSSLKeysV15{}, false, e