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/12/21 18:05:39 UTC
[GitHub] [trafficcontrol] srijeet0406 opened a new pull request #6441: Geo limit countries can now be parsed as an array and a string
srijeet0406 opened a new pull request #6441:
URL: https://github.com/apache/trafficcontrol/pull/6441
<!--
Thank you for contributing! Please be sure to read our contribution guidelines: https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md
If this closes or relates to an existing issue, please reference it using one of the following:
Closes: #6382
Related: #ISSUE
If this PR fixes a security vulnerability, DO NOT submit! Instead, contact
the Apache Traffic Control Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://apache.org/security regarding vulnerability disclosure.
-->
This PR closes https://github.com/apache/trafficcontrol/issues/6382 Geo Limit Countries should be an array
<!-- **^ Add meaningful description above** --><hr>
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this PR.
Feel free to add the name of a tool or script that is affected but not on the list.
-->
- Traffic Ops
- Traffic Portal
## What is the best way to verify this PR?
<!-- Please include here ALL the steps necessary to test your PR.
If your PR has tests (and most should), provide the steps needed to run the tests.
If not, please provide step-by-step instructions to test the PR manually and explain why your PR does not need tests. -->
Make sure all the tests pass.
Go to Traffic Portal and create a DS (or update an existing one) with `GeoLimit` set to 0.
Try setting the `Geo Limit Countries` with a string -> this should pass
Try setting the `Geo Limit Countries` with a string aray (`[US,CA]`)-> this should pass
Try setting the `Geo Limit Countries` with a malformed string array (`US,CA]`)-> this should fail
Try setting the `Geo Limit Countries` with a string array with alphabets and numbers (`[US,CA,12]`)-> this should fail
## If this is a bugfix, which Traffic Control versions contained the bug?
<!-- Delete this section if the PR is not a bugfix, or if the bug is only in the master branch.
Examples:
- 5.1.2
- 5.1.3 (RC1)
-->
- master
## PR submission checklist
- [x] This PR has tests <!-- If not, please delete this text and explain why this PR does not need tests. -->
- [x] This PR does not have documentation <!-- If not, please delete this text and explain why this PR does not need documentation. -->
- [x] This PR has a CHANGELOG.md entry <!-- A fix for a bug from an ATC release, an improvement, or a new feature should have a changelog entry. -->
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you 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.
-->
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6441: Geo limit countries can now be parsed as an array and a string
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6441:
URL: https://github.com/apache/trafficcontrol/pull/6441#discussion_r773473459
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.DNS.tpl.html
##########
@@ -941,7 +941,7 @@ <h3 ng-if="!open()">Previous Value</h3>
</div>
</label>
<div class="col-md-10 col-sm-10 col-xs-12">
- <input id="geoLimitCountries" name="geoLimitCountries" type="text" class="form-control" ng-model="deliveryService.geoLimitCountries" maxlength="255" pattern="[A-Z]+(,[A-Z]+)*">
+ <input id="geoLimitCountries" name="geoLimitCountries" type="text" class="form-control" ng-model="deliveryService.geoLimitCountries" maxlength="255" pattern="(\[[A-Z]+(,[A-Z]+)*\]|[A-Z]+(,[A-Z]+)*)">
Review comment:
Alright, so I'm going to back out the TP changes, but on the API side, I'll add the logic for the `geoLimitCountries` to be able to be either a string or an array of strings. Does that sound good?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6441: Geo limit countries can now be parsed as an array and a string
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6441:
URL: https://github.com/apache/trafficcontrol/pull/6441#discussion_r779039404
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -643,7 +643,7 @@ type DeliveryServiceNullableFieldsV11 struct {
// countries within which the Delivery Service's content ought to be made
// available. This has no effect if GeoLimit is not a pointer to exactly the
// value 2.
- GeoLimitCountries *string `json:"geoLimitCountries" db:"geo_limit_countries"`
+ GeoLimitCountries *interface{} `json:"geoLimitCountries" db:"geo_limit_countries"`
Review comment:
When `geoLimitCountries` is passed into the API as a string, it's still really an array, just one that's represented as a string. Because of that, I think if we're going to make a type change here it makes the most sense to use a string slice. To get it to parse independent of input type, there are a few ways but what I'd recommend is a type alias for `[]string` that implements [`encoding/json.Unmarshaler`](https://pkg.go.dev/encoding/json#Unmarshaler) to handle the two situations and parse country code array strings out into actual arrays immediately, which is currently being enforced as validation (and so in that case could/would need to be removed from the validation).
Also, the types of fields in legacy API version structures cannot be changed. While the change is non-breaking for the API, it is breaking the call signature of legacy clients using our Go client library. If anyone is using the `GeoLimitCountries` field of that struct - or any that nest it - then their programs will no longer compile after this change: https://go.dev/play/p/Dk7Eu8XbKt4
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 merged pull request #6441: Geo limit countries can now be parsed as an array and a string
Posted by GitBox <gi...@apache.org>.
ocket8888 merged pull request #6441:
URL: https://github.com/apache/trafficcontrol/pull/6441
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6441: Geo limit countries can now be parsed as an array and a string
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6441:
URL: https://github.com/apache/trafficcontrol/pull/6441#discussion_r791175342
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -577,6 +578,40 @@ type DeliveryServiceNullableV11 struct {
DeliveryServiceRemovedFieldsV11
}
+// GeoLimitCountriesType is the type alias that is used to represent the GeoLimitCountries attribute of the DeliveryService struct
Review comment:
GoDoc comments should be complete sentences - ending with punctuation.
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -577,6 +578,40 @@ type DeliveryServiceNullableV11 struct {
DeliveryServiceRemovedFieldsV11
}
+// GeoLimitCountriesType is the type alias that is used to represent the GeoLimitCountries attribute of the DeliveryService struct
+type GeoLimitCountriesType []string
+
+// UnmarshalJSON will unmarshal a byte slice into type GeoLimitCountriesType
Review comment:
Same as above RE: punctuation
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -577,6 +578,40 @@ type DeliveryServiceNullableV11 struct {
DeliveryServiceRemovedFieldsV11
}
+// GeoLimitCountriesType is the type alias that is used to represent the GeoLimitCountries attribute of the DeliveryService struct
+type GeoLimitCountriesType []string
+
+// UnmarshalJSON will unmarshal a byte slice into type GeoLimitCountriesType
+func (g *GeoLimitCountriesType) UnmarshalJSON(data []byte) error {
+ var err error
+ var initial = make([]string, 0)
+ var initialStr string
+ if err = json.Unmarshal(data, &initial); err != nil {
+ if err = json.Unmarshal(data, &initialStr); err != nil {
+ return err
+ }
+ if strings.Contains(initialStr, ",") {
+ initial = strings.Split(initialStr, ",")
+ } else {
+ initial = append(initial, initialStr)
+ }
+ }
+
+ if initial == nil || len(initial) == 0 {
+ g = nil
+ return nil
+ }
+ *g = initial
+ return nil
+
+}
+
+// MarshalJSON will marshal a GeoLimitCountriesType into a byte slice
Review comment:
Same as above RE: punctuation.
##########
File path: traffic_ops/testing/api/v4/deliveryservices_test.go
##########
@@ -98,6 +99,64 @@ func TestDeliveryServices(t *testing.T) {
})
}
+func CreateTestDeliveryServiceWithGeoLimitCountries(t *testing.T) {
+
+ if len(testData.DeliveryServices) == 0 {
+ t.Fatalf("no deliveryservices to run the test on, quitting")
+ }
+
+ cdn := createBlankCDN("geoLimitCDN", t)
+ opts := client.NewRequestOptions()
+ opts.QueryParameters.Set("name", "HTTP")
+ types, _, err := TOSession.GetTypes(opts)
+ if err != nil {
+ t.Fatalf("unable to get Types: %v - alerts: %+v", err, types.Alerts)
+ }
+ if len(types.Response) < 1 {
+ t.Fatal("expected at least one type")
+ }
+ customDS := getCustomDS(cdn.ID, types.Response[0].ID, "geo-limit-countries-test-ds-name", "edge", "https://test-geo-limit.com", "geo-limit-countries-test-ds-xml-id")
+ customDS.Protocol = util.IntPtr(0)
+ customDS.GeoLimit = util.IntPtr(2)
+ //geoLimitCountries := []string{"US ", "CA"}
+ customDS.GeoLimitCountries = []string{"US ", "CA"}
+
+ resp, _, err := TOSession.CreateDeliveryService(customDS, client.RequestOptions{})
+ if err != nil {
+ t.Errorf("expected no error while creating a new ds, but got %v", err)
+ }
+ if len(resp.Response) != 1 {
+ t.Fatalf("expected 1 response in return of a create DS request, but got %d", len(resp.Response))
+ }
+ if resp.Response[0].GeoLimitCountries == nil {
+ t.Fatalf("got nothing in geo limit countries")
+ }
+ arr := ([]string)(resp.Response[0].GeoLimitCountries)
+ if len(arr) != 2 || arr[0] != "US" || arr[1] != "CA" {
+ t.Errorf("expected geo limit countries: US,CA; actual: %s", arr)
+ }
+ opts = client.NewRequestOptions()
+ opts.QueryParameters.Set("xmlId", *customDS.XMLID)
+ deliveryServices, _, err := TOSession.GetDeliveryServices(opts)
+ if err != nil {
+ t.Fatalf("couldn't get ds: %v", err)
+ }
+ if len(deliveryServices.Response) != 1 {
+ t.Fatal("couldn't get exactly one ds in the response, quitting")
+ }
+ dsID := deliveryServices.Response[0].ID
+ customDS.GeoLimitCountries = []string{"US ", "CA", "12"}
+ _, _, err = TOSession.UpdateDeliveryService(*dsID, customDS, client.RequestOptions{})
Review comment:
This line (and L153) will segfault if Traffic Ops erroneously returns a Delivery Service with a null or omitted ID
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6441: Geo limit countries can now be parsed as an array and a string
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6441:
URL: https://github.com/apache/trafficcontrol/pull/6441#discussion_r773473459
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.DNS.tpl.html
##########
@@ -941,7 +941,7 @@ <h3 ng-if="!open()">Previous Value</h3>
</div>
</label>
<div class="col-md-10 col-sm-10 col-xs-12">
- <input id="geoLimitCountries" name="geoLimitCountries" type="text" class="form-control" ng-model="deliveryService.geoLimitCountries" maxlength="255" pattern="[A-Z]+(,[A-Z]+)*">
+ <input id="geoLimitCountries" name="geoLimitCountries" type="text" class="form-control" ng-model="deliveryService.geoLimitCountries" maxlength="255" pattern="(\[[A-Z]+(,[A-Z]+)*\]|[A-Z]+(,[A-Z]+)*)">
Review comment:
Alright, so I'm going to back out the TP changes(the improvement to TP can be handled as its own ticket), but on the API side, I'll add the logic for the `geoLimitCountries` to be able to be either a string or an array of strings. Does that sound good?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #6441: Geo limit countries can now be parsed as an array and a string
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #6441:
URL: https://github.com/apache/trafficcontrol/pull/6441#discussion_r830154366
##########
File path: traffic_ops/testing/api/v4/deliveryservices_test.go
##########
@@ -98,6 +99,67 @@ func TestDeliveryServices(t *testing.T) {
})
}
+func CreateTestDeliveryServiceWithGeoLimitCountries(t *testing.T) {
+
+ if len(testData.DeliveryServices) == 0 {
+ t.Fatalf("no deliveryservices to run the test on, quitting")
+ }
+
+ cdn := createBlankCDN("geoLimitCDN", t)
+ opts := client.NewRequestOptions()
+ opts.QueryParameters.Set("name", "HTTP")
+ types, _, err := TOSession.GetTypes(opts)
+ if err != nil {
+ t.Fatalf("unable to get Types: %v - alerts: %+v", err, types.Alerts)
+ }
+ if len(types.Response) < 1 {
+ t.Fatal("expected at least one type")
+ }
+ customDS := getCustomDS(cdn.ID, types.Response[0].ID, "geo-limit-countries-test-ds-name", "edge", "https://test-geo-limit.com", "geo-limit-countries-test-ds-xml-id")
+ customDS.Protocol = util.IntPtr(0)
+ customDS.GeoLimit = util.IntPtr(2)
+ //geoLimitCountries := []string{"US ", "CA"}
Review comment:
Can this commented line be removed?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6441: Geo limit countries can now be parsed as an array and a string
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6441:
URL: https://github.com/apache/trafficcontrol/pull/6441#discussion_r791175342
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -577,6 +578,40 @@ type DeliveryServiceNullableV11 struct {
DeliveryServiceRemovedFieldsV11
}
+// GeoLimitCountriesType is the type alias that is used to represent the GeoLimitCountries attribute of the DeliveryService struct
Review comment:
GoDoc comments should be complete sentences - ending with punctuation.
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -577,6 +578,40 @@ type DeliveryServiceNullableV11 struct {
DeliveryServiceRemovedFieldsV11
}
+// GeoLimitCountriesType is the type alias that is used to represent the GeoLimitCountries attribute of the DeliveryService struct
+type GeoLimitCountriesType []string
+
+// UnmarshalJSON will unmarshal a byte slice into type GeoLimitCountriesType
Review comment:
Same as above RE: punctuation
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -577,6 +578,40 @@ type DeliveryServiceNullableV11 struct {
DeliveryServiceRemovedFieldsV11
}
+// GeoLimitCountriesType is the type alias that is used to represent the GeoLimitCountries attribute of the DeliveryService struct
+type GeoLimitCountriesType []string
+
+// UnmarshalJSON will unmarshal a byte slice into type GeoLimitCountriesType
+func (g *GeoLimitCountriesType) UnmarshalJSON(data []byte) error {
+ var err error
+ var initial = make([]string, 0)
+ var initialStr string
+ if err = json.Unmarshal(data, &initial); err != nil {
+ if err = json.Unmarshal(data, &initialStr); err != nil {
+ return err
+ }
+ if strings.Contains(initialStr, ",") {
+ initial = strings.Split(initialStr, ",")
+ } else {
+ initial = append(initial, initialStr)
+ }
+ }
+
+ if initial == nil || len(initial) == 0 {
+ g = nil
+ return nil
+ }
+ *g = initial
+ return nil
+
+}
+
+// MarshalJSON will marshal a GeoLimitCountriesType into a byte slice
Review comment:
Same as above RE: punctuation.
##########
File path: traffic_ops/testing/api/v4/deliveryservices_test.go
##########
@@ -98,6 +99,64 @@ func TestDeliveryServices(t *testing.T) {
})
}
+func CreateTestDeliveryServiceWithGeoLimitCountries(t *testing.T) {
+
+ if len(testData.DeliveryServices) == 0 {
+ t.Fatalf("no deliveryservices to run the test on, quitting")
+ }
+
+ cdn := createBlankCDN("geoLimitCDN", t)
+ opts := client.NewRequestOptions()
+ opts.QueryParameters.Set("name", "HTTP")
+ types, _, err := TOSession.GetTypes(opts)
+ if err != nil {
+ t.Fatalf("unable to get Types: %v - alerts: %+v", err, types.Alerts)
+ }
+ if len(types.Response) < 1 {
+ t.Fatal("expected at least one type")
+ }
+ customDS := getCustomDS(cdn.ID, types.Response[0].ID, "geo-limit-countries-test-ds-name", "edge", "https://test-geo-limit.com", "geo-limit-countries-test-ds-xml-id")
+ customDS.Protocol = util.IntPtr(0)
+ customDS.GeoLimit = util.IntPtr(2)
+ //geoLimitCountries := []string{"US ", "CA"}
+ customDS.GeoLimitCountries = []string{"US ", "CA"}
+
+ resp, _, err := TOSession.CreateDeliveryService(customDS, client.RequestOptions{})
+ if err != nil {
+ t.Errorf("expected no error while creating a new ds, but got %v", err)
+ }
+ if len(resp.Response) != 1 {
+ t.Fatalf("expected 1 response in return of a create DS request, but got %d", len(resp.Response))
+ }
+ if resp.Response[0].GeoLimitCountries == nil {
+ t.Fatalf("got nothing in geo limit countries")
+ }
+ arr := ([]string)(resp.Response[0].GeoLimitCountries)
+ if len(arr) != 2 || arr[0] != "US" || arr[1] != "CA" {
+ t.Errorf("expected geo limit countries: US,CA; actual: %s", arr)
+ }
+ opts = client.NewRequestOptions()
+ opts.QueryParameters.Set("xmlId", *customDS.XMLID)
+ deliveryServices, _, err := TOSession.GetDeliveryServices(opts)
+ if err != nil {
+ t.Fatalf("couldn't get ds: %v", err)
+ }
+ if len(deliveryServices.Response) != 1 {
+ t.Fatal("couldn't get exactly one ds in the response, quitting")
+ }
+ dsID := deliveryServices.Response[0].ID
+ customDS.GeoLimitCountries = []string{"US ", "CA", "12"}
+ _, _, err = TOSession.UpdateDeliveryService(*dsID, customDS, client.RequestOptions{})
Review comment:
This line (and L153) will segfault if Traffic Ops erroneously returns a Delivery Service with a null or omitted ID
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6441: Geo limit countries can now be parsed as an array and a string
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6441:
URL: https://github.com/apache/trafficcontrol/pull/6441#discussion_r781332403
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -245,7 +245,8 @@ type DeliveryServiceV40 struct {
// TLSVersions is the list of explicitly supported TLS versions for cache
// servers serving the Delivery Service's content.
- TLSVersions []string `json:"tlsVersions" db:"tls_versions"`
+ TLSVersions []string `json:"tlsVersions" db:"tls_versions"`
+ GeoLimitCountriesList *GeoLimitCountriesType `json:"geoLimitCountriesList"`
Review comment:
I don't think we should add a new property for this, the existing `geoLimitCountries` JSON property can be re-used; when `encoding/json` unmarshals into a structure, it uses the same rules as the Go language to determine which property to unmarshal into.
Example: https://go.dev/play/p/chpKFwvn9X0
Because we rely heavily on nesting structures to avoid duplicating unchanged fields, we have a few duplicated data fields in the API structures, and accessing properties of nested structures instead of just directly (i.e. `fooV40.fooV31.Prop` instead of `fooV40.Prop` even if `Prop` only exists on `fooV40.fooV31`) is not supported by `lib/go-tc`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on pull request #6441: Geo limit countries can now be parsed as an array and a string
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on pull request #6441:
URL: https://github.com/apache/trafficcontrol/pull/6441#issuecomment-1011494546
> According to the title of this PR:
>
> > Geo limit countries can now be parsed as an array **_and_** a string
>
> (emphasis mine) but actually the old string-as-an-array doesn't work anymore: https://go.dev/play/p/AWo8KqQ6yvt
>
> I'm fine with making that breaking change, and since it's in a new major API version we are free to do that, but I want to be sure that the intention is to make that breaking change. Furthermore, if we're going to be breaking it in that way, then we don't need a type alias at all, it can just be `[]string` since that's the only valid input. It's also worth noting that the output (i.e. in API responses) type is now changed as well to be an array of strings, which is already a breaking change anyway.
I have changed the logic to be able to accept something like `"US,CA,IN"`, as well as `["US","CA","IN"]`.
As for the response part, yes, it will be an array of strings starting api v4.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6441: Geo limit countries can now be parsed as an array and a string
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6441:
URL: https://github.com/apache/trafficcontrol/pull/6441#discussion_r783408646
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -245,7 +245,8 @@ type DeliveryServiceV40 struct {
// TLSVersions is the list of explicitly supported TLS versions for cache
// servers serving the Delivery Service's content.
- TLSVersions []string `json:"tlsVersions" db:"tls_versions"`
+ TLSVersions []string `json:"tlsVersions" db:"tls_versions"`
+ GeoLimitCountries *GeoLimitCountriesType `json:"geoLimitCountries"`
Review comment:
`GeoLimitCountriesType` is already a reference type - `nil` is a valid value for a `[]string` - so I don't think this needs to be a pointer.
##########
File path: lib/go-tc/deliveryservices.go
##########
@@ -577,6 +578,36 @@ type DeliveryServiceNullableV11 struct {
DeliveryServiceRemovedFieldsV11
}
+type GeoLimitCountriesType []string
Review comment:
This type should probably have a GoDoc comment. Personally I would also add one to its `UnmarshalJSON` and `MarshalJSON` methods, but those are extremely common especially in our codebase so it probably doesn't add much value.
##########
File path: docs/source/api/v4/deliveryservices.rst
##########
@@ -105,6 +105,7 @@ Response Structure
:fqPacingRate: The :ref:`ds-fqpr`
:geoLimit: An integer that defines the :ref:`ds-geo-limit`
:geoLimitCountries: A string containing a comma-separated list defining the :ref:`ds-geo-limit-countries`
+:geoLimitCountriesList: A string or string array containing a comma-separated list defining the :ref:`ds-geo-limit-countries-list`
Review comment:
This isn't accurate anymore
##########
File path: docs/source/api/v4/deliveryservices_id.rst
##########
@@ -53,6 +53,7 @@ Request Structure
:fqPacingRate: The :ref:`ds-fqpr`
:geoLimit: An integer that defines the :ref:`ds-geo-limit`
:geoLimitCountries: A string containing a comma-separated list defining the :ref:`ds-geo-limit-countries`\ [#geolimit]_
+:geoLimitCountriesList: A string or string array containing a comma-separated list defining the :ref:`ds-geo-limit-countries-list`
Review comment:
This isn't accurate anymore
##########
File path: CHANGELOG.md
##########
@@ -17,6 +17,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
- Added definition for `heartbeat.polling.interval` for CDN Traffic Monitor config in API documentation.
- New `pkg` script options, `-h`, `-s`, `-S`, and `-L`.
- Added `Invalidation Type` (REFRESH or REFETCH) for invalidating content to Traffic Portal.
+- Added a new field `geoLimitCountriesList` to the `DeliveryService` structure to accept both an array as well as single strings for geo limit country codes
Review comment:
This is no longer true - also currently
> an array as well as single strings
isn't true currently, see above comment
##########
File path: docs/source/api/v4/deliveryservices_id.rst
##########
@@ -205,6 +206,7 @@ Response Structure
:fqPacingRate: The :ref:`ds-fqpr`
:geoLimit: An integer that defines the :ref:`ds-geo-limit`
:geoLimitCountries: A string containing a comma-separated list defining the :ref:`ds-geo-limit-countries`
+:geoLimitCountriesList: A string or string array containing a comma-separated list defining the :ref:`ds-geo-limit-countries-list`
Review comment:
This isn't accurate anymore
##########
File path: docs/source/api/v4/servers_id_deliveryservices.rst
##########
@@ -92,6 +92,7 @@ Response Structure
:fqPacingRate: The :ref:`ds-fqpr`
:geoLimit: An integer that defines the :ref:`ds-geo-limit`
:geoLimitCountries: A string containing a comma-separated list defining the :ref:`ds-geo-limit-countries`
+:geoLimitCountriesList: A string or string array containing a comma-separated list defining the :ref:`ds-geo-limit-countries-list`
Review comment:
This isn't accurate anymore
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -2136,9 +2174,13 @@ func setNilIfEmpty(ptrs ...**string) {
}
}
-func sanitize(ds *tc.DeliveryServiceV4) {
+func sanitize(ds *tc.DeliveryServiceV4) error {
Review comment:
It is not possible, it seems, for this function to return a non-`nil` error - why bother returning an error value that never needs to be checked in practice?
##########
File path: docs/source/overview/delivery_services.rst
##########
@@ -288,6 +288,12 @@ When `Geo Limit`_ is being used with this Delivery Service (and is set to exactl
| | | country code - one should exist for each allowed country code |
+------------------+---------------------------------------------------------------------------+------------------------------------------------------------------------------------------------+
+.. _ds-geo-limit-countries-list:
+
+Geo Limit Countries List
+------------------------
+When `Geo Limit`_ is being used with this Delivery Service (and is set to exactly ``2``), this is optionally a list of country codes to which access to content provided by the Delivery Service will be restricted. This can either be a single string representing a country code, or an array of strings. When creating a Delivery Service with this field or modifying the Geo Limit Countries List field on an existing Delivery Service, any amount of whitespace between country codes is permissible, as it will be removed on submission, but responses from the :ref:`to-api` should never include such whitespace.
+
Review comment:
This property doesn't exist in this PR anymore
##########
File path: docs/source/api/v4/deliveryservices.rst
##########
@@ -448,6 +450,7 @@ Response Structure
:fqPacingRate: The :ref:`ds-fqpr`
:geoLimit: An integer that defines the :ref:`ds-geo-limit`
:geoLimitCountries: A string containing a comma-separated list defining the :ref:`ds-geo-limit-countries`
+:geoLimitCountriesList: A string or string array containing a comma-separated list defining the :ref:`ds-geo-limit-countries-list`
Review comment:
This isn't accurate anymore
##########
File path: docs/source/api/v4/deliveryservices.rst
##########
@@ -297,6 +298,7 @@ Request Structure
:fqPacingRate: The :ref:`ds-fqpr`
:geoLimit: An integer that defines the :ref:`ds-geo-limit`
:geoLimitCountries: A string containing a comma-separated list defining the :ref:`ds-geo-limit-countries`\ [#geolimit]_
+:geoLimitCountriesList: A string or string array containing a comma-separated list defining the :ref:`ds-geo-limit-countries-list`
Review comment:
This isn't accurate anymore
##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go
##########
@@ -606,7 +609,8 @@ func (ds *TODeliveryService) Read(h http.Header, useIMS bool) ([]interface{}, er
switch {
// NOTE: it's required to handle minor version cases in a descending >= manner
case version.Major > 3:
- returnable = append(returnable, ds.RemoveLD1AndLD2())
+ dsV4 := ds.RemoveLD1AndLD2()
+ returnable = append(returnable, dsV4)
Review comment:
this doesn't appear to change anything - was this supposed to be included in this PR?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 merged pull request #6441: Geo limit countries can now be parsed as an array and a string
Posted by GitBox <gi...@apache.org>.
ocket8888 merged pull request #6441:
URL: https://github.com/apache/trafficcontrol/pull/6441
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6441: Geo limit countries can now be parsed as an array and a string
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6441:
URL: https://github.com/apache/trafficcontrol/pull/6441#discussion_r773358269
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.DNS.tpl.html
##########
@@ -941,7 +941,7 @@ <h3 ng-if="!open()">Previous Value</h3>
</div>
</label>
<div class="col-md-10 col-sm-10 col-xs-12">
- <input id="geoLimitCountries" name="geoLimitCountries" type="text" class="form-control" ng-model="deliveryService.geoLimitCountries" maxlength="255" pattern="[A-Z]+(,[A-Z]+)*">
+ <input id="geoLimitCountries" name="geoLimitCountries" type="text" class="form-control" ng-model="deliveryService.geoLimitCountries" maxlength="255" pattern="(\[[A-Z]+(,[A-Z]+)*\]|[A-Z]+(,[A-Z]+)*)">
Review comment:
Users should not be writing loose JSON syntax into our inputs, they should be able to handle that for the user. This document fragment actually doesn't need to change at all to make this change to Delivery Services, though IMO it would be an improvement to make it a true variable list of inputs like the "Consistent Hash Query Parameters".
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6441: Geo limit countries can now be parsed as an array and a string
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6441:
URL: https://github.com/apache/trafficcontrol/pull/6441#discussion_r773519863
##########
File path: traffic_portal/app/src/common/modules/form/deliveryService/form.deliveryService.DNS.tpl.html
##########
@@ -941,7 +941,7 @@ <h3 ng-if="!open()">Previous Value</h3>
</div>
</label>
<div class="col-md-10 col-sm-10 col-xs-12">
- <input id="geoLimitCountries" name="geoLimitCountries" type="text" class="form-control" ng-model="deliveryService.geoLimitCountries" maxlength="255" pattern="[A-Z]+(,[A-Z]+)*">
+ <input id="geoLimitCountries" name="geoLimitCountries" type="text" class="form-control" ng-model="deliveryService.geoLimitCountries" maxlength="255" pattern="(\[[A-Z]+(,[A-Z]+)*\]|[A-Z]+(,[A-Z]+)*)">
Review comment:
yeah, that should be fine.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org