You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2021/05/17 20:23:05 UTC

[GitHub] [trafficcontrol] srijeet0406 opened a new pull request #5857: CIAB: Use postgres as the default traffic vault backend

srijeet0406 opened a new pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857


   <!--
   ************ STOP!! ************
   If this Pull Request is intended to fix a security vulnerability, DO NOT submit it! Instead, contact
   the Apache Software Foundation Security Team at security@trafficcontrol.apache.org and follow the
   guidelines at https://www.apache.org/security/ regarding vulnerability disclosure.
   -->
   ## What does this PR (Pull Request) do?
   <!-- Explain the changes you made here. If this fixes an Issue, identify it by
   replacing the text in the checkbox item with the Issue number e.g.
   
   - [x] This PR fixes #9001 OR is not related to any Issue
   
   ^ This will automatically close Issue number 9001 when the Pull Request is
   merged (The '#' is important).
   
   Be sure you check the box properly, see the "The following criteria are ALL
   met by this PR" section for details.
   -->
   
   - [x] This PR is not related to any Issue <!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
   
   
   ## Which Traffic Control components are affected by this PR?
   <!-- Please delete all components from this list that are NOT affected by this
   Pull Request. Also, feel free to add the name of a tool or script that is
   affected but not on the list.
   
   Additionally, if this Pull Request does NOT affect documentation, please
   explain why documentation is not required. -->
   
   - CDN in a Box
   
   ## What is the best way to verify this PR?
   <!-- Please include here ALL the steps necessary to test your Pull Request. If
   it includes tests (and most should), outline here the steps needed to run the
   tests. If not, lay out the manual testing procedure and please explain why
   tests are unnecessary for this Pull Request. -->
   
   Spin up CIAB and make sure all the containers are up and running without errors.
   Now, exec into the `cdn-in-a-box_db` container and make sure you see all the tables in the `traffic_vault` database. 
   Create a delivery service from TP in CIAB and generate SSL keys for it. Now make sure that those keys exist in the `sslkey` table in the `traffic_vault` database. Do similar tests for URI sig keys and DNSSEC keys.
    
   ## If this is a bug fix, what versions of Traffic Control are affected?
   <!-- If this PR fixes a bug, please list here all of the affected versions - to
   the best of your knowledge. It's also pretty helpful to include a commit hash
   of where 'master' is at the time this PR is opened (if it affects master),
   because what 'master' means will change over time. For example, if this PR
   fixes a bug that's present in master (at commit hash '1df853c8'), in v4.0.0,
   and in the current 4.0.1 Release candidate (e.g. RC1), then this list would
   look like:
   
   - master (1df853c8)
   - 4.0.0
   - 4.0.1 (RC1)
   
   If you don't know what other versions might have this bug, AND don't know how
   to find the commit hash of 'master', then feel free to leave this section
   blank (or, preferably, delete it entirely).
    -->
   - master
   
   ## The following criteria are ALL met by this PR
   <!-- Check the boxes to signify that the associated statement is true. To
   "check a box", replace the space inside of the square brackets with an 'x'.
   e.g.
   
   - [ x] <- Wrong
   - [x ] <- Wrong
   - [] <- Wrong
   - [*] <- Wrong
   - [x] <- Correct!
   
   -->
   
   - [x] This PR includes tests 
   - [x] This PR does not include documentation
   - [x] This PR does not include an update to CHANGELOG.md
   - [x] This PR includes any and all required license headers
   - [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
   
   
   ## Additional Information
   <!-- If you would like to include any additional information on the PR for
   potential reviewers please put it here.
   
   Some examples of this would be:
   
   - Before and after screenshots/gifs of the Traffic Portal if it is affected
   - Links to other dependent Pull Requests
   - References to relevant context (e.g. new/updates to dependent libraries,
   mailing list records, blueprints)
   
   Feel free to leave this section blank (or, preferably, delete it entirely).
   -->
   
   <!--
   Licensed to the Apache Software Foundation (ASF) under one
   or more contributor license agreements.  See the NOTICE file
   distributed with this work for additional information
   regarding copyright ownership.  The ASF licenses this file
   to you under the Apache License, Version 2.0 (the
   "License"); you may not use this file except in compliance
   with the License.  You may obtain a copy of the License at
   
       http://www.apache.org/licenses/LICENSE-2.0
   
   Unless required by applicable law or agreed to in writing,
   software distributed under the License is distributed on an
   "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
   KIND, either express or implied.  See the License for the
   specific language governing permissions and limitations
   under the License.
   -->
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rawlinp commented on pull request #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#issuecomment-845495464


   I don't have those env vars set, but when I change the line to
   ```
   COPY infrastructure/cdn-in-a-box/traffic_ops/tv.conf conf/production/
   ```
   it builds. I'm currently working through what appeared to be expired certs in my CiaB which was causing the startup to get stuck in an infinite loop.


-- 
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] srijeet0406 commented on a change in pull request #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r633908686



##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -1004,6 +1018,14 @@ def generate_cdn_conf(questions, fname, automatic, root): # type: (list[Question
 		json.dump(existing_conf, conf_file, indent=indent)
 		print(file=conf_file)
 	logging.info("CDN configuration has been saved")
+	try:

Review comment:
       That was my thinking behind leaving it as a warning.




-- 
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 #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r635601602



##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -710,6 +721,30 @@ def unmarshal_config(dct): # type: (dict) -> dict[str, list[Question]]
 
 	return ret
 
+def write_encryption_key(aes_key_location): # type: (str) -> None
+	"""
+	Creates an AES encryption key for the postgres traffic vault backend
+
+	:param aes_key_location: Denotes the location of the aes encryption key file
+	:returns: None
+	"""
+	logging.info(aes_key_location)
+
+	cmd = ["/usr/bin/openssl", "rand", "-base64", "32"]

Review comment:
       Can this use the existing `exec_openssl()` function?

##########
File path: infrastructure/cdn-in-a-box/traffic_ops/tv.conf
##########
@@ -0,0 +1,11 @@
+

Review comment:
       Nit: First line of the file is empty




-- 
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 #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r635598199



##########
File path: infrastructure/cdn-in-a-box/traffic_ops/run-go.sh
##########
@@ -142,7 +148,12 @@ mkdir -p /var/log/traffic_ops
 touch "$TO_LOG_ERROR" "$TO_LOG_WARNING" "$TO_LOG_INFO" "$TO_LOG_DEBUG" "$TO_LOG_EVENT"
 tail -qf "$TO_LOG_ERROR" "$TO_LOG_WARNING" "$TO_LOG_INFO" "$TO_LOG_DEBUG" "$TO_LOG_EVENT" &
 
-traffic_ops_golang_command=(./bin/traffic_ops_golang -cfg "$CDNCONF" -dbcfg "$DBCONF" -riakcfg "$RIAKCONF");
+if [[ -z $TV_BACKEND ]]; then
+  traffic_ops_golang_command=(./bin/traffic_ops_golang -cfg "$CDNCONF" -dbcfg "$DBCONF" -riakcfg "$RIAKCONF");

Review comment:
       Nevermind, we chatted about this offline, and now I understand why this works the way it is. 😄 




-- 
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] srijeet0406 commented on pull request #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#issuecomment-845503637


   > That copies it to `/`, but then later in the Dockerfile it's trying to copy to `conf/production/`. Would it make sense to just copy to `conf/production/` to start? I have no idea why my build isn't working otherwise.
   Right, hence I added the second copy, because I wanted to group all the "base" copies together. But yes, I will copy it directly to `conf/production`.
   


-- 
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 #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r633857923



##########
File path: infrastructure/cdn-in-a-box/traffic_ops/run-go.sh
##########
@@ -138,11 +143,13 @@ cd /opt/traffic_ops/app;
 CDNCONF=/opt/traffic_ops/app/conf/cdn.conf
 DBCONF=/opt/traffic_ops/app/conf/production/database.conf
 RIAKCONF=/opt/traffic_ops/app/conf/production/riak.conf
+TV_DBCONF=/opt/traffic_ops/app/conf/production/tv.conf
 mkdir -p /var/log/traffic_ops
 touch "$TO_LOG_ERROR" "$TO_LOG_WARNING" "$TO_LOG_INFO" "$TO_LOG_DEBUG" "$TO_LOG_EVENT"
 tail -qf "$TO_LOG_ERROR" "$TO_LOG_WARNING" "$TO_LOG_INFO" "$TO_LOG_DEBUG" "$TO_LOG_EVENT" &
 
-traffic_ops_golang_command=(./bin/traffic_ops_golang -cfg "$CDNCONF" -dbcfg "$DBCONF" -riakcfg "$RIAKCONF");
+traffic_ops_golang_command=(./bin/traffic_ops_golang -cfg "$CDNCONF" -dbcfg "$DBCONF");
+#-riakcfg "$RIAKCONF");

Review comment:
       Can this commented line be removed?

##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -1004,6 +1018,17 @@ def generate_cdn_conf(questions, fname, automatic, root): # type: (list[Question
 		json.dump(existing_conf, conf_file, indent=indent)
 		print(file=conf_file)
 	logging.info("CDN configuration has been saved")
+	try:
+		traffic_vault_backend = existing_conf["traffic_ops_golang"]["traffic_vault_backend"]
+	except KeyError as e:
+		exception = ValueError("missing 'traffic_vault_backend' config_var")
+		exception.__cause__ = e
+		logging.warn("no traffic vault backend configured, using default postgres")
+
+	if traffic_vault_backend == "postgres":
+		return True
+	else:
+		return False

Review comment:
       This can be `return traffic_vault_backend == "postgres"`

##########
File path: infrastructure/cdn-in-a-box/traffic_ops/config.sh
##########
@@ -76,6 +81,19 @@ cdn_conf=/opt/traffic_ops/app/conf/cdn.conf
     },
     "use_ims": true,
     "traffic_ops_golang" : {
+          "traffic_vault_backend": "$TV_BACKEND",
+          "traffic_vault_config": {
+            "dbname": "$TV_DB_NAME",
+            "hostname": "$TV_DB_SERVER.$INFRA_SUBDOMAIN.$TLD_DOMAIN",
+            "user": "$TV_DB_USER",
+            "password": "$TV_DB_USER_PASS",
+            "port": ${TV_DB_PORT:-5432},
+            "ssl": false,
+            "conn_max_lifetime_seconds": 60,
+            "max_connections": 500,
+            "max_idle_connections": 30,
+            "query_timeout_seconds": 10

Review comment:
       `"query_timeout_seconds"` should be set to `${DEBUGGING_TIMEOUT:-60}` so debugging sessions don't time out.

##########
File path: infrastructure/cdn-in-a-box/traffic_ops/config.sh
##########
@@ -76,6 +81,19 @@ cdn_conf=/opt/traffic_ops/app/conf/cdn.conf
     },
     "use_ims": true,
     "traffic_ops_golang" : {
+          "traffic_vault_backend": "$TV_BACKEND",
+          "traffic_vault_config": {
+            "dbname": "$TV_DB_NAME",
+            "hostname": "$TV_DB_SERVER.$INFRA_SUBDOMAIN.$TLD_DOMAIN",
+            "user": "$TV_DB_USER",
+            "password": "$TV_DB_USER_PASS",
+            "port": ${TV_DB_PORT:-5432},
+            "ssl": false,

Review comment:
       ```json
   "ssl": false,
   ```
   is already in `cdn.conf`, no need to specify here

##########
File path: infrastructure/cdn-in-a-box/traffic_ops/config.sh
##########
@@ -32,9 +32,14 @@
 # TO_HOST
 # TO_PORT
 # TP_HOST
+# TV_DB_USER
+# TV_DB_NAME
+# TV_DB_SERVER
+# TV_DB_USER_PASS
+# TV_DB_PORT

Review comment:
       Can these be alphabetized? Some of the rest of the list is also unsorted, might as well sort it all.

##########
File path: infrastructure/cdn-in-a-box/traffic_ops/config.sh
##########
@@ -76,6 +81,19 @@ cdn_conf=/opt/traffic_ops/app/conf/cdn.conf
     },
     "use_ims": true,
     "traffic_ops_golang" : {
+          "traffic_vault_backend": "$TV_BACKEND",
+          "traffic_vault_config": {
+            "dbname": "$TV_DB_NAME",
+            "hostname": "$TV_DB_SERVER.$INFRA_SUBDOMAIN.$TLD_DOMAIN",
+            "user": "$TV_DB_USER",
+            "password": "$TV_DB_USER_PASS",
+            "port": ${TV_DB_PORT:-5432},
+            "ssl": false,
+            "conn_max_lifetime_seconds": 60,

Review comment:
       `"conn_max_lifetime_seconds"` should be set to `${DEBUGGING_TIMEOUT:-60}` so debugging sessions don't time out.

##########
File path: infrastructure/cdn-in-a-box/traffic_ops/run-go.sh
##########
@@ -30,16 +30,21 @@
 # ADMIN_USER
 # ADMIN_PASS
 # DOMAIN
+# TV_DB_USER
+# TV_DB_NAME
+# TV_BACKEND
+# TV_DB_SERVER
+# TV_DB_USER_PASS
+# TV_DB_PORT

Review comment:
       Same here, environment variables should be sorted.

##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -1004,6 +1018,17 @@ def generate_cdn_conf(questions, fname, automatic, root): # type: (list[Question
 		json.dump(existing_conf, conf_file, indent=indent)
 		print(file=conf_file)
 	logging.info("CDN configuration has been saved")
+	try:
+		traffic_vault_backend = existing_conf["traffic_ops_golang"]["traffic_vault_backend"]
+	except KeyError as e:
+		exception = ValueError("missing 'traffic_vault_backend' config_var")
+		exception.__cause__ = e
+		logging.warn("no traffic vault backend configured, using default postgres")

Review comment:
       The `exception` built here should be raised, the warning is not sufficient.




-- 
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 #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r634533695



##########
File path: infrastructure/cdn-in-a-box/traffic_ops/Dockerfile
##########
@@ -120,6 +120,9 @@ COPY infrastructure/cdn-in-a-box/enroller/server_template.json \
 	infrastructure/cdn-in-a-box/traffic_ops/tv.conf \
 	/
 
+# Generate the AES encryption key for traffic vault backend
+RUN echo $(openssl rand -base64 32) > aes.conf

Review comment:
       We should avoid storing any secrets in the built Docker image. This should be run from a script at runtime.




-- 
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 merged pull request #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
rawlinp merged pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857


   


-- 
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] srijeet0406 commented on a change in pull request #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r633910224



##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -1004,6 +1018,14 @@ def generate_cdn_conf(questions, fname, automatic, root): # type: (list[Question
 		json.dump(existing_conf, conf_file, indent=indent)
 		print(file=conf_file)
 	logging.info("CDN configuration has been saved")
+	try:

Review comment:
       I have set it back to a warning




-- 
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 #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r635588300



##########
File path: infrastructure/cdn-in-a-box/traffic_ops/run-go.sh
##########
@@ -142,7 +148,12 @@ mkdir -p /var/log/traffic_ops
 touch "$TO_LOG_ERROR" "$TO_LOG_WARNING" "$TO_LOG_INFO" "$TO_LOG_DEBUG" "$TO_LOG_EVENT"
 tail -qf "$TO_LOG_ERROR" "$TO_LOG_WARNING" "$TO_LOG_INFO" "$TO_LOG_DEBUG" "$TO_LOG_EVENT" &
 
-traffic_ops_golang_command=(./bin/traffic_ops_golang -cfg "$CDNCONF" -dbcfg "$DBCONF" -riakcfg "$RIAKCONF");
+if [[ -z $TV_BACKEND ]]; then
+  traffic_ops_golang_command=(./bin/traffic_ops_golang -cfg "$CDNCONF" -dbcfg "$DBCONF" -riakcfg "$RIAKCONF");

Review comment:
       so if both `-riakcfg` and `traffic_vault_backend` (in cdn.conf) are used, I believe `traffic_vault_backend` will take precedence. So we would also have to exclude the `traffic_vault_backend`-related config in cdn.conf in order to use this line to enable Riak. Or, we can actually specify the riak config in `traffic_vault_backend` in cdn.conf (and omit the `riakcfg`) if we would like to use Riak instead of postgres.




-- 
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 #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r635526871



##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -710,6 +721,30 @@ def unmarshal_config(dct): # type: (dict) -> dict[str, list[Question]]
 
 	return ret
 
+def write_encryption_key(aes_key_location): # type: (str) -> None
+	"""
+	Creates an AES encryption key for the postgres traffic vault backend
+
+	:param aes_key_location: Denotes the location of the aes encryption key file
+	:returns: None
+	"""
+	logging.info(aes_key_location)
+
+	cmd = ["/usr/bin/openssl", "rand", "-base64", "32"]
+	key = open(aes_key_location, 'w')

Review comment:
       Python2 supports `with`




-- 
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] srijeet0406 commented on pull request #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#issuecomment-845495794


   I just set those variables and rebuilt and it still builds and works fine for me.


-- 
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 #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#issuecomment-845496271


   Ahh sounds like I was off, glad you have it sorted out.


-- 
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 #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r636407224



##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -1004,6 +1039,19 @@ def generate_cdn_conf(questions, fname, automatic, root): # type: (list[Question
 		json.dump(existing_conf, conf_file, indent=indent)
 		print(file=conf_file)
 	logging.info("CDN configuration has been saved")
+	try:
+		traffic_vault_backend = existing_conf["traffic_ops_golang"]["traffic_vault_backend"]
+	except KeyError as e:
+		logging.warn("no traffic vault backend configured, using default postgres")

Review comment:
       I think this is supposed to be `logging.warning`

##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -710,6 +721,27 @@ def unmarshal_config(dct): # type: (dict) -> dict[str, list[Question]]
 
 	return ret
 
+def write_encryption_key(aes_key_location): # type: (str) -> None
+	"""
+	Creates an AES encryption key for the postgres traffic vault backend
+
+	:param aes_key_location: Denotes the location of the aes encryption key file
+	:returns: None
+	"""
+	logging.info(aes_key_location)
+
+	cmd = ["/usr/bin/openssl", "rand", "-base64", "32"]

Review comment:
       this `cmd` is no longer used




-- 
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 #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r635579991



##########
File path: traffic_ops/app/conf/cdn.conf
##########
@@ -34,8 +34,6 @@
         },
         "whitelisted_oauth_urls": [],
         "oauth_client_secret": "",
-        "traffic_vault_backend": "",

Review comment:
       I think we may want to keep these in here just to indicate that this is where traffic_vault is configured, but to keep them empty indicating that it's optional.




-- 
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 #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r635300236



##########
File path: traffic_ops/app/conf/cdn.conf
##########
@@ -10,6 +10,20 @@
         "workers" : 12
     },
     "traffic_ops_golang" : {
+        "traffic_vault_backend": "",

Review comment:
       you might've miscommitted these changes again

##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -1004,6 +1041,23 @@ def generate_cdn_conf(questions, fname, automatic, root): # type: (list[Question
 		json.dump(existing_conf, conf_file, indent=indent)
 		print(file=conf_file)
 	logging.info("CDN configuration has been saved")
+	try:
+		traffic_vault_backend = existing_conf["traffic_ops_golang"]["traffic_vault_backend"]
+	except KeyError as e:
+		exception = ValueError("missing 'traffic_vault_backend' config_var")
+		exception.__cause__ = e
+		logging.warn("no traffic vault backend configured, using default postgres")
+
+	if traffic_vault_backend == "postgres":
+		try:
+			traffic_vault_aes_encryption_location = existing_conf["traffic_ops_golang"]["traffic_vault_config"]["aes_key_location"]
+			write_encryption_key(traffic_vault_aes_encryption_location)
+		except KeyError as e:
+			exception = ValueError("missing 'aes_key_location' config_var")
+			exception.__cause__ = e

Review comment:
       since the exception isn't raised, these lines are unnecessary

##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -1004,6 +1041,23 @@ def generate_cdn_conf(questions, fname, automatic, root): # type: (list[Question
 		json.dump(existing_conf, conf_file, indent=indent)
 		print(file=conf_file)
 	logging.info("CDN configuration has been saved")
+	try:
+		traffic_vault_backend = existing_conf["traffic_ops_golang"]["traffic_vault_backend"]
+	except KeyError as e:
+		exception = ValueError("missing 'traffic_vault_backend' config_var")
+		exception.__cause__ = e

Review comment:
       since the exception isn't raised, these lines are unnecessary

##########
File path: infrastructure/cdn-in-a-box/traffic_ops/run-go.sh
##########
@@ -142,7 +148,7 @@ mkdir -p /var/log/traffic_ops
 touch "$TO_LOG_ERROR" "$TO_LOG_WARNING" "$TO_LOG_INFO" "$TO_LOG_DEBUG" "$TO_LOG_EVENT"
 tail -qf "$TO_LOG_ERROR" "$TO_LOG_WARNING" "$TO_LOG_INFO" "$TO_LOG_DEBUG" "$TO_LOG_EVENT" &
 
-traffic_ops_golang_command=(./bin/traffic_ops_golang -cfg "$CDNCONF" -dbcfg "$DBCONF" -riakcfg "$RIAKCONF");
+traffic_ops_golang_command=(./bin/traffic_ops_golang -cfg "$CDNCONF" -dbcfg "$DBCONF");

Review comment:
       If we wanted to run CiaB using Riak as the TV backend, how would we 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] rawlinp commented on pull request #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#issuecomment-845464819


   I'm getting this error when trying to build the trafficops container in CiaB:
   ```
   Step 23/24 : COPY tv.conf conf/production/
   COPY failed: stat /var/lib/docker/tmp/docker-builder422447319/tv.conf: no such file or directory
   ERROR: Service 'trafficops' failed to build : Build failed
   ```


-- 
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] srijeet0406 commented on pull request #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#issuecomment-845496292


   > I don't have those env vars set, but when I change the line to
   > 
   > ```
   > COPY infrastructure/cdn-in-a-box/traffic_ops/tv.conf conf/production/
   > ```
   > 
   > it builds. I'm currently working through what appeared to be expired certs in my CiaB which was causing the startup to get stuck in an infinite loop.
   
   I thought I took care of that in the upper `COPY` block:
   
   ```
   COPY infrastructure/cdn-in-a-box/enroller/server_template.json \
   	infrastructure/cdn-in-a-box/traffic_ops/config.sh \
   	infrastructure/cdn-in-a-box/traffic_ops/run-go.sh \
   	infrastructure/cdn-in-a-box/traffic_ops/to-access.sh \
   	infrastructure/cdn-in-a-box/dns/insert-self-into-dns.sh \
   	infrastructure/cdn-in-a-box/dns/set-dns.sh \
   	infrastructure/cdn-in-a-box/traffic_ops/set-to-ips-from-dns.sh \
   	infrastructure/cdn-in-a-box/traffic_ops/generate-certs.sh \
   	infrastructure/cdn-in-a-box/traffic_ops/adduser.pl \
   	infrastructure/cdn-in-a-box/traffic_ops/trafficops-init.sh \
   	infrastructure/cdn-in-a-box/variables.env \
   	infrastructure/cdn-in-a-box/traffic_ops/tv.conf \
   	/
   ```


-- 
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 #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r633891448



##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -229,10 +231,22 @@ DEFAULTS = {
 		Question("Traffic Ops database user", "traffic_ops", "user"),
 		Question("Password for Traffic Ops database user", "", "password", hidden=True)
 	],
+	TV_DATABASE_CONF_FILE: [

Review comment:
       this is confusing to me since these options actually live in `cdn.conf` -- shouldn't the questions be for that instead?

##########
File path: traffic_ops/app/conf/production/tv.conf
##########
@@ -0,0 +1,11 @@
+

Review comment:
       if this file is just for CiaB configuration we should probably keep it in the `cdn-in-a-box` directory

##########
File path: traffic_ops/app/conf/cdn.conf
##########
@@ -10,6 +10,19 @@
         "workers" : 12
     },
     "traffic_ops_golang" : {
+        "traffic_vault_backend": "postgres",

Review comment:
       Did you mean to commit these config changes? The default config should probably leave TV not configured since it's an optional component. 

##########
File path: infrastructure/cdn-in-a-box/traffic_ops/run-go.sh
##########
@@ -138,11 +143,12 @@ cd /opt/traffic_ops/app;
 CDNCONF=/opt/traffic_ops/app/conf/cdn.conf
 DBCONF=/opt/traffic_ops/app/conf/production/database.conf
 RIAKCONF=/opt/traffic_ops/app/conf/production/riak.conf
+TV_DBCONF=/opt/traffic_ops/app/conf/production/tv.conf

Review comment:
       this doesn't seem to be used anywhere

##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -1004,6 +1018,14 @@ def generate_cdn_conf(questions, fname, automatic, root): # type: (list[Question
 		json.dump(existing_conf, conf_file, indent=indent)
 		print(file=conf_file)
 	logging.info("CDN configuration has been saved")
+	try:

Review comment:
       a better way to do this would be:
   ```
   return existing_conf["traffic_ops_golang"].get("traffic_vault_backend") == "postgres"
   ```
   but since `traffic_vault_backend` is not a required field, should we actually be raising an exception 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] zrhoffman commented on pull request #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#issuecomment-845491231


   > > I'm getting this error when trying to build the trafficops container in CiaB:
   > > ```
   > > Step 23/24 : COPY tv.conf conf/production/
   > > COPY failed: stat /var/lib/docker/tmp/docker-builder422447319/tv.conf: no such file or directory
   > > ERROR: Service 'trafficops' failed to build : Build failed
   > > ```
   > 
   > Thats weird. I did just a clean build and everything works fine for me.
   > 
   > ```
   > docker-compose down --remove-orphans
   > docker volume prune
   > docker system prune
   > make very-clean
   > make all && docker-compose build --parallel && docker-compose -f docker-compose.yml -f docker-compose.expose-ports.yml up -d
   > ```
   
   Sounds like @rawlinp has `DOCKER_BUILDKIT=1` and `COMPOSE_DOCKER_CLI_BUILD=1` in his environment and you do not. Set those environment variables to reproduce and add the file as an exception to `infrastructure/cdn-in-a-box/traffic_ops/Dockerfile.dockerignore` to 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] srijeet0406 commented on a change in pull request #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r635338653



##########
File path: traffic_ops/app/conf/cdn.conf
##########
@@ -10,6 +10,20 @@
         "workers" : 12
     },
     "traffic_ops_golang" : {
+        "traffic_vault_backend": "",

Review comment:
       I think I might've misunderstood your previous comment. I thought you were referring to the values being committed, and not the keys. I get it now, will remove.




-- 
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] srijeet0406 commented on pull request #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#issuecomment-843453057


   > `infrastructure/cdn-in-a-box/traffic_ops/aes.conf` is an empty file -- did you mean to commit that?
   > 
   > Also, `.conf` makes it seem like a config file, but in reality it is a key file, so it might make more sense to use the `.key` suffix.
   
   I keep doing that, fixed now. Also, changed the suffix of the file.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] srijeet0406 commented on pull request #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#issuecomment-842715995


   > `traffic_ops_golang` is not running in the `trafficops` container. From the logs (downloadable as a GHA artifact):
   > 
   > ```go
   > trafficops_1      | 2021-05-17T20:33:47.807659531Z ERROR: traffic_ops_golang.go:286: 2021-05-17T20:33:46.928632269Z: failed to get Traffic Vault backend 'postgres': failed to load backend 'postgres': validating Postgres config: 'aes_key_location' cannot be blank
   > ```
   
   Yeah I think I'll need to add the config changes for Matt's AES 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 pull request #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#issuecomment-845498405


   That copies it to `/`, but then later in the Dockerfile it's trying to copy to `conf/production/`. Would it make sense to just copy to `conf/production/` to start? I have no idea why my build isn't working otherwise.


-- 
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] srijeet0406 commented on a change in pull request #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r633910080



##########
File path: traffic_ops/app/conf/cdn.conf
##########
@@ -10,6 +10,19 @@
         "workers" : 12
     },
     "traffic_ops_golang" : {
+        "traffic_vault_backend": "postgres",

Review comment:
       Nope, didn't want to commit that. removed now.




-- 
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] srijeet0406 commented on pull request #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#issuecomment-845489796


   > I'm getting this error when trying to build the trafficops container in CiaB:
   > 
   > ```
   > Step 23/24 : COPY tv.conf conf/production/
   > COPY failed: stat /var/lib/docker/tmp/docker-builder422447319/tv.conf: no such file or directory
   > ERROR: Service 'trafficops' failed to build : Build failed
   > ```
   
   Thats weird. I did just a clean build and everything works fine for me. 
   ```
   docker-compose down --remove-orphans
   docker volume prune
   docker system prune
   make very-clean
   make all && docker-compose build --parallel && docker-compose -f docker-compose.yml -f docker-compose.expose-ports.yml up -d
   ```


-- 
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 #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r634060422



##########
File path: infrastructure/cdn-in-a-box/traffic_ops/aes.conf
##########
@@ -0,0 +1 @@
+CZhbsfrdNqAC9tfZJ/x4tB/NgoXhbfLdFwauhhNedjE=

Review comment:
       CDN in a Box should generate this. If you want, it can be persisted in a bind mount like the CA directory is:
   
   https://github.com/apache/trafficcontrol/blob/c3878a2a997d48e106ea44bd72e34aa39598e2b9/infrastructure/cdn-in-a-box/docker-compose.yml#L73-L75
   
   But if it does not exist, CiaB should generate it, and no generated key should be tracked in the repo.




-- 
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] srijeet0406 commented on pull request #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#issuecomment-845490819


   I feel like the CI checks would have freaked out too, had the container building failed. 


-- 
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 #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r635626517



##########
File path: traffic_ops/app/conf/cdn.conf
##########
@@ -93,4 +93,4 @@
             "hmac_encoded" : ""
         }
     ]
-}
+}

Review comment:
       Missing newline at EOF




-- 
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 #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r635490536



##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -710,6 +721,30 @@ def unmarshal_config(dct): # type: (dict) -> dict[str, list[Question]]
 
 	return ret
 
+def write_encryption_key(aes_key_location): # type: (str) -> None
+	"""
+	Creates an AES encryption key for the postgres traffic vault backend
+
+	:param aes_key_location: Denotes the location of the aes encryption key file
+	:returns: None
+	"""
+	logging.info(aes_key_location)
+
+	cmd = ["/usr/bin/openssl", "rand", "-base64", "32"]
+	key = open(aes_key_location, 'w')

Review comment:
       File descriptor leak happens here if any of the following lines throw exceptions e.g. if `/usr/bin/openssl` does not exist or is not executable. This should either use context management like:
   ```python3
   with open(aes_key_location, "w") as key:
       # do stuff
   ```
   (not sure if ~~the dead version of Python nobody should ever use~~ Python2 supports that) or a `try`...`finally` like:
   ```python3
   key = open(aes_key_location, "w")
   try:
       # do stuff
   finally:
       key.close()
   ```

##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -905,7 +940,7 @@ def random_word(length = 12): # type: (int) -> str
 	word_chars = string.ascii_letters + string.digits + '_'
 	return ''.join(random.choice(word_chars) for _ in range(length))
 
-def generate_cdn_conf(questions, fname, automatic, root): # type: (list[Question], str, bool, str) -> None
+def generate_cdn_conf(questions, fname, automatic, root): # type: (list[Question], str, bool, str) -> bool
 	"""
 	Generates some properties of a cdn.conf file based on the passed questions.
 

Review comment:
       This doc string should specify what the function `:returns:`, now that it does.




-- 
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 #5857: CIAB: Use postgres as the default traffic vault backend

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5857:
URL: https://github.com/apache/trafficcontrol/pull/5857#discussion_r633918291



##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -1004,6 +1018,14 @@ def generate_cdn_conf(questions, fname, automatic, root): # type: (list[Question
 		json.dump(existing_conf, conf_file, indent=indent)
 		print(file=conf_file)
 	logging.info("CDN configuration has been saved")
+	try:

Review comment:
       > but since `traffic_vault_backend` is not a required field, should we actually be raising an exception in this case?
   
   Good point, warning makes sense.




-- 
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