You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "lzx404243 (via GitHub)" <gi...@apache.org> on 2023/03/06 22:27:18 UTC
[GitHub] [trafficserver] lzx404243 opened a new pull request, #9501: Enable external file loading for sni.yaml.
lzx404243 opened a new pull request, #9501:
URL: https://github.com/apache/trafficserver/pull/9501
This is cherry-picked from Yahoo's internal 9.1.x branch and originally authored by @shinrich .
Conflicts:
include/tscore/bwf_std_format.h
iocore/net/P_SNIActionPerformer.h
iocore/net/SSLSNIConfig.cc
proxy/IPAllow.cc
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1130077707
##########
doc/admin-guide/files/sni.yaml.en.rst:
##########
@@ -61,6 +61,12 @@ ip_allow Inbound Specify a list of client IP address, subnets
the connection. This list is comma separated. IPv4 and IPv6 addresses can be specified.
Here is an example list: 192.168.1.0/24,192.168.10.1-4. This would allow connections
from clients in the 19.168.1.0 network or in the range from 192.168.10.1 to 192.168.1.4.
+ Alternatively, the path to a file containing
+ the list of comma-separated IP addresses can
+ be specified in the form of
+ ``@path_to_file``. If a given file path does
Review Comment:
> In addition to ' and " there are a number of characters that are special (or reserved) and cannot be used as the first character of an unquoted scalar: [] {} > | * & ! % # ` @ ,.
https://docs.ansible.com/ansible/latest/reference_appendices/YAMLSyntax.html
We may want to be more specific about this (maybe with a sample) since the default file uses unquoted scalars and just replacing unquoted `127.0.0.1` with unquoted `@/x/y/z` can mean something else.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1129992911
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -93,6 +97,78 @@ NamedElement::set_regex_name(const std::string ®ex_name)
////
// SNIConfigParams
//
+// ----
+
+SNI_IpAllow::SNI_IpAllow(std::string &ip_allow_list, std::string const &servername)
+{
+ ts::TextView content{ip_allow_list};
Review Comment:
@SolidWallOfCode I don't know which type we should use here. There are too many options available. Maybe `swoc::TextView` ?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1130044052
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -93,6 +97,78 @@ NamedElement::set_regex_name(const std::string ®ex_name)
////
// SNIConfigParams
//
+// ----
+
+SNI_IpAllow::SNI_IpAllow(std::string &ip_allow_list, std::string const &servername)
+{
+ ts::TextView content{ip_allow_list};
+ if (content && content[0] == '@') {
+ std::error_code ec;
+ ts::file::path path{content.remove_prefix(1)};
+ ts::LocalBufferWriter<1024> w;
Review Comment:
This is now moved to SNIActionPerformer.cc. Addressed in the other conversation.
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -93,6 +97,78 @@ NamedElement::set_regex_name(const std::string ®ex_name)
////
// SNIConfigParams
//
+// ----
+
+SNI_IpAllow::SNI_IpAllow(std::string &ip_allow_list, std::string const &servername)
+{
+ ts::TextView content{ip_allow_list};
Review Comment:
This is now moved to `SNIActionPerformer.cc`. Addressed in the other conversation.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1129968327
##########
iocore/net/P_SNIActionPerformer.h:
##########
@@ -296,60 +296,19 @@ class SNI_IpAllow : public ActionItem
IpMap ip_map;
public:
- SNI_IpAllow(std::string &ip_allow_list, const std::string &servername)
- {
- // the server identified by item.fqdn requires ATS to do IP filtering
- if (ip_allow_list.length()) {
- IpAddr addr1;
- IpAddr addr2;
- // check format first
- // check if the input is a comma separated list of IPs
- ts::TextView content(ip_allow_list);
- while (!content.empty()) {
- ts::TextView list{content.take_prefix_at(',')};
- if (0 != ats_ip_range_parse(list, addr1, addr2)) {
- Debug("ssl_sni", "%.*s is not a valid format", static_cast<int>(list.size()), list.data());
- break;
- } else {
- Debug("ssl_sni", "%.*s added to the ip_allow list %s", static_cast<int>(list.size()), list.data(), servername.c_str());
- ip_map.fill(IpEndpoint().assign(addr1), IpEndpoint().assign(addr2), reinterpret_cast<void *>(1));
- }
- }
- }
- } // end function SNI_IpAllow
+ SNI_IpAllow(std::string &ip_allow_list, const std::string &servername); // end function SNI_IpAllow
Review Comment:
The comment is no longer necessary.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1130002269
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -93,6 +97,78 @@ NamedElement::set_regex_name(const std::string ®ex_name)
////
// SNIConfigParams
//
+// ----
+
+SNI_IpAllow::SNI_IpAllow(std::string &ip_allow_list, std::string const &servername)
+{
+ ts::TextView content{ip_allow_list};
Review Comment:
~Ah, `content` need to be `ts::TextView` for now because it's used by existing code in `load()`.~ I was confused. `content` here is not the content of an external 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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1127128543
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -37,10 +37,14 @@
#include "tscore/Diags.h"
#include "tscore/SimpleTokenizer.h"
#include "tscore/ink_memory.h"
+#include "tscpp/util/TextView.h"
+#include "tscore/ts_file.h"
#include "tscore/I_Layout.h"
#include "tscpp/util/TextView.h"
Review Comment:
No need for the added tscpp/util/TextView.h since one already exists at line 44.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1128250075
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -93,6 +96,78 @@ NamedElement::set_regex_name(const std::string ®ex_name)
////
// SNIConfigParams
//
+// ----
+
+SNI_IpAllow::SNI_IpAllow(std::string &ip_allow_list, std::string const &servername)
Review Comment:
I don't think P_SNIActionPerformer is the best place to have this at all, but why here?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9501: Enable external file loading for sni.yaml.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#issuecomment-1461019284
@bneradt @SolidWallOfCode I requested a couple of changes after your last reviews. Please go ahead and merge this PR if it looks good to you 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.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt merged pull request #9501: Enable external file loading for sni.yaml.
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt merged PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "SolidWallOfCode (via GitHub)" <gi...@apache.org>.
SolidWallOfCode commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1130164935
##########
iocore/net/SNIActionPerformer.cc:
##########
@@ -0,0 +1,96 @@
+/** @file
+
+ Implementation of SNIActionPerformer
+
+ @section license License
+
+ 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.
+ */
+
+#include "P_SNIActionPerformer.h"
+#include "tscore/ts_file.h"
+#include "tscpp/util/TextView.h"
+#include "tscore/BufferWriter.h"
+#include "tscore/bwf_std_format.h"
+
+SNI_IpAllow::SNI_IpAllow(std::string &ip_allow_list, std::string const &servername)
+{
+ ts::TextView content{ip_allow_list};
Review Comment:
If it's in libswoc, do not use the `ts` version unless it would require an extensive rewrite. For anything that's in both, the `ts` version is obsolete and being 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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1129994730
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -93,6 +97,78 @@ NamedElement::set_regex_name(const std::string ®ex_name)
////
// SNIConfigParams
//
+// ----
+
+SNI_IpAllow::SNI_IpAllow(std::string &ip_allow_list, std::string const &servername)
+{
+ ts::TextView content{ip_allow_list};
Review Comment:
And there may be swoc versions for other types used on following lines?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1130180791
##########
iocore/net/SNIActionPerformer.cc:
##########
@@ -0,0 +1,96 @@
+/** @file
+
+ Implementation of SNIActionPerformer
+
+ @section license License
+
+ 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.
+ */
+
+#include "P_SNIActionPerformer.h"
+#include "tscore/ts_file.h"
+#include "tscpp/util/TextView.h"
+#include "tscore/BufferWriter.h"
+#include "tscore/bwf_std_format.h"
+
+SNI_IpAllow::SNI_IpAllow(std::string &ip_allow_list, std::string const &servername)
+{
+ ts::TextView content{ip_allow_list};
Review Comment:
@SolidWallOfCode Gotcha. Thanks for clarifying.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1127134805
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -37,10 +37,14 @@
#include "tscore/Diags.h"
#include "tscore/SimpleTokenizer.h"
#include "tscore/ink_memory.h"
+#include "tscpp/util/TextView.h"
+#include "tscore/ts_file.h"
#include "tscore/I_Layout.h"
#include "tscpp/util/TextView.h"
Review Comment:
Good catch. Fixed!
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1130002269
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -93,6 +97,78 @@ NamedElement::set_regex_name(const std::string ®ex_name)
////
// SNIConfigParams
//
+// ----
+
+SNI_IpAllow::SNI_IpAllow(std::string &ip_allow_list, std::string const &servername)
+{
+ ts::TextView content{ip_allow_list};
Review Comment:
Ah, `content` need to be `ts::TextView` for now because it's used by existing code in `load()`.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1130154982
##########
doc/admin-guide/files/sni.yaml.en.rst:
##########
@@ -61,6 +61,12 @@ ip_allow Inbound Specify a list of client IP address, subnets
the connection. This list is comma separated. IPv4 and IPv6 addresses can be specified.
Here is an example list: 192.168.1.0/24,192.168.10.1-4. This would allow connections
from clients in the 19.168.1.0 network or in the range from 192.168.10.1 to 192.168.1.4.
+ Alternatively, the path to a file containing
+ the list of comma-separated IP addresses can
+ be specified in the form of
+ ``@path_to_file``. If a given file path does
Review Comment:
Good point. I updated the documentation to make that clear and add an example. See if you like the text and format.
<img width="1054" alt="image" src="https://user-images.githubusercontent.com/19499960/223867528-7a1b8abb-5cbe-4271-a06f-6378107ac293.png">
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9501: Enable external file loading for sni.yaml.
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#issuecomment-1458682691
Looks 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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1128221932
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -93,6 +96,78 @@ NamedElement::set_regex_name(const std::string ®ex_name)
////
// SNIConfigParams
//
+// ----
+
+SNI_IpAllow::SNI_IpAllow(std::string &ip_allow_list, std::string const &servername)
+{
+ ts::TextView content{ip_allow_list};
+ if (content && content[0] == '@') {
+ std::error_code ec;
+ ts::file::path path{content.remove_prefix(1)};
+ ts::LocalBufferWriter<1024> w;
+ if (path.is_relative()) {
+ path = ts::file::path(Layout::get()->sysconfdir) / path;
+ }
+ ip_allow_list = ts::file::load(path, ec);
+ if (ec) {
Review Comment:
updated the docs under the `ip_allow` configuration.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1129932433
##########
proxy/IPAllow.cc:
##########
@@ -60,6 +64,29 @@ bwformat(BufferWriter &w, Spec const &spec, YAML::Mark const &mark)
} // namespace swoc
+namespace YAML
+{
+template <> struct convert<ts::TextView> {
+ static Node
+ encode(ts::TextView const &tv)
Review Comment:
Good catch, thanks!
the `encode()` and `decode()` are from the parent of the incoming changes that shouldn't be merged in. Removed the functions and the corresponding includes.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1128418611
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -93,6 +96,78 @@ NamedElement::set_regex_name(const std::string ®ex_name)
////
// SNIConfigParams
//
+// ----
+
+SNI_IpAllow::SNI_IpAllow(std::string &ip_allow_list, std::string const &servername)
Review Comment:
I moved the implementation of `SNI_IpAllow` into a new file `iocore/net/SNIActionPerformer.cc`.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1129999365
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -93,6 +97,78 @@ NamedElement::set_regex_name(const std::string ®ex_name)
////
// SNIConfigParams
//
+// ----
+
+SNI_IpAllow::SNI_IpAllow(std::string &ip_allow_list, std::string const &servername)
+{
+ ts::TextView content{ip_allow_list};
+ if (content && content[0] == '@') {
+ std::error_code ec;
+ ts::file::path path{content.remove_prefix(1)};
+ ts::LocalBufferWriter<1024> w;
Review Comment:
This is only used when error occurs.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1129976646
##########
iocore/net/P_SNIActionPerformer.h:
##########
@@ -296,60 +296,19 @@ class SNI_IpAllow : public ActionItem
IpMap ip_map;
public:
- SNI_IpAllow(std::string &ip_allow_list, const std::string &servername)
- {
- // the server identified by item.fqdn requires ATS to do IP filtering
- if (ip_allow_list.length()) {
- IpAddr addr1;
- IpAddr addr2;
- // check format first
- // check if the input is a comma separated list of IPs
- ts::TextView content(ip_allow_list);
- while (!content.empty()) {
- ts::TextView list{content.take_prefix_at(',')};
- if (0 != ats_ip_range_parse(list, addr1, addr2)) {
- Debug("ssl_sni", "%.*s is not a valid format", static_cast<int>(list.size()), list.data());
- break;
- } else {
- Debug("ssl_sni", "%.*s added to the ip_allow list %s", static_cast<int>(list.size()), list.data(), servername.c_str());
- ip_map.fill(IpEndpoint().assign(addr1), IpEndpoint().assign(addr2), reinterpret_cast<void *>(1));
- }
- }
- }
- } // end function SNI_IpAllow
+ SNI_IpAllow(std::string &ip_allow_list, const std::string &servername); // end function SNI_IpAllow
Review Comment:
Removed the unnecessary comment.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1130175517
##########
doc/admin-guide/files/sni.yaml.en.rst:
##########
@@ -61,6 +61,12 @@ ip_allow Inbound Specify a list of client IP address, subnets
the connection. This list is comma separated. IPv4 and IPv6 addresses can be specified.
Here is an example list: 192.168.1.0/24,192.168.10.1-4. This would allow connections
from clients in the 19.168.1.0 network or in the range from 192.168.10.1 to 192.168.1.4.
+ Alternatively, the path to a file containing
+ the list of comma-separated IP addresses can
+ be specified in the form of
+ ``@path_to_file``. If a given file path does
Review Comment:
Looks good. Thanks.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1127180934
##########
iocore/net/SSLSNIConfig.cc:
##########
@@ -93,6 +96,78 @@ NamedElement::set_regex_name(const std::string ®ex_name)
////
// SNIConfigParams
//
+// ----
+
+SNI_IpAllow::SNI_IpAllow(std::string &ip_allow_list, std::string const &servername)
+{
+ ts::TextView content{ip_allow_list};
+ if (content && content[0] == '@') {
+ std::error_code ec;
+ ts::file::path path{content.remove_prefix(1)};
+ ts::LocalBufferWriter<1024> w;
+ if (path.is_relative()) {
+ path = ts::file::path(Layout::get()->sysconfdir) / path;
+ }
+ ip_allow_list = ts::file::load(path, ec);
+ if (ec) {
Review Comment:
This code allows the user to reference an external file that contains a list of IPs to filter on. Can you update the documentation with this functionality?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1129903261
##########
proxy/IPAllow.cc:
##########
@@ -60,6 +64,29 @@ bwformat(BufferWriter &w, Spec const &spec, YAML::Mark const &mark)
} // namespace swoc
+namespace YAML
+{
+template <> struct convert<ts::TextView> {
+ static Node
+ encode(ts::TextView const &tv)
Review Comment:
encode and decode don't seem to be called from anywhere on 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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1130011156
##########
iocore/net/SNIActionPerformer.cc:
##########
@@ -0,0 +1,96 @@
+/** @file
+
+ Implementation of SNIActionPerformer
+
+ @section license License
+
+ 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.
+ */
+
+#include "P_SNIActionPerformer.h"
+#include "tscore/ts_file.h"
+#include "tscpp/util/TextView.h"
+#include "tscore/BufferWriter.h"
+#include "tscore/bwf_std_format.h"
+
+SNI_IpAllow::SNI_IpAllow(std::string &ip_allow_list, std::string const &servername)
+{
+ ts::TextView content{ip_allow_list};
Review Comment:
@SolidWallOfCode I don't know which type we should use here. There are too many options available. Maybe `swoc::TextView` ?
And there may be swoc versions for other types as well.
Don't need to change existing code now but new code should use new types.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1130011956
##########
iocore/net/SNIActionPerformer.cc:
##########
@@ -0,0 +1,96 @@
+/** @file
+
+ Implementation of SNIActionPerformer
+
+ @section license License
+
+ 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.
+ */
+
+#include "P_SNIActionPerformer.h"
+#include "tscore/ts_file.h"
+#include "tscpp/util/TextView.h"
+#include "tscore/BufferWriter.h"
+#include "tscore/bwf_std_format.h"
+
+SNI_IpAllow::SNI_IpAllow(std::string &ip_allow_list, std::string const &servername)
+{
+ ts::TextView content{ip_allow_list};
+ if (content && content[0] == '@') {
+ std::error_code ec;
+ ts::file::path path{content.remove_prefix(1)};
+ ts::LocalBufferWriter<1024> w;
Review Comment:
This buffer is only needed when an error occurs.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #9501: Enable external file loading for sni.yaml.
Posted by "lzx404243 (via GitHub)" <gi...@apache.org>.
lzx404243 commented on code in PR #9501:
URL: https://github.com/apache/trafficserver/pull/9501#discussion_r1130043738
##########
iocore/net/SNIActionPerformer.cc:
##########
@@ -0,0 +1,96 @@
+/** @file
+
+ Implementation of SNIActionPerformer
+
+ @section license License
+
+ 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.
+ */
+
+#include "P_SNIActionPerformer.h"
+#include "tscore/ts_file.h"
+#include "tscpp/util/TextView.h"
+#include "tscore/BufferWriter.h"
+#include "tscore/bwf_std_format.h"
+
+SNI_IpAllow::SNI_IpAllow(std::string &ip_allow_list, std::string const &servername)
+{
+ ts::TextView content{ip_allow_list};
+ if (content && content[0] == '@') {
+ std::error_code ec;
+ ts::file::path path{content.remove_prefix(1)};
+ ts::LocalBufferWriter<1024> w;
Review Comment:
delayed the construction of `w` to the error path.
##########
iocore/net/SNIActionPerformer.cc:
##########
@@ -0,0 +1,96 @@
+/** @file
+
+ Implementation of SNIActionPerformer
+
+ @section license License
+
+ 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.
+ */
+
+#include "P_SNIActionPerformer.h"
+#include "tscore/ts_file.h"
+#include "tscpp/util/TextView.h"
+#include "tscore/BufferWriter.h"
+#include "tscore/bwf_std_format.h"
+
+SNI_IpAllow::SNI_IpAllow(std::string &ip_allow_list, std::string const &servername)
+{
+ ts::TextView content{ip_allow_list};
Review Comment:
I replaced the `ts::TextView`, `ts::file::path` and `ts::LocalBufferWriter` with the `swoc` equivalent.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org