Merge branch 'STOR-2040' of https://github.com/gmeghnag/oc into STOR-…

…2040

pkg/cli/admin/nodeimage/create_test.go

@@ -19,6 +19,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"os"
 	"slices"
 	"strings"
 	"testing"
@@ -34,6 +35,7 @@ import (
 	restclient "k8s.io/client-go/rest"
 	clientgotesting "k8s.io/client-go/testing"
 	"k8s.io/client-go/tools/remotecommand"
+	utilexec "k8s.io/utils/exec"
 
 	"github.com/distribution/distribution/v3/manifest/schema2"
 	configv1 "github.com/openshift/api/config/v1"
@@ -106,12 +108,26 @@ func strPtr(s string) *string {
 	return &s
 }
 
+func createCmdOutput(t *testing.T, r report) string {
+	t.Helper()
+	out, err := json.Marshal(r)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return string(out)
+}
+
 func TestRun(t *testing.T) {
 	ClusterVersion_4_16_ObjectFn := func(repo string, manifestDigest string) []runtime.Object {
 		cvobj := defaultClusterVersionObjectFn(repo, manifestDigest)
 		clusterVersion := cvobj[0].(*configv1.ClusterVersion)
 		clusterVersion.Status.Desired.Version = "4.16.6-x86_64"
-
+		return cvobj
+	}
+	ClusterVersion_4_17_ObjectFn := func(repo string, manifestDigest string) []runtime.Object {
+		cvobj := defaultClusterVersionObjectFn(repo, manifestDigest)
+		clusterVersion := cvobj[0].(*configv1.ClusterVersion)
+		clusterVersion.Status.Desired.Version = "4.17.4-x86_64"
 		return cvobj
 	}
 
@@ -124,6 +140,7 @@ func TestRun(t *testing.T) {
 		objects          func(string, string) []runtime.Object
 		remoteExecOutput string
 
+		expectedErrorCode    int
 		expectedError        string
 		expectedPod          func(t *testing.T, pod *corev1.Pod)
 		expectedRsyncInclude string
@@ -145,18 +162,26 @@ func TestRun(t *testing.T) {
 			expectedRsyncInclude: "boot-artifacts/*",
 		},
 		{
-			name:             "node-joiner tool failure",
-			nodesConfig:      defaultNodesConfigYaml,
-			objects:          defaultClusterVersionObjectFn,
-			remoteExecOutput: "1",
-			expectedError:    `image generation error: <nil> (exit code: 1)`,
+			name:        "node-joiner tool failure",
+			nodesConfig: defaultNodesConfigYaml,
+			objects:     defaultClusterVersionObjectFn,
+			remoteExecOutput: createCmdOutput(t, report{
+				stageHeader: stageHeader{
+					EndTime: time.Date(2024, 11, 14, 0, 0, 0, 0, time.UTC),
+				},
+				Result: &reportResult{
+					ExitCode:     127,
+					ErrorMessage: "Some error message",
+				},
+			}),
+			expectedErrorCode: 1,
+			expectedError:     `exit`,
 		},
 		{
-			name:             "node-joiner unsupported prior to 4.17",
-			nodesConfig:      defaultNodesConfigYaml,
-			objects:          ClusterVersion_4_16_ObjectFn,
-			remoteExecOutput: "1",
-			expectedError:    fmt.Sprintf("the 'oc adm node-image' command is only available for OpenShift versions %s and later", nodeJoinerMinimumSupportedVersion),
+			name:          "node-joiner unsupported prior to 4.17",
+			nodesConfig:   defaultNodesConfigYaml,
+			objects:       ClusterVersion_4_16_ObjectFn,
+			expectedError: fmt.Sprintf("the 'oc adm node-image' command is only available for OpenShift versions %s and later", nodeJoinerMinimumSupportedVersion),
 		},
 		{
 			name:          "missing cluster connection",
@@ -204,6 +229,12 @@ func TestRun(t *testing.T) {
 				}
 			},
 		},
+		{
+			name:             "basic report for ocp < 4.18",
+			nodesConfig:      defaultNodesConfigYaml,
+			objects:          ClusterVersion_4_17_ObjectFn,
+			remoteExecOutput: "0",
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
@@ -223,12 +254,23 @@ func TestRun(t *testing.T) {
 				objs = tc.objects(fakeReg.URL()[len("https://"):], fakeReg.fakeManifestDigest)
 			}
 
+			fakeRemoteExec.execOut = createCmdOutput(t, report{
+				stageHeader: stageHeader{
+					Identifier: "report-test",
+					EndTime:    time.Date(2024, 11, 14, 0, 0, 0, 0, time.UTC),
+				},
+				Result: &reportResult{
+					ExitCode: 0,
+				},
+			})
 			if tc.remoteExecOutput != "" {
 				fakeRemoteExec.execOut = tc.remoteExecOutput
 			}
 			// Create another fake for the copy action
 			fakeCp := &fakeCopier{}
 
+			var logBuffer bytes.Buffer
+
 			// Prepare the command options with all the fakes
 			o := &CreateOptions{
 				BaseNodeImageCommand: BaseNodeImageCommand{
@@ -238,6 +280,7 @@ func TestRun(t *testing.T) {
 					Client:         fakeClient,
 					Config:         fakeRestConfig,
 					remoteExecutor: fakeRemoteExec,
+					LogOut:         &logBuffer,
 				},
 				FSys: fakeFileSystem,
 				copyStrategy: func(o *rsync.RsyncOptions) rsync.CopyStrategy {
@@ -247,13 +290,14 @@ func TestRun(t *testing.T) {
 
 				AssetsDir:        tc.assetsDir,
 				GeneratePXEFiles: tc.generatePXEFiles,
+				fileWriter:       mockFileWriter{},
 			}
 			// Since the fake registry creates a self-signed cert, let's configure
 			// the command options accordingly
 			o.SecurityOptions.Insecure = true
 
 			err := o.Run()
-			assertContainerImageAndErrors(t, err, fakeReg, fakeClient, tc.expectedError, nodeJoinerContainer)
+			assertContainerImageAndErrors(t, err, fakeReg, fakeClient, tc.expectedErrorCode, tc.expectedError, nodeJoinerContainer)
 
 			// Perform additional checks on the generated node-joiner pod
 			if tc.expectedPod != nil {
@@ -276,6 +320,12 @@ func TestRun(t *testing.T) {
 	}
 }
 
+type mockFileWriter struct{}
+
+func (mockFileWriter) WriteFile(name string, data []byte, perm os.FileMode) error {
+	return nil
+}
+
 // fakeRegistry creates a fake Docker registry configured to serve the minimum
 // amount of data required to allow a successfull execution of the command to
 // retrieve the release info, and to extract the baremetal-installer pullspec.
@@ -546,7 +596,7 @@ func getTestPod(fakeClient *fake.Clientset, podName string) *corev1.Pod {
 	return pod
 }
 
-func assertContainerImageAndErrors(t *testing.T, runErr error, fakeReg *fakeRegistry, fakeClient *fake.Clientset, expectedError, podName string) {
+func assertContainerImageAndErrors(t *testing.T, runErr error, fakeReg *fakeRegistry, fakeClient *fake.Clientset, expectedErrorCode int, expectedError, podName string) {
 	if expectedError == "" {
 		if runErr != nil {
 			t.Fatalf("unexpected error: %v", runErr)
@@ -561,8 +611,13 @@ func assertContainerImageAndErrors(t *testing.T, runErr error, fakeReg *fakeRegi
 		if runErr == nil {
 			t.Fatalf("expected error not received: %s", expectedError)
 		}
-		if !strings.Contains(runErr.Error(), expectedError) {
-			t.Fatalf("expected error: %s, actual: %v", expectedError, runErr.Error())
+		if codeExitErr, ok := runErr.(utilexec.CodeExitError); ok {
+			if codeExitErr.Code != expectedErrorCode {
+				t.Fatalf("expected error code: %d, actual: %d", expectedErrorCode, codeExitErr.Code)
+			}
+		}
+		if runErr.Error() != expectedError {
+			t.Fatalf("expected error: %s, actual: %s", expectedError, runErr.Error())
 		}
 	}
 }

pkg/cli/admin/nodeimage/monitor.go

@@ -146,6 +146,7 @@ func (o *MonitorOptions) Run() error {
 	defer o.cleanup(ctx)
 
 	tasks := []func(context.Context) error{
+		o.checkMinSupportedVersion,
 		o.getNodeJoinerPullSpec,
 		o.createNamespace,
 		o.createServiceAccount,
@@ -160,7 +161,7 @@ func (o *MonitorOptions) Run() error {
 
 	podName := o.nodeJoinerPod.GetName()
 
-	if err := o.waitForContainerRunning(ctx); err != nil {
+	if err := o.waitForRunningPod(ctx); err != nil {
 		klog.Errorf("monitoring did not start: %s", err)
 		return fmt.Errorf("monitoring did not start for pod %s: %s", podName, err)
 	}

pkg/cli/admin/nodeimage/monitor_test.go

@@ -123,7 +123,7 @@ func TestMonitorRun(t *testing.T) {
 			o.SecurityOptions.Insecure = true
 
 			err := o.Run()
-			assertContainerImageAndErrors(t, err, fakeReg, fakeClient, tc.expectedError, nodeJoinerMonitorContainer)
+			assertContainerImageAndErrors(t, err, fakeReg, fakeClient, -1, tc.expectedError, nodeJoinerMonitorContainer)
 			if tc.expectedError == "" {
 				if fakeLogContent != logContents.String() {
 					t.Errorf("expected %v, actual %v", fakeLogContent, logContents.String())

pkg/cli/admin/release/image_mapper.go

@@ -66,7 +66,7 @@ func (p *Payload) Rewrite(allowTags bool, fn func(component string) imagereferen
 			continue
 		}
 		path := filepath.Join(p.path, file.Name())
-		data, err := ioutil.ReadFile(path)
+		data, err := os.ReadFile(path)
 		if err != nil {
 			return err
 		}
@@ -78,7 +78,7 @@ func (p *Payload) Rewrite(allowTags bool, fn func(component string) imagereferen
 			continue
 		}
 		klog.V(6).Infof("Rewrote\n%s\n\nto\n\n%s\n", string(data), string(out))
-		if err := ioutil.WriteFile(path, out, file.Mode()); err != nil {
+		if err := os.WriteFile(path, out, file.Mode()); err != nil {
 			return err
 		}
 	}
@@ -99,7 +99,7 @@ func (p *Payload) References() (*imageapi.ImageStream, error) {
 }
 
 func parseImageStream(path string) (*imageapi.ImageStream, error) {
-	data, err := ioutil.ReadFile(path)
+	data, err := os.ReadFile(path)
 	if os.IsNotExist(err) {
 		return nil, err
 	}
@@ -275,7 +275,7 @@ func NewImageMapper(images map[string]ImageReference) (ManifestMapper, error) {
 			ref := images[name]
 
 			suffix := parts[3]
-			klog.V(2).Infof("found repository %q with locator %q in the input, switching to %q (from pattern %s)", string(repository), string(suffix), ref.TargetPullSpec, pattern)
+			klog.V(2).Infof("found repository %q with locator %q in the input, switching to %q (from pattern %s)", repository, string(suffix), ref.TargetPullSpec, pattern)
 			switch {
 			case len(suffix) == 0:
 				// we found a repository, but no tag or digest (implied latest), or we got an exact match
@@ -335,9 +335,7 @@ func ComponentReferencesForImageStream(is *imageapi.ImageStream) (func(string) i
 	}, nil
 }
 
-const (
-	componentVersionFormat = `([\W]|^)0\.0\.1-snapshot([a-z0-9\-]*)`
-)
+var componentVersionRe = regexp.MustCompile(`(\W|^)0\.0\.1-snapshot([a-z0-9\-]*)`)
 
 // NewComponentVersionsMapper substitutes strings of the form 0.0.1-snapshot with releaseName and strings
 // of the form 0.0.1-snapshot-[component] with the version value located in versions, or returns an error.
@@ -352,17 +350,12 @@ func NewComponentVersionsMapper(releaseName string, versions ComponentVersions,
 	} else {
 		releaseName = ""
 	}
-	re, err := regexp.Compile(componentVersionFormat)
-	if err != nil {
-		return func([]byte) ([]byte, error) {
-			return nil, fmt.Errorf("component versions mapper regex: %v", err)
-		}
-	}
+
 	return func(data []byte) ([]byte, error) {
 		var missing []string
 		var conflicts []string
-		data = re.ReplaceAllFunc(data, func(part []byte) []byte {
-			matches := re.FindSubmatch(part)
+		data = componentVersionRe.ReplaceAllFunc(data, func(part []byte) []byte {
+			matches := componentVersionRe.FindSubmatch(part)
 			if matches == nil {
 				return part
 			}
@@ -415,7 +408,7 @@ var (
 	// reAllowedVersionKey limits the allowed component name to a strict subset
 	reAllowedVersionKey = regexp.MustCompile(`^[a-z0-9]+[\-a-z0-9]*[a-z0-9]+$`)
 	// reAllowedDisplayNameKey limits the allowed component name to a strict subset
-	reAllowedDisplayNameKey = regexp.MustCompile(`^[a-zA-Z0-9\-\:\s\(\)]+$`)
+	reAllowedDisplayNameKey = regexp.MustCompile(`^[a-zA-Z0-9\-:\s()]+$`)
 )
 
 // ComponentVersion includes the version and optional display name.
@@ -434,7 +427,7 @@ func (v ComponentVersion) String() string {
 // labels removed, but prerelease segments are preserved.
 type ComponentVersions map[string]ComponentVersion
 
-// OrderedKeys returns the keys in this map in lexigraphic order.
+// OrderedKeys returns the keys in this map in lexicographic order.
 func (v ComponentVersions) OrderedKeys() []string {
 	keys := make([]string, 0, len(v))
 	for k := range v {

pkg/cli/admin/release/image_mapper_test.go

@@ -5,11 +5,11 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"k8s.io/cli-runtime/pkg/genericiooptions"
 
 	imageapi "github.com/openshift/api/image/v1"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/util/diff"
 )
 
 func TestNewImageMapper(t *testing.T) {
@@ -265,8 +265,6 @@ func TestNewExactMapper(t *testing.T) {
 }
 
 func TestNewComponentVersionsMapper(t *testing.T) {
-	type args struct {
-	}
 	tests := []struct {
 		name        string
 		releaseName string
@@ -372,8 +370,6 @@ func TestNewComponentVersionsMapper(t *testing.T) {
 }
 
 func Test_parseComponentVersionsLabel(t *testing.T) {
-	type args struct {
-	}
 	tests := []struct {
 		name         string
 		label        string
@@ -700,18 +696,18 @@ func Test_loadImageStreamTransforms(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			ioStream := genericiooptions.NewTestIOStreamsDiscard()
-			got, got1, got2, err := loadImageStreamTransforms(tt.input, tt.local, tt.allowMissingImages, tt.src, ioStream.ErrOut)
+			got, gotTags, gotRefs, err := loadImageStreamTransforms(tt.input, tt.local, tt.allowMissingImages, tt.src, ioStream.ErrOut)
 			if (err != nil) != tt.wantErr {
 				t.Fatalf("loadImageStreamTransforms() error = %v, wantErr %v", err, tt.wantErr)
 			}
-			if !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("%s", diff.ObjectReflectDiff(got, tt.want))
+			if diff := cmp.Diff(tt.want, got); diff != "" {
+				t.Errorf("versions differ (-want +got):\n%s", diff)
 			}
-			if !reflect.DeepEqual(got1, tt.wantTags) {
-				t.Errorf("%s", diff.ObjectReflectDiff(got1, tt.wantTags))
+			if diff := cmp.Diff(tt.wantTags, gotTags); diff != "" {
+				t.Errorf("tags differ (-want +got):\n%s", diff)
 			}
-			if !reflect.DeepEqual(got2, tt.wantRefs) {
-				t.Errorf("%s", diff.ObjectReflectDiff(got2, tt.wantRefs))
+			if diff := cmp.Diff(tt.wantRefs, gotRefs); diff != "" {
+				t.Errorf("refs differ (-want +got):\n%s", diff)
 			}
 		})
 	}