-
Notifications
You must be signed in to change notification settings - Fork 55
fix: handle broken pipe error while rapidly reading from secrets mount #503
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
I left a dev session running on this branch for the past ~24 hrs and it's still running so I think this is indeed a viable fix! I'm still confused what vite is doing, as it seems the endless Secrets mount opened by reader eventually stop... maybe a separate issue to file with vite. |
mikesellitto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I tested locally with the following test which fails on master and succeeds with the proposed changes (feel free to commit this if you care to).
@seslattery thoughts?
index bccb47c..bdd2e78 100644
--- a/pkg/controllers/secrets_test.go
+++ b/pkg/controllers/secrets_test.go
@@ -17,9 +17,13 @@ limitations under the License.
package controllers
import (
+ "os"
+ "path/filepath"
"strings"
"testing"
+ "time"
+ "github.com/DopplerHQ/cli/pkg/utils"
"github.com/stretchr/testify/assert"
)
@@ -106,3 +110,61 @@ func TestSecretsToBytes(t *testing.T) {
t.Errorf("Unable to convert secrets to byte array in %s format", format)
}
}
+
+func TestMountSecrets(t *testing.T) {
+ if !utils.SupportsNamedPipes {
+ t.Skip("Named pipes not supported on this platform")
+ }
+
+ secrets := []byte(`{"SECRET_KEY":"secret_value"}`)
+ mountPath := filepath.Join(t.TempDir(), "secrets_mount")
+
+ path, cleanup, err := MountSecrets(secrets, mountPath, 1)
+ if !err.IsNil() {
+ t.Fatalf("MountSecrets failed: %v", err.Err)
+ }
+ defer cleanup()
+
+ time.Sleep(50 * time.Millisecond)
+
+ content, readErr := os.ReadFile(path)
+ assert.NoError(t, readErr)
+ assert.Equal(t, string(secrets), string(content))
+}
+
+func TestMountSecretsBrokenPipe(t *testing.T) {
+ if !utils.SupportsNamedPipes {
+ t.Skip("Named pipes not supported on this platform")
+ }
+
+ // large data to exceed pipe buffer and trigger EPIPE when reader closes early
+ secrets := make([]byte, 256*1024)
+ for i := range secrets {
+ secrets[i] = byte('A' + (i % 26))
+ }
+ mountPath := filepath.Join(t.TempDir(), "secrets_mount_epipe")
+
+ path, cleanup, err := MountSecrets(secrets, mountPath, 0)
+ if !err.IsNil() {
+ t.Fatalf("MountSecrets failed: %v", err.Err)
+ }
+ defer cleanup()
+
+ time.Sleep(50 * time.Millisecond)
+
+ for i := 0; i < 10; i++ {
+ f, openErr := os.OpenFile(path, os.O_RDONLY, 0)
+ if openErr != nil {
+ continue
+ }
+ f.Read(make([]byte, 1))
+ f.Close()
+ time.Sleep(5 * time.Millisecond)
+ }
+
+ time.Sleep(100 * time.Millisecond)
+
+ content, readErr := os.ReadFile(path)
+ assert.NoError(t, readErr, "mount should survive broken pipe")
+ assert.Equal(t, secrets, content)
+}
potentially fixes #502
I've been running this without the crash for about half an hour, so I think it solves it. Hard to tell, because I'm still getting spammed by the Secrets mount opened by reader message, but I don't think this would introduce any further breaking behavior either.