(Windows) Cannot Redirect the access_log output to (stderr/stdout/null)

[phlax]

description

On Windows there is no easy way to redirect access log output to standard output/error.

original description:

while documenting basic logging (#13927) it became apparent that some of the ~default (container) configs might not work on > windows, and this pointed to an underlying issue in terms of handling those paths and/or documenting something similar

one suggestion that came up was to have some kind of config identifier "$dev_stdout" eg, that would #justwork on both

[phlax]

@envoyproxy/windows-dev ive shifted the conversation about /dev/null/stdout/stderr to a new ticket

[phlax]

ref #13935

[phlax]

i guess this could also potentially apply to stdin although im not aware of it being used by envoy

[phlax]

one thing im still not clear about is whether the "note" about CON here https://www.envoyproxy.io/docs/envoy/latest/start/quick-start/run-envoy#debugging-envoy (just above the linked title) is correct

im happy to update/remove if it is not

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[davinci26]

I am proposing the following approach that will be part of filesystem_impl:

This is a runtime best-effort translation of common UNIX paths to Windows. Given that the set of known UNIX paths is small the translation should not add much overhead.

@@ -31,6 +31,14 @@ Api::IoCallBoolResult FileImplWin32::open(FlagSet in) {
     return resultSuccess(true);
   }

+  auto std_handle = toStandardHandle(path);
+  if (std_handle != INVALID_HANDLE) {
+    fd_ = GetStdHandle(std_handle);
+    return;
+  }
+
+  auto translated_path = translateFromUnixPath(path);
+
   auto flags = translateFlag(in);
   fd_ = CreateFileA(path_.c_str(), flags.access_, FILE_SHARE_READ | FILE_SHARE_WRITE, 0,
                     flags.creation_, 0, NULL);
@@ -274,5 +282,9 @@ bool InstanceImplWin32::illegalPath(const std::string& path) {
   return false;
 }

Where we have two different translation types:

1. Behind the toStandardHandle() function

{
  "/dev/stdout/" -> STD_OUTPUT_HANDLE
  "/dev/fd/1" -> STD_OUTPUT_HANDLE
  "/proc/pid/fd/1" -> STD_OUTPUT_HANDLE
}

(Similarly fd 0 will map to STD_INPUT_HANDLE and fd 2 will map to STD_ERROR_HANDLE)

2. Behind the translateFromUnixPath() function (note that this translation is also used in here):

{
   "/dev/console" -> CONOUT
   "/dev/tty" -> CONOUT
   "/dev/tty/nnn"  -> CONOUT
   "CONIN" -> CONOUT
   "CONOUT" -> CONOUT
   "CON" -> CONOUT
   "/dev/null" -> NUL
}

See https://docs.microsoft.com/en-us/windows/console/getstdhandle

I look for your feedback

[sunjayBhatia]

We might have to do some validation when in service mode that a user doesn't try to use stdout/err as the console functions/GetStdHandle will likely fail, something to test out at least

[davinci26]

We might have to do some validation when in service mode that a user doesn't try to use stdout/err as the console functions/GetStdHandle will likely fail, something to test out at least

These should be non-recoverable failures scenarios right? If you run an application as service or as a detached process without a console and you request the output on the console then Envoy is misconfigured and it should fail.

I will try out these scenarios there and some service tests as well, when the service code is merged.

[sunjayBhatia]

These should be non-recoverable failures scenarios right?

Yeah good point, good enough to make sure we have good error messages etc., though that becomes a little funny with services and redirecting logs etc. 🤔

[sunjayBhatia]

I think we could also constrain this to just access log output paths as well? I don't see much advantage at all going through the above logic and allowing these sorts of paths for files we're watching for config updates etc.

[davinci26]

I think we could also constrain this to just access log output paths as well?

the change above implicitly only affects the access_log_manager since its the only component that is writing to file. As a result it is the only one that goes through the FileImpl.

When we are watching for config updates since this is a read operation we rely on fileReadToEnd which is on the InstanceImpl api.

Since this redirection are only for write and not for read I agree that they only make sense in FileImpl. The only place that I was planning to implement the translation is InstanceImpl is in the fileExists function.

[jmarantz]

I'm trying to understand the goal of this bug and accompanying PR.

Do you want to be able to specify that access logs go to stdout/stderr in a configuration file? Why would you want to do that? For debug only?

[sunjayBhatia]

I'm trying to understand the goal of this bug and accompanying PR.

Do you want to be able to specify that access logs go to stdout/stderr in a configuration file? Why would you want to do that? For debug only?

See

envoy/examples/cors/frontend/front-envoy.yaml

Line 18 in a4733f6

path: /dev/stdout

this issue and PR are to allow us to have parity with Linux/macos and enable using the file access logger to output to stdout/error

e.g. in a containerized deployment you often have stdout/error captured by your container runtime and sent off to log aggregation etc. (rather than have a grpc service envoy sends access logs to) without a change of this sort we don’t really have a way to specify that we want this on windows AND you would have to know what platform you’re sending updates to if you are configuring access logging dynamically over xDS if we restricted it to platform specific paths

[jmarantz]

I'm still trying to wrap my arms around why this is a good thing, but it would be possible to have special platform-independent syntax for naming stdout/stderr, which is more like this PR. I'd like to ask other @envoyproxy/maintainers if they think it's a great plan to have a special name you could put into any path in a config which is interpreted as stdout/stderr rather than being opened as a file system.

I'm still not understanding why containers capturing stdout is better than using gRPC to relay to your aggregator.

From an implementation perspective, on Posix I'd still just referencing the already-open stdin/stderr/cerr/cout streams rather than trying to open a new one. You can use OO inheritance to or something to avoid special-casing in the existing FileSystem::File implementations, I think, as you definitely wouldn't want to allow closing stdin/stdout from that interface.

[sunjayBhatia]

I'm still not understanding why containers capturing stdout is better than using gRPC to relay to your aggregator.

This is probably a separate discussion, I don't think theres any value judgement which is "better" just that file based access logging is a feature that exists in Envoy and is in use so to have parity across platforms we need a solution to do the equivalent logging to stdout/err on Windows.

it would be possible to have special platform-independent syntax for naming stdout/stderr, which is more like this PR. I'd like to ask other @envoyproxy/maintainers if they think it's a great plan to have a special name you could put into any path in a config which is interpreted as stdout/stderr rather than being opened as a file system.

Yeah, this is a good question to raise again. We hadn't come to any consensus on what that name would be or gotten much input from outside so the PR went forward to emulate paths on Windows.

From an implementation perspective, on Posix I'd still just referencing the already-open stdin/stderr/cerr/cout streams rather than trying to open a new one. You can use OO inheritance to or something to avoid special-casing in the existing FileSystem::File implementations, I think, as you definitely wouldn't want to allow closing stdin/stdout from that interface.

This is valid, but might be something to do in a separate change targeting non-Windows since we're aiming to only affect Windows codepaths (and the Windows implementation doesn't open new handles to stdout/err, but maybe could use the builtin File *stdout/err instead of what we have today?)

cc @davinci26

[jmarantz]

@ggreenway suggested:

I think I'd rather have another field that is an enum, with values for stdout, stderr, and any other special files, and then make the validation allow either a path or this enum (can't retroactively make it oneof)

I like this better. The advantage is we wouldn't have "special" names for files to trigger different semantics. The main drawback is that we'd have to enumerate the places where we'd want this behavior (maybe just access logs) and add new APIs and code to handle that there.

[davinci26]

I think I'd rather have another field that is an enum, with values for stdout, stderr, and any other special files, and then make the validation allow either a path or this enum (can't retroactively make it oneof)

I'm still trying to wrap my arms around why this is a good thing, but it would be possible to have special platform-independent syntax for naming stdout/stderr, which is more like this PR. I'd like to ask other @envoyproxy/maintainers if they think it's a great plan to have a special name you could put into any path in a config which is interpreted as stdout/stderr rather than being opened as a file system.

There was a proposal regarding that but IMO I am not sure if bending windows to the UNIX standard makes more sense than introducing an envoy standard.

The reason for that is code samples/examples. We would like to have the work out of the box for Windows and if this is the case we would need to replace /dev/stdout references with envoy/stdout (name is just a sample to explain what I mean). From a user perspective the name envoy/stdout is going to add more mental friction.

I think I'd rather have another field that is an enum, with values for stdout, stderr, and any other special files, and then make the validation allow either a path or this enum (can't retroactively make it oneof)

This one works but then should we have the examples use this enum instead of the access_log path?

[jmarantz]

There's not really a Unix standard in play here. It's just a software engineering question of not having a pathname interpreted in a special way, so rather than inferring different semantics from the name, we explicitly put it in the enum.

I didn't totally understand the suggestion about the examples or the references to envoy/stdout, but if we are trying to put the access-log on stdout we'd have something like:

   access_log_destination: stdout

and not specify the path at all. Then if you want it in a file, you could say

  access_log_destination: file
  path: /var/logs/access.log

but the default value of access_log_destination would be file and so you could omit that, which keeps existing configs working, that just specify path.

[davinci26]

I think I did not make my concern clear.

If we go through we the enum approach then we should change all the samples in the current project that use path: /dev/stdout to use access_log_destination: stdout. I want to make sure that we are all ok with that.

[jmarantz]

The goal is to have the config files in the examples directory be platform-agnostic, is that right? So we need to change all those examples. That seems reasonable to me, no?

[davinci26]

Just making sure that we are all in agreement since examples are part of the public facing documentation for our customers (envoy users) and we will be using a new option.

Closed the previous PR so I can open a new with the new approach of an ENUM.

[wrowe]

We might have to do some validation when in service mode that a user doesn't try to use stdout/err as the console functions/GetStdHandle will likely fail, something to test out at least

Just to clarify, that shouldn't be (ultimately) necessary. Until any global error log configuration setting modifies the output handle, my implementation of stderr -> Windows Event Logger for service controlled binaries just created a pipe to capture stderr and drop it as windows events line-by-line. Easily expanded to do this for stdout too (in a different event priority, of course.) The trouble I've always run into is that although most errors go through the application's error logging schema, there will always be exceptions from dependency libraries choosing to write unexpected errors to stderr. Without piping these to the event logger, those get dropped and result in extra confusion.

[sunjayBhatia]

yeah, a separate issue for general envoy logs but we could implement something for Service mode where if you don't specify the --log-file parameter logs are output to the windows event log (maybe this isn't the default but an additional flag option instead, idk?)

we could honestly also even add an extension to output the http/tcp Access Logs to the Windows event log as well (analogous to the file and grpc log mechanisms)

[wrowe]

we could honestly also even add an extension to output the http/tcp Access Logs to the Windows event log as well (analogous to the file and grpc log mechanisms)

I think that's a great feature to roll out in 1.18.0 - with much more granularity (explicit grouping by type, id, source)!

It wouldn't replace the stderr level logging which we desperately need during startup though, since that extension would be instantiated much later.

For 1.17.0 we should be ok with a blend of --log-file, treating the SCM invocation as "experimental", and focusing on readiness of invocation as a console application under whatever framework the user chooses.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[wrowe]

Addressed in PR #15202, closing