fix: allow crictl config to create a missing explicit file

Author: immanuwell

.github/workflows/crio.yml

@@ -141,6 +141,7 @@ jobs:
 
           set +o errexit
           sudo -E PATH=$PATH make test-e2e \
+            GINKGO_FLAGS="--flake-attempts=3" \
             TESTFLAGS="-crictl-runtime-endpoint=unix://var/run/crio/crio.sock"
           TEST_RC=$?
           set -o errexit

Makefile

@@ -60,6 +60,7 @@ PRETTIER_VERSION = 3.8.3
 ZIZMOR_VERSION := v1.25.2
 
 GINKGO := $(BUILD_BIN_PATH)/ginkgo
+GINKGO_FLAGS ?=
 GOLANGCI_LINT_DIR := $(BUILD_BIN_PATH)/golangci-lint-$(GOLANGCI_LINT_VERSION)
 GOLANGCI_LINT := $(GOLANGCI_LINT_DIR)/golangci-lint
 ZEITGEIST := $(BUILD_BIN_PATH)/zeitgeist
@@ -267,7 +268,7 @@ PARALLEL ?= 8
 
 .PHONY: test-e2e
 test-e2e: $(GINKGO) ## Run the e2e test suite.
-	$(GINKGO) \
+	$(GINKGO) $(GINKGO_FLAGS) \
 		-r \
 		--randomize-all \
 		--randomize-suites \

cmd/crictl/config.go

@@ -17,6 +17,7 @@ limitations under the License.
 package main
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"strconv"
@@ -72,14 +73,26 @@ CRICTL OPTIONS:
 	Action: func(c *cli.Context) error {
 		configFile := c.String("config")
 		if _, err := os.Stat(configFile); err != nil {
-			if err := common.WriteConfig(nil, configFile); err != nil {
+			if !errors.Is(err, os.ErrNotExist) {
 				return err
 			}
+
+			if shouldCreateConfigFile(c) {
+				if err := common.WriteConfig(nil, configFile); err != nil {
+					return err
+				}
+			}
 		}
-		// Get config from file.
-		config, err := common.ReadConfig(configFile)
-		if err != nil {
-			return fmt.Errorf("load config file: %w", err)
+
+		config := &common.Config{}
+		if _, err := os.Stat(configFile); err == nil {
+			// Get config from file.
+			config, err = common.ReadConfig(configFile)
+			if err != nil {
+				return fmt.Errorf("load config file: %w", err)
+			}
+		} else if !errors.Is(err, os.ErrNotExist) {
+			return err
 		}
 
 		if c.IsSet("get") {
@@ -157,6 +170,18 @@ CRICTL OPTIONS:
 	},
 }
 
+func shouldCreateConfigFile(c *cli.Context) bool {
+	if c.IsSet("set") {
+		return true
+	}
+
+	if c.IsSet("get") || c.Bool("list") {
+		return false
+	}
+
+	return c.Args().First() != ""
+}
+
 func setValue(key, value string, config *common.Config) error {
 	switch key {
 	case common.RuntimeEndpoint:

cmd/crictl/config_command_test.go

@@ -0,0 +1,125 @@
+/*
+Copyright The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package main
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"strings"
+	"sync"
+	"testing"
+)
+
+var runWithArgsForTestMu sync.Mutex
+
+func TestConfigCommandCreatesMissingExplicitConfigOnSet(t *testing.T) {
+	t.Parallel()
+
+	configPath := filepath.Join(t.TempDir(), "nested", "crictl.yaml")
+
+	err := runWithArgsForTest(
+		t,
+		"crictl",
+		"--config",
+		configPath,
+		"config",
+		"--set",
+		"debug=true",
+	)
+	if err != nil {
+		t.Fatalf("run config --set: %v", err)
+	}
+
+	content, err := os.ReadFile(configPath)
+	if err != nil {
+		t.Fatalf("read config file: %v", err)
+	}
+
+	if !strings.Contains(string(content), "debug: true") {
+		t.Fatalf("expected config file to contain debug=true, got:\n%s", string(content))
+	}
+}
+
+func TestConfigCommandCreatesMissingExplicitConfigOnPositionalSet(t *testing.T) {
+	t.Parallel()
+
+	configPath := filepath.Join(t.TempDir(), "nested", "crictl.yaml")
+
+	err := runWithArgsForTest(
+		t,
+		"crictl",
+		"--config",
+		configPath,
+		"config",
+		"debug",
+		"true",
+	)
+	if err != nil {
+		t.Fatalf("run config positional set: %v", err)
+	}
+
+	content, err := os.ReadFile(configPath)
+	if err != nil {
+		t.Fatalf("read config file: %v", err)
+	}
+
+	if !strings.Contains(string(content), "debug: true") {
+		t.Fatalf("expected config file to contain debug=true, got:\n%s", string(content))
+	}
+}
+
+func TestConfigCommandListDoesNotCreateMissingExplicitConfig(t *testing.T) {
+	t.Parallel()
+
+	configPath := filepath.Join(t.TempDir(), "nested", "crictl.yaml")
+
+	err := runWithArgsForTest(
+		t,
+		"crictl",
+		"--config",
+		configPath,
+		"config",
+		"--list",
+	)
+	if err == nil {
+		t.Fatal("expected config --list to fail for a missing explicit config file")
+	}
+
+	_, err = os.Stat(configPath)
+	if !errors.Is(err, os.ErrNotExist) {
+		t.Fatalf("expected config file to remain absent, got err=%v", err)
+	}
+}
+
+func runWithArgsForTest(t *testing.T, args ...string) error {
+	t.Helper()
+
+	runWithArgsForTestMu.Lock()
+	t.Cleanup(func() {
+		runWithArgsForTestMu.Unlock()
+	})
+
+	oldArgs := os.Args
+	os.Args = args
+
+	t.Cleanup(func() {
+		os.Args = oldArgs
+	})
+
+	return run()
+}

cmd/crictl/main.go

@@ -18,6 +18,7 @@ package main
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -213,6 +214,12 @@ func run() error {
 			return fmt.Errorf("get executable path: %w", err)
 		}
 
+		if shouldCreateMissingExplicitConfigFile(context) {
+			if err := common.WriteConfig(nil, context.String("config")); err != nil {
+				return fmt.Errorf("create config file: %w", err)
+			}
+		}
+
 		if config, err = common.GetServerConfigFromFile(context.String("config"), exePath); err != nil {
 			if context.IsSet("config") {
 				return fmt.Errorf("get server config: %w", err)
@@ -316,3 +323,31 @@ func run() error {
 
 	return err
 }
+
+func shouldCreateMissingExplicitConfigFile(ctx *cli.Context) bool {
+	args := ctx.Args().Slice()
+	if !ctx.IsSet("config") || len(args) == 0 || args[0] != configCommand.Name {
+		return false
+	}
+
+	for _, arg := range args[1:] {
+		switch {
+		case arg == "--get" || strings.HasPrefix(arg, "--get="):
+			return false
+		case arg == "--list" || strings.HasPrefix(arg, "--list="):
+			return false
+		case arg == "--set" || strings.HasPrefix(arg, "--set="):
+			return missingConfigFile(ctx.String("config"))
+		case !strings.HasPrefix(arg, "-"):
+			return missingConfigFile(ctx.String("config"))
+		}
+	}
+
+	return false
+}
+
+func missingConfigFile(path string) bool {
+	_, err := os.Stat(path)
+
+	return errors.Is(err, os.ErrNotExist)
+}