From 3aae7b878369c8890c8084ffa6505538dfd876d1 Mon Sep 17 00:00:00 2001
From: iyear <ljyngup@gmail.com>
Date: Wed, 19 Oct 2022 10:11:45 +0800
Subject: [PATCH] Fix incorrect defer usage

Problem:
Using defer inside a loop can lead to resource leaks

Solution:
Judge newer file in the separate function

Signed-off-by: iyear <ljyngup@gmail.com>
---
 pkg/cluster/bootstrap.go | 65 ++++++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 25 deletions(-)

diff --git a/pkg/cluster/bootstrap.go b/pkg/cluster/bootstrap.go
index cf6dc8688e..86fdc2b7fa 100644
--- a/pkg/cluster/bootstrap.go
+++ b/pkg/cluster/bootstrap.go
@@ -328,35 +328,15 @@ func (c *Cluster) ReconcileBootstrapData(ctx context.Context, buf io.ReadSeeker,
 		}
 		logrus.Debugf("Reconciling %s at '%s'", pathKey, path)
 
-		f, err := os.Open(path)
+		updated, newer, err := isNewerFile(path, fileData)
 		if err != nil {
-			if os.IsNotExist(err) {
-				logrus.Warn(path + " doesn't exist. continuing...")
-				updateDisk = true
-				continue
-			}
-			return errors.Wrapf(err, "reconcile failed to open %s", pathKey)
+			return errors.Wrapf(err, "failed to get update status of %s", pathKey)
 		}
-		defer f.Close()
-
-		fData, err := io.ReadAll(f)
-		if err != nil {
-			return errors.Wrapf(err, "reconcile failed to read %s", pathKey)
+		if newer {
+			newerOnDisk = append(newerOnDisk, path)
 		}
 
-		if !bytes.Equal(fileData.Content, fData) {
-			updateDisk = true
-			info, err := f.Stat()
-			if err != nil {
-				return errors.Wrapf(err, "reconcile failed to stat %s", pathKey)
-			}
-
-			if info.ModTime().Unix()-fileData.Timestamp.Unix() >= systemTimeSkew {
-				newerOnDisk = append(newerOnDisk, path)
-			} else {
-				logrus.Warn(path + " will be updated from the datastore.")
-			}
-		}
+		updateDisk = updateDisk || updated
 	}
 
 	if c.config.ClusterReset {
@@ -384,6 +364,41 @@ func (c *Cluster) ReconcileBootstrapData(ctx context.Context, buf io.ReadSeeker,
 	return nil
 }
 
+// isNewerFile compares the file from disk and datastore, and returns
+// update status.
+func isNewerFile(path string, file bootstrap.File) (updated bool, newerOnDisk bool, _ error) {
+	f, err := os.Open(path)
+	if err != nil {
+		if os.IsNotExist(err) {
+			logrus.Warn(path + " doesn't exist. continuing...")
+			return true, false, nil
+		}
+		return false, false, errors.Wrapf(err, "reconcile failed to open")
+	}
+	defer f.Close()
+
+	data, err := io.ReadAll(f)
+	if err != nil {
+		return false, false, errors.Wrapf(err, "reconcile failed to read")
+	}
+
+	if bytes.Equal(file.Content, data) {
+		return false, false, nil
+	}
+
+	info, err := f.Stat()
+	if err != nil {
+		return false, false, errors.Wrapf(err, "reconcile failed to stat")
+	}
+
+	if info.ModTime().Unix()-file.Timestamp.Unix() >= systemTimeSkew {
+		return true, true, nil
+	}
+
+	logrus.Warn(path + " will be updated from the datastore.")
+	return true, false, nil
+}
+
 // httpBootstrap retrieves bootstrap data (certs and keys, etc) from the remote server via HTTP
 // and loads it into the ControlRuntimeBootstrap struct. Unlike the storage bootstrap path,
 // this data does not need to be decrypted since it is generated on-demand by an existing server.