From 5e140430711c2b2b309eb6d61c45e9022c1c484f Mon Sep 17 00:00:00 2001 From: Thomas Guettler Date: Mon, 16 Dec 2024 16:15:03 +0100 Subject: [PATCH] robot test was flaky, fixed it. --- hcloud/cloud.go | 2 +- internal/hotreload/hotreload.go | 52 ++++++++++++++++------ internal/robot/client/cache/client.go | 2 +- internal/robot/client/cache/client_test.go | 31 ++++++++++--- 4 files changed, 66 insertions(+), 21 deletions(-) diff --git a/hcloud/cloud.go b/hcloud/cloud.go index 84995974..02fb9ae8 100644 --- a/hcloud/cloud.go +++ b/hcloud/cloud.go @@ -105,7 +105,7 @@ func (lt *LoggingTransport) RoundTrip(req *http.Request) (resp *http.Response, e func newHcloudClient(rootDir string) (*hcloud.Client, error) { secretDir := filepath.Join(rootDir, "etc", "hetzner-secret") - token, err := hotreload.ReadHcloudCredentialsFromDirectory(secretDir) + token, err := hotreload.GetInitialHcloudCredentialsFromDirectory(secretDir) if err != nil { klog.V(1).Infof("reading Hetzner Cloud token from directory failed. Will try env var: %s", err.Error()) token = os.Getenv(hcloudTokenENVVar) diff --git a/internal/hotreload/hotreload.go b/internal/hotreload/hotreload.go index f9b4f72b..c0996a37 100644 --- a/internal/hotreload/hotreload.go +++ b/internal/hotreload/hotreload.go @@ -16,18 +16,23 @@ import ( ) var ( + // fsnotify creates several events for a single update of a mounted secret. + // To avoid multiple reloads, we store the old values and only reload when + // the values have changed. oldRobotUser string oldRobotPassword string + oldHcloudToken string + // RobotReloadCounter gets incremented when the credentials get reloaded. // Mosty used for testing. RobotReloadCounter uint64 - robotMutex sync.Mutex - oldHcloudToken string // HcloudTokenReloadCounter gets incremented when the credentials get reloaded. // Mosty used for testing. HcloudTokenReloadCounter uint64 - hcloudMutex sync.Mutex + + hcloudMutex sync.Mutex + robotMutex sync.Mutex ) // Watch the mounted secrets. Reload the credentials, when the files get updated. The robotClient can be nil. @@ -48,19 +53,19 @@ func Watch(hetznerSecretDirectory string, hcloudClient *hcloud.Client, robotClie var err error switch baseName { case "robot-user": - err = LoadRobotCredentials(hetznerSecretDirectory, robotClient) + err = loadRobotCredentials(hetznerSecretDirectory, robotClient) case "robot-password": - err = LoadRobotCredentials(hetznerSecretDirectory, robotClient) + err = loadRobotCredentials(hetznerSecretDirectory, robotClient) case "hcloud": - err = LoadHcloudCredentials(hetznerSecretDirectory, hcloudClient) + err = loadHcloudCredentials(hetznerSecretDirectory, hcloudClient) case "..data": // The files (for example hcloud) are symlinks to ..data/. For example to ../data/hcloud // This means the files/symlinks don't change. When the secrets get changed, then // a new ..data directory gets created. This is done by Kubernetes to make the // update atomic. - err = LoadHcloudCredentials(hetznerSecretDirectory, hcloudClient) + err = loadHcloudCredentials(hetznerSecretDirectory, hcloudClient) if robotClient != nil { - err = errors.Join(err, LoadRobotCredentials(hetznerSecretDirectory, robotClient)) + err = errors.Join(err, loadRobotCredentials(hetznerSecretDirectory, robotClient)) } default: klog.Infof("Ignoring fsnotify event for %q: %s", baseName, event.String()) @@ -95,10 +100,10 @@ func isValidEvent(event fsnotify.Event) bool { return false } -func LoadRobotCredentials(hetznerSecretDirectory string, robotClient robotclient.Client) error { +func loadRobotCredentials(hetznerSecretDirectory string, robotClient robotclient.Client) error { robotMutex.Lock() defer robotMutex.Unlock() - username, password, err := ReadRobotCredentialsFromDirectory(hetznerSecretDirectory) + username, password, err := readRobotCredentials(hetznerSecretDirectory) if err != nil { return fmt.Errorf("reading robot credentials from secret: %w", err) } @@ -116,7 +121,17 @@ func LoadRobotCredentials(hetznerSecretDirectory string, robotClient robotclient return nil } -func ReadRobotCredentialsFromDirectory(hetznerSecretDirectory string) (username, password string, err error) { +func GetInitialRobotCredentials(hetznerSecretDirectory string) (username, password string, err error) { + u, p, err := readRobotCredentials(hetznerSecretDirectory) + if err != nil { + return "", "", fmt.Errorf("readRobotCredentials: %w", err) + } + oldRobotUser = u + oldRobotPassword = p + return u, p, nil +} + +func readRobotCredentials(hetznerSecretDirectory string) (username, password string, err error) { robotUserNameFile := filepath.Join(hetznerSecretDirectory, "robot-user") robotPasswordFile := filepath.Join(hetznerSecretDirectory, "robot-password") u, err := os.ReadFile(robotUserNameFile) @@ -130,11 +145,11 @@ func ReadRobotCredentialsFromDirectory(hetznerSecretDirectory string) (username, return strings.TrimSpace(string(u)), strings.TrimSpace(string(p)), nil } -func LoadHcloudCredentials(hetznerSecretDirectory string, hcloudClient *hcloud.Client) error { +func loadHcloudCredentials(hetznerSecretDirectory string, hcloudClient *hcloud.Client) error { hcloudMutex.Lock() defer hcloudMutex.Unlock() op := "hcloud/updateHcloudToken" - token, err := ReadHcloudCredentialsFromDirectory(hetznerSecretDirectory) + token, err := readHcloudCredentials(hetznerSecretDirectory) if err != nil { return fmt.Errorf("%s: %w", op, err) } @@ -151,7 +166,16 @@ func LoadHcloudCredentials(hetznerSecretDirectory string, hcloudClient *hcloud.C return nil } -func ReadHcloudCredentialsFromDirectory(hetznerSecretDirectory string) (string, error) { +func GetInitialHcloudCredentialsFromDirectory(hetznerSecretDirectory string) (string, error) { + token, err := readHcloudCredentials(hetznerSecretDirectory) + if err != nil { + return "", fmt.Errorf("readHcloudCredentials: %w", err) + } + oldHcloudToken = token + return token, nil +} + +func readHcloudCredentials(hetznerSecretDirectory string) (string, error) { hcloudTokenFile := filepath.Join(hetznerSecretDirectory, "hcloud") data, err := os.ReadFile(hcloudTokenFile) if err != nil { diff --git a/internal/robot/client/cache/client.go b/internal/robot/client/cache/client.go index 6107c63f..b1ed34b1 100644 --- a/internal/robot/client/cache/client.go +++ b/internal/robot/client/cache/client.go @@ -72,7 +72,7 @@ func NewCachedRobotClient(rootDir string, httpClient *http.Client, baseURL strin return nil, nil } } else { - robotUser, robotPassword, err = hotreload.ReadRobotCredentialsFromDirectory(secretsDir) + robotUser, robotPassword, err = hotreload.GetInitialRobotCredentials(secretsDir) if err != nil { return nil, fmt.Errorf("%s: %w", op, err) } diff --git a/internal/robot/client/cache/client_test.go b/internal/robot/client/cache/client_test.go index 1e2386ce..ebf6cb06 100644 --- a/internal/robot/client/cache/client_test.go +++ b/internal/robot/client/cache/client_test.go @@ -8,6 +8,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "sync/atomic" "testing" "time" @@ -30,6 +31,12 @@ func Test_updateRobotCredentials(t *testing.T) { err = os.MkdirAll(filepath.Join(tmp, "etc", "hetzner-secret"), 0o755) require.NoError(t, err) + err = os.Symlink("..data/robot-user", filepath.Join(tmp, "etc", "hetzner-secret", "robot-user")) + require.NoError(t, err) + + err = os.Symlink("..data/robot-password", filepath.Join(tmp, "etc", "hetzner-secret", "robot-password")) + require.NoError(t, err) + err = writeCredentials(tmp, "my-robot-user", "my-robot-password") require.NoError(t, err) @@ -42,7 +49,7 @@ func Test_updateRobotCredentials(t *testing.T) { json.NewEncoder(w).Encode([]models.ServerResponse{ { Server: models.Server{ - ServerIP: "123.123.123.123", + ServerIP: "123.123.123.12", ServerIPv6Net: "2a01:f48:111:4221::", ServerNumber: 321, Name: "bm-server1", @@ -61,12 +68,15 @@ func Test_updateRobotCredentials(t *testing.T) { require.NoError(t, err) require.Len(t, servers, 1) - oldCount := hotreload.RobotReloadCounter + var oldCount uint64 + + atomic.LoadUint64(&hotreload.RobotReloadCounter) err = writeCredentials(tmp, "user2", "password2") require.NoError(t, err) start := time.Now() for { - if hotreload.RobotReloadCounter > oldCount { + // if hotreload.RobotReloadCounter > oldCount { + if atomic.LoadUint64(&hotreload.RobotReloadCounter) > oldCount { break } if time.Since(start) > time.Second*3 { @@ -82,16 +92,27 @@ func Test_updateRobotCredentials(t *testing.T) { } func writeCredentials(tmpDir, user, password string) error { - err := os.WriteFile(filepath.Join(tmpDir, "etc", "hetzner-secret", "robot-user"), + newDir := filepath.Join(tmpDir, "etc", "hetzner-secret", "..dataNew") + if err := os.MkdirAll(newDir, 0o700); err != nil { + return err + } + err := os.WriteFile(filepath.Join(newDir, "robot-user"), []byte(user), 0o600) if err != nil { return err } - err = os.WriteFile(filepath.Join(tmpDir, "etc", "hetzner-secret", "robot-password"), + err = os.WriteFile(filepath.Join(newDir, "robot-password"), []byte(password), 0o600) if err != nil { return err } + targetDir := filepath.Join(tmpDir, "etc", "hetzner-secret", "..data") + if err := os.RemoveAll(targetDir); err != nil { + return err + } + if err := os.Rename(newDir, targetDir); err != nil { + return err + } return nil }