pkg/server: replace pkg/util/server with pkg/server by rfratto · Pull Request #1476 · grafana-cold-storage/agent

rfratto · 2022-03-09T22:41:00Z

pkg/server is a new top-level subsystem which exposes an HTTP and gRPC server. It was forked off of weaveworks/common/server with direct support for reloading settings we wish to keep as dynamic.

The following YAML fields are intended to be dynamically updatable:

server.log_level
server.log_format
server.http_tls_config
server.grpc_tls_config

Other fields are intended to be static and should only be set via command line flag. Their YAML counterparts are now DEPRECATED and will be removed in v0.26.0. As of this commit, reloading the config file will fail if a non-dynamic field has changed in the server block.

Closes #1336.
Closes #1156.

Flag changes

Many flag names corresponding to the server have changed. These will be documented as breaking changes over previous versions of the agent.

The following flags are NEW:

-server.http.enable-tls
-server.grpc.enable-tls
-server.http.address
-server.grpc.address

The -server.*-enable-tls flags must now be passed to enable TLS for HTTP and gRPC respectively. This is a change from the previous behavior of dynamically enabling TLS when a certificate pair is provided.

-server.http.address and -server.grpc.address are intended to be host:port addresses on which to listen for HTTP and gRPC traffic. They replace the -server.*-listen-address and -server.*-listen-port flags.

The following flags have been RENAMED:

-server.log.source-ips.enabled (renamed from -server.log-source-ips-enabled)
-server.log.source-ips.header (renamed from -server.log-source-ips-header)
-server.log.source-ips.regex (renamed from -server.log-source-ips-regex)
-server.http.network (renamed from -server.http-listen-network)
-server.http.conn-limit (renamed from -server.http-conn-limit)
-server.http.read-timeout (renamed from -server.http-read-timeout)
-server.http.write-timeout (renamed from -server.http-write-timeout)
-server.http.idle-timeout (renamed from -server.http-idle-timeout)
-server.grpc.network (renamed from -server.grpc-listen-network)
-server.grpc.conn-limit (renamed from -server.grpc-conn-limit)
-server.grpc.max-recv-msg-size-bytes (renamed from -server.grpc-max-recv-msg-size-bytes)
-server.grpc.max-send-msg-size-bytes (renamed from -server.grpc-max-send-msg-size-bytes)
-server.grpc.max-concurrent-streams (renamed from -server.grpc-max-concurrent-streams)

The following flags have been REMOVED:

-log.level (use server.log_level in the YAML instead)
-log.format (use server.log_format in the YAML instead)
-server.http-tls-cert-path (use server.http_tls_config in the YAML instead)
-server.http-tls-key-path (use server.http_tls_config in the YAML instead)
-server.http-tls-client-auth (use server.http_tls_config in the YAML instead)
-server.http-tls-ca-path (use server.http_tls_config in the YAML instead)
-server.grpc-tls-cert-path (use server.grpc_tls_config in the YAML instead)
-server.grpc-tls-key-path (use server.grpc_tls_config in the YAML instead)
-server.grpc-tls-client-auth (use server.grpc_tls_config in the YAML instead)
-server.grpc-tls-ca-path (use server.grpc_tls_config in the YAML instead)
-server.http-listen-address (use the -server.http.address flag instead)
-server.http-listen-port (use the -server.http.address flag instead)
-server.grpc-listen-address (use the -server.grpc.address flag instead)
-server.grpc-listen-port (use the -server.grpc.address flag instead)
-server.path-prefix (usage of this flag was never supported and would cause issues)

-server.path-prefix and the equivalent YAML field server.path_prefix have been removed. Setting this option would lead to undefined behavior for various parts of the agent. Being able to prepend an API prefix may return in the future if it is requested.

Flags not listed here are unchanged.

Other changes

All jsonnet and config file examples no longer use the deprecated field names.

Todo

CHANGELOG updated
Documentation added
Tests updated
Locally test to double-check that self-scraping still works

rfratto · 2022-03-09T22:43:59Z

+// RegisterFlags registers flags for c to the given FlagSet.
+func (f *Flags) RegisterFlags(fs *flag.FlagSet) {
+	d := DefaultFlags
+
+	fs.BoolVar(&f.RegisterInstrumentation, "server.register-instrumentation", d.RegisterInstrumentation, "Register the intrumentation handlers (e.g., /metrics)")
+	fs.DurationVar(&f.GracefulShutdownTimeout, "server.graceful-shutdown-timeout", d.GracefulShutdownTimeout, "Timeout for a graceful server shutdown")
+	fs.BoolVar(&f.LogSourceIPs, "server.log.source-ips.enabled", d.LogSourceIPs, "Log IP address of client for incoming requests")
+	fs.StringVar(&f.LogSourceIPsHeader, "server.log.source-ips.header", d.LogSourceIPsHeader, "Header field storing the source IPs. Only used if server.log-source-ips-enabled is true. Defaults to Forwarded, X-Real-IP, and X-Forwarded-For")
+	fs.StringVar(&f.LogSourceIPsRegex, "server.log.source-ips.regex", d.LogSourceIPsRegex, "Regex for matching the source IPs. Only used if server.log-source-ips-enabled is true. Defaults to Forwarded, X-Real-IP, and X-Forwarded-For") // TODO(rfratto): help text here seems wrong?
+
+	f.HTTP.RegisterFlags(fs)
+	f.GRPC.RegisterFlags(fs)
+}
+
+// RegisterFlags registers flags for c to the given FlagSet.
+func (f *HTTPFlags) RegisterFlags(fs *flag.FlagSet) {
+	d := DefaultHTTPFlags
+
+	fs.BoolVar(&f.UseTLS, "server.http.enable-tls", d.UseTLS, "Enable TLS for the HTTP server.")
+	fs.StringVar(&f.ListenAddress, "server.http.address", d.ListenAddress, "HTTP server listen host:port. Takes precedence over YAML listen flags when set.")
+	fs.StringVar(&f.ListenNetwork, "server.http.network", d.ListenNetwork, "HTTP server listen network")
+	fs.IntVar(&f.ConnLimit, "server.http.conn-limit", d.ConnLimit, "Maximum number of simultaneous HTTP connections (0 = unlimited)")
+	fs.DurationVar(&f.ReadTimeout, "server.http.read-timeout", d.ReadTimeout, "HTTP server read timeout")
+	fs.DurationVar(&f.WriteTimeout, "server.http.write-timeout", d.WriteTimeout, "HTTP server write timeout")
+	fs.DurationVar(&f.IdleTimeout, "server.http.idle-timeout", d.IdleTimeout, "HTTP server idle timeout")
+}
+
+// RegisterFlags registers flags for c to the given FlagSet.
+func (f *GRPCFlags) RegisterFlags(fs *flag.FlagSet) {
+	d := DefaultGRPCFlags
+
+	fs.BoolVar(&f.UseTLS, "server.grpc.enable-tls", d.UseTLS, "Enable TLS for the gRPC server.")
+	fs.StringVar(&f.ListenAddress, "server.grpc.address", d.ListenAddress, "gRPC server listen host:port. Takes precedence over YAML listen flags when set.")
+	fs.StringVar(&f.ListenNetwork, "server.grpc.network", d.ListenNetwork, "gRPC server listen network")
+	fs.IntVar(&f.ConnLimit, "server.grpc.conn-limit", d.ConnLimit, "Maximum number of simultaneous gRPC connections (0 = unlimited)")
+	fs.IntVar(&f.MaxRecvMsgSize, "server.grpc.max-recv-msg-size-bytes", d.MaxRecvMsgSize, "Maximum size in bytes for received gRPC messages")
+	fs.IntVar(&f.MaxSendMsgSize, "server.grpc.max-send-msg-size-bytes", d.MaxSendMsgSize, "Maximum size in bytes for send gRPC messages")
+	fs.UintVar(&f.MaxConcurrentStreams, "server.grpc.max-concurrent-streams", d.MaxConcurrentStreams, "Maximum number of concurrent gRPC streams (0 = unlimited)")
+	fs.DurationVar(&f.MaxConnectionIdle, "server.grpc.keepalive.max-connection-idle", d.MaxConnectionIdle, "Time to wait before closing idle gRPC connections")
+	fs.DurationVar(&f.MaxConnectionAge, "server.grpc.keepalive.max-connection-age", d.MaxConnectionAge, "Maximum age for any gRPC connection for a graceful shutdown")
+	fs.DurationVar(&f.MaxConnectionAgeGrace, "server.grpc.keepalive.max-connection-age-grace", d.MaxConnectionAgeGrace, "Grace period to forceibly close connections after a graceful shutdown starts")
+	fs.DurationVar(&f.KeepaliveTime, "server.grpc.keepalive.time", d.KeepaliveTime, "Frequency to send keepalive pings from the server")
+	fs.DurationVar(&f.KeepaliveTimeout, "server.grpc.keepalive.timeout", d.KeepaliveTimeout, "How long to wait for a keepalive pong before closing the connection")
+	fs.DurationVar(&f.MinTimeBetweenPings, "server.grpc.keepalive.min-time-between-pings", d.MinTimeBetweenPings, "Maximum frequency that clients may send pings at")
+	fs.BoolVar(&f.PingWithoutStreamAllowed, "server.grpc.keepalive.ping-without-stream-allowed", d.PingWithoutStreamAllowed, "Allow clients to send pings without having a gRPC stream")
+}


I deliberately chose to not support the old flag names as "deprecated" here, instead opting for the hard breaking change of the flag names.

Should we support the old names too or leave it as-is?

I am pro-breaking them

rfratto

Gave myself a quick review, which is way easier to do on GitHub than it is locally.

~~One extra thing: I want to make sure that I run the integrations locally before I take it out of draft to make sure self-scraping still works for both v1 and v2 integrations.~~ (I've tested this locally and everything still works w/r/t self-scraping)

rfratto · 2022-03-09T22:47:28Z

-		if cfg != nil {
-			cfg.Server.Log = cfgLogger
-		}


FYI: This isn't necessary anymore since pkg/server.New doesn't read a Log field like the old New function did.

rfratto · 2022-03-09T22:49:32Z

      - url: https://prometheus-us-central1.grafana.net/api/prom/push
        basic_auth:
-          username: 12345
+          username: xyz


@mattdurham I changed all the usernames to xyz as sample text since 12345 could be easily confused with the port number we tend to recommend.

rfratto · 2022-03-09T22:51:43Z


 # Any user defined arguments
-CUSTOM_ARGS=""
+CUSTOM_ARGS="-server.http.address=127.0.0.1:9090 -server.grpc.address=127.0.0.1:9091"


FYI: These used to be set on the config file, so I've moved them to flags that can be overridden.

rfratto · 2022-03-09T22:56:03Z

 		"-config.file=/var/lib/grafana-agent/config/agent.yml",
 		"-config.expand-env=true",
 		"-reload-port=8081",
+		"-server.http.address=0.0.0.0:8080",


FYI: This moves setting the port from hard-coded in the YAML file to hard-coded as an argument. The same change is made for the metrics pods.

We'll want to include in the upgrade guide that v0.24.0 of the operator will only be able to deploy agents from v0.24.0 onward, since this flag name is new.

pkg/server is a new top-level subsystem which exposes an HTTP and gRPC server. It was forked off of weaveworks/common/server with direct support for reloading settings we wish to keep as dynamic. The following YAML fields are intended to be dynamically updatable: * server.log_level * server.log_format * server.http_tls_config * server.grpc_tls_config Other fields are intended to be static and should only be set via command line flag. Their YAML counterparts are now DEPRECATED and will be removed in v0.26.0. As of this commit, reloading the config file will fail if a non-dynamic field has changed in the server block. Closes grafana-cold-storage#1336. Flag changes ------------ Many flag names corresponding to the server have changed. The following flags are NEW: * `-server.http.enable-tls` * `-server.grpc.enable-tls` * `-server.http.address` * `-server.grpc.address` The `-server.*-enable-tls` flags must now be passed to enable TLS for HTTP and gRPC respectively. This is a change from the previous behavior of dynamically enabling TLS when a certificate pair is provided. `-server.http.address` and `-server.grpc.address` are intended to be `host:port` addresses on which to listen for HTTP and gRPC traffic. They replace the `-server.*-listen-address` and `-server.*-listen-port` flags. The following flags have been RENAMED: * `-server.log.source-ips.enabled` (renamed from `-server.log-source-ips-enabled`) * `-server.log.source-ips.header` (renamed from `-server.log-source-ips-header`) * `-server.log.source-ips.regex` (renamed from `-server.log-source-ips-regex`) * `-server.http.network` (renamed from `-server.http-listen-network`) * `-server.http.conn-limit` (renamed from `-server.http-conn-limit`) * `-server.http.read-timeout` (renamed from `-server.http-read-timeout`) * `-server.http.write-timeout` (renamed from `-server.http-write-timeout`) * `-server.http.idle-timeout` (renamed from `-server.http-idle-timeout`) * `-server.grpc.network` (renamed from `-server.grpc-listen-network`) * `-server.grpc.conn-limit` (renamed from `-server.grpc-conn-limit`) * `-server.grpc.max-recv-msg-size-bytes` (renamed from `-server.grpc-max-recv-msg-size-bytes`) * `-server.grpc.max-send-msg-size-bytes` (renamed from `-server.grpc-max-send-msg-size-bytes`) * `-server.grpc.max-concurrent-streams` (renamed from `-server.grpc-max-concurrent-streams`) The following flags have been REMOVED: * `-log.level` (use `server.log_level` in the YAML instead) * `-log.format` (use `server.log_format` in the YAML instead) * `-server.http-tls-cert-path` (use `server.http_tls_config` in the YAML instead) * `-server.http-tls-key-path` (use `server.http_tls_config` in the YAML instead) * `-server.http-tls-client-auth` (use `server.http_tls_config` in the YAML instead) * `-server.http-tls-ca-path` (use `server.http_tls_config` in the YAML instead) * `-server.grpc-tls-cert-path` (use `server.grpc_tls_config` in the YAML instead) * `-server.grpc-tls-key-path` (use `server.grpc_tls_config` in the YAML instead) * `-server.grpc-tls-client-auth` (use `server.grpc_tls_config` in the YAML instead) * `-server.grpc-tls-ca-path` (use `server.grpc_tls_config` in the YAML instead) * `-server.http-listen-address` (use the `-server.http.address` flag instead) * `-server.http-listen-port` (use the `-server.http.address` flag instead) * `-server.grpc-listen-address` (use the `-server.grpc.address` flag instead) * `-server.grpc-listen-port` (use the `-server.grpc.address` flag instead) * `-server.path-prefix` (usage of this flag was never supported and would cause issues) `-server.path-prefix` and the equivalent YAML field `server.path_prefix` have been removed. Setting this option would lead to undefined behavior for various parts of the agent. Being able to prepend an API prefix may return in the future if it is requested. Flags not listed here are unchanged. Other changes ------------- All jsonnet and config file examples no longer use the deprecated field names.

TODOs about documentation have been "documented" in the original commit message, and updating user documentation is being tracked externally on the PR.

rfratto · 2022-03-10T12:46:08Z

I've updated the user documentation to reflect the changes here and this is ready for review now.

The biggest remaining question is if we want to use this opportunity to move away from the default :80 listen address (or maybe that can be a separate PR?)

mattdurham

Overall great, I think the windows installer looks odd though and unsure about using 80 has the default.

mattdurham

LGTM

…1:12346 This commit changes the default listen addresses to be 127.0.0.1:12345 (for HTTP) and 127.0.0.1:12346 (for gRPC). This makes listening on all interfaces opt-in rather than opt-out, avoiding accidental overexposure of access. Closes grafana-cold-storage#852. Additionally, the `-reload-addr` and `-reload-port` flags have been removed as a follow up to grafana-cold-storage#1476. Now that the HTTP and gRPC server are static for the lifetime of the application, it is impossible for the user to change their configuration file in such a way to cause it to shut down while performing a reload. This means that the `-reload-addr` and `-reload-port` no longer have a use and can be removed safely.

* server: Change default listen addresses to 127.0.0.1:12345 / 127.0.0.1:12346 This commit changes the default listen addresses to be 127.0.0.1:12345 (for HTTP) and 127.0.0.1:12346 (for gRPC). This makes listening on all interfaces opt-in rather than opt-out, avoiding accidental overexposure of access. Closes #852. Additionally, the `-reload-addr` and `-reload-port` flags have been removed as a follow up to #1476. Now that the HTTP and gRPC server are static for the lifetime of the application, it is impossible for the user to change their configuration file in such a way to cause it to shut down while performing a reload. This means that the `-reload-addr` and `-reload-port` no longer have a use and can be removed safely. * fix extra references to old flags / port defaults * config: fix test * fix test assertion for reload address change

rfratto commented Mar 9, 2022

View reviewed changes

rfratto added 8 commits March 10, 2022 07:44

pkg/server: make Test_tlsListener resilient to different environments

bcb3eb9

pkg/integrations: fix setting self-scraping host/port

e898767

pkg/server: remove outdated TODOs

3b2346e

TODOs about documentation have been "documented" in the original commit message, and updating user documentation is being tracked externally on the PR.

pkg/server: fix help text for -server.log.source-ips.regex

461fb76

cmd/agent: fix Windows compilation error

db40b2e

pkg/server: loosen test restriction on checking for reused port error

fd6131e

document changes

ae1ed0f

rfratto force-pushed the server-block-field-deprecation branch from 34f1073 to ae1ed0f Compare March 10, 2022 12:45

rfratto marked this pull request as ready for review March 10, 2022 12:46

rfratto requested review from gaantunes and rgeyer as code owners March 10, 2022 12:46

rfratto requested a review from mattdurham March 10, 2022 12:46

mattdurham reviewed Mar 10, 2022

View reviewed changes

Comment thread CHANGELOG.md Outdated

mattdurham reviewed Mar 10, 2022

View reviewed changes

Comment thread pkg/server/server.go

mattdurham suggested changes Mar 10, 2022

View reviewed changes

rfratto added 3 commits March 10, 2022 12:13

packaging/windows: fix arguments passed to NSIS

45249a2

CHANGELOG: add missing authorship

6fb9459

pkg/server: split server.New implementation into multiple functions

9cc7574

rfratto requested a review from mattdurham March 10, 2022 17:32

pkg/server: add error checks back

f3f3587

rfratto added this to the v0.24.0 milestone Mar 10, 2022

mattdurham approved these changes Mar 10, 2022

View reviewed changes

rfratto merged commit b0c1e45 into grafana-cold-storage:main Mar 10, 2022

rfratto deleted the server-block-field-deprecation branch March 10, 2022 18:07

rfratto mentioned this pull request Mar 10, 2022

tools/crow: make sure that default settings are applied to crow #1480

Merged

rfratto mentioned this pull request Mar 10, 2022

server: Change default HTTP/gRPC listen address #1481

Merged

github-actions Bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Apr 3, 2024

github-actions Bot locked as resolved and limited conversation to collaborators Apr 3, 2024

Uh oh!

Conversation

rfratto commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Flag changes

Other changes

Todo

Uh oh!

rfratto Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

mattdurham Mar 10, 2022

Choose a reason for hiding this comment

Uh oh!

rfratto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rfratto Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rfratto Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

rfratto Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rfratto Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rfratto commented Mar 10, 2022

Uh oh!

Uh oh!

Uh oh!

mattdurham left a comment

Choose a reason for hiding this comment

Uh oh!

mattdurham left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rfratto commented Mar 9, 2022 •

edited

Loading

rfratto left a comment •

edited

Loading

rfratto Mar 9, 2022 •

edited

Loading

rfratto Mar 9, 2022 •

edited

Loading

rfratto Mar 9, 2022 •

edited

Loading