[GitHub] [trafficcontrol] rob05c opened a new pull request #6101: Add t3c ds tls versions field

[GitBox <gi...@apache.org>]

rob05c opened a new pull request #6101:
URL: https://github.com/apache/trafficcontrol/pull/6101


   Adds t3c using the new Delivery Service TLS Versions field, if it exists.
   
   Also updates t3c to API 4.0, needed for the new field.
   
   Includes tests.
   No docs, no interface change (and the DS TLS Versions field is already documented)
   No changelog, no interface change (other than the DS TLS Versions field, which is already documented)
   
   ## Which Traffic Control components are affected by this PR?
   - Traffic Control Cache Config (T3C, formerly ORT)
   
   ## What is the best way to verify this PR?
   Run tests. Run t3c with a DS with the TLS Versions field, verify the values are set in sni.yaml or ssl_server_name.yaml as expected.
   
   ## If this is a bugfix, which Traffic Control versions contained the bug?
   Not a bug fix.
   
   
   ## PR submission checklist
   - [x] This PR has tests <!-- If not, please delete this text and explain why this PR does not need tests. -->
   - [x] This PR has documentation <!-- If not, please delete this text and explain why this PR does not need documentation. -->
   - [x] This PR has a CHANGELOG.md entry <!-- A fix for a bug from an ATC release, an improvement, or a new feature should have a changelog entry. -->
   - [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
   


-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] rob05c commented on a change in pull request #6101: Add t3c ds tls versions field

[GitBox <gi...@apache.org>]

rob05c commented on a change in pull request #6101:
URL: https://github.com/apache/trafficcontrol/pull/6101#discussion_r688042320



##########
File path: cache-config/t3c/t3c.go
##########
@@ -64,20 +65,19 @@ func main() {
 	}
 
 	app := "t3c-" + cmd
-	args := append([]string{app}, os.Args[2:]...)
 
-	ex, err := os.Executable()
+	appPath, err := exec.LookPath(app)
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "error getting application information: "+err.Error()+"\n")
-		os.Exit(ExitCodeExeErr)
+		fmt.Fprintf(os.Stderr, "error finding path to '"+app+"': "+err.Error())
+		os.Exit(ExitCodeCommandLookupErr)
 	}
-	dir := filepath.Dir(ex)
-	appDir := filepath.Join(dir, app) // TODO use path, not exact dir of this exe
+
+	args := append([]string{app}, os.Args[2:]...)
 
 	env := os.Environ()
 
-	if err := syscall.Exec(appDir, args, env); err != nil {
-		fmt.Fprintf(os.Stderr, "error executing sub-command: "+err.Error()+"\n")
+	if err := syscall.Exec(appPath, args, env); err != nil {

Review comment:
       This warrants some explanation: this was a sort-of bug I found while working on this.
   So `syscall.Exec` is calling `execve`, which takes the full path of the executable, _not_ just the name. Just the name appears to work on some *NIX but not others (specifically, it seems to work on OS X, but not CentOS).
   
   So, what we were doing, was getting the path of the current exe (t3c) and prepending that to the app name (e.g. t3c-apply).
   
   This worked as long as they were in the same directory. Which they are with the provided RPM. But we shouldn't rely on that, apps calling other apps should always use the PATH. Specifically, if you're developing and have apps in different build dirs, this then broke things like `t3c apply`. Most app execution methods do this for you, but `execve` doesn't. (And we very specifically want execve not anything else, because it fully replaces the running app with the called one, rather that a subshell or sub-process or anything else messy).
   
   TLDR this is the right solution: we lookup the executable's path from the OS, and then call `execve` with that full path, which fully replaces the running `t3c` with the subcommand like `t3c-apply`.




-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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

[GitHub] [trafficcontrol] zrhoffman merged pull request #6101: Add t3c ds tls versions field

[GitBox <gi...@apache.org>]

zrhoffman merged pull request #6101:
URL: https://github.com/apache/trafficcontrol/pull/6101


   


-- 
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: issues-unsubscribe@trafficcontrol.apache.org

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