Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

TELEMETRY_HTTP_PROXY env var to proxy telemetry requests#47466

Merged
DaedalusG merged 6 commits into
mainfrom
sqs/telemetry-http-proxy
Feb 23, 2023
Merged

TELEMETRY_HTTP_PROXY env var to proxy telemetry requests#47466
DaedalusG merged 6 commits into
mainfrom
sqs/telemetry-http-proxy

Conversation

@sqs

@sqs sqs commented Feb 8, 2023 •
edited by DaedalusG
Loading

Copy link
Copy Markdown
Member

The env var TELEMETRY_HTTP_PROXY, if set, causes all telemetry requests (update checks and pings) to be proxied through an HTTP proxy.

cc @toddherskovitz

Test plan

@DaedalusG
Testing using the following light weight proxy, and an sg dev environment

package main

import (
	"github.com/elazarl/goproxy"
	"log"
	"flag"
	"net/http"
)

func main() {
	verbose := flag.Bool("v", false, "should every proxy request be logged to stdout")
	addr := flag.String("addr", ":8080", "proxy listen address")
	flag.Parse()
	proxy := goproxy.NewProxyHttpServer()
	proxy.Verbose = *verbose
	log.Fatal(http.ListenAndServe(*addr, proxy))
}

@sqs
TELEMETRY_HTTP_PROXY env var to proxy telemetry requests
00abcf1 
The env var TELEMETRY_HTTP_PROXY, if set, causes all telemetry requests (update checks and pings) to be proxied through an HTTP proxy.
@cla-bot cla-bot Bot added the cla-signed label Feb 8, 2023
@sqs sqs added the hackathon label Feb 8, 2023
@emidoots

emidoots commented Feb 8, 2023

Copy link
Copy Markdown
Member

neat! I assume some HN conversation spawned this idea :)

maybe consider respecting TELEMETRY=false too as an opt-out env var?

@sqs

sqs commented Feb 10, 2023

Copy link
Copy Markdown
Member Author

@slimsag This is actually a customer request so they can send telemetry to us.

@camdencheek Someone told me you wanted to take this one or had already? Or did I hear wrong?

@camdencheek

camdencheek commented Feb 11, 2023

Copy link
Copy Markdown
Member

@sqs I don't remember hearing about this, but I am working on a different fix for the same customer, so maybe that's what you heard?

@sqs

sqs commented Feb 14, 2023

Copy link
Copy Markdown
Member Author

@camdencheek Ah, that must have been the source of confusion.

@DaedalusG Do you want to take this fix from here? I can help.

@DaedalusG

DaedalusG commented Feb 14, 2023

Copy link
Copy Markdown
Contributor

Yes please @sqs , I've been working to test this one, was hoping to make more progress Monday but got swamped with issues 😅

@DaedalusG

DaedalusG commented Feb 21, 2023

Copy link
Copy Markdown
Contributor

It looks like we actually honor the standard HTTPS_PROXY

sg.config.overwrite.yaml

commands:
  frontend:
    env:
      # TELEMETRY_HTTP_PROXY: "http://127.0.0.1:8080"
      HTTPS_PROXY: "http://127.0.0.1:8080"
      # NO_PROXY: *.company.com

sg logs

[       frontend] !!! USING REGULAR PROXY ENV VAR !!!
[       frontend] INFO frontend cli/serve_cmd.go:261 ✱ Sourcegraph is ready at: https://sourcegraph.test:3443
[       frontend] !!! Response Status Code  200 !!!
[       frontend] INFO frontend.updatecheck updatecheck/client.go:755 telemetry proxy {"TELEMETRY_HTTP_PROXY=": ""}

goproxy

λ ~/proxy-test/goproxy/ go run ./main.go -v
2023/02/21 00:25:26 [001] INFO: Running 0 CONNECT handlers
2023/02/21 00:25:26 [001] INFO: Accepting CONNECT to sourcegraph.com:443

@DaedalusG

DaedalusG commented Feb 21, 2023

Copy link
Copy Markdown
Contributor

The new TELEMETRY_HTTP_PROXY also works

sg.config.overwrite.yaml

commands:
  frontend:
    env:
      TELEMETRY_HTTP_PROXY: "http://127.0.0.1:8080"
      # HTTPS_PROXY: "http://127.0.0.1:8080"
      # NO_PROXY: *.apple.com

sg logs

[       frontend] !!!!!! USING PROXY FOR TELEMETRY !!!!!!
[       frontend] !!! Response Status Code  200 !!!
[       frontend] INFO frontend.updatecheck updatecheck/client.go:755 telemetry proxy {"TELEMETRY_HTTP_PROXY=": "http://127.0.0.1:8080"}

proxy logs

λ ~/proxy-test/goproxy/ go run ./main.go
2023/02/21 01:00:10 [001] INFO: Running 0 CONNECT handlers
2023/02/21 01:00:10 [001] INFO: Accepting CONNECT to sourcegraph.com:443
2023/02/21 01:36:54 [002] INFO: Running 0 CONNECT handlers
2023/02/21 01:36:54 [002] INFO: Accepting CONNECT to sourcegraph.com:443
2023/02/21 03:19:13 [003] INFO: Running 0 CONNECT handlers
2023/02/21 03:19:13 [003] INFO: Accepting CONNECT to sourcegraph.com:443

@DaedalusG

DaedalusG commented Feb 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

After mulling it over even though setting HTTPS_PROXY will send the frontend traffic through a proxy will likely work, and during testing I didnt see any unexpected requests go to the test proxy server -- its probably better to keep this discrete and stick with TELEMETRY_HTTP_PROXY to avoid any unexpected side effects

@DaedalusG DaedalusG marked this pull request as ready for review February 22, 2023 04:27
@sourcegraph-bot

sourcegraph-bot commented Feb 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 7fe2e79...6d59a93.

Notify File(s)
@sourcegraph/delivery doc/admin/pings.md

keegancsmith
keegancsmith approved these changes Feb 22, 2023
View reviewed changes

@keegancsmith keegancsmith left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I still would prefer relying on the generic http_proxy envvars. If the customer doesn't like that idea, then lets ship this. But I would assume that if the customer in question was worried about doing too much external traffic, they can configure there proxy to lock us down.

@DaedalusG DaedalusG enabled auto-merge (squash) February 23, 2023 20:44
@DaedalusG DaedalusG merged commit 4cfddd1 into main Feb 23, 2023
@DaedalusG DaedalusG deleted the sqs/telemetry-http-proxy branch February 23, 2023 23:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@keegancsmith keegancsmith keegancsmith approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed hackathon

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@sqs @emidoots @camdencheek @DaedalusG @sourcegraph-bot @keegancsmith