NET-5186 Fix issue where consul-dataplane attempts to write to a read-only file system location#253
Conversation
nathancoleman
added
pr/no-changelog
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The net_bind_service capability is lost when running the envoy process from a consul-dataplane process that does not have the capability
…s.TempDir This amounts to using `envoy --config-yaml` -- which also accepts JSON -- instead of `envoy --config-path`
nathancoleman
force-pushed
the
setcap-envoy-only
branch
from
7a8a11b to
b3b442d
Compare
nathancoleman
force-pushed
the
setcap-envoy-only
branch
from
91b631e to
e96130d
Compare
| args := append( | ||
| []string{ | ||
| "--config-path", cfgPath, | ||
| "--config-yaml", cfgYaml, |
There was a problem hiding this comment.
Alongside the setcap changes applied in #238 , this is the meat of the change. Instead of writing the config to disk at os.TempDir() and then passing the filepath to the envoy process, the config yaml -- or JSON in our case -- is passed directly. JSON is allowed here according to the envoy docs.
| "resource_api_version": "V3" | ||
| }, | ||
| } | ||
| }`) |
There was a problem hiding this comment.
I aimed to test with some more robust json config vs. just "hello world" to ensure that the config retains its integrity when being passed through as a command line argument.
| require.Empty(t, envoyOut.String()) | ||
| require.Empty(t, envoyErr.String()) |
There was a problem hiding this comment.
If the process hit an error case, such as a missing flag expected by fake-envoy, you would never know about it in this test other than seeing the output file never being written.
With this change, we check for the output file with just an assert -- which allows the test to continue on -- and then make assertions about the output from Envoy so that errors become obvious when you're developing tests.
| require.Eventually(t, func() bool { | ||
| return p.cmd.Process.Signal(syscall.Signal(0)) == os.ErrProcessDone | ||
| err := p.cmd.Process.Signal(syscall.Signal(0)) | ||
| return errors.Is(err, os.ErrProcessDone) |
There was a problem hiding this comment.
This was something the linter suggested as an improvement in order to support wrapped errors
| set -e | ||
|
|
||
| config_path="" | ||
| config_yaml="" |
There was a problem hiding this comment.
This fake envoy process will just stash the value of the --config-yaml argument it receives and write it to a file later so that the calling test can make assertions on it. The changes in this test are just around handling --config-yaml instead of --config-path.
nathancoleman
changed the title
|
Applying the |
nathancoleman
added
backport/1.1
4 participants
Running
setcapon the consul-dataplane binary results in a process that cannot see theTMPDIRenvar intended to influence where the envoy bootstrap config is written to. This is due to a previously-uknown-to-us side effect of glibc, described here.As a result, it attempts to write to the
/tmpdirectory instead of/consul/connect-inject-- configured for injected sidecars here -- and gets aread-only file systemerror.We previously considered dropping
setcapfor theconsul-dataplanebinary; however, theenvoybinary that is forked from dataplane cannot itself have a capability that the consul-dataplane process does not have.Instead, I've opted to keep
setcap net_bind_servicefor both envoy and consul-dataplane. We can work around the fact thatTMPDIRis invisible toconsul-dataplaneby not writing to the file system at all and usingenvoy --config-yaml <yaml or json string>where we were previously usingenvoy --config-path <string>.