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 2020/04/29 16:55:24 UTC
[GitHub] [trafficcontrol] ocket8888 opened a new pull request #4671: Ciab atstccfg
ocket8888 opened a new pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671
## What does this PR (Pull Request) do?
- [ ] This PR is not related to any Issue (I think)
This PR depends on #4288 .
Adds `atstccfg` to CDN-in-a-Box, allowing for a better representation of a true ATC system, and eliminating the dependency on my personal PyPI hosting of an old version of the TO Python client.
Also fixes the `atstccfg` docs, since the current behavior does not match what is documented.
## Which Traffic Control components are affected by this PR?
- CDN in a Box
- Documentation
- Traffic Ops ORT (Python version and one or two minor changes to ATSTCCFG)
## What is the best way to verify this PR?
The old `test` directory for ORT.py had a single test in it for setting API versions on configuration file objects returned by the old 1.x "meta config" route. Since that's now handled by ATSTCCFG, it's been removed. Instead, there's an executable `doctest-runner.py` file at the top of the ORT.py package that runs the doctests which already existed to some degree. In fact, they helped me find some bugs when I was building this. So just run that file to check for errors in the examples in docstrings.
But to be really sure, you'll need to start up CDN-in-a-Box and make sure that the `BADASS` run doesn't fail (which means ORT.py performed every operation it needs to perform and did it successfully). If it does fail, you'll see "Failed" echoed in the edge/mid cache servers' logs. The containers don't crash though, so that the problem can be debugged - so be careful not to assume that. If everything goes well, the "clocks" in TP should be cleared for edge and mid, and both should be marked available by TM.
## If this is a bug fix, what versions of Traffic Control are affected?
This technically includes a docs bug fix, which is present in:
- 4.0
- 4.1 (RC0)
- master
## The following criteria are ALL met by this PR
- [x] This PR includes tests
- [x] This PR includes documentation
- [x] An update to CHANGELOG.md is not necessary
- [x] This PR includes any and all required license headers
- [x] This PR does not include a database migration
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY**
----------------------------------------------------------------
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 #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418600654
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/utils.py
##########
@@ -96,3 +97,83 @@ def getJSONResponse(uri:str, cookies:dict = None, verify:bool = True) -> dict:
logging.debug("Response: %r\n%r", response.headers, response.content)
return response.json()
+
+def parse_multipart(raw: str) -> typing.List[typing.Tuple[str, str]]:
+ """
+ Parses a multipart/mixed-type payload and returns each contiguous chunk.
+
+ :param raw: The raw payload - without any HTTP status line.
+ :returns: A list where each element is a tuple where the first element is a chunk of the message. All headers are discarded except 'Path', which is the second element of each tuple if it was found in the chunk.
+ :raises: ValueError if the raw payload cannot be parsed as a multipart/mixed-type message.
+
+ >>> testdata = '''MIME-Version: 1.0\\r
+ ... Content-Type: multipart/mixed; boundary="test"\\r
+ ... \\r
+ ... --test\\r
+ ... Content-Type: text/plain; charset=us-ascii\\r
+ ... Path: /path/to/ats/root/directory/etc/trafficserver/fname\\r
+ ... \\r
+ ... # A fake testing file that wasn't generated at all on some date
+ ... CONFIG proxy.config.way.too.many.period.separated.words INT 1
+ ...
+ ... --test\\r
+ ... Content-Type: text/plain; charset=utf8\\r
+ ... Path: /path/to/ats/root/directory/etc/trafficserver/othername\\r
+ ... \\r
+ ... # The same header again
+ ... CONFIG proxy.config.the.same.insane.chain.of.words.again.but.the.last.one.is.different INT 0
+ ...
+ ... --test--\\r
+ ... '''
+ >>> output = parse_multipart(testdata)
+ >>> print(output[0][0])
+ # A fake testing file that wasn't generated at all on some date
+ CONFIG proxy.config.way.too.many.period.separated.words INT 1
+
+ >>> output[0][1]
+ '/path/to/ats/root/directory/etc/trafficserver/fname'
+ >>> print(output[1][0])
+ # The same header again
+ CONFIG proxy.config.the.same.insane.chain.of.words.again.but.the.last.one.is.different INT 0
+
+ >>> output[1][1]
+ '/path/to/ats/root/directory/etc/trafficserver/othername'
+ """
+ try:
+ hdr_index = raw.index("\r\n\r\n")
+ headers = {line.split(':')[0].casefold(): line.split(':')[1] for line in raw[:hdr_index].splitlines()}
+ except (IndexError, ValueError) as e:
+ raise ValueError("Invalid or corrupt multipart header") from e
+
+ ctype = headers.get("content-type")
+ if not ctype:
+ raise ValueError("Message is missing 'Content-Type' header")
+
+ try:
+ param_index = ctype.index(";")
+ params = {param.split('=')[0].strip(): param.split('=')[1].strip() for param in ctype[param_index+1:].split(';')}
Review comment:
TBH the existing way is more readable, it makes sense to stick with that for maintainability.
----------------------------------------------------------------
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 #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r445017792
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/config_files.py
##########
@@ -60,24 +60,37 @@ class ConfigFile():
contents = "" #: The full contents of the file - as configured in TO, not the on-disk contents
sanitizedContents = "" #: Will store the contents after sanitization
- def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/"):
+ def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/", *unused_args, contents: str = None, path: str = None):
"""
Constructs a :class:`ConfigFile` object from a raw API response
:param raw: A raw config file from an API response
:param toURL: The URL of a valid Traffic Ops host
:param tsroot: The absolute path to the root of an Apache Traffic Server installation
+ :param contents: Directly constructs a ConfigFile from the passed contents. Must be used with path, and causes raw to be ignored.
+ :param path: Sets the full path to the file when constructing ConfigFiles directly from contents.
:raises ValueError: if ``raw`` does not faithfully represent a configuration file
>>> a = ConfigFile({"fnameOnDisk": "test",
... "location": "/path/to",
- ... "apiURI":"/test",
- ... "scope": servers}, "http://example.com/")
+ ... "apiUri":"/test",
+ ... "scope": "servers"}, "http://example.com/")
>>> a
ConfigFile(path='/path/to/test', URI='http://example.com/test', scope='servers')
>>> a.SSLdir
- "/etc/trafficserver/ssl"
+ '/etc/trafficserver/ssl'
+ >>> ConfigFile(contents='testquest', path='/path/to/test')
+ ConfigFile(path='/path/to/test', URI=None, scope=None)
"""
+ self.SSLdir = os.path.join(tsroot, "etc", "trafficserver", "ssl")
Review comment:
I get
```
[zrhoffman@computer cdn-in-a-box]$ docker-compose exec mid curl -v https://edge.demo1.mycdn.ciab.test/
* About to connect() to edge.demo1.mycdn.ciab.test port 443 (#0)
* Trying 172.19.0.3...
* Connected to edge.demo1.mycdn.ciab.test (172.19.0.3) port 443 (#0)
* Initializing NSS with certpath: sql:/etc/pki/nssdb
* CAfile: /etc/pki/tls/certs/ca-bundle.crt
CApath: none
* NSS error -12286 (SSL_ERROR_NO_CYPHER_OVERLAP)
* Cannot communicate securely with peer: no common encryption algorithm(s).
* Closing connection 0
curl: (35) Cannot communicate securely with peer: no common encryption algorithm(s).
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r445008680
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/config_files.py
##########
@@ -60,24 +60,37 @@ class ConfigFile():
contents = "" #: The full contents of the file - as configured in TO, not the on-disk contents
sanitizedContents = "" #: Will store the contents after sanitization
- def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/"):
+ def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/", *unused_args, contents: str = None, path: str = None):
"""
Constructs a :class:`ConfigFile` object from a raw API response
:param raw: A raw config file from an API response
:param toURL: The URL of a valid Traffic Ops host
:param tsroot: The absolute path to the root of an Apache Traffic Server installation
+ :param contents: Directly constructs a ConfigFile from the passed contents. Must be used with path, and causes raw to be ignored.
+ :param path: Sets the full path to the file when constructing ConfigFiles directly from contents.
:raises ValueError: if ``raw`` does not faithfully represent a configuration file
>>> a = ConfigFile({"fnameOnDisk": "test",
... "location": "/path/to",
- ... "apiURI":"/test",
- ... "scope": servers}, "http://example.com/")
+ ... "apiUri":"/test",
+ ... "scope": "servers"}, "http://example.com/")
>>> a
ConfigFile(path='/path/to/test', URI='http://example.com/test', scope='servers')
>>> a.SSLdir
- "/etc/trafficserver/ssl"
+ '/etc/trafficserver/ssl'
+ >>> ConfigFile(contents='testquest', path='/path/to/test')
+ ConfigFile(path='/path/to/test', URI=None, scope=None)
"""
+ self.SSLdir = os.path.join(tsroot, "etc", "trafficserver", "ssl")
Review comment:
That's odd. I can `curl` through the cache using HTTPS just 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418697668
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
Yes `traffic_ops_ort.pl` requires systemd support, but the ORT RPM itself does not. `traffic_ops_ort.pl` itself is 91KB (less when compressed inside the RPM), and considering it (and `supermicro_udev_mapper.pl`) can be removed from the image in the same layer that the RPM is installed, the existence of `traffic_ops_ort.pl` in the RPM is not alone enough of a reason to build from source in the Makefile.
Installing the RPM with no dependencies seems reasonable to me, we don't need Perl anything installed in order to run ATSTCCFG.
----------------------------------------------------------------
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 #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r444662048
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/to_api.py
##########
@@ -156,31 +179,21 @@ def getMyConfigFiles(self, conf:Configuration) -> typing.List[dict]:
:raises ConnectionError: when something goes wrong communicating with Traffic Ops
"""
logging.info("Fetching list of configuration files from Traffic Ops")
+ atstccfg_cmd = self.atstccfg_cmd
+ if conf.mode is Configuration.Modes.REVALIDATE:
+ atstccfg_cmd = self.atstccfg_cmd + ["--revalidate-only"]
for _ in range(self.retries):
try:
- # The API function decorator confuses pylint into thinking this doesn't return
- #pylint: disable=E1111
- myFiles = self.get_server_config_files(host_name=self.hostname)
- #pylint: enable=E1111
- break
- except (InvalidJSONError, LoginError, OperationError, RequestException) as e:
+ proc = subprocess.run(atstccfg_cmd, stderr=subprocess.PIPE, stdout=subprocess.PIPE)
+ logging.debug("Raw response: %s", proc.stdout.decode())
+ if proc.stderr.decode():
+ logging.error(proc.stderr.decode())
Review comment:
Getting an info-level message logged as an error
```
edge_1 | ERROR: 2020-06-24 05:56:02,814 line 190 in to_api.getMyConfigFiles: INFO: toreq.go:51: 2020-06-24T05:56:02.6064406Z: URL: 'https://trafficops.infra.ciab.test:443' User: 'admin' Pass len: '8'
```
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/config_files.py
##########
@@ -60,24 +60,37 @@ class ConfigFile():
contents = "" #: The full contents of the file - as configured in TO, not the on-disk contents
sanitizedContents = "" #: Will store the contents after sanitization
- def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/"):
+ def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/", *unused_args, contents: str = None, path: str = None):
"""
Constructs a :class:`ConfigFile` object from a raw API response
:param raw: A raw config file from an API response
:param toURL: The URL of a valid Traffic Ops host
:param tsroot: The absolute path to the root of an Apache Traffic Server installation
+ :param contents: Directly constructs a ConfigFile from the passed contents. Must be used with path, and causes raw to be ignored.
+ :param path: Sets the full path to the file when constructing ConfigFiles directly from contents.
:raises ValueError: if ``raw`` does not faithfully represent a configuration file
>>> a = ConfigFile({"fnameOnDisk": "test",
... "location": "/path/to",
- ... "apiURI":"/test",
- ... "scope": servers}, "http://example.com/")
+ ... "apiUri":"/test",
+ ... "scope": "servers"}, "http://example.com/")
>>> a
ConfigFile(path='/path/to/test', URI='http://example.com/test', scope='servers')
>>> a.SSLdir
- "/etc/trafficserver/ssl"
+ '/etc/trafficserver/ssl'
+ >>> ConfigFile(contents='testquest', path='/path/to/test')
+ ConfigFile(path='/path/to/test', URI=None, scope=None)
"""
+ self.SSLdir = os.path.join(tsroot, "etc", "trafficserver", "ssl")
Review comment:
This SSL error still exists. Even putting
```bash
ln -s /opt/trafficserver/etc/trafficserver/ssl /etc/trafficserver/ssl
```
would make it work for now. If this PR doesn't somehow accommodate the SSL directory that atstccfg is hard-coded to use, it would be blocked by #4677.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r445013571
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/to_api.py
##########
@@ -156,31 +179,21 @@ def getMyConfigFiles(self, conf:Configuration) -> typing.List[dict]:
:raises ConnectionError: when something goes wrong communicating with Traffic Ops
"""
logging.info("Fetching list of configuration files from Traffic Ops")
+ atstccfg_cmd = self.atstccfg_cmd
+ if conf.mode is Configuration.Modes.REVALIDATE:
+ atstccfg_cmd = self.atstccfg_cmd + ["--revalidate-only"]
for _ in range(self.retries):
try:
- # The API function decorator confuses pylint into thinking this doesn't return
- #pylint: disable=E1111
- myFiles = self.get_server_config_files(host_name=self.hostname)
- #pylint: enable=E1111
- break
- except (InvalidJSONError, LoginError, OperationError, RequestException) as e:
+ proc = subprocess.run(atstccfg_cmd, stderr=subprocess.PIPE, stdout=subprocess.PIPE)
+ logging.debug("Raw response: %s", proc.stdout.decode())
+ if proc.stderr.decode():
+ logging.error(proc.stderr.decode())
Review comment:
I just did that because I can't know ahead of time what kinds of messages are in stderr. Since there could be errors, I bring it up to the error level. The only way to fix that would be to iterate the lines of output, parse them and then log each one to the appropriate level, which I thought was a waste of time (and also possibly fruitless, since atstccfg is supposedly meant to be a black box).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418748477
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
No, I mean there will _be_ an RPM, but the RPM just contains the built binaries. So what I'm saying is just build the binaries and pull those into CiaB instead of building the binaries, putting them in an RPM, pulling that into CiaB, then unwrapping the RPM and pulling out the binaries.
----------------------------------------------------------------
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 #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r440246311
##########
File path: traffic_ops/ort/atstccfg/config/config.go
##########
@@ -140,6 +140,9 @@ func GetCfg() (Cfg, error) {
if toPass == "" {
toPass = os.Getenv("TO_PASS")
}
+ if toPass == "" {
+ toPass = os.Getenv("TO_PASSWORD")
+ }
Review comment:
That makes sense. Even though it's in the documentation, these lines could probably use a comment so people know that `TO_PASSWORD` is the preferred one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418748477
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
No, I mean there will _be_ an RPM, but the RPM just contains the built binaries. So what I'm saying is just build the binaries and pull those into CiaB instead of building the binaries, putting them in an RPM, pulling that into CiaB, then unwrapping the RPM and pulling out the binaries.
Like, Traffic Ops has an RPM, but that doesn't mean CiaB needs to use it. It's in the same repo, so you can just build `traffic_ops_golang` and then directly add that to the container.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418772735
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
The container's CPU architecture can only match your CPU architecture. Idk how you would break that.
But actually, I didn't make this clear: CiaB today _is_ used for testing and so whatever I'd like for the future, having the right Go version etc. _is_ important today.
I was waiting for the "ORT has its own build" PR to merge before updating this so it can use that new build command to generate the RPM. However, I just thought of something: what if I just build it from source from within the `cache/Dockerfile` container? Still no unnecessary RPM work, but we get the correct environment. Would everyone be good with that?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418771897
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
As a dev environment it will only work fine if your CPU architecture matches the container's CPU architecture, right?
----------------------------------------------------------------
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 #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418670797
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/config_files.py
##########
@@ -90,15 +103,14 @@ def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/"):
except (KeyError, TypeError, IndexError) as e:
raise ValueError from e
- self.SSLdir = os.path.join(tsroot, "etc", "trafficserver", "ssl")
Review comment:
>> PEP8
>no.
Yes. 😄
----------------------------------------------------------------
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] mattjackson220 merged pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
mattjackson220 merged pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418598106
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/to_api.py
##########
@@ -113,37 +152,21 @@ def getMyPackages(self) -> typing.List[packaging.Package]:
"""
logging.info("Fetching this server's package list from Traffic Ops")
- # Ah, read-only properties that gut functionality, my favorite.
- tmp = self.api_base_url
- self._api_base_url = urljoin(self._server_url, '/').rstrip('/') + '/'
-
- packagesPath = '/'.join(("ort", self.hostname, "packages"))
+ atstccfg_cmd = self.atstccfg_cmd + ["--get-data=packages"]
for _ in range(self.retries):
try:
- myPackages = self.get(packagesPath)
- break
- except (LoginError, OperationError, InvalidJSONError, RequestException) as e:
+ proc = subprocess.run(atstccfg_cmd, stderr=subprocess.PIPE, stdout=subprocess.PIPE)
+ logging.debug("Raw output: %s", proc.stdout.decode())
+ if proc.stderr.decode():
+ logging.error("proc.stderr.decode()")
+ if proc.returncode == 0:
+ return [Package(p) for p in json.loads(proc.stdout.decode())]
Review comment:
Oh, looks like I didn't do that. Weird, because I think this was like that before and I didn't change that import...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418739679
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
> _"Alternatively, installing the ORT RPM helps lay the groundwork for actually running the official ORT-thing in CIAB. Since ORT is will be undergoing a redesign/rewrite in the near future, it might be best to start planning on getting it into CIAB."_
Only if we're going to install _that_ by RPM. If it were up to me, there would be no RPMs and things would build directly from the source tree. I know that's not popular because of how CIAB has to flex between being a testing and a development environment, but that's just how I feel.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r420329514
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
Yeah I guess that's true.
I still don't like using RPMs, but I feel more strongly about just getting atstccfg to run inside CiaB. So I'm just gonna put this into a draft status pending the merge of #2776 .
----------------------------------------------------------------
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 #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418708054
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
I get what you are saying about the time taken to build the ORT RPM though, since it is build alongside the Traffic Ops RPM. #2776 (separate ORT build) seems like a good way to speed it up. But because these PRs are pretty much independent, #2776 shouldn't hold up 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418782823
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
> The container's CPU architecture can only match your CPU architecture. Idk how you would break that.
I assume that the VM used by Docker For Mac might have a different virtualized CPU architecture than the underlying Mac, but I could be wrong about that and don't feel like verifying. Either way, something built on Mac cannot be assumed to run correctly on Linux.
> Would everyone be good with that?
That's better but still not ideal. I don't really see why using the RPM is a problem. The RPM would only need rebuilt if you were testing changes to atstccfg, but it doesn't really make sense to develop atstccfg in CIAB when CIAB isn't using traffic_ops_ort.pl.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418294027
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/utils.py
##########
@@ -96,3 +97,83 @@ def getJSONResponse(uri:str, cookies:dict = None, verify:bool = True) -> dict:
logging.debug("Response: %r\n%r", response.headers, response.content)
return response.json()
+
+def parse_multipart(raw: str) -> typing.List[typing.Tuple[str, str]]:
+ """
+ Parses a multipart/mixed-type payload and returns each contiguous chunk.
+
+ :param raw: The raw payload - without any HTTP status line.
+ :returns: A list where each element is a tuple where the first element is a chunk of the message. All headers are discarded except 'Path', which is the second element of each tuple if it was found in the chunk.
+ :raises: ValueError if the raw payload cannot be parsed as a multipart/mixed-type message.
+
+ >>> testdata = '''MIME-Version: 1.0\\r
+ ... Content-Type: multipart/mixed; boundary="test"\\r
+ ... \\r
+ ... --test\\r
+ ... Content-Type: text/plain; charset=us-ascii\\r
+ ... Path: /path/to/ats/root/directory/etc/trafficserver/fname\\r
+ ... \\r
+ ... # A fake testing file that wasn't generated at all on some date
+ ... CONFIG proxy.config.way.too.many.period.separated.words INT 1
+ ...
+ ... --test\\r
+ ... Content-Type: text/plain; charset=utf8\\r
+ ... Path: /path/to/ats/root/directory/etc/trafficserver/othername\\r
+ ... \\r
+ ... # The same header again
+ ... CONFIG proxy.config.the.same.insane.chain.of.words.again.but.the.last.one.is.different INT 0
+ ...
+ ... --test--\\r
+ ... '''
+ >>> output = parse_multipart(testdata)
+ >>> print(output[0][0])
+ # A fake testing file that wasn't generated at all on some date
+ CONFIG proxy.config.way.too.many.period.separated.words INT 1
+
+ >>> output[0][1]
+ '/path/to/ats/root/directory/etc/trafficserver/fname'
+ >>> print(output[1][0])
+ # The same header again
+ CONFIG proxy.config.the.same.insane.chain.of.words.again.but.the.last.one.is.different INT 0
+
+ >>> output[1][1]
+ '/path/to/ats/root/directory/etc/trafficserver/othername'
+ """
+ try:
+ hdr_index = raw.index("\r\n\r\n")
+ headers = {line.split(':')[0].casefold(): line.split(':')[1] for line in raw[:hdr_index].splitlines()}
+ except (IndexError, ValueError) as e:
+ raise ValueError("Invalid or corrupt multipart header") from e
+
+ ctype = headers.get("content-type")
+ if not ctype:
+ raise ValueError("Message is missing 'Content-Type' header")
+
+ try:
+ param_index = ctype.index(";")
+ params = {param.split('=')[0].strip(): param.split('=')[1].strip() for param in ctype[param_index+1:].split(';')}
Review comment:
Yeah, or if I was allowed to require Python3.8 I could use those fancy temporary expression-scoped variable bindings.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418772735
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
The container's CPU architecture can only match your CPU architecture. Idk how you would break that.
But actually, I didn't make this clear: CiaB today _is_ used for testing and so whatever I'd like for the future, having the right Go version etc. _is_ important today.
I was waiting for the "ORT has its own build" PR to merge before updating this so it can use that new build command to generate the RPM. However, I just thought of something: what if I just build it from source from within the `cache/Dockerfile` container? Still no unnecessary RPM work, but we get the correct environment. Would everyone be good with that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418334714
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/config_files.py
##########
@@ -60,24 +60,37 @@ class ConfigFile():
contents = "" #: The full contents of the file - as configured in TO, not the on-disk contents
sanitizedContents = "" #: Will store the contents after sanitization
- def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/"):
+ def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/", *unused_args, contents: str = None, path: str = None):
"""
Constructs a :class:`ConfigFile` object from a raw API response
:param raw: A raw config file from an API response
:param toURL: The URL of a valid Traffic Ops host
:param tsroot: The absolute path to the root of an Apache Traffic Server installation
+ :param contents: Directly constructs a ConfigFile from the passed contents. Must be used with path, and causes raw to be ignored.
+ :param path: Sets the full path to the file when constructing ConfigFiles directly from contents.
:raises ValueError: if ``raw`` does not faithfully represent a configuration file
>>> a = ConfigFile({"fnameOnDisk": "test",
... "location": "/path/to",
- ... "apiURI":"/test",
- ... "scope": servers}, "http://example.com/")
+ ... "apiUri":"/test",
+ ... "scope": "servers"}, "http://example.com/")
>>> a
ConfigFile(path='/path/to/test', URI='http://example.com/test', scope='servers')
>>> a.SSLdir
- "/etc/trafficserver/ssl"
+ '/etc/trafficserver/ssl'
+ >>> ConfigFile(contents='testquest', path='/path/to/test')
+ ConfigFile(path='/path/to/test', URI=None, scope=None)
"""
+ self.SSLdir = os.path.join(tsroot, "etc", "trafficserver", "ssl")
Review comment:
Actually, that is prefixed with `tsroot`. It's `atstccfg` that's assuming everything's installed under `/opt/trafficserver`. As stated in the Profile's Parameters, that's not where it should go, and is currently a bug in `atstccfg` if it's putting it anywhere else:
```json
{
"configFile": "ssl_multicert.config",
"name": "location",
"value": "/etc/trafficserver"
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418238608
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/utils.py
##########
@@ -96,3 +97,83 @@ def getJSONResponse(uri:str, cookies:dict = None, verify:bool = True) -> dict:
logging.debug("Response: %r\n%r", response.headers, response.content)
return response.json()
+
+def parse_multipart(raw: str) -> typing.List[typing.Tuple[str, str]]:
+ """
+ Parses a multipart/mixed-type payload and returns each contiguous chunk.
+
+ :param raw: The raw payload - without any HTTP status line.
+ :returns: A list where each element is a tuple where the first element is a chunk of the message. All headers are discarded except 'Path', which is the second element of each tuple if it was found in the chunk.
+ :raises: ValueError if the raw payload cannot be parsed as a multipart/mixed-type message.
+
+ >>> testdata = '''MIME-Version: 1.0\\r
+ ... Content-Type: multipart/mixed; boundary="test"\\r
+ ... \\r
+ ... --test\\r
+ ... Content-Type: text/plain; charset=us-ascii\\r
+ ... Path: /path/to/ats/root/directory/etc/trafficserver/fname\\r
+ ... \\r
+ ... # A fake testing file that wasn't generated at all on some date
+ ... CONFIG proxy.config.way.too.many.period.separated.words INT 1
+ ...
+ ... --test\\r
+ ... Content-Type: text/plain; charset=utf8\\r
+ ... Path: /path/to/ats/root/directory/etc/trafficserver/othername\\r
+ ... \\r
+ ... # The same header again
+ ... CONFIG proxy.config.the.same.insane.chain.of.words.again.but.the.last.one.is.different INT 0
+ ...
+ ... --test--\\r
+ ... '''
+ >>> output = parse_multipart(testdata)
+ >>> print(output[0][0])
+ # A fake testing file that wasn't generated at all on some date
+ CONFIG proxy.config.way.too.many.period.separated.words INT 1
+
+ >>> output[0][1]
+ '/path/to/ats/root/directory/etc/trafficserver/fname'
+ >>> print(output[1][0])
+ # The same header again
+ CONFIG proxy.config.the.same.insane.chain.of.words.again.but.the.last.one.is.different INT 0
+
+ >>> output[1][1]
+ '/path/to/ats/root/directory/etc/trafficserver/othername'
+ """
+ try:
+ hdr_index = raw.index("\r\n\r\n")
+ headers = {line.split(':')[0].casefold(): line.split(':')[1] for line in raw[:hdr_index].splitlines()}
+ except (IndexError, ValueError) as e:
+ raise ValueError("Invalid or corrupt multipart header") from e
+
+ ctype = headers.get("content-type")
+ if not ctype:
+ raise ValueError("Message is missing 'Content-Type' header")
+
+ try:
+ param_index = ctype.index(";")
+ params = {param.split('=')[0].strip(): param.split('=')[1].strip() for param in ctype[param_index+1:].split(';')}
+ except (IndexError, ValueError) as e:
+ raise ValueError("Invalid or corrupt 'Content-Type' header") from e
+
+ boundary = params.get("boundary", "").strip('"\'')
+ if not boundary:
+ raise ValueError("'Content-Type' header missing 'boundary' parameter")
+
+ chunks = raw.split(f"--{boundary}")[1:] #ignore prologue
Review comment:
see above comment about PEP8/linting compliance
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418701818
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
I was really hoping to get away with not installing the RPM here, because I think that taking the time to build an entire RPM just to unwrap it for one binary is a waste of time for a dev environment, and this is the only image where it hasn't been done in the past (besides the non-ATC ones).
If you really insist then I'll change it, but just know that it'd make me very sad.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418238279
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/to_api.py
##########
@@ -113,37 +152,21 @@ def getMyPackages(self) -> typing.List[packaging.Package]:
"""
logging.info("Fetching this server's package list from Traffic Ops")
- # Ah, read-only properties that gut functionality, my favorite.
- tmp = self.api_base_url
- self._api_base_url = urljoin(self._server_url, '/').rstrip('/') + '/'
-
- packagesPath = '/'.join(("ort", self.hostname, "packages"))
+ atstccfg_cmd = self.atstccfg_cmd + ["--get-data=packages"]
for _ in range(self.retries):
try:
- myPackages = self.get(packagesPath)
- break
- except (LoginError, OperationError, InvalidJSONError, RequestException) as e:
+ proc = subprocess.run(atstccfg_cmd, stderr=subprocess.PIPE, stdout=subprocess.PIPE)
+ logging.debug("Raw output: %s", proc.stdout.decode())
+ if proc.stderr.decode():
+ logging.error("proc.stderr.decode()")
+ if proc.returncode == 0:
+ return [Package(p) for p in json.loads(proc.stdout.decode())]
Review comment:
`from .package import Package`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418770098
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
None of that's a problem unless CiaB is being used for testing deployments. As a dev environment it works 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418701818
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
I was really hoping to get away with not installing the RPM here, because I think that taking the time to build an entire RPM just to unwrap it for one binary is a waste of time for a dev environment, and this is the only image where it hasn't been done in the past (besides the non-ATC ones).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418748477
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
No, I mean there will _be_ an RPM, but the RPM just contains the built binaries. So what I'm saying is just build the binaries and pull those into CiaB instead of building the binaries, putting them in an RPM, pulling that into CiaB, then unwrapping the RPM and pulling out the binaries.
Like, Traffic Ops has an RPM, but that doesn't mean CiaB needs to use it. It's in the same repo, so you can just build `traffic_ops_golang` and then directly add that to the container (of course the fact that TO Perl exists blows that out of the water entirely).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418701035
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/config_files.py
##########
@@ -90,15 +103,14 @@ def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/"):
except (KeyError, TypeError, IndexError) as e:
raise ValueError from e
- self.SSLdir = os.path.join(tsroot, "etc", "trafficserver", "ssl")
Review comment:
fite me irl
----------------------------------------------------------------
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 #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418292341
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/utils.py
##########
@@ -96,3 +97,83 @@ def getJSONResponse(uri:str, cookies:dict = None, verify:bool = True) -> dict:
logging.debug("Response: %r\n%r", response.headers, response.content)
return response.json()
+
+def parse_multipart(raw: str) -> typing.List[typing.Tuple[str, str]]:
+ """
+ Parses a multipart/mixed-type payload and returns each contiguous chunk.
+
+ :param raw: The raw payload - without any HTTP status line.
+ :returns: A list where each element is a tuple where the first element is a chunk of the message. All headers are discarded except 'Path', which is the second element of each tuple if it was found in the chunk.
+ :raises: ValueError if the raw payload cannot be parsed as a multipart/mixed-type message.
+
+ >>> testdata = '''MIME-Version: 1.0\\r
+ ... Content-Type: multipart/mixed; boundary="test"\\r
+ ... \\r
+ ... --test\\r
+ ... Content-Type: text/plain; charset=us-ascii\\r
+ ... Path: /path/to/ats/root/directory/etc/trafficserver/fname\\r
+ ... \\r
+ ... # A fake testing file that wasn't generated at all on some date
+ ... CONFIG proxy.config.way.too.many.period.separated.words INT 1
+ ...
+ ... --test\\r
+ ... Content-Type: text/plain; charset=utf8\\r
+ ... Path: /path/to/ats/root/directory/etc/trafficserver/othername\\r
+ ... \\r
+ ... # The same header again
+ ... CONFIG proxy.config.the.same.insane.chain.of.words.again.but.the.last.one.is.different INT 0
+ ...
+ ... --test--\\r
+ ... '''
+ >>> output = parse_multipart(testdata)
+ >>> print(output[0][0])
+ # A fake testing file that wasn't generated at all on some date
+ CONFIG proxy.config.way.too.many.period.separated.words INT 1
+
+ >>> output[0][1]
+ '/path/to/ats/root/directory/etc/trafficserver/fname'
+ >>> print(output[1][0])
+ # The same header again
+ CONFIG proxy.config.the.same.insane.chain.of.words.again.but.the.last.one.is.different INT 0
+
+ >>> output[1][1]
+ '/path/to/ats/root/directory/etc/trafficserver/othername'
+ """
+ try:
+ hdr_index = raw.index("\r\n\r\n")
+ headers = {line.split(':')[0].casefold(): line.split(':')[1] for line in raw[:hdr_index].splitlines()}
+ except (IndexError, ValueError) as e:
+ raise ValueError("Invalid or corrupt multipart header") from e
+
+ ctype = headers.get("content-type")
+ if not ctype:
+ raise ValueError("Message is missing 'Content-Type' header")
+
+ try:
+ param_index = ctype.index(";")
+ params = {param.split('=')[0].strip(): param.split('=')[1].strip() for param in ctype[param_index+1:].split(';')}
Review comment:
Not that it's more readable, but to avoid using `.split('=')` twice, this could be
```python
dict([part.strip() for part in param.split('=')] for param in ctype[param_index+1:].split(';'))
```
----------------------------------------------------------------
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 #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418244053
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/config_files.py
##########
@@ -90,15 +103,14 @@ def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/"):
except (KeyError, TypeError, IndexError) as e:
raise ValueError from e
- self.SSLdir = os.path.join(tsroot, "etc", "trafficserver", "ssl")
Review comment:
Yeah, adding a pylint.rc for Python ORT + conforming to it should be its own 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r445041966
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/config_files.py
##########
@@ -60,24 +60,37 @@ class ConfigFile():
contents = "" #: The full contents of the file - as configured in TO, not the on-disk contents
sanitizedContents = "" #: Will store the contents after sanitization
- def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/"):
+ def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/", *unused_args, contents: str = None, path: str = None):
"""
Constructs a :class:`ConfigFile` object from a raw API response
:param raw: A raw config file from an API response
:param toURL: The URL of a valid Traffic Ops host
:param tsroot: The absolute path to the root of an Apache Traffic Server installation
+ :param contents: Directly constructs a ConfigFile from the passed contents. Must be used with path, and causes raw to be ignored.
+ :param path: Sets the full path to the file when constructing ConfigFiles directly from contents.
:raises ValueError: if ``raw`` does not faithfully represent a configuration file
>>> a = ConfigFile({"fnameOnDisk": "test",
... "location": "/path/to",
- ... "apiURI":"/test",
- ... "scope": servers}, "http://example.com/")
+ ... "apiUri":"/test",
+ ... "scope": "servers"}, "http://example.com/")
>>> a
ConfigFile(path='/path/to/test', URI='http://example.com/test', scope='servers')
>>> a.SSLdir
- "/etc/trafficserver/ssl"
+ '/etc/trafficserver/ssl'
+ >>> ConfigFile(contents='testquest', path='/path/to/test')
+ ConfigFile(path='/path/to/test', URI=None, scope=None)
"""
+ self.SSLdir = os.path.join(tsroot, "etc", "trafficserver", "ssl")
Review comment:
I don't see that error... But the readiness container is having problems, so I'll try the symlink fix
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r441124559
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
Alright, it's done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418239744
##########
File path: traffic_ops/ort/atstccfg/config/config.go
##########
@@ -140,6 +140,9 @@ func GetCfg() (Cfg, error) {
if toPass == "" {
toPass = os.Getenv("TO_PASS")
}
+ if toPass == "" {
+ toPass = os.Getenv("TO_PASSWORD")
+ }
Review comment:
Because along with `TO_USER` and `TO_URL`, `TO_PASSWORD` is used by the `to_access` package, the `to_access.sh` script, the `compare` and `genConfigFileRoutes.py` tools, and some other things which make it semi-standard. But `TO_PASS` was there first, so I didn't want to break compatibility with existing systems by changing it. So instead I added the more normal one and switched the documentation to only mention that, and then later we might be able to get rid of the one-off `TO_PASS` support.
----------------------------------------------------------------
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 #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r445025260
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/config_files.py
##########
@@ -60,24 +60,37 @@ class ConfigFile():
contents = "" #: The full contents of the file - as configured in TO, not the on-disk contents
sanitizedContents = "" #: Will store the contents after sanitization
- def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/"):
+ def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/", *unused_args, contents: str = None, path: str = None):
"""
Constructs a :class:`ConfigFile` object from a raw API response
:param raw: A raw config file from an API response
:param toURL: The URL of a valid Traffic Ops host
:param tsroot: The absolute path to the root of an Apache Traffic Server installation
+ :param contents: Directly constructs a ConfigFile from the passed contents. Must be used with path, and causes raw to be ignored.
+ :param path: Sets the full path to the file when constructing ConfigFiles directly from contents.
:raises ValueError: if ``raw`` does not faithfully represent a configuration file
>>> a = ConfigFile({"fnameOnDisk": "test",
... "location": "/path/to",
- ... "apiURI":"/test",
- ... "scope": servers}, "http://example.com/")
+ ... "apiUri":"/test",
+ ... "scope": "servers"}, "http://example.com/")
>>> a
ConfigFile(path='/path/to/test', URI='http://example.com/test', scope='servers')
>>> a.SSLdir
- "/etc/trafficserver/ssl"
+ '/etc/trafficserver/ssl'
+ >>> ConfigFile(contents='testquest', path='/path/to/test')
+ ConfigFile(path='/path/to/test', URI=None, scope=None)
"""
+ self.SSLdir = os.path.join(tsroot, "etc", "trafficserver", "ssl")
Review comment:
From the edge logs:
```
edge_1 | ============ Processing File: video_demo1_mycdn_ciab_test_cert.cer ============
edge_1 | INFO: 2020-06-24 16:21:38,187 line 223 in config_files.update: File /opt/trafficserver/etc/trafficserver/ssl/video_demo1_mycdn_ciab_test_cert.cer will be created
edge_1 | [Jun 24 16:21:41.550] Server {0x7f3080ed1880} ERROR: SSL::139846298179712:error:02001002:system library:fopen:No such file or directory:bss_file.c:175:fopen('/etc/trafficserver/ssl/video_demo1_mycdn_ciab_test_cert.cer','r')
```
----------------------------------------------------------------
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 #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r445023350
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/config_files.py
##########
@@ -60,24 +60,37 @@ class ConfigFile():
contents = "" #: The full contents of the file - as configured in TO, not the on-disk contents
sanitizedContents = "" #: Will store the contents after sanitization
- def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/"):
+ def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/", *unused_args, contents: str = None, path: str = None):
"""
Constructs a :class:`ConfigFile` object from a raw API response
:param raw: A raw config file from an API response
:param toURL: The URL of a valid Traffic Ops host
:param tsroot: The absolute path to the root of an Apache Traffic Server installation
+ :param contents: Directly constructs a ConfigFile from the passed contents. Must be used with path, and causes raw to be ignored.
+ :param path: Sets the full path to the file when constructing ConfigFiles directly from contents.
:raises ValueError: if ``raw`` does not faithfully represent a configuration file
>>> a = ConfigFile({"fnameOnDisk": "test",
... "location": "/path/to",
- ... "apiURI":"/test",
- ... "scope": servers}, "http://example.com/")
+ ... "apiUri":"/test",
+ ... "scope": "servers"}, "http://example.com/")
>>> a
ConfigFile(path='/path/to/test', URI='http://example.com/test', scope='servers')
>>> a.SSLdir
- "/etc/trafficserver/ssl"
+ '/etc/trafficserver/ssl'
+ >>> ConfigFile(contents='testquest', path='/path/to/test')
+ ConfigFile(path='/path/to/test', URI=None, scope=None)
"""
+ self.SSLdir = os.path.join(tsroot, "etc", "trafficserver", "ssl")
Review comment:
Doing that succeeds for TR and fails when redirected to the cache
```
[zrhoffman@computer cdn-in-a-box]$ docker-compose exec mid curl -vL https://video.demo1.mycdn.ciab.test/
* About to connect() to video.demo1.mycdn.ciab.test port 443 (#0)
* Trying 172.19.0.12...
* Connected to video.demo1.mycdn.ciab.test (172.19.0.12) port 443 (#0)
* Initializing NSS with certpath: sql:/etc/pki/nssdb
* CAfile: /etc/pki/tls/certs/ca-bundle.crt
CApath: none
* SSL connection using TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
* Server certificate:
* subject: CN=*.demo1.mycdn.ciab.test,O=CDN-in-a-Box,L=Denver,ST=Colorado,C=US
* start date: Dec 23 22:55:46 2019 GMT
* expire date: Dec 22 22:55:46 2020 GMT
* common name: *.demo1.mycdn.ciab.test
* issuer: E=no-reply@infra.ciab.test,CN=CDN-in-a-Box Intermediate CA,OU=CDN-in-a-Box,O=CDN-in-a-Box,L=Denver,ST=Colorado,C=US
> GET / HTTP/1.1
> User-Agent: curl/7.29.0
> Host: video.demo1.mycdn.ciab.test
> Accept: */*
>
< HTTP/1.1 302 Found
< Location: https://edge.demo1.mycdn.ciab.test/
< Content-Length: 0
< Date: Wed, 24 Jun 2020 16:31:34 GMT
<
* Connection #0 to host video.demo1.mycdn.ciab.test left intact
* Issue another request to this URL: 'https://edge.demo1.mycdn.ciab.test/'
* About to connect() to edge.demo1.mycdn.ciab.test port 443 (#1)
* Trying 172.19.0.3...
* Connected to edge.demo1.mycdn.ciab.test (172.19.0.3) port 443 (#1)
* CAfile: /etc/pki/tls/certs/ca-bundle.crt
CApath: none
* NSS error -12286 (SSL_ERROR_NO_CYPHER_OVERLAP)
* Cannot communicate securely with peer: no common encryption algorithm(s).
* Closing connection 1
curl: (35) Cannot communicate securely with peer: no common encryption algorithm(s).
```
----------------------------------------------------------------
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 #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r445014793
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/to_api.py
##########
@@ -156,31 +179,21 @@ def getMyConfigFiles(self, conf:Configuration) -> typing.List[dict]:
:raises ConnectionError: when something goes wrong communicating with Traffic Ops
"""
logging.info("Fetching list of configuration files from Traffic Ops")
+ atstccfg_cmd = self.atstccfg_cmd
+ if conf.mode is Configuration.Modes.REVALIDATE:
+ atstccfg_cmd = self.atstccfg_cmd + ["--revalidate-only"]
for _ in range(self.retries):
try:
- # The API function decorator confuses pylint into thinking this doesn't return
- #pylint: disable=E1111
- myFiles = self.get_server_config_files(host_name=self.hostname)
- #pylint: enable=E1111
- break
- except (InvalidJSONError, LoginError, OperationError, RequestException) as e:
+ proc = subprocess.run(atstccfg_cmd, stderr=subprocess.PIPE, stdout=subprocess.PIPE)
+ logging.debug("Raw response: %s", proc.stdout.decode())
+ if proc.stderr.decode():
+ logging.error(proc.stderr.decode())
Review comment:
Agreed, naively printing it sounds preferable in this case
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418238066
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/config_files.py
##########
@@ -90,15 +103,14 @@ def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/"):
except (KeyError, TypeError, IndexError) as e:
raise ValueError from e
- self.SSLdir = os.path.join(tsroot, "etc", "trafficserver", "ssl")
Review comment:
> PEP8
no.
But we do have a pylintrc file that's supposed to be enforced at `traffic_control/clients/python/.pylintrc`. Trouble is, that file was introduced well after ORT.py was written and there are quite possibly hundreds of problems.
So what I'm getting at is I can remove the extra line, but in general linter compliance is a larger effort that should be done outside of 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418741562
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
I believe the ORT replacement will be installed via RPM -- the 5, 10, 20, however many binaries/things there are going to be, should all be included in the same RPM. At least, that's what I understand out of the blueprint review so far.
----------------------------------------------------------------
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 #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418170361
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -51,7 +53,7 @@ TS_SOURCE := $(wildcard ../../traffic_stats/**/*)
.PHONY: clean very-clean all nearly-all debug
# Default target; builds all pre-requisite rpms from source trees
-all: traffic_monitor/traffic_monitor.rpm traffic_portal/traffic_portal.rpm traffic_ops/traffic_ops.rpm traffic_router/traffic_router.rpm traffic_router/tomcat.rpm traffic_stats/traffic_stats.rpm
+all: ../../traffic_ops/ort/atstccfg/atstccfg traffic_monitor/traffic_monitor.rpm traffic_portal/traffic_portal.rpm traffic_ops/traffic_ops.rpm traffic_router/traffic_router.rpm traffic_router/tomcat.rpm traffic_stats/traffic_stats.rpm
Review comment:
Should reference the ORT RPM
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
This should copy the Traffic Ops ORT RPM, not build from source.
##########
File path: infrastructure/cdn-in-a-box/cache/Dockerfile
##########
@@ -48,11 +48,13 @@ RUN mkdir -p /var/trafficserver /opt/ort && \
RUN setcap CAP_NET_BIND_SERVICE=+eip /bin/traffic_server && setcap CAP_NET_BIND_SERVICE=+eip /bin/traffic_manager && setcap CAP_NET_BIND_SERVICE=+eip /bin/trafficserver && setcap CAP_NET_BIND_SERVICE=+eip /bin/traffic_cop
-ADD infrastructure/cdn-in-a-box/ort /opt/ort/
-
WORKDIR /opt
-RUN touch /var/log/ort.log && pip3 install Apache-TrafficControl==1.1.3 && pip3 install ./ort && cp ort/traffic_ops_ort.crontab /etc/cron.d/traffic_ops_ort-cron-template && mkdir -p /opt/init.d && cp ort/traffic_ops_ort.logrotate /etc/logrotate.d/ort
+ADD infrastructure/cdn-in-a-box/ort /opt/ort/
+ADD traffic_control/clients/python /opt/Apache-TrafficControl/
+
+RUN touch /var/log/ort.log && pip3 install ./Apache-TrafficControl && pip3 install ./ort && cp ort/traffic_ops_ort.crontab /etc/cron.d/traffic_ops_ort-cron-template && mkdir -p /opt/init.d && cp ort/traffic_ops_ort.logrotate /etc/logrotate.d/ort
Review comment:
Each command should be on its own line for readability.
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/config_files.py
##########
@@ -90,15 +103,14 @@ def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/"):
except (KeyError, TypeError, IndexError) as e:
raise ValueError from e
- self.SSLdir = os.path.join(tsroot, "etc", "trafficserver", "ssl")
Review comment:
Nit: Now that this line is removed, there are 2 blank lines in a row, but method definitions in a class [should be separated by only 1 blank line](https://www.python.org/dev/peps/pep-0008/#blank-lines).
```shell
autopep8 -ri .
```
##########
File path: infrastructure/cdn-in-a-box/ort/doctest-runner.py
##########
@@ -1,3 +1,5 @@
+#!/usr/bin/python3
Review comment:
Since the `tests` directory no longer exists,
```shell
python3 -m unittest discover -s tests
```
now fails. That command should be removed from the README and `unittest` should be removed as a dev dependency. Maybe add a note about `doctest-runner.py`, too
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/to_api.py
##########
@@ -156,31 +179,32 @@ def getMyConfigFiles(self, conf:Configuration) -> typing.List[dict]:
:raises ConnectionError: when something goes wrong communicating with Traffic Ops
"""
logging.info("Fetching list of configuration files from Traffic Ops")
+ atstccfg_cmd = self.atstccfg_cmd
+ if conf.mode is Configuration.Modes.REVALIDATE:
+ atstccfg_cmd = self.atstccfg_cmd + ["--revalidate-only"]
for _ in range(self.retries):
try:
- # The API function decorator confuses pylint into thinking this doesn't return
- #pylint: disable=E1111
- myFiles = self.get_server_config_files(host_name=self.hostname)
- #pylint: enable=E1111
- break
- except (InvalidJSONError, LoginError, OperationError, RequestException) as e:
+ proc = subprocess.run(atstccfg_cmd, stderr=subprocess.PIPE, stdout=subprocess.PIPE)
+ logging.debug("Raw response: %s", proc.stdout.decode())
+ if proc.stderr.decode():
+ logging.error(proc.stderr.decode())
+ if proc.returncode == 0:
+ return [ConfigFile(tsroot=conf.tsroot, contents=x[0], path=x[1]) for x in utils.parse_multipart(proc.stdout.decode())]
+ except (subprocess.SubprocessError, ValueError, OSError) as e:
logging.debug("config file fetch failure: %r", e, exc_info=True, stack_info=True)
- else:
- raise ConnectionError("Failed to fetch configuration files from Traffic Ops")
- logging.debug("Raw response from Traffic Ops: %s", myFiles[1].text)
- myFiles = myFiles[0]
- self.setConfigFileAPIVersion(myFiles)
+ raise ConnectionError("Failed to fetch configuration files from Traffic Ops")
- try:
- conf.serverInfo = ServerInfo(myFiles.info)
- # if there's a reverse proxy, switch urls.
- if conf.serverInfo.toRevProxyUrl and not conf.rev_proxy_disable:
- self._server_url = conf.serverInfo.toRevProxyUrl
- self._api_base_url = urljoin(self._server_url, '/api/%s' % self.VERSION).rstrip('/') + '/'
- return myFiles.configFiles
- except (KeyError, AttributeError, ValueError) as e:
- raise ConnectionError("Malformed response from Traffic Ops to update status request!") from e
+ # Server-info sanitization is now done by atstccfg
+ # try:
+ # conf.serverInfo = ServerInfo(myFiles.info)
+ # # if there's a reverse proxy, switch urls.
+ # if conf.serverInfo.toRevProxyUrl and not conf.rev_proxy_disable:
+ # self._server_url = conf.serverInfo.toRevProxyUrl
+ # self._api_base_url = urljoin(self._server_url, '/api/%s' % self.VERSION).rstrip('/') + '/'
+ # return myFiles.configFiles
+ # except (KeyError, AttributeError, ValueError) as e:
+ # raise ConnectionError("Malformed response from Traffic Ops to update status request!") from e
Review comment:
These commented lines can be removed, right?
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/utils.py
##########
@@ -96,3 +97,83 @@ def getJSONResponse(uri:str, cookies:dict = None, verify:bool = True) -> dict:
logging.debug("Response: %r\n%r", response.headers, response.content)
return response.json()
+
+def parse_multipart(raw: str) -> typing.List[typing.Tuple[str, str]]:
+ """
+ Parses a multipart/mixed-type payload and returns each contiguous chunk.
+
+ :param raw: The raw payload - without any HTTP status line.
+ :returns: A list where each element is a tuple where the first element is a chunk of the message. All headers are discarded except 'Path', which is the second element of each tuple if it was found in the chunk.
+ :raises: ValueError if the raw payload cannot be parsed as a multipart/mixed-type message.
+
+ >>> testdata = '''MIME-Version: 1.0\\r
+ ... Content-Type: multipart/mixed; boundary="test"\\r
+ ... \\r
+ ... --test\\r
+ ... Content-Type: text/plain; charset=us-ascii\\r
+ ... Path: /path/to/ats/root/directory/etc/trafficserver/fname\\r
+ ... \\r
+ ... # A fake testing file that wasn't generated at all on some date
+ ... CONFIG proxy.config.way.too.many.period.separated.words INT 1
+ ...
+ ... --test\\r
+ ... Content-Type: text/plain; charset=utf8\\r
+ ... Path: /path/to/ats/root/directory/etc/trafficserver/othername\\r
+ ... \\r
+ ... # The same header again
+ ... CONFIG proxy.config.the.same.insane.chain.of.words.again.but.the.last.one.is.different INT 0
+ ...
+ ... --test--\\r
+ ... '''
+ >>> output = parse_multipart(testdata)
+ >>> print(output[0][0])
+ # A fake testing file that wasn't generated at all on some date
+ CONFIG proxy.config.way.too.many.period.separated.words INT 1
+
+ >>> output[0][1]
+ '/path/to/ats/root/directory/etc/trafficserver/fname'
+ >>> print(output[1][0])
+ # The same header again
+ CONFIG proxy.config.the.same.insane.chain.of.words.again.but.the.last.one.is.different INT 0
+
+ >>> output[1][1]
+ '/path/to/ats/root/directory/etc/trafficserver/othername'
+ """
+ try:
+ hdr_index = raw.index("\r\n\r\n")
+ headers = {line.split(':')[0].casefold(): line.split(':')[1] for line in raw[:hdr_index].splitlines()}
+ except (IndexError, ValueError) as e:
+ raise ValueError("Invalid or corrupt multipart header") from e
+
+ ctype = headers.get("content-type")
+ if not ctype:
+ raise ValueError("Message is missing 'Content-Type' header")
+
+ try:
+ param_index = ctype.index(";")
+ params = {param.split('=')[0].strip(): param.split('=')[1].strip() for param in ctype[param_index+1:].split(';')}
+ except (IndexError, ValueError) as e:
+ raise ValueError("Invalid or corrupt 'Content-Type' header") from e
+
+ boundary = params.get("boundary", "").strip('"\'')
+ if not boundary:
+ raise ValueError("'Content-Type' header missing 'boundary' parameter")
+
+ chunks = raw.split(f"--{boundary}")[1:] #ignore prologue
Review comment:
Nit: Inline comments should start with [should start with a # and a single space](https://www.python.org/dev/peps/pep-0008/#inline-comments).
Fix formatting with the `autopep8` module:
```shell
autopep8 -ri .
```
##########
File path: traffic_ops/ort/atstccfg/config/config.go
##########
@@ -140,6 +140,9 @@ func GetCfg() (Cfg, error) {
if toPass == "" {
toPass = os.Getenv("TO_PASS")
}
+ if toPass == "" {
+ toPass = os.Getenv("TO_PASSWORD")
+ }
Review comment:
Why 2 separate environment variables for the same purpose?
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/config_files.py
##########
@@ -60,24 +60,37 @@ class ConfigFile():
contents = "" #: The full contents of the file - as configured in TO, not the on-disk contents
sanitizedContents = "" #: Will store the contents after sanitization
- def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/"):
+ def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/", *unused_args, contents: str = None, path: str = None):
"""
Constructs a :class:`ConfigFile` object from a raw API response
:param raw: A raw config file from an API response
:param toURL: The URL of a valid Traffic Ops host
:param tsroot: The absolute path to the root of an Apache Traffic Server installation
+ :param contents: Directly constructs a ConfigFile from the passed contents. Must be used with path, and causes raw to be ignored.
+ :param path: Sets the full path to the file when constructing ConfigFiles directly from contents.
:raises ValueError: if ``raw`` does not faithfully represent a configuration file
>>> a = ConfigFile({"fnameOnDisk": "test",
... "location": "/path/to",
- ... "apiURI":"/test",
- ... "scope": servers}, "http://example.com/")
+ ... "apiUri":"/test",
+ ... "scope": "servers"}, "http://example.com/")
>>> a
ConfigFile(path='/path/to/test', URI='http://example.com/test', scope='servers')
>>> a.SSLdir
- "/etc/trafficserver/ssl"
+ '/etc/trafficserver/ssl'
+ >>> ConfigFile(contents='testquest', path='/path/to/test')
+ ConfigFile(path='/path/to/test', URI=None, scope=None)
"""
+ self.SSLdir = os.path.join(tsroot, "etc", "trafficserver", "ssl")
Review comment:
My edge container is getting errors finding the cert. It is looking for `/etc/trafficserver/ssl/video_demo1_mycdn_ciab_test_cert.cer`, but in the container, the file exists at `/opt/trafficserver/etc/trafficserver/ssl/video_demo1_mycdn_ciab_test_cert.cer`. Should the path be prefixed with `tsroot`?:
```
edge_1 | [Apr 30 19:05:14.221] Server {0x7ff575ebd700} NOTE: loading SSL certificate configuration from /etc/trafficserver/ssl_multicert.config
edge_1 | [Apr 30 19:05:14.221] Server {0x7ff575ebd700} ERROR: SSL::140692222105344:error:02001002:system library:fopen:No such file or directory:bss_file.c:175:fopen('/etc/trafficserver/ssl/video_demo1_mycdn_ciab_test_cert.cer','r')
edge_1 | [Apr 30 19:05:14.221] Server {0x7ff575ebd700} ERROR: SSL::140692222105344:error:2006D080:BIO routines:BIO_new_file:no such file:bss_file.c:182
edge_1 | [Apr 30 19:05:14.221] Server {0x7ff575ebd700} ERROR: failed to load certificate chain from /etc/trafficserver/ssl/video_demo1_mycdn_ciab_test_cert.cer
edge_1 | [Apr 30 19:05:14.240] Server {0x7ff575ebd700} NOTE: failed to reload ssl_multicert.config
edge_1 | [Apr 30 19:05:14.240] Server {0x7ff575ebd700} NOTE: loading SSL certificate configuration from /etc/trafficserver/ssl_multicert.config
edge_1 | [Apr 30 19:05:14.240] Server {0x7ff575ebd700} ERROR: SSL::140692222105344:error:02001002:system library:fopen:No such file or directory:bss_file.c:175:fopen('/etc/trafficserver/ssl/video_demo1_mycdn_ciab_test_cert.cer','r')
edge_1 | [Apr 30 19:05:14.240] Server {0x7ff575ebd700} ERROR: SSL::140692222105344:error:2006D080:BIO routines:BIO_new_file:no such file:bss_file.c:182
edge_1 | [Apr 30 19:05:14.241] Server {0x7ff575ebd700} ERROR: failed to load certificate chain from /etc/trafficserver/ssl/video_demo1_mycdn_ciab_test_cert.cer
edge_1 | [Apr 30 19:05:14.253] Server {0x7ff575ebd700} NOTE: failed to reload ssl_multicert.config
```
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/to_api.py
##########
@@ -113,37 +152,21 @@ def getMyPackages(self) -> typing.List[packaging.Package]:
"""
logging.info("Fetching this server's package list from Traffic Ops")
- # Ah, read-only properties that gut functionality, my favorite.
- tmp = self.api_base_url
- self._api_base_url = urljoin(self._server_url, '/').rstrip('/') + '/'
-
- packagesPath = '/'.join(("ort", self.hostname, "packages"))
+ atstccfg_cmd = self.atstccfg_cmd + ["--get-data=packages"]
for _ in range(self.retries):
try:
- myPackages = self.get(packagesPath)
- break
- except (LoginError, OperationError, InvalidJSONError, RequestException) as e:
+ proc = subprocess.run(atstccfg_cmd, stderr=subprocess.PIPE, stdout=subprocess.PIPE)
+ logging.debug("Raw output: %s", proc.stdout.decode())
+ if proc.stderr.decode():
+ logging.error("proc.stderr.decode()")
+ if proc.returncode == 0:
+ return [Package(p) for p in json.loads(proc.stdout.decode())]
Review comment:
What is `Package()`? It looks undefined.
----------------------------------------------------------------
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 #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#issuecomment-644774743
There is a merge conflict in `traffic_ops_ort/atstccfg/toreqnew/toreqnew.go` from #2776 being merged
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418238406
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/to_api.py
##########
@@ -156,31 +179,32 @@ def getMyConfigFiles(self, conf:Configuration) -> typing.List[dict]:
:raises ConnectionError: when something goes wrong communicating with Traffic Ops
"""
logging.info("Fetching list of configuration files from Traffic Ops")
+ atstccfg_cmd = self.atstccfg_cmd
+ if conf.mode is Configuration.Modes.REVALIDATE:
+ atstccfg_cmd = self.atstccfg_cmd + ["--revalidate-only"]
for _ in range(self.retries):
try:
- # The API function decorator confuses pylint into thinking this doesn't return
- #pylint: disable=E1111
- myFiles = self.get_server_config_files(host_name=self.hostname)
- #pylint: enable=E1111
- break
- except (InvalidJSONError, LoginError, OperationError, RequestException) as e:
+ proc = subprocess.run(atstccfg_cmd, stderr=subprocess.PIPE, stdout=subprocess.PIPE)
+ logging.debug("Raw response: %s", proc.stdout.decode())
+ if proc.stderr.decode():
+ logging.error(proc.stderr.decode())
+ if proc.returncode == 0:
+ return [ConfigFile(tsroot=conf.tsroot, contents=x[0], path=x[1]) for x in utils.parse_multipart(proc.stdout.decode())]
+ except (subprocess.SubprocessError, ValueError, OSError) as e:
logging.debug("config file fetch failure: %r", e, exc_info=True, stack_info=True)
- else:
- raise ConnectionError("Failed to fetch configuration files from Traffic Ops")
- logging.debug("Raw response from Traffic Ops: %s", myFiles[1].text)
- myFiles = myFiles[0]
- self.setConfigFileAPIVersion(myFiles)
+ raise ConnectionError("Failed to fetch configuration files from Traffic Ops")
- try:
- conf.serverInfo = ServerInfo(myFiles.info)
- # if there's a reverse proxy, switch urls.
- if conf.serverInfo.toRevProxyUrl and not conf.rev_proxy_disable:
- self._server_url = conf.serverInfo.toRevProxyUrl
- self._api_base_url = urljoin(self._server_url, '/api/%s' % self.VERSION).rstrip('/') + '/'
- return myFiles.configFiles
- except (KeyError, AttributeError, ValueError) as e:
- raise ConnectionError("Malformed response from Traffic Ops to update status request!") from e
+ # Server-info sanitization is now done by atstccfg
+ # try:
+ # conf.serverInfo = ServerInfo(myFiles.info)
+ # # if there's a reverse proxy, switch urls.
+ # if conf.serverInfo.toRevProxyUrl and not conf.rev_proxy_disable:
+ # self._server_url = conf.serverInfo.toRevProxyUrl
+ # self._api_base_url = urljoin(self._server_url, '/api/%s' % self.VERSION).rstrip('/') + '/'
+ # return myFiles.configFiles
+ # except (KeyError, AttributeError, ValueError) as e:
+ # raise ConnectionError("Malformed response from Traffic Ops to update status request!") from e
Review comment:
yes, and I meant to do that
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418339041
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/config_files.py
##########
@@ -60,24 +60,37 @@ class ConfigFile():
contents = "" #: The full contents of the file - as configured in TO, not the on-disk contents
sanitizedContents = "" #: Will store the contents after sanitization
- def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/"):
+ def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/", *unused_args, contents: str = None, path: str = None):
"""
Constructs a :class:`ConfigFile` object from a raw API response
:param raw: A raw config file from an API response
:param toURL: The URL of a valid Traffic Ops host
:param tsroot: The absolute path to the root of an Apache Traffic Server installation
+ :param contents: Directly constructs a ConfigFile from the passed contents. Must be used with path, and causes raw to be ignored.
+ :param path: Sets the full path to the file when constructing ConfigFiles directly from contents.
:raises ValueError: if ``raw`` does not faithfully represent a configuration file
>>> a = ConfigFile({"fnameOnDisk": "test",
... "location": "/path/to",
- ... "apiURI":"/test",
- ... "scope": servers}, "http://example.com/")
+ ... "apiUri":"/test",
+ ... "scope": "servers"}, "http://example.com/")
>>> a
ConfigFile(path='/path/to/test', URI='http://example.com/test', scope='servers')
>>> a.SSLdir
- "/etc/trafficserver/ssl"
+ '/etc/trafficserver/ssl'
+ >>> ConfigFile(contents='testquest', path='/path/to/test')
+ ConfigFile(path='/path/to/test', URI=None, scope=None)
"""
+ self.SSLdir = os.path.join(tsroot, "etc", "trafficserver", "ssl")
Review comment:
Yeah, I opened an issue for it: https://github.com/apache/trafficcontrol/issues/4677
----------------------------------------------------------------
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 #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418704916
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
Alternatively, installing the ORT RPM helps lay the groundwork for actually running the official ORT-thing in CIAB. Since ORT is will be undergoing a redesign/rewrite in the near future, it might be best to start planning on getting it into CIAB.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418599558
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/utils.py
##########
@@ -96,3 +97,83 @@ def getJSONResponse(uri:str, cookies:dict = None, verify:bool = True) -> dict:
logging.debug("Response: %r\n%r", response.headers, response.content)
return response.json()
+
+def parse_multipart(raw: str) -> typing.List[typing.Tuple[str, str]]:
+ """
+ Parses a multipart/mixed-type payload and returns each contiguous chunk.
+
+ :param raw: The raw payload - without any HTTP status line.
+ :returns: A list where each element is a tuple where the first element is a chunk of the message. All headers are discarded except 'Path', which is the second element of each tuple if it was found in the chunk.
+ :raises: ValueError if the raw payload cannot be parsed as a multipart/mixed-type message.
+
+ >>> testdata = '''MIME-Version: 1.0\\r
+ ... Content-Type: multipart/mixed; boundary="test"\\r
+ ... \\r
+ ... --test\\r
+ ... Content-Type: text/plain; charset=us-ascii\\r
+ ... Path: /path/to/ats/root/directory/etc/trafficserver/fname\\r
+ ... \\r
+ ... # A fake testing file that wasn't generated at all on some date
+ ... CONFIG proxy.config.way.too.many.period.separated.words INT 1
+ ...
+ ... --test\\r
+ ... Content-Type: text/plain; charset=utf8\\r
+ ... Path: /path/to/ats/root/directory/etc/trafficserver/othername\\r
+ ... \\r
+ ... # The same header again
+ ... CONFIG proxy.config.the.same.insane.chain.of.words.again.but.the.last.one.is.different INT 0
+ ...
+ ... --test--\\r
+ ... '''
+ >>> output = parse_multipart(testdata)
+ >>> print(output[0][0])
+ # A fake testing file that wasn't generated at all on some date
+ CONFIG proxy.config.way.too.many.period.separated.words INT 1
+
+ >>> output[0][1]
+ '/path/to/ats/root/directory/etc/trafficserver/fname'
+ >>> print(output[1][0])
+ # The same header again
+ CONFIG proxy.config.the.same.insane.chain.of.words.again.but.the.last.one.is.different INT 0
+
+ >>> output[1][1]
+ '/path/to/ats/root/directory/etc/trafficserver/othername'
+ """
+ try:
+ hdr_index = raw.index("\r\n\r\n")
+ headers = {line.split(':')[0].casefold(): line.split(':')[1] for line in raw[:hdr_index].splitlines()}
+ except (IndexError, ValueError) as e:
+ raise ValueError("Invalid or corrupt multipart header") from e
+
+ ctype = headers.get("content-type")
+ if not ctype:
+ raise ValueError("Message is missing 'Content-Type' header")
+
+ try:
+ param_index = ctype.index(";")
+ params = {param.split('=')[0].strip(): param.split('=')[1].strip() for param in ctype[param_index+1:].split(';')}
Review comment:
I'm not too word about the performance cost of double-split, but I think if I was I'd stop doing this in a comprehension entirely. Do you think I should do that?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418288397
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
The ORT RPM not only includes `traffic_ops_ort.pl` but also requires systemd support. Which is why we weren't doing it that way in the first place.
----------------------------------------------------------------
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 #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r418752522
##########
File path: infrastructure/cdn-in-a-box/Makefile
##########
@@ -73,6 +75,8 @@ traffic_router/tomcat.rpm: ../../dist/tomcat-$(SPECIAL_SEASONING)
cp -f $? $@
traffic_stats/traffic_stats.rpm: ../../dist/traffic_stats-$(SPECIAL_SAUCE)
cp -f $? $@
+../../traffic_ops/ort/atstccfg/atstccfg: $(ORT_SOURCE)
+ go build -o $@ ../../traffic_ops/ort/atstccfg
Review comment:
Building the binary directly like this is ignoring the entire build environment that is set up via the docker build -- things like the particular version of Go used to build the binary, etc. This assumes you have `go` installed locally, which is not always the case, not to mention the version you have installed locally might not even match the "official" version used in the rpm build container. Also, if the CPU architecture from where you ran `make` differs from the CPU architecture of the container it's used in, it's not going to work in the container.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4671: Ciab atstccfg
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4671:
URL: https://github.com/apache/trafficcontrol/pull/4671#discussion_r445019417
##########
File path: infrastructure/cdn-in-a-box/ort/traffic_ops_ort/config_files.py
##########
@@ -60,24 +60,37 @@ class ConfigFile():
contents = "" #: The full contents of the file - as configured in TO, not the on-disk contents
sanitizedContents = "" #: Will store the contents after sanitization
- def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/"):
+ def __init__(self, raw:dict = None, toURL:str = "", tsroot:str = "/", *unused_args, contents: str = None, path: str = None):
"""
Constructs a :class:`ConfigFile` object from a raw API response
:param raw: A raw config file from an API response
:param toURL: The URL of a valid Traffic Ops host
:param tsroot: The absolute path to the root of an Apache Traffic Server installation
+ :param contents: Directly constructs a ConfigFile from the passed contents. Must be used with path, and causes raw to be ignored.
+ :param path: Sets the full path to the file when constructing ConfigFiles directly from contents.
:raises ValueError: if ``raw`` does not faithfully represent a configuration file
>>> a = ConfigFile({"fnameOnDisk": "test",
... "location": "/path/to",
- ... "apiURI":"/test",
- ... "scope": servers}, "http://example.com/")
+ ... "apiUri":"/test",
+ ... "scope": "servers"}, "http://example.com/")
>>> a
ConfigFile(path='/path/to/test', URI='http://example.com/test', scope='servers')
>>> a.SSLdir
- "/etc/trafficserver/ssl"
+ '/etc/trafficserver/ssl'
+ >>> ConfigFile(contents='testquest', path='/path/to/test')
+ ConfigFile(path='/path/to/test', URI=None, scope=None)
"""
+ self.SSLdir = os.path.join(tsroot, "etc", "trafficserver", "ssl")
Review comment:
Right, but try the actual DS URL: `https://video.demo1.mycdn.ciab.test`
----------------------------------------------------------------
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