Repository: ossf/package-analysis
Pull request #763 (2023-06-13 03:24:32)

Add feature flags to make it easier to enable/disable features - specifically File Contents Saving
Writing file contents in production is causing issues and needs to be toggled off.

Since enabling/disabling is a common pattern, this PR adds a feature flag library to make it very easy to control features.

In particular this will allow better control over production issues.


This change also updates prod configs to set the flag. Also introduces some logging refactor code too.


Patch:
diff --git a/cmd/analyze/main.go b/cmd/analyze/main.go
index 4e987dd..4d0bd5e 100644
--- a/cmd/analyze/main.go
+++ b/cmd/analyze/main.go
@@ -9,6 +9,7 @@ import (
 	"strings"
 
 	"github.com/ossf/package-analysis/internal/analysis"
+	"github.com/ossf/package-analysis/internal/featureflags"
 	"github.com/ossf/package-analysis/internal/log"
 	"github.com/ossf/package-analysis/internal/pkgmanager"
 	"github.com/ossf/package-analysis/internal/resultstore"
@@ -33,6 +34,7 @@ var (
 	customSandbox       = flag.String("sandbox-image", "", "override default dynamic analysis sandbox with custom image")
 	customAnalysisCmd   = flag.String("analysis-command", "", "override default dynamic analysis script path (use with custom sandbox image)")
 	listModes           = flag.Bool("list-modes", false, "prints out a list of available analysis modes")
+	features            = flag.String("feature-flags", "", "override default feature flag settings")
 	help                = flag.Bool("help", false, "print help on available options")
 	analysisMode        = utils.CommaSeparatedFlags("mode", []string{"static", "dynamic"},
 		"list of analysis modes to run, separated by commas. Use -list-modes to see available options")
@@ -157,6 +159,11 @@ func main() {
 	analysisMode.InitFlag()
 	flag.Parse()
 
+	if err := featureflags.Update(*features); err != nil {
+		log.Fatal("Failed to parse flags", "error", err)
+		return
+	}
+
 	if *help {
 		flag.Usage()
 		return
diff --git a/cmd/worker/main.go b/cmd/worker/main.go
index d99c995..4db5cfb 100644
--- a/cmd/worker/main.go
+++ b/cmd/worker/main.go
@@ -10,6 +10,7 @@ import (
 	"os"
 	"path"
 
+	"go.uber.org/zap"
 	"gocloud.dev/blob"
 	_ "gocloud.dev/blob/fileblob"
 	_ "gocloud.dev/blob/gcsblob"
@@ -18,6 +19,7 @@ import (
 	_ "gocloud.dev/pubsub/gcppubsub"
 	_ "gocloud.dev/pubsub/kafkapubsub"
 
+	"github.com/ossf/package-analysis/internal/featureflags"
 	"github.com/ossf/package-analysis/internal/log"
 	"github.com/ossf/package-analysis/internal/notification"
 	"github.com/ossf/package-analysis/internal/pkgmanager"
@@ -231,12 +233,18 @@ func messageLoop(ctx context.Context, subURL, packagesBucket, notificationTopicU
 }
 
 func main() {
+	logger := log.Initialize(os.Getenv("LOGGER_ENV"))
+
 	ctx := context.Background()
 	subURL := os.Getenv("OSSMALWARE_WORKER_SUBSCRIPTION")
 	packagesBucket := os.Getenv("OSSF_MALWARE_ANALYSIS_PACKAGES")
 	notificationTopicURL := os.Getenv("OSSF_MALWARE_NOTIFICATION_TOPIC")
 	enableProfiler := os.Getenv("OSSF_MALWARE_ANALYSIS_ENABLE_PROFILER")
 
+	if err := featureflags.Update(os.Getenv("OSSF_MALWARE_FEATURE_FLAGS")); err != nil {
+		logger.Fatal("Failed to parse feature flags", zap.Error(err))
+	}
+
 	resultsBuckets := resultBucketPaths{
 		dynamicAnalysis: os.Getenv("OSSF_MALWARE_ANALYSIS_RESULTS"),
 		staticAnalysis:  os.Getenv("OSSF_MALWARE_STATIC_ANALYSIS_RESULTS"),
@@ -249,28 +257,29 @@ func main() {
 		noPull: os.Getenv("OSSF_SANDBOX_NOPULL") != "",
 	}
 
-	log.Initialize(os.Getenv("LOGGER_ENV"))
 	sandbox.InitNetwork()
 
 	// If configured, start a webserver so that Go's pprof can be accessed for
 	// debugging and profiling.
 	if enableProfiler != "" {
 		go func() {
-			log.Info("Starting profiler")
+			logger.Info("Starting profiler")
 			http.ListenAndServe(":6060", nil)
 		}()
 	}
 
 	// Log the configuration of the worker at startup so we can observe it.
-	log.Info("Starting worker",
-		"subscription", subURL,
-		"package_bucket", packagesBucket,
-		"results_bucket", resultsBuckets.dynamicAnalysis,
-		"static_results_bucket", resultsBuckets.staticAnalysis,
-		"file_write_results_bucket", resultsBuckets.fileWrites,
-		"image_tag", imageSpec.tag,
-		"image_nopull", fmt.Sprintf("%v", imageSpec.noPull),
-		"topic_notification", notificationTopicURL)
+	logger.With(
+		zap.String("subscription", subURL),
+		zap.String("package_bucket", packagesBucket),
+		zap.String("results_bucket", resultsBuckets.dynamicAnalysis),
+		zap.String("static_results_bucket", resultsBuckets.staticAnalysis),
+		zap.String("file_write_results_bucket", resultsBuckets.fileWrites),
+		zap.String("image_tag", imageSpec.tag),
+		zap.Bool("image_nopull", imageSpec.noPull),
+		zap.String("topic_notification", notificationTopicURL),
+		zap.Reflect("feature_flags", featureflags.State()),
+	).Info("Starting worker")
 
 	err := messageLoop(ctx, subURL, packagesBucket, notificationTopicURL, imageSpec, resultStores)
 	if err != nil {
diff --git a/infra/worker/scaler.yaml b/infra/worker/scaler.yaml
index 8046a59..2266a08 100644
--- a/infra/worker/scaler.yaml
+++ b/infra/worker/scaler.yaml
@@ -4,7 +4,7 @@ metadata:
   name: pubsub
 spec:
   minReplicas: 1
-  maxReplicas: 1900
+  maxReplicas: 1500
   metrics:
   - external:
       metric:
diff --git a/infra/worker/workers-set.yaml b/infra/worker/workers-set.yaml
index 35232a5..8ed4e04 100644
--- a/infra/worker/workers-set.yaml
+++ b/infra/worker/workers-set.yaml
@@ -34,6 +34,8 @@ spec:
           value: gs://ossf-malware-analysis-packages
         - name: OSSF_MALWARE_NOTIFICATION_TOPIC
           value: gcppubsub://projects/ossf-malware-analysis/topics/analysis-notify
+        - name: OSSF_MALWARE_FEATURE_FLAGS
+          value: -WriteFileContents
         securityContext:
           privileged: true
         volumeMounts:
@@ -55,4 +57,9 @@ spec:
       - name: worker-tmp
         emptyDir:
           sizeLimit: 5Gi
+  strategy:
+    type: "RollingUpdate"
+    rollingUpdate:
+      maxUnavailable: 10
+      maxSurge: 1
   
diff --git a/internal/featureflags/featureflags.go b/internal/featureflags/featureflags.go
new file mode 100644
index 0000000..6519606
--- /dev/null
+++ b/internal/featureflags/featureflags.go
@@ -0,0 +1,70 @@
+package featureflags
+
+import (
+	"errors"
+	"fmt"
+	"strings"
+)
+
+var ErrUndefinedFlag = errors.New("flag is undefined")
+
+var flagRegistry = make(map[string]*FeatureFlag)
+
+// FeatureFlag stores the state for a single flag.
+//
+// Call Enabled() to see if the flag is enabled.
+type FeatureFlag struct {
+	isEnabled bool
+}
+
+// new registers the flag and sets the default enabled state.
+func new(name string, defaultEnabled bool) *FeatureFlag {
+	ff := &FeatureFlag{
+		isEnabled: defaultEnabled,
+	}
+	flagRegistry[name] = ff
+	return ff
+}
+
+// Enabled returns whether or not the feature is enabled.
+func (ff *FeatureFlag) Enabled() bool {
+	return ff.isEnabled
+}
+
+// Update changes the internal state of the flags based on flags passed in.
+//
+// flags is a comma separated list of flag names. If a flag name is present it
+// will be enabled. If a flag name is preceeded with a "-" character it will be
+// disabled.
+//
+// For example: "MyFeature,-ExperimentalFeature" will enable the flag "MyFeature"
+// and disable the flag "ExperimentalFeature".
+//
+// If a flag is undefined an error wrapping ErrUndefinedFlag will be returned.
+func Update(flags string) error {
+	if flags == "" {
+		return nil
+	}
+	for _, n := range strings.Split(flags, ",") {
+		isEnabled := true
+		if n[0] == '-' {
+			isEnabled = false
+			n = n[1:]
+		}
+		if ff, ok := flagRegistry[n]; ok {
+			ff.isEnabled = isEnabled
+		} else {
+			return fmt.Errorf("%w: %s", ErrUndefinedFlag, n)
+		}
+	}
+	return nil
+}
+
+// State returns a representation of the flags that are enabled and disabled.
+func State() map[string]bool {
+	s := make(map[string]bool)
+	for k, v := range flagRegistry {
+		s[k] = v.Enabled()
+	}
+	return s
+}
diff --git a/internal/featureflags/features.go b/internal/featureflags/features.go
new file mode 100644
index 0000000..0480a42
--- /dev/null
+++ b/internal/featureflags/features.go
@@ -0,0 +1,7 @@
+package featureflags
+
+var (
+	// WriteFileContents will store the contents of write observed from strace
+	// data during dynamic analysis.
+	WriteFileContents = new("WriteFileContents", true)
+)
diff --git a/internal/strace/strace.go b/internal/strace/strace.go
index 5c26e80..56582ce 100644
--- a/internal/strace/strace.go
+++ b/internal/strace/strace.go
@@ -14,6 +14,7 @@ import (
 	"strings"
 	"unicode"
 
+	"github.com/ossf/package-analysis/internal/featureflags"
 	"github.com/ossf/package-analysis/internal/log"
 	"github.com/ossf/package-analysis/internal/utils"
 )
@@ -152,6 +153,10 @@ func (r *Result) recordFileAccess(file string, read, write, del bool) {
 
 func (r *Result) recordFileWrite(file string, writeBuffer []byte, bytesWritten int64) {
 	r.recordFileAccess(file, false, true, false)
+	if !featureflags.WriteFileContents.Enabled() {
+		// Abort writing file contents when feature is disabled.
+		return
+	}
 	hash := sha256.New()
 	hash.Write(writeBuffer)
 	writeID := hex.EncodeToString(hash.Sum(nil))
diff --git a/internal/worker/save_data.go b/internal/worker/save_data.go
index d458246..e711e61 100644
--- a/internal/worker/save_data.go
+++ b/internal/worker/save_data.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/ossf/package-analysis/internal/featureflags"
 	"github.com/ossf/package-analysis/internal/log"
 	"github.com/ossf/package-analysis/internal/pkgmanager"
 	"github.com/ossf/package-analysis/internal/resultstore"
@@ -34,6 +35,10 @@ func SaveDynamicAnalysisData(ctx context.Context, pkg *pkgmanager.Pkg, dest Resu
 	if err := saveExecutionLog(ctx, pkg, dest, data); err != nil {
 		return err
 	}
+	if !featureflags.WriteFileContents.Enabled() {
+		// Abort writing file contents when feature is disabled.
+		return nil
+	}
 	if err := SaveFileWritesData(ctx, pkg, dest, data); err != nil {
 		return err
 	}


Test patch:
diff --git a/internal/featureflags/featureflags_test.go b/internal/featureflags/featureflags_test.go
new file mode 100644
index 0000000..ea8347b
--- /dev/null
+++ b/internal/featureflags/featureflags_test.go
@@ -0,0 +1,86 @@
+package featureflags
+
+import (
+	"reflect"
+	"testing"
+)
+
+func resetRegistry() {
+	flagRegistry = make(map[string]*FeatureFlag)
+}
+
+func TestFlagDefault_True(t *testing.T) {
+	resetRegistry()
+	ff := new("TestFlag", true)
+	if !ff.Enabled() {
+		t.Error("Enabled() = false; want true")
+	}
+}
+
+func TestFlagDefault_False(t *testing.T) {
+	resetRegistry()
+	ff := new("TestFlag", false)
+	if ff.Enabled() {
+		t.Error("Enabled() = true; want false")
+	}
+}
+
+func TestFlagUpdate_SingleFlag(t *testing.T) {
+	resetRegistry()
+	ff := new("TestFlag", false)
+	Update("TestFlag")
+
+	if !ff.Enabled() {
+		t.Error("Enabled() = false; want true")
+	}
+}
+
+func TestFlagUpdate_SingleFlagOff(t *testing.T) {
+	resetRegistry()
+	ff := new("TestFlag", true)
+	Update("-TestFlag")
+
+	if ff.Enabled() {
+		t.Error("Enabled() = true; want false")
+	}
+}
+
+func TestFlagUpdate_MultiFlags(t *testing.T) {
+	resetRegistry()
+	new("TestFlag1", false)
+	new("TestFlag2", true)
+	new("TestFlag3", false)
+	Update("TestFlag1,-TestFlag2,TestFlag3")
+	want := map[string]bool{
+		"TestFlag1": true,
+		"TestFlag2": false,
+		"TestFlag3": true,
+	}
+	if got := State(); !reflect.DeepEqual(want, got) {
+		t.Errorf("State() = %v; want %v", got, want)
+	}
+}
+
+func TestFlagUpdate_MultiFlags_EmptyString(t *testing.T) {
+	resetRegistry()
+	new("TestFlag1", false)
+	new("TestFlag2", true)
+	new("TestFlag3", false)
+	Update("")
+	want := map[string]bool{
+		"TestFlag1": false,
+		"TestFlag2": true,
+		"TestFlag3": false,
+	}
+	if got := State(); !reflect.DeepEqual(want, got) {
+		t.Errorf("State() = %v; want %v", got, want)
+	}
+}
+
+func TestFlagUpdate_Error(t *testing.T) {
+	resetRegistry()
+	err := Update("TestFlag")
+	if err == nil {
+		t.Errorf("Update() = nil; want an error")
+	}
+}