ocket8888 commented on a change in pull request #5681:
URL: https://github.com/apache/trafficcontrol/pull/5681#discussion_r602397999
##########
File path: infrastructure/cdn-in-a-box/traffic_ops/Dockerfile
##########
@@ -35,56 +35,72 @@ RUN set -o nounset -o errexit && \
if [[ "${RHEL_VERSION%%.*}" -eq 7 ]]; then \
use_repo=''; \
enable_repo=''; \
+ llvm_version=5.0; \
# needed for llvm-toolset-7-clang, which is needed for
postgresql13-devel-13.2-1PGDG, required by TO rpm
dnf -y install gcc centos-release-scl-rh; \
else \
use_repo='--repo=pgdg13'; \
enable_repo='--enablerepo=powertools'; \
+ llvm_version=''; \
fi; \
dnf -y install
"https://download.postgresql.org/pub/repos/yum/reporpms/EL-${RHEL_VERSION%%.*}-x86_64/pgdg-redhat-repo-latest.noarch.rpm";;
\
- # libicu required by postgresql13
- dnf -y install libicu; \
- dnf -y $use_repo -- install postgresql13; \
dnf -y install epel-release; \
- dnf -y $enable_repo install \
+ dnf -y install \
+ # libicu is required by postgresql13
+ libicu \
+ # libicu-devel, clang-devel, and llvm-devel are required by
postgresql13-devel
Review comment:
but why do we need postgresql13-devel?
##########
File path: infrastructure/cdn-in-a-box/traffic_ops/Dockerfile
##########
@@ -99,6 +115,8 @@ COPY
infrastructure/cdn-in-a-box/enroller/server_template.json \
infrastructure/cdn-in-a-box/traffic_ops/trafficops-init.sh \
infrastructure/cdn-in-a-box/variables.env \
/
+COPY infrastructure/cdn-in-a-box/traffic_ops_data /traffic_ops_data
+COPY traffic_router/core/src/test/resources/geo/GeoLite2-City.mmdb.gz
/opt/traffic_ops/app/public/
Review comment:
Why move these steps? I think these change much, much less often than
the RPM you want to use for CiaB, so putting them before the RPM addition and
installation is more effective for caching build layers.
##########
File path: infrastructure/cdn-in-a-box/traffic_ops/config.sh
##########
@@ -100,108 +92,103 @@ cat <<-EOF >/opt/traffic_ops/app/conf/cdn.conf
"log_location_info": "$TO_LOG_INFO",
"log_location_debug": "$TO_LOG_DEBUG",
"log_location_event": "$TO_LOG_EVENT",
- "max_db_connections": 20,
"db_conn_max_lifetime_seconds": ${DEBUGGING_TIMEOUT:-60},
- "db_query_timeout_seconds": ${DEBUGGING_TIMEOUT:-20},
- "backend_max_connections": {
- "mojolicious": 4
- },
- "whitelisted_oauth_urls": [],
- "oauth_client_secret": "",
- "routing_blacklist": {
- "ignore_unknown_routes": false,
- "disabled_routes": []
- },
- "supported_ds_metrics": [ "kbps", "tps_total", "tps_2xx", "tps_3xx",
"tps_4xx", "tps_5xx" ]
Review comment:
Why get rid of all this? Particularly, without `supported_ds_metrics`,
the Traffic Stats endpoints in the TO API for Delivery Service-based stats
won't be able to return any data. I don't think `postinstall` adds quite all of
these configuration options.
##########
File path: traffic_ops/install/bin/_postinstall
##########
@@ -1,5 +1,5 @@
-#!/usr/bin/perl
-
+#!/usr/bin/env bash
Review comment:
Did you change this Python code at all? I can't tell, the diff is so
massive. I think if you use `git mv` to rename the files it'll make the diff
cleaner, but I'm not sure since there was already a file with this name...
##########
File path: infrastructure/cdn-in-a-box/traffic_ops/config.sh
##########
@@ -100,108 +92,103 @@ cat <<-EOF >/opt/traffic_ops/app/conf/cdn.conf
"log_location_info": "$TO_LOG_INFO",
"log_location_debug": "$TO_LOG_DEBUG",
"log_location_event": "$TO_LOG_EVENT",
- "max_db_connections": 20,
"db_conn_max_lifetime_seconds": ${DEBUGGING_TIMEOUT:-60},
- "db_query_timeout_seconds": ${DEBUGGING_TIMEOUT:-20},
- "backend_max_connections": {
- "mojolicious": 4
- },
- "whitelisted_oauth_urls": [],
- "oauth_client_secret": "",
- "routing_blacklist": {
- "ignore_unknown_routes": false,
- "disabled_routes": []
- },
- "supported_ds_metrics": [ "kbps", "tps_total", "tps_2xx", "tps_3xx",
"tps_4xx", "tps_5xx" ]
- },
- "cors" : {
- "access_control_allow_origin" : "*"
+ "db_query_timeout_seconds": ${DEBUGGING_TIMEOUT:-20}
},
"to" : {
- "base_url" : "https://$TO_FQDN";,
- "email_from" : "no-reply@$INFRA_SUBDOMAIN.$TLD_DOMAIN",
- "no_account_found_msg" : "A Traffic Ops user account is required for
access. Please contact your Traffic Ops user administrator."
Review comment:
Why get rid of these two things? Particularly, omitting `base_url` will
cause a segfault in the password reset endpoint's handler. I don't believe
`postinstall` will add that.
##########
File path: infrastructure/cdn-in-a-box/traffic_ops/config.sh
##########
@@ -100,108 +92,103 @@ cat <<-EOF >/opt/traffic_ops/app/conf/cdn.conf
"log_location_info": "$TO_LOG_INFO",
"log_location_debug": "$TO_LOG_DEBUG",
"log_location_event": "$TO_LOG_EVENT",
- "max_db_connections": 20,
"db_conn_max_lifetime_seconds": ${DEBUGGING_TIMEOUT:-60},
- "db_query_timeout_seconds": ${DEBUGGING_TIMEOUT:-20},
- "backend_max_connections": {
- "mojolicious": 4
- },
- "whitelisted_oauth_urls": [],
- "oauth_client_secret": "",
- "routing_blacklist": {
- "ignore_unknown_routes": false,
- "disabled_routes": []
- },
- "supported_ds_metrics": [ "kbps", "tps_total", "tps_2xx", "tps_3xx",
"tps_4xx", "tps_5xx" ]
- },
- "cors" : {
- "access_control_allow_origin" : "*"
+ "db_query_timeout_seconds": ${DEBUGGING_TIMEOUT:-20}
},
"to" : {
- "base_url" : "https://$TO_FQDN";,
- "email_from" : "no-reply@$INFRA_SUBDOMAIN.$TLD_DOMAIN",
- "no_account_found_msg" : "A Traffic Ops user account is required for
access. Please contact your Traffic Ops user administrator."
+ "email_from" : "no-reply@$INFRA_SUBDOMAIN.$TLD_DOMAIN"
},
"portal" : {
"base_url" : "https://$TP_HOST.$INFRA_SUBDOMAIN.$TLD_DOMAIN/#!/";,
- "email_from" : "no-reply@$INFRA_SUBDOMAIN.$TLD_DOMAIN",
- "pass_reset_path" : "user",
- "user_register_path" : "user"
- },
- "secrets" : [
- "$TO_SECRET"
- ],
- "geniso" : {
- "iso_root_path" : "/opt/traffic_ops/app/public"
+ "email_from" : "no-reply@$INFRA_SUBDOMAIN.$TLD_DOMAIN"
},
- "inactivity_timeout" : 60,
"smtp" : {
"enabled" : true,
- "user" : "",
- "password" : "",
"address" : "${SMTP_FQDN}:${SMTP_PORT}"
},
"InfluxEnabled": true,
"influxdb_conf_path": "/opt/traffic_ops/app/conf/production/influx.conf",
"lets_encrypt" : {
- "user_email" : "",
- "send_expiration_email": false,
- "convert_self_signed": false,
- "renew_days_before_expiration": 30,
"environment": "staging"
- },
- "acme_renewal": {
- "summary_email": "",
- "renew_days_before_expiration": 30
- },
- "acme_accounts": [
- {
- "acme_provider" : "",
- "user_email" : "",
- "acme_url" : "",
- "kid" : "",
- "hmac_encoded" : ""
- }
- ]
Review comment:
I don't think `postinstall` will add LE/ACME things.
##########
File path: infrastructure/cdn-in-a-box/traffic_ops/config.sh
##########
@@ -100,108 +92,103 @@ cat <<-EOF >/opt/traffic_ops/app/conf/cdn.conf
"log_location_info": "$TO_LOG_INFO",
"log_location_debug": "$TO_LOG_DEBUG",
"log_location_event": "$TO_LOG_EVENT",
- "max_db_connections": 20,
"db_conn_max_lifetime_seconds": ${DEBUGGING_TIMEOUT:-60},
- "db_query_timeout_seconds": ${DEBUGGING_TIMEOUT:-20},
- "backend_max_connections": {
- "mojolicious": 4
- },
- "whitelisted_oauth_urls": [],
- "oauth_client_secret": "",
- "routing_blacklist": {
- "ignore_unknown_routes": false,
- "disabled_routes": []
- },
- "supported_ds_metrics": [ "kbps", "tps_total", "tps_2xx", "tps_3xx",
"tps_4xx", "tps_5xx" ]
- },
- "cors" : {
- "access_control_allow_origin" : "*"
+ "db_query_timeout_seconds": ${DEBUGGING_TIMEOUT:-20}
},
"to" : {
- "base_url" : "https://$TO_FQDN";,
- "email_from" : "no-reply@$INFRA_SUBDOMAIN.$TLD_DOMAIN",
- "no_account_found_msg" : "A Traffic Ops user account is required for
access. Please contact your Traffic Ops user administrator."
+ "email_from" : "no-reply@$INFRA_SUBDOMAIN.$TLD_DOMAIN"
},
"portal" : {
"base_url" : "https://$TP_HOST.$INFRA_SUBDOMAIN.$TLD_DOMAIN/#!/";,
- "email_from" : "no-reply@$INFRA_SUBDOMAIN.$TLD_DOMAIN",
- "pass_reset_path" : "user",
- "user_register_path" : "user"
- },
- "secrets" : [
- "$TO_SECRET"
- ],
- "geniso" : {
- "iso_root_path" : "/opt/traffic_ops/app/public"
+ "email_from" : "no-reply@$INFRA_SUBDOMAIN.$TLD_DOMAIN"
},
- "inactivity_timeout" : 60,
"smtp" : {
"enabled" : true,
- "user" : "",
- "password" : "",
"address" : "${SMTP_FQDN}:${SMTP_PORT}"
},
"InfluxEnabled": true,
"influxdb_conf_path": "/opt/traffic_ops/app/conf/production/influx.conf",
"lets_encrypt" : {
- "user_email" : "",
- "send_expiration_email": false,
- "convert_self_signed": false,
- "renew_days_before_expiration": 30,
"environment": "staging"
- },
- "acme_renewal": {
- "summary_email": "",
- "renew_days_before_expiration": 30
- },
- "acme_accounts": [
- {
- "acme_provider" : "",
- "user_email" : "",
- "acme_url" : "",
- "kid" : "",
- "hmac_encoded" : ""
- }
- ]
+ }
}
EOF
+))"
-cat <<-EOF >/opt/traffic_ops/app/conf/production/database.conf
+<<RIAK_CONF cat >/opt/traffic_ops/app/conf/production/riak.conf
{
- "description": "Local PostgreSQL database on port 5432",
- "dbname": "$DB_NAME",
- "hostname": "$DB_FQDN",
- "user": "$DB_USER",
- "password": "$DB_USER_PASS",
- "port": "$DB_PORT",
- "ssl": false,
- "type": "Pg"
-}
-EOF
-
-cat <<-EOF >/opt/traffic_ops/app/db/dbconf.yml
-version: "1.0"
-name: dbconf.yml
-
-production:
- driver: postgres
- open: host=$DB_FQDN port=$DB_PORT user=$DB_USER password=$DB_USER_PASS
dbname=$DB_NAME sslmode=disable
-test:
- driver: postgres
- open: host=$DB_FQDN port=$DB_PORT user=$DB_USER password=$DB_USER_PASS
dbname=to_test sslmode=disable
-EOF
-
-cat <<-EOF >/opt/traffic_ops/app/conf/production/riak.conf
-{ "user": "$TV_RIAK_USER",
+ "MaxTLSVersion": "1.1",
"password": "$TV_RIAK_PASSWORD",
- "MaxTLSVersion": "1.1"
+ "user": "$TV_RIAK_USER"
}
-EOF
+RIAK_CONF
-cat <<-EOF >/opt/traffic_ops/app/conf/production/influx.conf
+<<INFLUX_CONF cat >/opt/traffic_ops/app/conf/production/influx.conf
{
- "user": "$INFLUXDB_ADMIN_USER",
- "password": "$INFLUXDB_ADMIN_PASSWORD",
- "secure": false
+ "password": "$INFLUXDB_ADMIN_PASSWORD",
+ "secure": false,
+ "user": "$INFLUXDB_ADMIN_USER"
}
-EOF
+INFLUX_CONF
+
+install_bin=/opt/traffic_ops/install/bin
+input_json="${install_bin}/input.json"
+echo "$(jq "$(<<JQ_FILTER cat
Review comment:
I feel like `envsubst` would be a lot easier to follow; probably using a
new JSON input file specifically for CiaB.
##########
File path: traffic_ops/install/bin/input.json
##########
@@ -1,179 +1,151 @@
{
- "/opt/traffic_ops/app/conf/production/database.conf":[
+ "/opt/traffic_ops/app/conf/cdn.conf": [
{
- "Database type":"Pg",
- "config_var":"type"
+ "Generate a new secret?": "yes",
+ "config_var": "genSecret"
},
{
- "Database name":"traffic_ops_db",
- "config_var":"dbname"
+ "Number of secrets to keep?": "10",
+ "config_var": "keepSecrets"
},
{
- "Database server hostname IP or FQDN":"localhost",
- "config_var":"hostname"
+ "Number of workers?": "12",
+ "config_var": "workers"
},
{
- "Database port number":"5432",
- "config_var":"port"
- },
- {
- "Traffic Ops database user":"traffic_ops",
- "config_var":"user"
- },
- {
- "Traffic Ops database password":"default",
- "config_var":"password",
- "hidden":"1"
+ "Traffic Ops url?": "https://[::]";,
+ "config_var": "base_url"
}
],
- "/opt/traffic_ops/app/db/dbconf.yml":[
+ "/opt/traffic_ops/app/conf/ldap.conf": [
{
- "Database server root (admin) user":"root",
- "config_var":"dbAdminUser"
+ "Do you want to set up LDAP?": "no",
+ "config_var": "setupLdap"
},
{
- "Database server admin password":"default",
- "config_var":"dbAdminPw",
- "hidden":"1"
+ "LDAP server hostname": "",
+ "config_var": "hostname"
},
{
- "Download Maxmind Database?":"yes",
- "config_var":"maxmind"
- }
- ],
- "/opt/traffic_ops/app/conf/cdn.conf":[
- {
- "Generate a new secret?":"yes",
- "config_var":"genSecret"
+ "LDAP Admin DN": "",
+ "config_var": "admin_dn"
},
{
- "Number of secrets to keep?":"10",
- "config_var":"keepSecrets"
+ "LDAP Admin Password": "",
+ "config_var": "password",
+ "hidden": "1"
},
{
- "Port to serve on?":"443",
- "config_var":"port"
+ "LDAP Search Base": "",
+ "config_var": "search_base"
}
],
- "/opt/traffic_ops/app/conf/ldap.conf":[
+ "/opt/traffic_ops/app/conf/production/database.conf": [
{
- "Do you want to set up LDAP?":"no",
- "config_var":"setupLdap"
+ "Database type": "Pg",
+ "config_var": "type"
},
{
- "LDAP server hostname":"",
- "config_var":"hostname"
+ "Database name": "traffic_ops_db",
+ "config_var": "dbname"
},
{
- "LDAP Admin DN":"",
- "config_var":"admin_dn"
+ "Database server hostname IP or FQDN": "localhost",
+ "config_var": "hostname"
},
{
- "LDAP Admin Password":"",
- "config_var":"password",
- "hidden":"1"
+ "Database port number": "5432",
+ "config_var": "port"
},
{
- "LDAP Search Base":"",
- "config_var":"search_base"
+ "Traffic Ops database user": "dbuser",
+ "config_var": "user"
+ },
+ {
+ "Traffic Ops database password": "dbpass",
+ "config_var": "password",
+ "hidden": "1"
Review comment:
Can we add something to the deprecation? The Perl version of
`_postinstall` only supports `hidden` properties as strings containing an
integer encoding a boolean concept (I think?), but in Python it'll happily work
with just booleans. And they really ought to be boolean. I've got other
complaints about the format of these input JSON files, but that's the big 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:
[email protected]
