ocket8888 commented on code in PR #7669:
URL: https://github.com/apache/trafficcontrol/pull/7669#discussion_r1282504399
##########
infrastructure/cdn-in-a-box/enroller/Dockerfile:
##########
@@ -42,6 +42,9 @@ COPY ./traffic_ops/toclientlib/
/go/src/github.com/apache/trafficcontrol/traffic
COPY ./traffic_ops/v4-client/
/go/src/github.com/apache/trafficcontrol/traffic_ops/v4-client/
COPY ./infrastructure/cdn-in-a-box/
/go/src/github.com/apache/trafficcontrol/infrastructure/cdn-in-a-box/
+# varnishcfg requires t3c for ToData struct and not needed for enroller
+RUN rm -rf /go/src/github.com/apache/trafficcontrol/lib/varnishcfg
+
Review Comment:
I wouldn't worry about it; the enroller is pulling in a lot of things it
doesn't strictly need. We can evaluate it if it becomes a problem, and try to
clean it up a bit, but varnishcfg is small compared to the rest of the cruft so
there's no point worrying about it
##########
cache-config/t3c-apply/torequest/torequest.go:
##########
@@ -1188,7 +1192,13 @@ func (r *TrafficOpsReq) StartServices(syncdsUpdate
*UpdateStatus, metaData *t3cu
log.Errorln("ATS configuration has changed. The new
config will be picked up the next time ATS is started.")
} else if serviceNeeds == t3cutil.ServiceNeedsReload {
log.Infoln("ATS configuration has changed, Running
'traffic_ctl config reload' now.")
- if _, _, err :=
util.ExecCommand(config.TSHome+config.TrafficCtl, "config", "reload"); err !=
nil {
+ reloadCommand := config.TSHome + config.TrafficCtl
+ reloadArgs := []string{"config", "reload"}
+ if cfg.CacheType == "varnish" {
+ reloadCommand = "varnishreload"
Review Comment:
This seems to be relying on `varnishreload` being in the running user's
`$PATH` - with ATS cache servers we typically install everything under
`/opt/trafficserver` (not that I think that's a good idea, personally) but it
seems like that won't work for varnish caches if everything winds up installed
under e.g. `/opt/varnish`. Or, at least, not without some extra work. Is that
intentional, or is there some reason why it wouldn't find `varnishreload` in
the same directory as the `varnish` binary?
##########
lib/varnishcfg/vclbuilder.go:
##########
@@ -0,0 +1,73 @@
+package varnishcfg
Review Comment:
At least one file in a package should have a GoDoc comment.
##########
cache-config/t3c-apply/config/config.go:
##########
@@ -533,6 +535,9 @@ If any of the related flags are also set, they override the
mode's default behav
if tsHome != "" {
TSHome = tsHome
tsConfigDir = tsHome + "/etc/trafficserver"
+ if cache != nil && *cache == "varnish" {
+ tsConfigDir = tsHome + "/etc/varnish"
Review Comment:
It looks like for varnish cache servers, this will cause it to look for
configuration files under `/etc/trafficserver/etc/varnish` (by default I think
`tsHome` is `/opt/trafficserver` so that winds up being
`/opt/trafficserver/etc/trafficserver/etc/varnish`) - is that really how we
want to structure that?
##########
infrastructure/cdn-in-a-box/varnish/Dockerfile:
##########
@@ -0,0 +1,63 @@
+# 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.
+
+ARG BASE_IMAGE=rockylinux \
+ RHEL_VERSION=8
+FROM ${BASE_IMAGE}:${RHEL_VERSION} AS common-varnish-cache-config-layers
+ARG RHEL_VERSION=8
+# Makes RHEL_VERSION available at runtime
+ENV RHEL_VERSION="$RHEL_VERSION"
+
+RUN dnf module disable varnish -y && yum install -y epel-release
+
+RUN curl -s
https://packagecloud.io/install/repositories/varnishcache/varnish73/script.rpm.sh
| bash
+
+RUN yum install varnish-7.3.0-1.el8.x86_64 -y
Review Comment:
Do you really want to pin down to a specific release like that? If there's a
major security bug that gets fixed with no breaking changes in v7.3.0-2 then
we'd have to update this script. Also not sure it's a good idea to hard-code
the CPU architecture; a lot of ATC devs use Macs, for example, and their new
chipset isn't x86_64.
##########
infrastructure/cdn-in-a-box/varnish/Dockerfile:
##########
@@ -0,0 +1,63 @@
+# 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.
+
+ARG BASE_IMAGE=rockylinux \
+ RHEL_VERSION=8
+FROM ${BASE_IMAGE}:${RHEL_VERSION} AS common-varnish-cache-config-layers
+ARG RHEL_VERSION=8
+# Makes RHEL_VERSION available at runtime
+ENV RHEL_VERSION="$RHEL_VERSION"
+
+RUN dnf module disable varnish -y && yum install -y epel-release
+
+RUN curl -s
https://packagecloud.io/install/repositories/varnishcache/varnish73/script.rpm.sh
| bash
+
+RUN yum install varnish-7.3.0-1.el8.x86_64 -y
+
+RUN dnf install -y bind-utils kyotocabinet-libs initscripts iproute net-tools
nmap-ncat gettext autoconf automake libtool gcc-c++ cronie glibc-devel
openssl-devel git perl && \
+ dnf install -y jq logrotate findutils && \
+ dnf clean all
+
+
+COPY infrastructure/cdn-in-a-box/varnish/run.sh
infrastructure/cdn-in-a-box/traffic_ops/to-access.sh
infrastructure/cdn-in-a-box/enroller/server_template.json /
+
+COPY infrastructure/cdn-in-a-box/dns/set-dns.sh \
+ infrastructure/cdn-in-a-box/dns/insert-self-into-dns.sh \
+ /usr/local/sbin/
+
+
+COPY infrastructure/cdn-in-a-box/varnish/systemctl.sh /usr/bin/systemctl
Review Comment:
is it absolutely necessary to use systemd? Doing that in a Docker container
is prone to problems and headaches
##########
cache-config/t3c-generate/cfgfile/varnish.go:
##########
@@ -0,0 +1,46 @@
+package cfgfile
+
+import (
+ "github.com/apache/trafficcontrol/cache-config/t3c-generate/config"
+ "github.com/apache/trafficcontrol/cache-config/t3cutil"
+ "github.com/apache/trafficcontrol/lib/varnishcfg"
+)
+
+/*
+ * 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.
+ */
Review Comment:
license header should go above imports, below package declaration
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
