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/02/11 23:18:48 UTC
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5514: Acme auto renew
ocket8888 commented on a change in pull request #5514:
URL: https://github.com/apache/trafficcontrol/pull/5514#discussion_r574889917
##########
File path: docs/source/api/v1/letsencrypt_autorenew.rst
##########
@@ -36,54 +36,20 @@ No parameters available
Response Structure
------------------
-:LetsEncryptExpirations: A list of objects with information regarding certificate expiration for all Let's Encrypt certificates
-
- :XmlId: The :term:`Delivery Service`'s uniquely identifying :ref:`ds-xmlid`
- :Version: An integer that defines the "version" of the key - which may be thought of as the sequential generation; that is, the higher the number the more recent the key
- :Expiration: The expiration date of the certificate for the :term:`Delivery Service` in :rfc:`3339` format
- :AuthType: The authority type of the certificate for the :term:`Delivery Service`
- :Error: Any errors received in the renewal process
-
-:SelfSignedExpirations: A list of objects with information regarding certificate expiration for all self signed certificates
-
- :XmlId: The :term:`Delivery Service`'s uniquely identifying :ref:`ds-xmlid`
- :Version: An integer that defines the "version" of the key - which may be thought of as the sequential generation; that is, the higher the number the more recent the key
- :Expiration: The expiration date of the certificate for the :term:`Delivery Service` in :rfc:`3339` format
- :AuthType: The authority type of the certificate for the :term:`Delivery Service`
- :Error: Any errors received in the renewal process
-
-:OtherExpirations: A list of objects with information regarding certificate expiration for all other certificates
-
- :XmlId: The :term:`Delivery Service`'s uniquely identifying :ref:`ds-xmlid`
- :Version: An integer that defines the "version" of the key - which may be thought of as the sequential generation; that is, the higher the number the more recent the key
- :Expiration: The expiration date of the certificate for the :term:`Delivery Service` in :rfc:`3339` format
- :AuthType: The authority type of the certificate for the :term:`Delivery Service`
- :Error: Any errors received in the renewal process
.. code-block:: http
:caption: Response Example
HTTP/1.1 200 OK
Content-Type: application/json
- { "response": {
- "LetsEncryptExpirations": [
- {
- "XmlId":"demo2",
- "Version":1,
- "Expiration":"2020-08-18T13:53:06Z",
- "AuthType":"Lets Encrypt",
- "Error":null
- }
- ],
- "SelfSignedExpirations": [
- {
- "XmlId":"demo1",
- "Version":3,
- "Expiration":"2020-08-18T13:53:06Z",
- "AuthType":"Self Signed",
- "Error":null
- }
- ],
- "OtherExpirations":null
- }}
+ { "alerts": [
+ {
+ "text": "This endpoint is deprecated, please use letsencrypt/autorenew instead",
+ "level": "warning"
+ },
+ {
+ "text": "Beginning async call to renew certificates. This may take a few minutes.",
+ "level": "success"
+ }
+ ]}
Review comment:
You can't change the response structure of old API versions. If there's really no way to meaningfully reproduce it, then you can just use meaningless data, but this is a breaking client change to a released API version.
##########
File path: docs/source/api/v4/acme_autorenew.rst
##########
@@ -0,0 +1,49 @@
+..
+..
+.. 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.
+..
+
+.. _to-api-acnme-autorenew:
+
+******************
+``acme_autorenew``
+******************
+
+``POST``
+========
+Generates SSL certificates and private keys for all :term:`Delivery Services` that have certificates expiring within the configured time. This uses:abbr:`ACME (Automatic Certificate Management Environment)` or Let's Encrypt depending on the certificate.
+
+:Auth. Required: Yes
+:Roles Required: "admin" or "operations"
+:Response Type: Object
Review comment:
I think the response type is actually `undefined` here.
##########
File path: docs/source/api/v4/acme_autorenew.rst
##########
@@ -0,0 +1,49 @@
+..
+..
+.. 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.
+..
+
+.. _to-api-acnme-autorenew:
+
+******************
+``acme_autorenew``
+******************
+
+``POST``
+========
+Generates SSL certificates and private keys for all :term:`Delivery Services` that have certificates expiring within the configured time. This uses:abbr:`ACME (Automatic Certificate Management Environment)` or Let's Encrypt depending on the certificate.
+
+:Auth. Required: Yes
+:Roles Required: "admin" or "operations"
+:Response Type: Object
+
+Request Structure
+-----------------
+No parameters available
+
+
+Response Structure
+------------------
+
+.. code-block:: http
+ :caption: Response Example
+
+ HTTP/1.1 200 OK
+ Content-Type: application/json
+
+ { "alerts": [
+ {
+ "text": "Beginning async call to renew certificates. This may take a few minutes.",
+ "level": "success"
+ }
+ ]}
Review comment:
indentation within code blocks can use spaces if you want, but the first indent that associates the code with the `.. code-block` directive **must** be a tab. I think in this particular case this won't render properly.
##########
File path: docs/source/api/v4/acme_autorenew.rst
##########
@@ -0,0 +1,49 @@
+..
+..
+.. 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.
+..
+
+.. _to-api-acnme-autorenew:
+
+******************
+``acme_autorenew``
+******************
+
+``POST``
+========
+Generates SSL certificates and private keys for all :term:`Delivery Services` that have certificates expiring within the configured time. This uses:abbr:`ACME (Automatic Certificate Management Environment)` or Let's Encrypt depending on the certificate.
+
+:Auth. Required: Yes
+:Roles Required: "admin" or "operations"
+:Response Type: Object
+
+Request Structure
+-----------------
+No parameters available
+
+
+Response Structure
+------------------
+
+.. code-block:: http
+ :caption: Response Example
+
+ HTTP/1.1 200 OK
Review comment:
From [the API guidelines](https://traffic-control-cdn.readthedocs.io/en/latest/development/api_guidelines.html#accepted):
> _"`202 Accepted` MUST be used when the server is performing some task asynchronously (e.g. refreshing DNSSEC keys) but the status of that task cannot be ascertained at the current time._"
There's also:
> _"Endpoints that create asynchronous jobs SHOULD provide a URI to which the client may send GET requests to obtain a representation of the job’s current state in the Location HTTP header. They MAY also provide an info-level Alert that provides the same or similar information in a more human-friendly manner."_
and the section has more information on what that endpoint should look like, but that's totally optional, and can even be added at a later date in a different PR, if you want. Just wanted to put that on your radar.
##########
File path: docs/source/admin/traffic_ops.rst
##########
@@ -349,9 +356,7 @@ This file deals with the configuration parameters of running Traffic Ops itself.
.. versionadded:: 4.1
:user_email: A required email address to create an account with Let's Encrypt or to receive expiration updates. If this is not included then `rate limits <https://letsencrypt.org/docs/rate-limits>`_ may apply for the number of certificates.
- :send_expiration_email: A boolean option to send email summarizing certificate expiration status
:convert_self_signed: A boolean option to convert self signed to Let's Encrypt certificates as they expire. This only works for certificates labeled as Self Signed in the Certificate Source field.
- :renew_days_before_expiration: Set the number of days before expiration date to renew certificates.
Review comment:
Removing these configuration file properties is a breaking change that needs a major release cycle deprecation period.
----------------------------------------------------------------
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