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/04/30 19:54:05 UTC
[GitHub] [trafficcontrol] rob05c opened a new pull request #5802: Add Per-DS HTTP/2 and TLS Versions Support
rob05c opened a new pull request #5802:
URL: https://github.com/apache/trafficcontrol/pull/5802
Adds per-DS H2 and TLS version support, via ssl_server_name.yaml and sni.yaml config generation. See the docs in the PR for details.
Includes tests.
Includes docs.
Includes changelog.
- [x] This PR is not related to any other Issue
## Which Traffic Control components are affected by this PR?
- Traffic Ops ORT
## What is the best way to verify this PR?
Run tests. Run t3c with new args and verify ssl_server_name.yaml or sni.yaml is created appropriately with specified defaults, add parameters to configure a DS and run t3c and verify files are created with specified settings.
## If this is a bug fix, what versions of Traffic Control are affected?
Not a bug fix.
## The following criteria are ALL met by this PR
- [x] This PR includes tests OR I have explained why tests are unnecessary
- [x] This PR includes documentation OR I have explained why documentation is unnecessary
- [x] This PR includes an update to CHANGELOG.md OR such an update is not necessary
- [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)
## Additional Information
--
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 #5802: Add Per-DS HTTP/2 and TLS Versions Support
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5802:
URL: https://github.com/apache/trafficcontrol/pull/5802#discussion_r624310449
##########
File path: docs/source/overview/profiles_and_parameters.rst
##########
@@ -495,6 +495,8 @@ This configuration file is generated entirely from :term:`Cache Group` relations
- ``psel.qstring_handling``
- ``not_a_parent`` - unlike the other Parameters listed (which have a 1:1 correspondence with Apache Traffic Server configuration options), this Parameter affects the generation of :term:`parent` relationships between :term:`cache servers`. When a Parameter with this :ref:`parameter-name` and Config File exists on a :ref:`Profile <profiles>` used by a :term:`cache server`, it will not be added as a :term:`parent` of any other :term:`cache server`, regardless of :term:`Cache Group` hierarchy. Under ordinary circumstances, there's no real reason for this Parameter to exist.
- ``try_all_primaries_before_secondary`` - on a Delivery Service Profile, if this exists, try all primary parents before failing over to secondary parents, which may be ideal if objects are unlikely to be in cache. The default behavior is to immediately fail to a secondary, which is ideal if objects are likely to be in cache, as the first consistent-hashed secondary parent will be the primary parent in its own cachegroup and therefore receive requests for that object from clients near its own cachegroup.
+- ``enable_h2`` - on a Delivery Service Profile, if this is ``true``, enable HTTP/2 for client requests. Note ATS must also be listening for HTTP/2 in records.config, or this will have no effect. Note this setting is not in the parent.config config file, but either ssl_server_name.yaml in ATS 8 or sni.yaml in ATS 9. The Parameter has the "parent.config" config file for consistency.
Review comment:
We should group all these (including `try_all_primaries_before_secondary`) in https://traffic-control-cdn.readthedocs.io/en/latest/overview/delivery_services.html somewhere, since they're really like "extended fields" IMO
--
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] rob05c commented on a change in pull request #5802: Add Per-DS HTTP/2 and TLS Versions Support
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5802:
URL: https://github.com/apache/trafficcontrol/pull/5802#discussion_r626938636
##########
File path: docs/source/overview/delivery_services.rst
##########
@@ -1023,3 +1023,20 @@ A text-based unique identifier for a Delivery Service. Many :ref:`to-api` endpoi
.. [#cardinality] In source code and :ref:`to-api` responses, the "Long Description" fields of a Delivery Service are "0-indexed" - hence the names differing slightly from the ones displayed in user-friendly UIs.
.. [#dupOrigin] These Delivery Services Types are vulnerable to what this writer likes to call the "Duplicate Origin Problem". This problem is tracked by :issue:`3537`.
.. [#httpOnlyRegex] These regular expression types can only appear in the Match List of HTTP-:ref:`Routed <ds-types>` Delivery Services.
+
+
+Parameters
+----------------------
Review comment:
Changed
--
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 #5802: Add Per-DS HTTP/2 and TLS Versions Support
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5802:
URL: https://github.com/apache/trafficcontrol/pull/5802#discussion_r625899091
##########
File path: CHANGELOG.md
##########
@@ -36,6 +36,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
- Added integration to use ACME to generate new SSL certificates.
- Add a Federation to the Ansible Dataset Loader
- Added asynchronous status to ACME certificate generation.
+- Added per Delivery Service HTTP/2 and TLS Versions support, via ssl_server_name.yaml and sni.yaml. See overview/profiles_and_parameters and t3c docs.
Review comment:
`See overview/profiles_and_parameters` is no longer accurate
##########
File path: docs/source/overview/delivery_services.rst
##########
@@ -1023,3 +1023,20 @@ A text-based unique identifier for a Delivery Service. Many :ref:`to-api` endpoi
.. [#cardinality] In source code and :ref:`to-api` responses, the "Long Description" fields of a Delivery Service are "0-indexed" - hence the names differing slightly from the ones displayed in user-friendly UIs.
.. [#dupOrigin] These Delivery Services Types are vulnerable to what this writer likes to call the "Duplicate Origin Problem". This problem is tracked by :issue:`3537`.
.. [#httpOnlyRegex] These regular expression types can only appear in the Match List of HTTP-:ref:`Routed <ds-types>` Delivery Services.
+
+
+Parameters
+----------------------
Review comment:
nit: number of dashes should match the length of the header
--
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] rob05c commented on a change in pull request #5802: Add Per-DS HTTP/2 and TLS Versions Support
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5802:
URL: https://github.com/apache/trafficcontrol/pull/5802#discussion_r625195384
##########
File path: docs/source/overview/profiles_and_parameters.rst
##########
@@ -495,6 +495,8 @@ This configuration file is generated entirely from :term:`Cache Group` relations
- ``psel.qstring_handling``
- ``not_a_parent`` - unlike the other Parameters listed (which have a 1:1 correspondence with Apache Traffic Server configuration options), this Parameter affects the generation of :term:`parent` relationships between :term:`cache servers`. When a Parameter with this :ref:`parameter-name` and Config File exists on a :ref:`Profile <profiles>` used by a :term:`cache server`, it will not be added as a :term:`parent` of any other :term:`cache server`, regardless of :term:`Cache Group` hierarchy. Under ordinary circumstances, there's no real reason for this Parameter to exist.
- ``try_all_primaries_before_secondary`` - on a Delivery Service Profile, if this exists, try all primary parents before failing over to secondary parents, which may be ideal if objects are unlikely to be in cache. The default behavior is to immediately fail to a secondary, which is ideal if objects are likely to be in cache, as the first consistent-hashed secondary parent will be the primary parent in its own cachegroup and therefore receive requests for that object from clients near its own cachegroup.
+- ``enable_h2`` - on a Delivery Service Profile, if this is ``true``, enable HTTP/2 for client requests. Note ATS must also be listening for HTTP/2 in records.config, or this will have no effect. Note this setting is not in the parent.config config file, but either ssl_server_name.yaml in ATS 8 or sni.yaml in ATS 9. The Parameter has the "parent.config" config file for consistency.
Review comment:
Done.
--
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] ezelkow1 commented on pull request #5802: Add Per-DS HTTP/2 and TLS Versions Support
Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on pull request #5802:
URL: https://github.com/apache/trafficcontrol/pull/5802#issuecomment-831469720
Tested with a test box changing between an 8x and 9x profile. First on 8x saw it properly generate the ssl_server_name.yaml based on the cmd line options I used (default-client-enable-h2=true and tls-versions=1.3,1.1). Then changed it's profile to a 9x profile, saw it generate the sni.yaml (and the ssl_server_name.yaml), how the sni version had disable_h2: true, and listed all tls variants in the list. The ssl_server_name was still correct, but since this is a 9x profile it would be unused
--
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] rob05c commented on pull request #5802: Add Per-DS HTTP/2 and TLS Versions Support
Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #5802:
URL: https://github.com/apache/trafficcontrol/pull/5802#issuecomment-831600645
@ezelkow1 Fixed. It did have a check for that, but the `if { warn` was missing a `continue`. Fixed, and added a unit test for that.
--
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 #5802: Add Per-DS HTTP/2 and TLS Versions Support
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5802:
URL: https://github.com/apache/trafficcontrol/pull/5802#discussion_r625925762
##########
File path: docs/source/overview/delivery_services.rst
##########
@@ -1023,3 +1023,20 @@ A text-based unique identifier for a Delivery Service. Many :ref:`to-api` endpoi
.. [#cardinality] In source code and :ref:`to-api` responses, the "Long Description" fields of a Delivery Service are "0-indexed" - hence the names differing slightly from the ones displayed in user-friendly UIs.
.. [#dupOrigin] These Delivery Services Types are vulnerable to what this writer likes to call the "Duplicate Origin Problem". This problem is tracked by :issue:`3537`.
.. [#httpOnlyRegex] These regular expression types can only appear in the Match List of HTTP-:ref:`Routed <ds-types>` Delivery Services.
+
+
+Parameters
+----------------------
+Features which are new, experimental, or not significant enough to be first-class Delivery Service fields are often added as Parameters. To use these, add a Profile to the Delivery Service, with the given Parameter assigned.
+
+parent.config
+'''''''''''''
+The following Parameters must have the Config File ``parent.config`` to take effect.
+
+- ``try_all_primaries_before_secondary`` - on a Delivery Service Profile, if this exists, try all primary parents before failing over to secondary parents, which may be ideal if objects are unlikely to be in cache. The default behavior is to immediately fail to a secondary, which is ideal if objects are likely to be in cache, as the first consistent-hashed secondary parent will be the primary parent in its own cachegroup and therefore receive requests for that object from clients near its own cachegroup.
+- ``enable_h2`` - on a Delivery Service Profile, if this is ``true``, enable HTTP/2 for client requests. Note ATS must also be listening for HTTP/2 in records.config, or this will have no effect. Note this setting is not in the parent.config config file, but either ssl_server_name.yaml in ATS 8 or sni.yaml in ATS 9. The Parameter has the "parent.config" config file for consistency.
+- ``tls_versions`` - on a Delivery Service Profile, if this exists, enable the given comma-delimited TLS versions for client requests. For example, ``1.1,1.2,1.3``. Note ATS must also be accepting those TLS versions in records.config, or this will have no effect. Note this setting is not in the parent.config config file, but either ssl_server_name.yaml in ATS 8 or sni.yaml in ATS 9. The Parameter has the "parent.config" config file for consistency.
+
+Additionally, :term:`Delivery Service` :ref:`Profiles <ds-profile>` can have special Parameters with the :ref:`parameter-name` "mso.parent_retry" to :ref:`multi-site-origin-qht`.
+
+.. seealso:: To see how the :ref:`Values <parameter-value>` of these Parameters are interpreted, refer to the `Apache Traffic Server documentation on the parent.config configuration file <https://docs.trafficserver.apache.org/en/7.1.x/admin-guide/files/parent.config.en.html>`_
Review comment:
I think it's fine to put these here, but it'd be nice to have a `.. seealso` link in the Profiles and Parameters overview section that takes people here, so that you can still reach any specially handled Parameter definition from that one overview
--
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] rob05c commented on a change in pull request #5802: Add Per-DS HTTP/2 and TLS Versions Support
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5802:
URL: https://github.com/apache/trafficcontrol/pull/5802#discussion_r626959660
##########
File path: docs/source/overview/delivery_services.rst
##########
@@ -1023,3 +1023,20 @@ A text-based unique identifier for a Delivery Service. Many :ref:`to-api` endpoi
.. [#cardinality] In source code and :ref:`to-api` responses, the "Long Description" fields of a Delivery Service are "0-indexed" - hence the names differing slightly from the ones displayed in user-friendly UIs.
.. [#dupOrigin] These Delivery Services Types are vulnerable to what this writer likes to call the "Duplicate Origin Problem". This problem is tracked by :issue:`3537`.
.. [#httpOnlyRegex] These regular expression types can only appear in the Match List of HTTP-:ref:`Routed <ds-types>` Delivery Services.
+
+
+Parameters
+----------------------
+Features which are new, experimental, or not significant enough to be first-class Delivery Service fields are often added as Parameters. To use these, add a Profile to the Delivery Service, with the given Parameter assigned.
+
+parent.config
+'''''''''''''
+The following Parameters must have the Config File ``parent.config`` to take effect.
+
+- ``try_all_primaries_before_secondary`` - on a Delivery Service Profile, if this exists, try all primary parents before failing over to secondary parents, which may be ideal if objects are unlikely to be in cache. The default behavior is to immediately fail to a secondary, which is ideal if objects are likely to be in cache, as the first consistent-hashed secondary parent will be the primary parent in its own cachegroup and therefore receive requests for that object from clients near its own cachegroup.
+- ``enable_h2`` - on a Delivery Service Profile, if this is ``true``, enable HTTP/2 for client requests. Note ATS must also be listening for HTTP/2 in records.config, or this will have no effect. Note this setting is not in the parent.config config file, but either ssl_server_name.yaml in ATS 8 or sni.yaml in ATS 9. The Parameter has the "parent.config" config file for consistency.
+- ``tls_versions`` - on a Delivery Service Profile, if this exists, enable the given comma-delimited TLS versions for client requests. For example, ``1.1,1.2,1.3``. Note ATS must also be accepting those TLS versions in records.config, or this will have no effect. Note this setting is not in the parent.config config file, but either ssl_server_name.yaml in ATS 8 or sni.yaml in ATS 9. The Parameter has the "parent.config" config file for consistency.
+
+Additionally, :term:`Delivery Service` :ref:`Profiles <ds-profile>` can have special Parameters with the :ref:`parameter-name` "mso.parent_retry" to :ref:`multi-site-origin-qht`.
+
+.. seealso:: To see how the :ref:`Values <parameter-value>` of these Parameters are interpreted, refer to the `Apache Traffic Server documentation on the parent.config configuration file <https://docs.trafficserver.apache.org/en/7.1.x/admin-guide/files/parent.config.en.html>`_
Review comment:
Added
--
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] rob05c commented on a change in pull request #5802: Add Per-DS HTTP/2 and TLS Versions Support
Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #5802:
URL: https://github.com/apache/trafficcontrol/pull/5802#discussion_r626941335
##########
File path: CHANGELOG.md
##########
@@ -36,6 +36,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
- Added integration to use ACME to generate new SSL certificates.
- Add a Federation to the Ansible Dataset Loader
- Added asynchronous status to ACME certificate generation.
+- Added per Delivery Service HTTP/2 and TLS Versions support, via ssl_server_name.yaml and sni.yaml. See overview/profiles_and_parameters and t3c docs.
Review comment:
Changed
--
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] ezelkow1 edited a comment on pull request #5802: Add Per-DS HTTP/2 and TLS Versions Support
Posted by GitBox <gi...@apache.org>.
ezelkow1 edited a comment on pull request #5802:
URL: https://github.com/apache/trafficcontrol/pull/5802#issuecomment-831469720
Tested with a test box changing between an 8x and 9x profile. First on 8x saw it properly generate the ssl_server_name.yaml based on the cmd line options I used (default-client-enable-h2=true and tls-versions=1.3,1.1). Then changed it's profile to a 9x profile, saw it generate the sni.yaml (and the ssl_server_name.yaml), however the sni version had disable_h2: true, and listed all tls variants in the list. The ssl_server_name was still correct, but since this is a 9x profile it would be unused
--
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] dneuman64 merged pull request #5802: Add Per-DS HTTP/2 and TLS Versions Support
Posted by GitBox <gi...@apache.org>.
dneuman64 merged pull request #5802:
URL: https://github.com/apache/trafficcontrol/pull/5802
--
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] ezelkow1 commented on pull request #5802: Add Per-DS HTTP/2 and TLS Versions Support
Posted by GitBox <gi...@apache.org>.
ezelkow1 commented on pull request #5802:
URL: https://github.com/apache/trafficcontrol/pull/5802#issuecomment-831522552
problem fixed with latest pushes (fixed cmdline issues, duplicate files are because of params). Only issue that I see left is if you give it a bad param name for a TLS version, you end up with an empty entry in the list of tls_versions in the yaml file
--
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