Add parents to varnish cache by AbdelrahmanElawady · Pull Request #7669 · apache/trafficcontrol

AbdelrahmanElawady · 2023-07-21T18:30:38Z

Add the least required changes to run Varnish in a topology with parents by reusing some implementation already made in go-atscfg and add Dockerfile for Varnish to be testable in CIAB.

Which Traffic Control components are affected by this PR?

Traffic Control Cache Config (t3c, formerly ORT)
CDN in a Box

What is the best way to verify this PR?

Using CIAB:

change the dockerfile path to point to varnish dir in either edge or mid.
change the status to ONLINE in the cache server you choose overriding Traffic Monitor reports. for edge it will be here.
remove chkconfig and package parameters from edge or mid profile.
start docker-compose and curl the CDN domain from enroller container.
check varnish container logs to verify it properly handled the request.
try again and it should be a hit returning cached object.

Note: since both mid servers use the same profile Varnish can run either as the two mid caches or just edge. It can't run as all servers for now as the router does not route to servers if Traffic Monitor does not poll any cache server. So, either both mid servers or edge.

PR submission checklist

This PR has tests
This PR has documentation
This PR has a CHANGELOG.md entry
This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

AbdelrahmanElawady · 2023-07-21T18:36:22Z

Also, will create issues for all TODOs added to be tracked and also changes that need to be done for proper integration with TC

codecov · 2023-07-21T18:40:00Z

Codecov Report

Merging #7669 (c42d0d4) into master (75ec56f) will decrease coverage by 38.16%.
Report is 60 commits behind head on master.
The diff coverage is 15.98%.

@@              Coverage Diff              @@
##             master    #7669       +/-   ##
=============================================
- Coverage     65.05%   26.89%   -38.16%     
  Complexity       98       98               
=============================================
  Files           314      691      +377     
  Lines         12365    81057    +68692     
  Branches        907       90      -817     
=============================================
+ Hits           8044    21803    +13759     
- Misses         3968    57180    +53212     
- Partials        353     2074     +1721

Flag	Coverage Δ
golib_unit	`48.01% <46.95%> (?)`
grove_unit	`4.60% <ø> (?)`
t3c_unit	`4.92% <3.44%> (?)`
traffic_monitor_unit	`21.30% <0.00%> (?)`
traffic_ops_unit	`22.15% <4.30%> (?)`
traffic_portal_v2	`?`
traffic_stats_unit	`10.14% <ø> (?)`
unit_tests	`23.61% <14.68%> (-50.17%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
cache-config/t3c-apply/config/config.go	`0.00% <0.00%> (ø)`
cache-config/t3c-apply/t3c-apply.go	`0.00% <0.00%> (ø)`
cache-config/t3c-apply/torequest/cmd.go	`0.00% <0.00%> (ø)`
cache-config/t3c-generate/cfgfile/varnish.go	`0.00% <0.00%> (ø)`
cache-config/t3c-generate/config/config.go	`0.81% <0.00%> (-0.08%)`	⬇️
cache-config/t3c-generate/t3c-generate.go	`0.00% <0.00%> (ø)`
lib/go-atscfg/parentabstraction.go	`80.00% <ø> (ø)`
lib/go-tc/federation_resolver.go	`45.45% <0.00%> (ø)`
lib/varnishcfg/vcl.go	`0.00% <0.00%> (ø)`
lib/varnishcfg/vclbuilder.go	`0.00% <0.00%> (ø)`
... and 20 more

... and 570 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

… t3c-apply

limited · 2023-07-31T14:01:56Z

+    echo "varnishd is already running."
+  else
+    echo "Starting varnishd..."
+    "$VARNISHD_EXECUTABLE" -f /opt/trafficserver/etc/trafficserver/default.vcl


Consider moving default.vcl into a different directory, not trafficserver related?

limited · 2023-07-31T15:16:48Z

+
+	// has to be before other subroutines for variables initialization
+	txt += fmt.Sprint("sub vcl_init {\n")
+	for _, entry := range v.subroutines["vcl_init"] {


are we guaranteed to always have a vcl_init or do we need to check for presence first?

limited · 2023-07-31T15:34:57Z

+	// only change request host from edge servers which typically has multiple request FQDNs or
+	// one request FQDN that is not the origin.
+	if len(requestFQDNs) > 1 || (len(requestFQDNs) == 1 && requestFQDNs[0] != svc.DestDomain) {
+		lines = append(lines, fmt.Sprintf("\tset req.http.host = \"%s\";", svc.DestDomain))


Might want to consider moving this to vcl_backend_fetch, and doing set bereq.http.host=... so that the host header is only changed on the request to the backend, and the original host header is preserved in the client request processing

limited · 2023-07-31T15:40:00Z

+	lines := make([]string, 0)
+	directorType := getDirectorType(retryPolicy)
+	sticky := ""
+	if directorType == "fallback" && retryPolicy == atscfg.ParentAbstractionServiceRetryPolicyLatched {


Consider adding a separate sticky return from getDirectorType so all that evaluation happens in the same place

limited · 2023-07-31T15:48:38Z

Left a few comments, still need to test it out

…readable

ocket8888 · 2023-08-02T23:22:47Z

 		TSHome = tsHome
 		tsConfigDir = tsHome + "/etc/trafficserver"
+		if cache != nil && *cache == "varnish" {
+			tsConfigDir = tsHome + "/etc/varnish"


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?

I believe the config dir will be either home + /etc/trafficserver, home + /etc/varnish or if --trafficserver-home flag is not used it will default to /opt/trafficerver/etc/trafficserver and won't go into the if block (which is not good) other than that it will be home + /etc/varnish for varnish case. For example specifying --trafficserver-home flag with /opt/cache will write the config to /opt/cache/etc/varnish and that is what is done in varnish entrypoint.

I think ultimately default home and config dir shouldn't be related to TS. However, this change will affect CIAB, tests, workflows and some other code depending on that. So, Maybe it would be better if it is done in a separate PR?

ocket8888 · 2023-08-02T23:26:49Z

+			reloadCommand := config.TSHome + config.TrafficCtl
+			reloadArgs := []string{"config", "reload"}
+			if cfg.CacheType == "varnish" {
+				reloadCommand = "varnishreload"


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?

It is done that way because Varnish currently is installed under root. So, varnishreload is currently installed in user's $PATH. It will be a problem indeed if Varnish is installed under different directory. Should Varnish be installed under different directory?

ocket8888 · 2023-08-02T23:27:34Z

+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.
+ */


license header should go above imports, below package declaration

ocket8888 · 2023-08-02T23:34:31Z

+# varnishcfg requires t3c for ToData struct and not needed for enroller
+RUN rm -rf /go/src/github.com/apache/trafficcontrol/lib/varnishcfg
+


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

Maybe the comment can be improved, but it is not removed for optimization reasons. if we remove this line it won't be able to build the enroller binary because varnishcfg requires t3c-util package for the ConfigData struct here. So, we will need to copy t3c packages too. t3c also requires some other packages not included with the enroller, so also they will need to be copied. The problem will be in keeping track of all these packages and what they require and any changes in the future. So I Just remove it instead of managing all that when it is not needed. But I believe it might be better to make varnishcfg not depend on t3c-util

ocket8888 · 2023-08-02T23:36:28Z

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


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.

ocket8888 · 2023-08-02T23:38:12Z

+    /usr/local/sbin/
+
+
+COPY infrastructure/cdn-in-a-box/varnish/systemctl.sh /usr/bin/systemctl


is it absolutely necessary to use systemd? Doing that in a Docker container is prone to problems and headaches

Since t3c-apply only manages services using systemctl (or service in sysV systems) Traffic Server image and Varnish image both include a script replacing systemctl to start, restart, show status, enable and stop the cache service. There is no actual invoking of systemd, the scripts just mimics the behavior of systemctl regarding services

ocket8888 · 2023-08-02T23:39:41Z

@@ -0,0 +1,73 @@
+package varnishcfg


At least one file in a package should have a GoDoc comment.

limited · 2023-08-04T20:44:39Z

Verified CIAB works to pull traffic through varnish following instructions above

* Add varnishcfg package and parent configuration * Add Varnish Dockerfile to be used in CIAB * Add license to Varnsih Dockerfile * Add systemctl.sh to handle Varnish service and integrate Varnish with t3c-apply * Move host changes to BE fetch, change varnish dir and make test more readable * Remove Varnish package release and arch, add GoDoc and move licenses * Move license text

zrhoffman added new feature A new feature, capability or behavior low impact affects only a small portion of a CDN, and cannot itself break one cdn-in-a-box related to the Docker-based CDN-in-a-Box system cache-config Cache config generation Varnish related to Varnish labels Jul 21, 2023

AbdelrahmanElawady added 4 commits July 28, 2023 22:07

Add varnishcfg package and parent configuration

2fc572e

Add Varnish Dockerfile to be used in CIAB

7b4a027

Add license to Varnsih Dockerfile

6a59baa

Add systemctl.sh to handle Varnish service and integrate Varnish with…

6a5c26a

… t3c-apply

AbdelrahmanElawady force-pushed the add-parents-varnish branch from ca0b1ba to 6a5c26a Compare July 28, 2023 19:09

limited reviewed Jul 31, 2023

View reviewed changes

Move host changes to BE fetch, change varnish dir and make test more …

312c673

…readable

ocket8888 reviewed Aug 2, 2023

View reviewed changes

Remove Varnish package release and arch, add GoDoc and move licenses

fc55c79

AbdelrahmanElawady mentioned this pull request Aug 4, 2023

Add access control to varnish #7700

Merged

4 tasks

limited approved these changes Aug 4, 2023

View reviewed changes

AbdelrahmanElawady requested a review from ocket8888 August 8, 2023 16:16

Move license text

c42d0d4

ocket8888 approved these changes Aug 14, 2023

View reviewed changes

ocket8888 merged commit fe12a38 into apache:master Aug 14, 2023

zrhoffman mentioned this pull request Aug 15, 2023

Add t3cutil for more Go tests #7717

Merged

4 tasks

This was referenced Aug 16, 2023

Document how to run Varnish with CIAB #7724

Closed

Add HTTPS support to Varnish cache using Hitch #7725

Merged

AbdelrahmanElawady deleted the add-parents-varnish branch August 25, 2023 20:04

		# varnishcfg requires t3c for ToData struct and not needed for enroller
		RUN rm -rf /go/src/github.com/apache/trafficcontrol/lib/varnishcfg


		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

		/usr/local/sbin/


		COPY infrastructure/cdn-in-a-box/varnish/systemctl.sh /usr/bin/systemctl

Conversation

AbdelrahmanElawady commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which Traffic Control components are affected by this PR?

What is the best way to verify this PR?

PR submission checklist

Uh oh!

AbdelrahmanElawady commented Jul 21, 2023

Uh oh!

codecov Bot commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

limited commented Jul 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

limited commented Aug 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AbdelrahmanElawady commented Jul 21, 2023 •

edited

Loading

codecov Bot commented Jul 21, 2023 •

edited

Loading