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/07 17:13:40 UTC
[GitHub] [trafficcontrol] rawlinp opened a new pull request #5721: Refactor riak trafficvault interface
rawlinp opened a new pull request #5721:
URL: https://github.com/apache/trafficcontrol/pull/5721
## What does this PR (Pull Request) do?
This PR refactors the existing Riak-based Traffic Vault integration in Traffic Ops into an interface-based implementation, making it easier to add support for new Traffic Vault backends (such as PostgreSQL).
## Which Traffic Control components are affected by this PR?
- Documentation
- Traffic Ops
- Traffic Vault
## What is the best way to verify this PR?
There are a large number of endpoints that depend on Traffic Vault:
```
- GET /api/$version/cdns/name/:name/sslkeys
- GET /api/$version/deliveryservices/xmlId/#xmlid/sslkeys
- GET /api/$version/deliveryservices/hostname/#hostname/sslkeys
- POST /api/$version/deliveryservices/sslkeys/generate
- POST /api/$version/deliveryservices/sslkeys/add
- GET /api/$version/deliveryservices/xmlId/:xmlid/sslkeys/delete
- POST /api/$version/deliveryservices/xmlId/:xmlId/urlkeys/generate
- POST /api/$version/deliveryservices/xmlId/:xmlId/urlkeys/copyFromXmlId/:copyFromXmlId
- GET /api/$version/deliveryservices/xmlId/:xmlId/urlkeys
- GET /api/$version/deliveryservices/:id/urlkeys
- GET /api/$version/cdns/name/:name/dnsseckeys
- POST /api/$version/cdns/dnsseckeys/generate
- GET /api/$version/cdns/dnsseckeys/refresh
- GET /api/$version/cdns/name/:name/dnsseckeys/delete
- POST /api/$version/cdns/:name/dnsseckeys/ksk/generate
- GET /api/$version/deliveryservices/:xmlID/urisignkeys
- POST /api/$version/deliveryservices/:xmlID/urisignkeys
- PUT /api/$version/deliveryservices/:xmlID/urisignkeys
- DELETE /api/$version/deliveryservices/:xmlID/urisignkeys
- GET /api/$version/vault/bucket/:bucket/key/:key/values
- GET /api/$version/vault/ping
- PUT /api/$version/snapshot (certs from deleted DSes are deleted from Riak)
- PUT /api/$version/deliveryservice/:id (creating DNSSEC keys)
```
Unfortunately, these routes do not have TO API tests since historically TO API tests haven't supported using Riak, and the few tests that _do_ use Riak require it to be in CIAB. So, I had to manually test each of these API endpoints after converting them.
Fortunately, starting CIAB _does_ test several of these endpoints, so that can test the functionality a bit. However, I plan to add TO API tests for these endpoints once we've added support for PostgreSQL as a Traffic Vault backend, because it will be easier to support that in the TO API tests since they already require a running PostgreSQL database.
## 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] zrhoffman commented on pull request #5721: Refactor riak trafficvault interface
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5721:
URL: https://github.com/apache/trafficcontrol/pull/5721#issuecomment-815120326
> Files changed: 45
> Docs and code both LGTM
This PR should be subjected to a bit more scrutiny than that 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] zrhoffman edited a comment on pull request #5721: Refactor riak trafficvault interface
Posted by GitBox <gi...@apache.org>.
zrhoffman edited a comment on pull request #5721:
URL: https://github.com/apache/trafficcontrol/pull/5721#issuecomment-815162457
> I looked through every single line of code. Took me about two hours. Found nothing to comment on.
Fair enough, that makes more sense. And as @rawlinp explained, a lot of this is moving things around. Don't hold up this PR on my account.
--
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 pull request #5721: Refactor riak trafficvault interface
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5721:
URL: https://github.com/apache/trafficcontrol/pull/5721#issuecomment-815162457
> I looked through every single line of code. Took me about two hours. Found nothing to comment on.
Fair enough, that makes more sense. And as @rawlinp explained, it's a lot of this is moving things around. Don't hold up this PR on my account.
--
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 merged pull request #5721: Refactor riak trafficvault interface
Posted by GitBox <gi...@apache.org>.
ocket8888 merged pull request #5721:
URL: https://github.com/apache/trafficcontrol/pull/5721
--
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 #5721: Refactor riak trafficvault interface
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5721:
URL: https://github.com/apache/trafficcontrol/pull/5721#discussion_r608993758
##########
File path: lib/go-tc/deliveryservice_ssl_keys.go
##########
@@ -201,7 +203,16 @@ func checkNilOrEmpty(s *string) bool {
return s == nil || *s == ""
}
-type RiakPingResp struct {
+// URISignerKeyset is the container for the CDN URI signing keys.
+type URISignerKeyset struct {
+ RenewalKid *string `json:"renewal_kid"`
+ Keys []jwk.EssentialHeader `json:"keys"`
+}
Review comment:
Since the `URISignerKeyset` struct is now part of the `tc` package, its fields should be documented.
--
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 #5721: Refactor riak trafficvault interface
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5721:
URL: https://github.com/apache/trafficcontrol/pull/5721#discussion_r608968122
##########
File path: lib/go-tc/deliveryservice_ssl_keys.go
##########
@@ -211,7 +222,9 @@ type DNSSECKeys map[string]DNSSECKeySet
// DNSSECKeysV11 is the DNSSEC keys object stored in Riak. The map key strings are both DeliveryServiceNames and CDNNames.
Review comment:
I know the PR does not cause this, but this is a good time to move this Godoc to the `DNSSECKeysV11` type: https://github.com/apache/trafficcontrol/blob/6cfb7042084fb755ef14c0ff1df62d55bc3961e9/lib/go-tc/deliveryservice_ssl_keys.go#L228-L229
--
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 #5721: Refactor riak trafficvault interface
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5721:
URL: https://github.com/apache/trafficcontrol/pull/5721#discussion_r609113190
##########
File path: lib/go-tc/deliveryservice_ssl_keys.go
##########
@@ -211,7 +222,9 @@ type DNSSECKeys map[string]DNSSECKeySet
// DNSSECKeysV11 is the DNSSEC keys object stored in Riak. The map key strings are both DeliveryServiceNames and CDNNames.
Review comment:
Done
##########
File path: lib/go-tc/deliveryservice_ssl_keys.go
##########
@@ -201,7 +203,16 @@ func checkNilOrEmpty(s *string) bool {
return s == nil || *s == ""
}
-type RiakPingResp struct {
+// URISignerKeyset is the container for the CDN URI signing keys.
+type URISignerKeyset struct {
+ RenewalKid *string `json:"renewal_kid"`
+ Keys []jwk.EssentialHeader `json:"keys"`
+}
Review comment:
As I'm unfamiliar with URI signing, I'm not sure I could do the descriptions justice. I moved this here in order to avoid a circular dependency.
##########
File path: docs/source/development/traffic_ops.rst
##########
@@ -161,7 +161,7 @@ Traffic Ops Project Tree Overview
- config/ - Defines configuration structures and methods for reading them in from files
- dbhelpers/ - Assorted utilities that provide functionality for common database tasks, e.g. "Get a user by email"
- plugin/ - The Traffic Ops plugin system, with examples
- - riaksvc/ - In addition to handling routes that deal with storing secrets in or retrieving secrets from Traffic Vault, this package provides a library of functions for interacting with Traffic Vault for other handlers to use.
+ - trafficvault/ - This package provides the Traffic Vault interface and associated backend implementations for other handlers to interact with Traffic Vault.
Review comment:
Yeah, I was on the fence with that because `trafficvault` doesn't contain any route handling code and is purely just the integrations w/ the Traffic Vault datastore (currently just Riak), so it would be nice to keep it that way. I was also considering deprecating that "get bucket keys" route altogether because it is basically just a passthrough to Riak, and that isn't really necessary IMO and doesn't really make sense for other backends. I will propose that to the list.
--
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 #5721: Refactor riak trafficvault interface
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5721:
URL: https://github.com/apache/trafficcontrol/pull/5721#discussion_r608968122
##########
File path: lib/go-tc/deliveryservice_ssl_keys.go
##########
@@ -211,7 +222,9 @@ type DNSSECKeys map[string]DNSSECKeySet
// DNSSECKeysV11 is the DNSSEC keys object stored in Riak. The map key strings are both DeliveryServiceNames and CDNNames.
Review comment:
I know the PR does not cause this, but this is a good time to move this Godoc to the `DNSSECKeysV11` type: https://github.com/apache/trafficcontrol/blob/6cfb7042084fb755ef14c0ff1df62d55bc3961e9/lib/go-tc/deliveryservice_ssl_keys.go#L229
##########
File path: lib/go-tc/deliveryservice_ssl_keys.go
##########
@@ -201,7 +203,16 @@ func checkNilOrEmpty(s *string) bool {
return s == nil || *s == ""
}
-type RiakPingResp struct {
+// URISignerKeyset is the container for the CDN URI signing keys.
+type URISignerKeyset struct {
+ RenewalKid *string `json:"renewal_kid"`
+ Keys []jwk.EssentialHeader `json:"keys"`
+}
Review comment:
Since the `URISignerKeyset` is now part of the `tc` package, its fields should be documented.
##########
File path: docs/source/development/traffic_ops.rst
##########
@@ -161,7 +161,7 @@ Traffic Ops Project Tree Overview
- config/ - Defines configuration structures and methods for reading them in from files
- dbhelpers/ - Assorted utilities that provide functionality for common database tasks, e.g. "Get a user by email"
- plugin/ - The Traffic Ops plugin system, with examples
- - riaksvc/ - In addition to handling routes that deal with storing secrets in or retrieving secrets from Traffic Vault, this package provides a library of functions for interacting with Traffic Vault for other handlers to use.
+ - trafficvault/ - This package provides the Traffic Vault interface and associated backend implementations for other handlers to interact with Traffic Vault.
Review comment:
If we're going to have a package name `trafficvault`, the contents of the `vault` package should be moved there, too.
--
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 pull request #5721: Refactor riak trafficvault interface
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5721:
URL: https://github.com/apache/trafficcontrol/pull/5721#issuecomment-815136896
I will say, even though this touches 45 files, it is actually deceptively smaller than it looks. The underlying Riak functionality is mostly unmodified, but I had to touch every single call site to use the `TrafficVault` interface and move some things around. I also privatized the underlying Riak functions so that the interface is the only way to do anything w/ Riak. I tried to put those "non-changes" into their own commits if it helps review -- e.g. https://github.com/apache/trafficcontrol/pull/5721/commits/29b1263d02337ec43571c0fc67e7cf6f0ef02277.
--
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 #5721: Refactor riak trafficvault interface
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5721:
URL: https://github.com/apache/trafficcontrol/pull/5721#discussion_r609035175
##########
File path: docs/source/development/traffic_ops.rst
##########
@@ -161,7 +161,7 @@ Traffic Ops Project Tree Overview
- config/ - Defines configuration structures and methods for reading them in from files
- dbhelpers/ - Assorted utilities that provide functionality for common database tasks, e.g. "Get a user by email"
- plugin/ - The Traffic Ops plugin system, with examples
- - riaksvc/ - In addition to handling routes that deal with storing secrets in or retrieving secrets from Traffic Vault, this package provides a library of functions for interacting with Traffic Vault for other handlers to use.
+ - trafficvault/ - This package provides the Traffic Vault interface and associated backend implementations for other handlers to interact with Traffic Vault.
Review comment:
If we're going to have a package named `trafficvault`, the contents of the `vault` package should be moved there, too.
--
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 pull request #5721: Refactor riak trafficvault interface
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5721:
URL: https://github.com/apache/trafficcontrol/pull/5721#issuecomment-815121996
I looked through every single line of code. Found nothing to comment on.
--
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 #5721: Refactor riak trafficvault interface
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5721:
URL: https://github.com/apache/trafficcontrol/pull/5721#discussion_r609035175
##########
File path: docs/source/development/traffic_ops.rst
##########
@@ -161,7 +161,7 @@ Traffic Ops Project Tree Overview
- config/ - Defines configuration structures and methods for reading them in from files
- dbhelpers/ - Assorted utilities that provide functionality for common database tasks, e.g. "Get a user by email"
- plugin/ - The Traffic Ops plugin system, with examples
- - riaksvc/ - In addition to handling routes that deal with storing secrets in or retrieving secrets from Traffic Vault, this package provides a library of functions for interacting with Traffic Vault for other handlers to use.
+ - trafficvault/ - This package provides the Traffic Vault interface and associated backend implementations for other handlers to interact with Traffic Vault.
Review comment:
If we're going to have a `trafficvault` package, the contents of the `vault` package should be moved there, too.
--
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 edited a comment on pull request #5721: Refactor riak trafficvault interface
Posted by GitBox <gi...@apache.org>.
ocket8888 edited a comment on pull request #5721:
URL: https://github.com/apache/trafficcontrol/pull/5721#issuecomment-815121996
I looked through every single line of code. Took me about two hours. Found nothing to comment on.
--
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