Optimize dynamic loading of IaaS Credentials using credential file ti…

…mestamps (#670)

* Dynamic access credential rotation unit tests written for all providers

* Unit tests for GetSnapstoreSecretModifiedTime() use idiomatic `time` functions

* Unit tests reworked to fix concourse build

* Changes to test GinkgoT and os temp directories for unit tests for dynamic loading

* Added a direct os.Stat to ensure the correct time is being fetched

* Workaround to change the modification time of a credential file in the unit test
* Writing to the file does not change the modification time on the concourse machine.
  This is worked around by recreating the credential file.
* Better logging during the tests for easier debugging.

* Changed the directory used by the dynamic credential loading unit tests
* The directory is set to /test/credential to store the files.
* Cleaner logging.

* Added test to verify concourse's time modification os call

* Added a time.Sleep() before modifying the file.

* Replaced random file deletion test with file deletion of each possible credential file

* Unit tests now use a sleep to fix issues on concourse, .ci/unit_test restored so all tests run.

* Addressing review comments for units tests of dynamic credential loading 1

* Dynamic credential loading unit tests: removed file creation and updation logs

* `GetSnapstoreSecretModifiedTime` unit tests rewritten using `GinkgoT().TempDir()` and structs for cleaner tests.
* Only Ginkgo provided temporary directories are used for the unit tests.
* Structs defined where each instance holds all relevant information for the test.
* Same tests cover both directory flow and JSON flow.

* Addressing review comments
* Changed a few function definition comments
* Changed the verb to `%q` in Ginkgo nodes' strings for better string formatting

.ci/integration_test

@@ -155,6 +155,16 @@ EOF
 [default]
 region = $3
 EOF
+  temp_dir=$(mktemp -d)
+  credentials_file="${temp_dir}/credentials.json"
+  cat <<EOF >"${credentials_file}"
+{
+  "accessKeyID": "$1",
+  "secretAccessKey": "$2",
+  "region": "$3"
+}
+EOF
+  export AWS_APPLICATION_CREDENTIALS_JSON="${credentials_file}"
 }
 
 function create_aws_secret() {

pkg/snapshot/snapshotter/snapshotter.go

@@ -54,8 +54,7 @@ const (
 )
 
 var (
-	emptyStruct   struct{}
-	snapstoreHash = make(map[string]interface{})
+	emptyStruct struct{}
 )
 
 // event is wrapper over etcd event to keep track of time of event
@@ -83,31 +82,32 @@ func NewSnapshotterConfig() *brtypes.SnapshotterConfig {
 
 // Snapshotter is a struct for etcd snapshot taker
 type Snapshotter struct {
-	logger               *logrus.Entry
-	etcdConnectionConfig *brtypes.EtcdConnectionConfig
-	store                brtypes.SnapStore
-	config               *brtypes.SnapshotterConfig
-	compressionConfig    *compressor.CompressionConfig
-	healthConfig         *brtypes.HealthConfig
-	schedule             cron.Schedule
-	PrevSnapshot         *brtypes.Snapshot
-	PrevFullSnapshot     *brtypes.Snapshot
-	PrevDeltaSnapshots   brtypes.SnapList
-	fullSnapshotReqCh    chan bool
-	deltaSnapshotReqCh   chan struct{}
-	fullSnapshotAckCh    chan result
-	deltaSnapshotAckCh   chan result
-	fullSnapshotTimer    *time.Timer
-	deltaSnapshotTimer   *time.Timer
-	events               []byte
-	watchCh              clientv3.WatchChan
-	etcdWatchClient      *clientv3.Watcher
-	cancelWatch          context.CancelFunc
-	SsrStateMutex        *sync.Mutex
-	SsrState             brtypes.SnapshotterState
-	lastEventRevision    int64
-	K8sClientset         client.Client
-	snapstoreConfig      *brtypes.SnapstoreConfig
+	logger                 *logrus.Entry
+	etcdConnectionConfig   *brtypes.EtcdConnectionConfig
+	store                  brtypes.SnapStore
+	config                 *brtypes.SnapshotterConfig
+	compressionConfig      *compressor.CompressionConfig
+	healthConfig           *brtypes.HealthConfig
+	schedule               cron.Schedule
+	PrevSnapshot           *brtypes.Snapshot
+	PrevFullSnapshot       *brtypes.Snapshot
+	PrevDeltaSnapshots     brtypes.SnapList
+	fullSnapshotReqCh      chan bool
+	deltaSnapshotReqCh     chan struct{}
+	fullSnapshotAckCh      chan result
+	deltaSnapshotAckCh     chan result
+	fullSnapshotTimer      *time.Timer
+	deltaSnapshotTimer     *time.Timer
+	events                 []byte
+	watchCh                clientv3.WatchChan
+	etcdWatchClient        *clientv3.Watcher
+	cancelWatch            context.CancelFunc
+	SsrStateMutex          *sync.Mutex
+	SsrState               brtypes.SnapshotterState
+	lastEventRevision      int64
+	K8sClientset           client.Client
+	snapstoreConfig        *brtypes.SnapstoreConfig
+	lastSecretModifiedTime time.Time
 }
 
 // NewSnapshotter returns the snapshotter object.
@@ -310,13 +310,19 @@ func (ssr *Snapshotter) takeFullSnapshot(isFinal bool) (*brtypes.Snapshot, error
 	// close previous watch and client.
 	ssr.closeEtcdClient()
 
-	var err error
-
-	// Update the snapstore object before taking every full snapshot
-	// Refer: https://github.com/gardener/etcd-backup-restore/issues/422
-	ssr.store, err = snapstore.GetSnapstore(ssr.snapstoreConfig)
+	// Update the snapstore object before taking a full snapshot if the credentials have changed
+	// Refer: https://github.com/gardener/etcd-backup-restore/issues/449
+	hasSecretUpdated, err := ssr.hasSnapStoreSecretUpdated()
 	if err != nil {
-		return nil, fmt.Errorf("failed to create snapstore from configured storage provider: %v", err)
+		return nil, fmt.Errorf("error checking if the credentials were updated %v", err)
+	}
+	if hasSecretUpdated {
+		var err error
+		ssr.store, err = snapstore.GetSnapstore(ssr.snapstoreConfig)
+		if err != nil {
+			return nil, fmt.Errorf("failed to create snapstore from configured storage provider: %v", err)
+		}
+		ssr.logger.Info("Updated the snapstore object with new credentials")
 	}
 
 	clientFactory := etcdutil.NewFactory(*ssr.etcdConnectionConfig)
@@ -441,17 +447,19 @@ func (ssr *Snapshotter) TakeDeltaSnapshot() (*brtypes.Snapshot, error) {
 	}
 	ssr.events = append(ssr.events, byte(']'))
 
-	isSecretUpdated := ssr.checkSnapstoreSecretUpdate()
-	if isSecretUpdated {
+	// Update the snapstore object before taking a delta snapshot if the credentials have changed
+	// Refer: https://github.com/gardener/etcd-backup-restore/issues/449
+	hasSecretUpdated, err := ssr.hasSnapStoreSecretUpdated()
+	if err != nil {
+		return nil, fmt.Errorf("error checking if the credentials were updated %v", err)
+	}
+	if hasSecretUpdated {
 		var err error
-
-		// Update the snapstore object before taking every delta snapshot
-		// Refer: https://github.com/gardener/etcd-backup-restore/issues/422
 		ssr.store, err = snapstore.GetSnapstore(ssr.snapstoreConfig)
 		if err != nil {
 			return nil, fmt.Errorf("failed to create snapstore from configured storage provider: %v", err)
 		}
-		ssr.logger.Info("updated the snapstore object with new credentials")
+		ssr.logger.Info("Updated the snapstore object with new credentials")
 	}
 
 	// compressionSuffix is useful in backward compatibility(restoring from uncompressed snapshots).
@@ -736,20 +744,22 @@ func (ssr *Snapshotter) resetFullSnapshotTimer() error {
 	return nil
 }
 
-func (ssr *Snapshotter) checkSnapstoreSecretUpdate() bool {
-	ssr.logger.Debug("checking the hash of snapstore secret...")
-	newSnapstoreSecretHash, err := snapstore.GetSnapstoreSecretHash(ssr.snapstoreConfig)
+// hasSnapStoreSecretUpdated checks if the snapstore secret has been updated
+func (ssr *Snapshotter) hasSnapStoreSecretUpdated() (bool, error) {
+	ssr.logger.Debug("checking the timestamp of snapstore secret...")
+	newSecretModifiedTime, err := snapstore.GetSnapstoreSecretModifiedTime(ssr.snapstoreConfig.Provider)
 	if err != nil {
-		return true
+		return false, fmt.Errorf("error checking the modification time of the access credentials  %v", err)
 	}
 
-	if snapstoreHash[ssr.snapstoreConfig.Provider] == newSnapstoreSecretHash {
-		return false
+	// the secret has not been modified
+	if !newSecretModifiedTime.After(ssr.lastSecretModifiedTime) {
+		return false, nil
 	}
 
-	//update the map with latest newSnapstoreHash
-	snapstoreHash[ssr.snapstoreConfig.Provider] = newSnapstoreSecretHash
-	return true
+	// update the previous modification time with the latest modification time
+	ssr.lastSecretModifiedTime = newSecretModifiedTime
+	return true, nil
 }
 
 // IsFullSnapshotRequiredAtStartup checks whether to take a full snapshot or not during the startup of backup-restore.

pkg/snapstore/abs_snapstore.go

@@ -23,9 +23,11 @@ import (
 	"net/url"
 	"os"
 	"path"
+	"path/filepath"
 	"sort"
 	"strings"
 	"sync"
+	"time"
 
 	"github.com/Azure/azure-storage-blob-go/azblob"
 	"github.com/sirupsen/logrus"
@@ -34,8 +36,8 @@ import (
 )
 
 const (
-	absCredentialFile     = "AZURE_APPLICATION_CREDENTIALS"
-	absCredentialJSONFile = "AZURE_APPLICATION_CREDENTIALS_JSON"
+	absCredentialDirectory = "AZURE_APPLICATION_CREDENTIALS"
+	absCredentialJSONFile  = "AZURE_APPLICATION_CREDENTIALS_JSON"
 )
 
 // ABSSnapStore is an ABS backed snapstore.
@@ -90,7 +92,7 @@ func getCredentials(prefixString string) (string, string, error) {
 		return credentials.StorageAccount, credentials.SecretKey, nil
 	}
 
-	if dir, isSet := os.LookupEnv(prefixString + absCredentialFile); isSet {
+	if dir, isSet := os.LookupEnv(prefixString + absCredentialDirectory); isSet {
 		credentials, err := readABSCredentialFiles(dir)
 		if err != nil {
 			return "", "", fmt.Errorf("error getting credentials from %v dir", dir)
@@ -347,30 +349,31 @@ func (a *ABSSnapStore) Delete(snap brtypes.Snapshot) error {
 	return nil
 }
 
-// ABSSnapStoreHash calculates and returns the hash of azure object storage snapstore secret.
-func ABSSnapStoreHash(config *brtypes.SnapstoreConfig) (string, error) {
-	if dir, isSet := os.LookupEnv(absCredentialFile); isSet {
-		absConfig, err := readABSCredentialFiles(dir)
+// GetABSCredentialsLastModifiedTime returns the latest modification timestamp of the ABS credential file(s)
+func GetABSCredentialsLastModifiedTime() (time.Time, error) {
+	if dir, isSet := os.LookupEnv(absCredentialDirectory); isSet {
+		// credential files which are essential for creating the snapstore
+		credentialFiles := []string{"storageKey", "storageAccount"}
+		for i := range credentialFiles {
+			credentialFiles[i] = filepath.Join(dir, credentialFiles[i])
+		}
+		absTimeStamp, err := getLatestCredentialsModifiedTime(credentialFiles)
 		if err != nil {
-			return "", fmt.Errorf("error getting credentials from %v directory", dir)
+			return time.Time{}, fmt.Errorf("failed to get ABS credential timestamp from the directory %v with error: %v", dir, err)
 		}
-		return getABSHash(absConfig), nil
+		return absTimeStamp, nil
 	}
 
 	if filename, isSet := os.LookupEnv(absCredentialJSONFile); isSet {
-		absConfig, err := readABSCredentialsJSON(filename)
+		credentialFiles := []string{filename}
+		absTimeStamp, err := getLatestCredentialsModifiedTime(credentialFiles)
 		if err != nil {
-			return "", fmt.Errorf("error getting credentials using %v file", filename)
+			return time.Time{}, fmt.Errorf("failed to fetch file information of the ABS JSON credential file %v with error: %v", filename, err)
 		}
-		return getABSHash(absConfig), nil
+		return absTimeStamp, nil
 	}
 
-	return "", nil
-}
-
-func getABSHash(config *absCredentials) string {
-	data := fmt.Sprintf("%s%s", config.SecretKey, config.StorageAccount)
-	return getHash(data)
+	return time.Time{}, fmt.Errorf("no environment variable set for the ABS credential file")
 }
 
 func isABSConfigEmpty(config *absCredentials) error {

pkg/snapstore/gcs_snapstore.go

@@ -24,6 +24,7 @@ import (
 	"sort"
 	"strings"
 	"sync"
+	"time"
 
 	"cloud.google.com/go/storage"
 	stiface "github.com/gardener/etcd-backup-restore/pkg/snapstore/gcs"
@@ -259,24 +260,16 @@ func (s *GCSSnapStore) Delete(snap brtypes.Snapshot) error {
 	return s.client.Bucket(s.bucket).Object(objectName).Delete(context.TODO())
 }
 
-// GCSSnapStoreHash calculates and returns the hash of GCS snapstore secret.
-func GCSSnapStoreHash(config *brtypes.SnapstoreConfig) (string, error) {
-	if _, isSet := os.LookupEnv(storeCredentials); isSet {
-		if file := os.Getenv(storeCredentials); file != "" {
-			credjson, err := readGCSCredentialsFile(file)
-			if err != nil {
-				return "", fmt.Errorf("error getting credentials from %v ", file)
-			}
-			return getHash(credjson), nil
+// GetGCSCredentialsLastModifiedTime returns the latest modification timestamp of the GCS credential file
+func GetGCSCredentialsLastModifiedTime() (time.Time, error) {
+	if filename, isSet := os.LookupEnv(storeCredentials); isSet {
+		credentialFiles := []string{filename}
+		gcsTimeStamp, err := getLatestCredentialsModifiedTime(credentialFiles)
+		if err != nil {
+			return time.Time{}, fmt.Errorf("failed to fetch file information of the GCS JSON credential file %v with error: %v", filename, err)
 		}
+		return gcsTimeStamp, nil
 	}
-	return "", nil
-}
 
-func readGCSCredentialsFile(filename string) ([]byte, error) {
-	b, err := os.ReadFile(filename)
-	if err != nil {
-		return nil, err
-	}
-	return b, nil
+	return time.Time{}, fmt.Errorf("no environment variable set for the GCS credential file")
 }

pkg/snapstore/ocs_s3_snapstore.go

@@ -18,14 +18,16 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
+	"path/filepath"
 	"strconv"
+	"time"
 
 	brtypes "github.com/gardener/etcd-backup-restore/pkg/types"
 )
 
 const (
-	ocsCredentialFile         string = "OPENSHIFT_APPLICATION_CREDENTIALS"
-	ocsCredentialFileJSONFile string = "OPENSHIFT_APPLICATION_CREDENTIALS_JSON"
+	ocsCredentialDirectory string = "OPENSHIFT_APPLICATION_CREDENTIALS"
+	ocsCredentialJSONFile  string = "OPENSHIFT_APPLICATION_CREDENTIALS_JSON"
 )
 
 type ocsAuthOptions struct {
@@ -48,15 +50,15 @@ func NewOCSSnapStore(config *brtypes.SnapstoreConfig) (*S3SnapStore, error) {
 }
 
 func getOCSAuthOptions(prefix string) (*ocsAuthOptions, error) {
-	if filename, isSet := os.LookupEnv(prefix + ocsCredentialFileJSONFile); isSet {
+	if filename, isSet := os.LookupEnv(prefix + ocsCredentialJSONFile); isSet {
 		ao, err := readOCSCredentialsJSON(filename)
 		if err != nil {
 			return nil, fmt.Errorf("error getting credentials using %v file", filename)
 		}
 		return ao, nil
 	}
 
-	if dir, isSet := os.LookupEnv(prefix + ocsCredentialFile); isSet {
+	if dir, isSet := os.LookupEnv(prefix + ocsCredentialDirectory); isSet {
 		ao, err := readOCSCredentialFromDir(dir)
 		if err != nil {
 			return nil, fmt.Errorf("error getting credentials from %v directory", dir)
@@ -177,17 +179,29 @@ func isOCSConfigEmpty(config ocsAuthOptions) error {
 	return fmt.Errorf("ocs s3 credentials: region, secretAccessKey, endpoint or accessKeyID is missing")
 }
 
-// OCSSnapStoreHash calculates and returns the hash of OCS snapstore secret.
-func OCSSnapStoreHash(config *brtypes.SnapstoreConfig) (string, error) {
-	ao, err := getOCSAuthOptions("")
-	if err != nil {
-		return "", err
+// GetOCSCredentialsLastModifiedTime returns the latest modification timestamp of the OCS credential file(s)
+func GetOCSCredentialsLastModifiedTime() (time.Time, error) {
+	if dir, isSet := os.LookupEnv(ocsCredentialDirectory); isSet {
+		// credential files which are essential for creating the snapstore
+		credentialFiles := []string{"accessKeyID", "region", "endpoint", "secretAccessKey"}
+		for i := range credentialFiles {
+			credentialFiles[i] = filepath.Join(dir, credentialFiles[i])
+		}
+		ocsTimeStamp, err := getLatestCredentialsModifiedTime(credentialFiles)
+		if err != nil {
+			return time.Time{}, fmt.Errorf("failed to get OCS credential timestamp from the directory %v with error: %v", dir, err)
+		}
+		return ocsTimeStamp, nil
 	}
 
-	return getOCSHash(ao), nil
-}
+	if filename, isSet := os.LookupEnv(ocsCredentialJSONFile); isSet {
+		credentialFiles := []string{filename}
+		ocsTimeStamp, err := getLatestCredentialsModifiedTime(credentialFiles)
+		if err != nil {
+			return time.Time{}, fmt.Errorf("failed to fetch file information of the OCS JSON credential file %v with error: %v", filename, err)
+		}
+		return ocsTimeStamp, nil
+	}
 
-func getOCSHash(config *ocsAuthOptions) string {
-	data := fmt.Sprintf("%s%s%s%s", config.AccessKeyID, config.Endpoint, config.Region, config.SecretAccessKey)
-	return getHash(data)
+	return time.Time{}, fmt.Errorf("no environment variable set for the OCS credential file")
 }