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 2021/05/04 16:49:22 UTC
[GitHub] [trafficcontrol] rawlinp opened a new pull request #5810: Fix CDN DNSSEC key generation timeout
rawlinp opened a new pull request #5810:
URL: https://github.com/apache/trafficcontrol/pull/5810
## What does this PR (Pull Request) do?
With large numbers of delivery services, serially generating DNSSEC keys
for a CDN can take longer than the default 20 second timeout, causing
the DNSSEC key generation operation to time out and fail. Instead,
generate DNSSEC keys in parallel (by default using 66% of the available
CPUs) in order to complete DNSSEC key generation more quickly. Also, use
a separate DB transaction timeout for writing the changelog.
- [x] This PR fixes #5732
## Which Traffic Control components are affected by this PR?
- Documentation
- Traffic Control Client (Go)
- Traffic Ops
- Traffic Vault
## What is the best way to verify this PR?
In an environment with a large number of delivery services (e.g. > 500), generate DNSSEC keys for the CDN. The operation should succeed.
## The following criteria are ALL met by this PR
- [x] This PR includes tests
- [x] This PR includes documentation
- [x] This PR includes an update to CHANGELOG.md
- [x] This PR includes any and all required license headers
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5810: Fix CDN DNSSEC key generation timeout
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5810:
URL: https://github.com/apache/trafficcontrol/pull/5810#discussion_r626148285
##########
File path: lib/go-tc/cdns_dnssec.go
##########
@@ -0,0 +1,163 @@
+package tc
+
+/*
+ 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 (
+ "database/sql"
+ "errors"
+ "strconv"
+ "strings"
+ "time"
+
+ "github.com/apache/trafficcontrol/lib/go-util"
+)
+
+const (
+ DNSSECKSKType = "ksk"
+ DNSSECZSKType = "zsk"
+ DNSSECKeyStatusNew = "new"
+ DNSSECKeyStatusExpired = "expired"
+ DNSSECStatusExisting = "existing"
+)
+
+type CDNDNSSECKeysResponse struct {
+ Response DNSSECKeys `json:"response"`
+ Alerts
+}
+
+type GenerateCDNDNSSECKeysResponse struct {
+ Response string `json:"response"`
+ Alerts
+}
+
+type DeleteCDNDNSSECKeysResponse GenerateCDNDNSSECKeysResponse
+
+// DNSSECKeys is the DNSSEC keys as stored in Riak, plus the DS record text.
+type DNSSECKeys map[string]DNSSECKeySet
+
+// Deprecated: use DNSSECKeysTrafficVault instead
+type DNSSECKeysRiak DNSSECKeysV11
+
+type DNSSECKeysTrafficVault DNSSECKeysV11
+
+// DNSSECKeysV11 is the DNSSEC keys object stored in Riak. The map key strings are both DeliveryServiceNames and CDNNames.
+type DNSSECKeysV11 map[string]DNSSECKeySetV11
+
+type DNSSECKeySet struct {
+ ZSK []DNSSECKey `json:"zsk"`
+ KSK []DNSSECKey `json:"ksk"`
+}
+
+// DNSSECKeySetV11 is a DNSSEC key set (ZSK and KSK), as stored in Riak.
+// This is specifically the key data, without the DS record text (which can be computed), and is also the format used in API 1.1 through 1.3.
+type DNSSECKeySetV11 struct {
+ ZSK []DNSSECKeyV11 `json:"zsk"`
+ KSK []DNSSECKeyV11 `json:"ksk"`
+}
+
+type DNSSECKey struct {
+ DNSSECKeyV11
+ DSRecord *DNSSECKeyDSRecord `json:"dsRecord,omitempty"`
+}
+
+type DNSSECKeyV11 struct {
+ InceptionDateUnix int64 `json:"inceptionDate"`
+ ExpirationDateUnix int64 `json:"expirationDate"`
+ Name string `json:"name"`
+ TTLSeconds uint64 `json:"ttl,string"`
+ Status string `json:"status"`
+ EffectiveDateUnix int64 `json:"effectiveDate"`
+ Public string `json:"public"`
+ Private string `json:"private"`
+ DSRecord *DNSSECKeyDSRecordV11 `json:"dsRecord,omitempty"`
+}
+
+// DNSSECKeyDSRecordRiak is a DNSSEC key DS record, as stored in Riak.
+// This is specifically the key data, without the DS record text (which can be computed), and is also the format used in API 1.1 through 1.3.
+type DNSSECKeyDSRecordRiak DNSSECKeyDSRecordV11
+
+type DNSSECKeyDSRecord struct {
+ DNSSECKeyDSRecordV11
+ Text string `json:"text"`
+}
+
+type DNSSECKeyDSRecordV11 struct {
+ Algorithm int64 `json:"algorithm,string"`
+ DigestType int64 `json:"digestType,string"`
+ Digest string `json:"digest"`
+}
+
+// CDNDNSSECGenerateReqDate is the date accepted by CDNDNSSECGenerateReq.
+// This will unmarshal a UNIX epoch integer, a RFC3339 string, the old format string used by Perl '2018-08-21+14:26:06', and the old format string sent by the Portal '2018-08-21 14:14:42'.
+// This exists to fix a critical bug, see https://github.com/apache/trafficcontrol/issues/2723 - it SHOULD NOT be used by any other endpoint.
+type CDNDNSSECGenerateReqDate int64
+
+func (i *CDNDNSSECGenerateReqDate) UnmarshalJSON(d []byte) error {
+ const oldPortalDateFormat = `2006-01-02 15:04:05`
+ const oldPerlUIDateFormat = `2006-01-02+15:04:05`
+ if len(d) == 0 {
+ return errors.New("empty object")
+ }
+ if d[0] == '"' {
+ d = d[1 : len(d)-1] // strip JSON quotes, to accept the UNIX epoch as a string or number
+ }
+ if di, err := strconv.ParseInt(string(d), 10, 64); err == nil {
+ *i = CDNDNSSECGenerateReqDate(di)
+ return nil
+ }
+ if t, err := time.Parse(time.RFC3339, string(d)); err == nil {
+ *i = CDNDNSSECGenerateReqDate(t.Unix())
+ return nil
+ }
+ if t, err := time.Parse(oldPortalDateFormat, string(d)); err == nil {
+ *i = CDNDNSSECGenerateReqDate(t.Unix())
+ return nil
+ }
+ if t, err := time.Parse(oldPerlUIDateFormat, string(d)); err == nil {
+ *i = CDNDNSSECGenerateReqDate(t.Unix())
+ return nil
+ }
+ return errors.New("invalid date")
+}
+
+type CDNDNSSECGenerateReq struct {
+ // Key is the CDN name, as documented in the API documentation.
+ Key *string `json:"key"`
+ TTL *util.JSONIntStr `json:"ttl"`
+ KSKExpirationDays *util.JSONIntStr `json:"kskExpirationDays"`
+ ZSKExpirationDays *util.JSONIntStr `json:"zskExpirationDays"`
+ EffectiveDateUnix *CDNDNSSECGenerateReqDate `json:"effectiveDate"`
+}
+
+func (r CDNDNSSECGenerateReq) Validate(tx *sql.Tx) error {
+ errs := []string{}
+ if r.Key == nil {
+ errs = append(errs, "key (cdn name) must be set")
+ }
+ if r.TTL == nil {
+ errs = append(errs, "ttl must be set")
+ }
+ if r.KSKExpirationDays == nil {
+ errs = append(errs, "kskExpirationDays must be set")
+ }
+ if r.ZSKExpirationDays == nil {
+ errs = append(errs, "zskExpirationDays must be set")
+ }
+ // effective date is optional
+ if len(errs) > 0 {
+ return errors.New("missing fields: " + strings.Join(errs, "; "))
+ }
Review comment:
I didn't add this code, I just moved it into a more targeted file and removed the `r.Name` check. But I could change it to use that function instead.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5810: Fix CDN DNSSEC key generation timeout
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5810:
URL: https://github.com/apache/trafficcontrol/pull/5810#discussion_r626118440
##########
File path: docs/source/api/v2/cdns_dnsseckeys_generate.rst
##########
@@ -32,7 +32,6 @@ Request Structure
:effectiveDate: An optional string containing the date and time at which the newly-generated :abbr:`ZSK (Zone-Signing Key)` and :abbr:`KSK (Key-Signing Key)` become effective, in :RFC:`3339` format. Defaults to the current time if not specified.
:key: Name of the CDN
:kskExpirationDays: Expiration (in days) for the :abbr:`KSKs (Key-Signing Keys)`
-:name: Domain name used by the CDN
Review comment:
Oh, this endpoint only has a POST method. I wouldn't have removed it even if it's no longer used, personally, but I suppose it's not a big deal.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5810: Fix CDN DNSSEC key generation timeout
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5810:
URL: https://github.com/apache/trafficcontrol/pull/5810#discussion_r626109312
##########
File path: docs/source/api/v2/cdns_dnsseckeys_generate.rst
##########
@@ -32,7 +32,6 @@ Request Structure
:effectiveDate: An optional string containing the date and time at which the newly-generated :abbr:`ZSK (Zone-Signing Key)` and :abbr:`KSK (Key-Signing Key)` become effective, in :RFC:`3339` format. Defaults to the current time if not specified.
:key: Name of the CDN
:kskExpirationDays: Expiration (in days) for the :abbr:`KSKs (Key-Signing Keys)`
-:name: Domain name used by the CDN
Review comment:
It's not really a breaking change, it's a completely unused field that has no effect on behavior/data whatsoever. API users can continue to send it, it will just be totally ignored (as it basically was before, except without the unnecessary validation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5810: Fix CDN DNSSEC key generation timeout
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5810:
URL: https://github.com/apache/trafficcontrol/pull/5810#discussion_r626146257
##########
File path: docs/source/api/v2/cdns_dnsseckeys_generate.rst
##########
@@ -32,7 +32,6 @@ Request Structure
:effectiveDate: An optional string containing the date and time at which the newly-generated :abbr:`ZSK (Zone-Signing Key)` and :abbr:`KSK (Key-Signing Key)` become effective, in :RFC:`3339` format. Defaults to the current time if not specified.
:key: Name of the CDN
:kskExpirationDays: Expiration (in days) for the :abbr:`KSKs (Key-Signing Keys)`
-:name: Domain name used by the CDN
Review comment:
I know, but I think we should reserve the right to remove things like this when it makes sense.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman merged pull request #5810: Fix CDN DNSSEC key generation timeout
Posted by GitBox <gi...@apache.org>.
zrhoffman merged pull request #5810:
URL: https://github.com/apache/trafficcontrol/pull/5810
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5810: Fix CDN DNSSEC key generation timeout
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5810:
URL: https://github.com/apache/trafficcontrol/pull/5810#discussion_r626104069
##########
File path: docs/source/api/v2/cdns_dnsseckeys_generate.rst
##########
@@ -32,7 +32,6 @@ Request Structure
:effectiveDate: An optional string containing the date and time at which the newly-generated :abbr:`ZSK (Zone-Signing Key)` and :abbr:`KSK (Key-Signing Key)` become effective, in :RFC:`3339` format. Defaults to the current time if not specified.
:key: Name of the CDN
:kskExpirationDays: Expiration (in days) for the :abbr:`KSKs (Key-Signing Keys)`
-:name: Domain name used by the CDN
Review comment:
I don't think we can make this breaking change to the v2 API, as it's already been released
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #5810: Fix CDN DNSSEC key generation timeout
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5810:
URL: https://github.com/apache/trafficcontrol/pull/5810#discussion_r626110333
##########
File path: traffic_ops/traffic_ops_golang/cdn/dnssec.go
##########
@@ -86,7 +94,26 @@ func CreateDNSSECKeys(w http.ResponseWriter, r *http.Request) {
api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("generating and storing DNSSEC CDN keys: "+err.Error()))
return
}
- api.CreateChangeLogRawTx(api.ApiChange, "CDN: "+string(cdnName)+", ID: "+strconv.Itoa(cdnID)+", ACTION: Generated DNSSEC keys", inf.User, inf.Tx.Tx)
+ // NOTE: using a separate transaction (with its own timeout) for the changelog because the main
+ // transaction can time out if DNSSEC generation takes too long
+ db, err := api.GetDB(r.Context())
+ if err != nil {
+ api.HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("generating CDN DNSSEC keys: getting DB from request context for changelog: "+err.Error()))
+ return
+ }
+ logCtx, logCancel := context.WithTimeout(r.Context(), time.Duration(30)*time.Second)
Review comment:
This can be just
```go
logCtx, logCancel := context.WithTimeout(r.Context(), 30*time.Second)
```
##########
File path: lib/go-tc/cdns_dnssec.go
##########
@@ -0,0 +1,163 @@
+package tc
+
+/*
+ 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 (
+ "database/sql"
+ "errors"
+ "strconv"
+ "strings"
+ "time"
+
+ "github.com/apache/trafficcontrol/lib/go-util"
+)
+
+const (
+ DNSSECKSKType = "ksk"
+ DNSSECZSKType = "zsk"
+ DNSSECKeyStatusNew = "new"
+ DNSSECKeyStatusExpired = "expired"
+ DNSSECStatusExisting = "existing"
+)
+
+type CDNDNSSECKeysResponse struct {
+ Response DNSSECKeys `json:"response"`
+ Alerts
+}
+
+type GenerateCDNDNSSECKeysResponse struct {
+ Response string `json:"response"`
+ Alerts
+}
+
+type DeleteCDNDNSSECKeysResponse GenerateCDNDNSSECKeysResponse
+
+// DNSSECKeys is the DNSSEC keys as stored in Riak, plus the DS record text.
+type DNSSECKeys map[string]DNSSECKeySet
+
+// Deprecated: use DNSSECKeysTrafficVault instead
+type DNSSECKeysRiak DNSSECKeysV11
+
+type DNSSECKeysTrafficVault DNSSECKeysV11
+
+// DNSSECKeysV11 is the DNSSEC keys object stored in Riak. The map key strings are both DeliveryServiceNames and CDNNames.
+type DNSSECKeysV11 map[string]DNSSECKeySetV11
+
+type DNSSECKeySet struct {
+ ZSK []DNSSECKey `json:"zsk"`
+ KSK []DNSSECKey `json:"ksk"`
+}
+
+// DNSSECKeySetV11 is a DNSSEC key set (ZSK and KSK), as stored in Riak.
+// This is specifically the key data, without the DS record text (which can be computed), and is also the format used in API 1.1 through 1.3.
+type DNSSECKeySetV11 struct {
+ ZSK []DNSSECKeyV11 `json:"zsk"`
+ KSK []DNSSECKeyV11 `json:"ksk"`
+}
+
+type DNSSECKey struct {
+ DNSSECKeyV11
+ DSRecord *DNSSECKeyDSRecord `json:"dsRecord,omitempty"`
+}
+
+type DNSSECKeyV11 struct {
+ InceptionDateUnix int64 `json:"inceptionDate"`
+ ExpirationDateUnix int64 `json:"expirationDate"`
+ Name string `json:"name"`
+ TTLSeconds uint64 `json:"ttl,string"`
+ Status string `json:"status"`
+ EffectiveDateUnix int64 `json:"effectiveDate"`
+ Public string `json:"public"`
+ Private string `json:"private"`
+ DSRecord *DNSSECKeyDSRecordV11 `json:"dsRecord,omitempty"`
+}
+
+// DNSSECKeyDSRecordRiak is a DNSSEC key DS record, as stored in Riak.
+// This is specifically the key data, without the DS record text (which can be computed), and is also the format used in API 1.1 through 1.3.
+type DNSSECKeyDSRecordRiak DNSSECKeyDSRecordV11
+
+type DNSSECKeyDSRecord struct {
+ DNSSECKeyDSRecordV11
+ Text string `json:"text"`
+}
+
+type DNSSECKeyDSRecordV11 struct {
+ Algorithm int64 `json:"algorithm,string"`
+ DigestType int64 `json:"digestType,string"`
+ Digest string `json:"digest"`
+}
+
+// CDNDNSSECGenerateReqDate is the date accepted by CDNDNSSECGenerateReq.
+// This will unmarshal a UNIX epoch integer, a RFC3339 string, the old format string used by Perl '2018-08-21+14:26:06', and the old format string sent by the Portal '2018-08-21 14:14:42'.
+// This exists to fix a critical bug, see https://github.com/apache/trafficcontrol/issues/2723 - it SHOULD NOT be used by any other endpoint.
+type CDNDNSSECGenerateReqDate int64
+
+func (i *CDNDNSSECGenerateReqDate) UnmarshalJSON(d []byte) error {
+ const oldPortalDateFormat = `2006-01-02 15:04:05`
+ const oldPerlUIDateFormat = `2006-01-02+15:04:05`
+ if len(d) == 0 {
+ return errors.New("empty object")
+ }
+ if d[0] == '"' {
+ d = d[1 : len(d)-1] // strip JSON quotes, to accept the UNIX epoch as a string or number
+ }
+ if di, err := strconv.ParseInt(string(d), 10, 64); err == nil {
+ *i = CDNDNSSECGenerateReqDate(di)
+ return nil
+ }
+ if t, err := time.Parse(time.RFC3339, string(d)); err == nil {
+ *i = CDNDNSSECGenerateReqDate(t.Unix())
+ return nil
+ }
+ if t, err := time.Parse(oldPortalDateFormat, string(d)); err == nil {
+ *i = CDNDNSSECGenerateReqDate(t.Unix())
+ return nil
+ }
+ if t, err := time.Parse(oldPerlUIDateFormat, string(d)); err == nil {
+ *i = CDNDNSSECGenerateReqDate(t.Unix())
+ return nil
+ }
+ return errors.New("invalid date")
+}
+
+type CDNDNSSECGenerateReq struct {
+ // Key is the CDN name, as documented in the API documentation.
+ Key *string `json:"key"`
+ TTL *util.JSONIntStr `json:"ttl"`
+ KSKExpirationDays *util.JSONIntStr `json:"kskExpirationDays"`
+ ZSKExpirationDays *util.JSONIntStr `json:"zskExpirationDays"`
+ EffectiveDateUnix *CDNDNSSECGenerateReqDate `json:"effectiveDate"`
+}
+
+func (r CDNDNSSECGenerateReq) Validate(tx *sql.Tx) error {
+ errs := []string{}
+ if r.Key == nil {
+ errs = append(errs, "key (cdn name) must be set")
+ }
+ if r.TTL == nil {
+ errs = append(errs, "ttl must be set")
+ }
+ if r.KSKExpirationDays == nil {
+ errs = append(errs, "kskExpirationDays must be set")
+ }
+ if r.ZSKExpirationDays == nil {
+ errs = append(errs, "zskExpirationDays must be set")
+ }
+ // effective date is optional
+ if len(errs) > 0 {
+ return errors.New("missing fields: " + strings.Join(errs, "; "))
+ }
Review comment:
Any reason not to use `tovalidate.ToErrors(validation.Errors{})` to validate required fields?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org