You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficcontrol.apache.org by zr...@apache.org on 2021/01/15 16:52:22 UTC
[trafficcontrol] branch master updated: Fixed https status code
when updating CDN for a DS for existing SSL keys (#5417)
This is an automated email from the ASF dual-hosted git repository.
zrhoffman pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficcontrol.git
The following commit(s) were added to refs/heads/master by this push:
new 95269da Fixed https status code when updating CDN for a DS for existing SSL keys (#5417)
95269da is described below
commit 95269da8c149ac535acb0139e8572c810c8432f6
Author: rimashah25 <22...@users.noreply.github.com>
AuthorDate: Fri Jan 15 09:52:04 2021 -0700
Fixed https status code when updating CDN for a DS for existing SSL keys (#5417)
* Updated https status code.
* Fixed TP documentation.
* Updated based on review comments and added a function to check SSLVersion
* Removed warning from DS (DNS, HTTP, Steering) wrt routing name
* Updated getOldDetails() and updateV31(). Also checking if sslKeyVersion are existing prior to updating a DS
* Fixed unit test.
* Updated changelog
---
CHANGELOG.md | 1 +
.../deliveryservice/deliveryservices.go | 70 ++++++++--------------
.../deliveryservice/deliveryservices_test.go | 4 +-
.../form.deliveryService.DNS.tpl.html | 5 +-
.../form.deliveryService.HTTP.tpl.html | 5 +-
.../form.deliveryService.Steering.tpl.html | 5 +-
6 files changed, 33 insertions(+), 57 deletions(-)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index b116cf0..9752354 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -16,6 +16,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
- Pinned external actions used by Documentation Build and TR Unit Tests workflows to commit SHA-1 and the Docker image used by the Weasel workflow to a SHA-256 digest
### Fixed
+- [#5341](https://github.com/apache/trafficcontrol/issues/5341) - For a DS with existing SSLKeys, fixed HTTP status code from 403 to 400 when updating CDN and Routing Name (in TO) and made CDN and Routing Name fields immutable (in TP).
- [#5192](https://github.com/apache/trafficcontrol/issues/5192) - Fixed TO log warnings when generating snapshots for topology-based delivery services.
- [#5284](https://github.com/apache/trafficcontrol/issues/5284) - Fixed error message when creating a server with non-existent profile
- [#5287](https://github.com/apache/trafficcontrol/issues/5287) - Fixed error message when creating a Cache Group with no typeId
diff --git a/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go b/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
index da2b559..454df88 100644
--- a/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
+++ b/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
@@ -844,18 +844,23 @@ func updateV31(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, dsV31 *
return nil, http.StatusInternalServerError, nil, errors.New("getting delivery service type during update: " + err.Error())
}
- // oldHostName will be used to determine if SSL Keys need updating - this will be empty if the DS doesn't have SSL keys, because DS types without SSL keys may not have regexes, and thus will fail to get a host name.
- oldHostName := ""
oldCDNName := ""
oldRoutingName := ""
errCode := http.StatusOK
var userErr error
var sysErr error
if dsType.HasSSLKeys() {
- oldHostName, oldCDNName, oldRoutingName, userErr, sysErr, errCode = getOldDetails(*ds.ID, tx)
+ oldCDNName, oldRoutingName, userErr, sysErr, errCode = getOldDetails(*ds.ID, tx)
if userErr != nil || sysErr != nil {
return nil, errCode, userErr, sysErr
}
+ exists, err := getSSLVersion(*ds.XMLID, tx)
+ if err != nil {
+ return nil, http.StatusInternalServerError, nil, fmt.Errorf("querying delivery service with sslKeyVersion failed: %s", err)
+ }
+ if exists && (oldCDNName != *ds.CDNName || oldRoutingName != *ds.RoutingName) {
+ return nil, http.StatusBadRequest, errors.New("delivery service has ssl keys that cannot be automatically changed, therefore CDN and routing name are immutable"), nil
+ }
}
// TODO change DeepCachingType to implement sql.Valuer and sql.Scanner, so sqlx struct scan can be used.
@@ -993,7 +998,7 @@ func updateV31(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, dsV31 *
cdnDomain, err := getCDNDomain(*ds.ID, tx) // need to get the domain again, in case it changed.
if err != nil {
- return nil, http.StatusInternalServerError, nil, errors.New("getting CDN domain after update: " + err.Error())
+ return nil, http.StatusInternalServerError, nil, errors.New("getting CDN domain: (" + cdnDomain + ") after update: " + err.Error())
}
matchLists, err := GetDeliveryServicesMatchLists([]string{*ds.XMLID}, tx)
@@ -1006,21 +1011,6 @@ func updateV31(w http.ResponseWriter, r *http.Request, inf *api.APIInfo, dsV31 *
ds.MatchList = &ml
}
- // newHostName will be used to determine if SSL Keys need updating - this will be empty if the DS doesn't have SSL keys, because DS types without SSL keys may not have regexes, and thus will fail to get a host name.
- newHostName := ""
- newCDNName := ""
- if dsType.HasSSLKeys() {
- newCDNName, err = getCDNName(*ds.CDNID, tx)
- newHostName, err = getHostName(ds.Protocol, *ds.Type, *ds.RoutingName, *ds.MatchList, cdnDomain)
- if err != nil {
- return nil, http.StatusInternalServerError, nil, errors.New("getting cdnname/hostname after update: " + err.Error())
- }
- }
-
- if newDSType.HasSSLKeys() && (oldHostName != newHostName || oldCDNName != newCDNName || oldRoutingName != *ds.RoutingName) {
- return nil, http.StatusForbidden, nil, errors.New("delivery service has ssl keys that cannot be automatically changed")
- }
-
if err := EnsureParams(tx, *ds.ID, *ds.XMLID, ds.EdgeHeaderRewrite, ds.MidHeaderRewrite, ds.RegexRemap, ds.CacheURL, ds.SigningAlgorithm, newDSType, ds.MaxOriginConnections); err != nil {
return nil, http.StatusInternalServerError, nil, errors.New("ensuring ds parameters:: " + err.Error())
}
@@ -1182,43 +1172,23 @@ func selectMaxLastUpdatedQuery(where string) string {
select max(last_updated) as t from last_deleted l where l.table_name='deliveryservice') as res`
}
-func getOldDetails(id int, tx *sql.Tx) (string, string, string, error, error, int) {
+func getOldDetails(id int, tx *sql.Tx) (string, string, error, error, int) {
q := `
-SELECT ds.xml_id, ds.protocol, type.name, ds.routing_name, cdn.domain_name, cdn.name
+SELECT ds.routing_name, cdn.name
FROM deliveryservice as ds
-JOIN type ON ds.type = type.id
JOIN cdn ON ds.cdn_id = cdn.id
WHERE ds.id=$1
`
- xmlID := ""
- protocol := (*int)(nil)
- dsTypeStr := ""
routingName := ""
- cdnDomain := ""
cdnName := ""
- if err := tx.QueryRow(q, id).Scan(&xmlID, &protocol, &dsTypeStr, &routingName, &cdnDomain, &cdnName); err != nil {
+ if err := tx.QueryRow(q, id).Scan(&routingName, &cdnName); err != nil {
if err == sql.ErrNoRows {
- return "", "", "", fmt.Errorf("querying delivery service %v host name: no such delivery service exists", id), nil, http.StatusNotFound
+ return "", "", fmt.Errorf("querying delivery service %v host name: no such delivery service exists", id), nil, http.StatusNotFound
}
- return "", "", "", nil, fmt.Errorf("querying delivery service %v host name: "+err.Error(), id), http.StatusInternalServerError
- }
- dsType := tc.DSTypeFromString(dsTypeStr)
- if dsType == tc.DSTypeInvalid {
- return "", "", "", errors.New("getting delivery services matchlist: got invalid delivery service type '" + dsTypeStr + "'"), nil, http.StatusBadRequest
- }
- matchLists, err := GetDeliveryServicesMatchLists([]string{xmlID}, tx)
- if err != nil {
- return "", "", "", nil, errors.New("getting delivery services matchlist: " + err.Error()), http.StatusInternalServerError
- }
- matchList, ok := matchLists[xmlID]
- if !ok {
- return "", "", "", errors.New("delivery service has no match lists (is your delivery service missing regexes?)"), nil, http.StatusBadRequest
+ return "", "", nil, fmt.Errorf("querying delivery service %v host name: "+err.Error(), id), http.StatusInternalServerError
}
- host, err := getHostName(protocol, dsType, routingName, matchList, cdnDomain) // protocol defaults to 0: doesn't need to check Valid()
- if err != nil {
- return "", "", "", nil, errors.New("getting hostname: " + err.Error()), http.StatusInternalServerError
- }
- return host, cdnName, routingName, nil, nil, http.StatusOK
+
+ return cdnName, routingName, nil, nil, http.StatusOK
}
func getTypeFromID(id int, tx *sql.Tx) (tc.DSType, error) {
@@ -1826,6 +1796,14 @@ func GetXMLID(tx *sql.Tx, id int) (string, bool, error) {
return xmlID, true, nil
}
+// getSSLVersion reports a boolean value, confirming whether DS has a SSL version or not
+func getSSLVersion(xmlId string, tx *sql.Tx) (bool, error) {
+ var exists bool
+ row := tx.QueryRow(`SELECT EXISTS(SELECT * FROM deliveryservice WHERE xml_id = $1 AND ssl_key_version>=1)`, xmlId)
+ err := row.Scan(&exists)
+ return exists, err
+}
+
func selectQuery() string {
return `
SELECT
diff --git a/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_test.go b/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_test.go
index d89c043..555419b 100644
--- a/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_test.go
+++ b/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices_test.go
@@ -44,8 +44,8 @@ func TestGetOldDetails(t *testing.T) {
rows := sqlmock.NewRows([]string{""})
mock.ExpectBegin()
- mock.ExpectQuery("SELECT ds.xml_id, ds.protocol, type.name, ds.routing_name, cdn.domain_name, cdn.name").WillReturnRows(rows)
- _, _, _, userErr, _, code := getOldDetails(1, db.MustBegin().Tx)
+ mock.ExpectQuery("SELECT ds.routing_name, cdn.name").WillReturnRows(rows)
+ _, _, userErr, _, code := getOldDetails(1, db.MustBegin().Tx)
if userErr == nil {
t.Fatalf("expected error %v, but got none", expected)
}
diff --git a/traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.DNS.tpl.html b/traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.DNS.tpl.html
index 841aaa4..11ed857 100644
--- a/traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.DNS.tpl.html
+++ b/traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.DNS.tpl.html
@@ -171,7 +171,7 @@ under the License.
</div>
</label>
<div class="col-md-10 col-sm-10 col-xs-12">
- <select id="cdn" name="cdn" class="form-control" ng-model="deliveryService.cdnId" ng-options="cdn.id as cdn.name for cdn in cdns" required>
+ <select id="cdn" name="cdn" class="form-control" ng-model="deliveryService.cdnId" ng-options="cdn.id as cdn.name for cdn in cdns" required ng-disabled="(!settings.isNew && deliveryService.sslKeyVersion)">
<option hidden disabled selected value="">Select...</option>
</select>
<small class="input-error" ng-show="hasPropertyError(generalConfig.cdnId, 'required')">Required</small>
@@ -745,8 +745,7 @@ under the License.
</div>
</label>
<div class="col-md-10 col-sm-10 col-xs-12">
- <small class="input-warning" ng-show="!settings.isNew && routingConfig.routingName.$dirty">Warning: Changing the routing name may require SSL certificates to be updated.</small>
- <input id="routingName" name="routingName" type="text" class="form-control" placeholder="Routing name used for the delivery service resulting in FQDN = <routing name>.<key>.<CDN domain>" ng-model="deliveryService.routingName" maxlength="48" pattern="[a-z0-9]([a-z\-0-9]*[a-z0-9])?" required>
+ <input id="routingName" name="routingName" type="text" class="form-control" placeholder="Routing name used for the delivery service resulting in FQDN = <routing name>.<key>.<CDN domain>" ng-model="deliveryService.routingName" maxlength="48" pattern="[a-z0-9]([a-z\-0-9]*[a-z0-9])?" required ng-disabled="(!settings.isNew && deliveryService.sslKeyVersion)">
<small class="input-error" ng-show="hasPropertyError(routingConfig.routingName, 'required')">Required</small>
<small class="input-error" ng-show="hasPropertyError(routingConfig.routingName, 'maxlength')">Too Long</small>
<small class="input-error" ng-show="hasPropertyError(routingConfig.routingName, 'pattern')">Must be a valid DNS label (no special characters, capital letters, periods, underscores, or spaces and cannot begin or end with a hyphen)</small>
diff --git a/traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.HTTP.tpl.html b/traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.HTTP.tpl.html
index b1b811d..6866cf7 100644
--- a/traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.HTTP.tpl.html
+++ b/traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.HTTP.tpl.html
@@ -171,7 +171,7 @@ under the License.
</div>
</label>
<div class="col-md-10 col-sm-10 col-xs-12">
- <select id="cdn" name="cdn" class="form-control" ng-model="deliveryService.cdnId" ng-options="cdn.id as cdn.name for cdn in cdns" required>
+ <select id="cdn" name="cdn" class="form-control" ng-model="deliveryService.cdnId" ng-options="cdn.id as cdn.name for cdn in cdns" required ng-disabled="(!settings.isNew && deliveryService.sslKeyVersion)">
<option hidden disabled selected value="">Select...</option>
</select>
<small class="input-error" ng-show="hasPropertyError(generalConfig.cdnId, 'required')">Required</small>
@@ -745,8 +745,7 @@ under the License.
</div>
</label>
<div class="col-md-10 col-sm-10 col-xs-12">
- <small class="input-warning" ng-show="!settings.isNew && routingConfig.routingName.$dirty">Warning: Changing the routing name may require SSL certificates to be updated.</small>
- <input id="routingName" name="routingName" type="text" class="form-control" placeholder="Routing name used for the delivery service resulting in FQDN = <routing name>.<key>.<CDN domain>" ng-model="deliveryService.routingName" maxlength="48" pattern="[a-z0-9]([a-z\-0-9]*[a-z0-9])?" required>
+ <input id="routingName" name="routingName" type="text" class="form-control" placeholder="Routing name used for the delivery service resulting in FQDN = <routing name>.<key>.<CDN domain>" ng-model="deliveryService.routingName" maxlength="48" pattern="[a-z0-9]([a-z\-0-9]*[a-z0-9])?" required ng-disabled="(!settings.isNew && deliveryService.sslKeyVersion)">
<small class="input-error" ng-show="hasPropertyError(routingConfig.routingName, 'required')">Required</small>
<small class="input-error" ng-show="hasPropertyError(routingConfig.routingName, 'maxlength')">Too Long</small>
<small class="input-error" ng-show="hasPropertyError(routingConfig.routingName, 'pattern')">Must be a valid DNS label (no special characters, capital letters, periods, underscores, or spaces and cannot begin or end with a hyphen)</small>
diff --git a/traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.Steering.tpl.html b/traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.Steering.tpl.html
index 25ec723..667f79f 100644
--- a/traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.Steering.tpl.html
+++ b/traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.Steering.tpl.html
@@ -167,7 +167,7 @@ under the License.
</div>
</label>
<div class="col-md-10 col-sm-10 col-xs-12">
- <select id="cdn" name="cdn" class="form-control" ng-model="deliveryService.cdnId" ng-options="cdn.id as cdn.name for cdn in cdns" required>
+ <select id="cdn" name="cdn" class="form-control" ng-model="deliveryService.cdnId" ng-options="cdn.id as cdn.name for cdn in cdns" required ng-disabled="(!settings.isNew && deliveryService.sslKeyVersion)">
<option hidden disabled selected value="">Select...</option>
</select>
<small class="input-error" ng-show="hasPropertyError(generalConfig.cdnId, 'required')">Required</small>
@@ -319,8 +319,7 @@ under the License.
</div>
</label>
<div class="col-md-10 col-sm-10 col-xs-12">
- <small class="input-warning" ng-show="!settings.isNew && routingConfig.routingName.$dirty">Warning: Changing the routing name may require SSL certificates to be updated.</small>
- <input id="routingName" name="routingName" type="text" class="form-control" placeholder="Routing name used for the delivery service resulting in FQDN = <routing name>.<key>.<CDN domain>" ng-model="deliveryService.routingName" maxlength="48" pattern="[a-z0-9]([a-z\-0-9]*[a-z0-9])?" required>
+ <input id="routingName" name="routingName" type="text" class="form-control" placeholder="Routing name used for the delivery service resulting in FQDN = <routing name>.<key>.<CDN domain>" ng-model="deliveryService.routingName" maxlength="48" pattern="[a-z0-9]([a-z\-0-9]*[a-z0-9])?" required ng-disabled="(!settings.isNew && deliveryService.sslKeyVersion)">
<small class="input-error" ng-show="hasPropertyError(routingConfig.routingName, 'required')">Required</small>
<small class="input-error" ng-show="hasPropertyError(routingConfig.routingName, 'maxlength')">Too Long</small>
<small class="input-error" ng-show="hasPropertyError(routingConfig.routingName, 'pattern')">Must be a valid DNS label (no special characters, capital letters, periods, underscores, or spaces and cannot begin or end with a hyphen)</small>