Merge pull request #135 from AlexVulaj/remove-mustgather-images

remove ability to pass in custom mustgather images

README.md

@@ -18,24 +18,6 @@ spec:
 
 This request will collect the standard must-gather info and upload it to case `#02527285` using the credentials found in the `caseManagementCreds` secret.
 
-## Adding other must-gather images
-In this example we are using a specific service account (which must have cluster-admin permissions as per must-gather requirements), and we are specifying a couple of additional must gather images to be run for the `kubevirt` and `ocs` subsystem. If not specified, serviceAccountRef.Name will default to `default`. Also the standard must gather image: `quay.io/openshift/origin-must-gather:latest` is always added by default.
-
-```yaml
-apiVersion: managed.openshift.io/v1alpha1
-kind: MustGather
-metadata:
-  name: example-mustgather-full
-spec:
-  caseID: '02527285'
-  caseManagementAccountSecretRef:
-    name: case-management-creds
-  serviceAccountRef:
-    name: must-gather-admin
-  mustGatherImages:
-  - quay.io/kubevirt/must-gather:latest
-  - quay.io/ocs-dev/ocs-must-gather
-```
 ## Collecting Audit logs
 The field `audit` is **false** by default unless explicetely set to **true**.
 This will generate the default collection of audit logs as per [the collection script: gather_audit_logs](https://github.com/openshift/must-gather/blob/master/collection-scripts/gather_audit_logs)

api/v1alpha1/mustgather_types.go

@@ -39,11 +39,6 @@ type MustGatherSpec struct {
 	/* +kubebuilder:default:="{Name:default}" */
 	ServiceAccountRef corev1.LocalObjectReference `json:"serviceAccountRef,omitempty"`
 
-	// The list of must gather images to run, optional, it will default to: $DEFAULT_MUST_GATHER_IMAGE
-	// +kubebuilder:validation:Optional
-	// +listType=set
-	MustGatherImages []string `json:"mustGatherImages,omitempty"`
-
 	// A flag to specify if audit logs must be collected
 	// See documentation for further information.
 	// +kubebuilder:default:=false

build/Dockerfile

@@ -17,7 +17,7 @@ COPY . /src
 
 RUN make go-build
 
-FROM registry.access.redhat.com/ubi8/ubi-minimal:8.10-896
+FROM registry.access.redhat.com/ubi8/ubi-minimal:8.10-896.1716497715
 ENV OPERATOR=/usr/local/bin/must-gather-operator \
     OPERATOR_BIN=must-gather-operator \
     USER_UID=1001 \

build/Dockerfile.olm-registry

@@ -4,7 +4,7 @@ COPY ${SAAS_OPERATOR_DIR} manifests
 RUN initializer --permissive
 
 # ubi-micro does not work for clusters with fips enabled unless we make OpenSSL available
-FROM registry.access.redhat.com/ubi8/ubi-minimal:8.10-896
+FROM registry.access.redhat.com/ubi8/ubi-minimal:8.10-896.1716497715
 
 COPY --from=builder /bin/registry-server /bin/registry-server
 COPY --from=builder /bin/grpc_health_probe /bin/grpc_health_probe

build/templates/job.template.yaml

@@ -18,13 +18,11 @@ spec:
         - effect: NoSchedule
           key: node-role.kubernetes.io/infra
           operator: Exists
-      backoffLimit: 6
-      restartPolicy: OnFailure     
+      restartPolicy: OnFailure
       shareProcessNamespace: true
       containers:
 {{- $audit := .Spec.Audit}}        
 {{- $timeout := .Spec.MustGatherTimeout.Duration}}
-{{ range $index, $element := .Spec.MustGatherImages }} 
       - command:
         - /bin/bash
         - -c 
@@ -39,12 +37,11 @@ spec:
             echo "Gather timed out."
             exit 0
           fi
-        image: {{ $element }}
-        name: gather-{{ $index }}
+        image: quay.io/openshift/origin-must-gather:MUST_GATHER_IMAGE_DONT_CHANGE
+        name: gather
         volumeMounts:
         - mountPath: /must-gather
           name: must-gather-output
-{{- end }}
       - command:
         - /bin/bash
         - -c

controllers/mustgather/mustgather_controller.go

@@ -20,14 +20,14 @@ import (
 	"context"
 	goerror "errors"
 	"fmt"
+	"github.com/blang/semver/v4"
 	"github.com/go-logr/logr"
 	configv1 "github.com/openshift/api/config/v1"
 	mustgatherv1alpha1 "github.com/openshift/must-gather-operator/api/v1alpha1"
 	"github.com/openshift/must-gather-operator/pkg/k8sutil"
 	"github.com/openshift/must-gather-operator/pkg/localmetrics"
 	"github.com/redhat-cop/operator-utils/pkg/util"
 	"github.com/redhat-cop/operator-utils/pkg/util/templates"
-	"github.com/scylladb/go-set/strset"
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -47,25 +47,13 @@ import (
 const ControllerName = "mustgather-controller"
 
 const templateFileNameEnv = "JOB_TEMPLATE_FILE_NAME"
-const defaultMustGatherImageEnv = "DEFAULT_MUST_GATHER_IMAGE"
 const defaultMustGatherNamespace = "openshift-must-gather-operator"
 
 var log = logf.Log.WithName(ControllerName)
 
-var defaultMustGatherImage string
-
 var jobTemplate *template.Template
 
-func init() {
-	var ok bool
-	defaultMustGatherImage, ok = os.LookupEnv(defaultMustGatherImageEnv)
-	if !ok {
-		defaultMustGatherImage = "quay.io/openshift/origin-must-gather:latest"
-	}
-	fmt.Println("using default must gather image: " + defaultMustGatherImage)
-}
-
-func initializeTemplate() (*template.Template, error) {
+func initializeTemplate(clusterVersion string) (*template.Template, error) {
 	templateFileName, ok := os.LookupEnv(templateFileNameEnv)
 	if !ok {
 		templateFileName = "/etc/templates/job.template.yaml"
@@ -76,14 +64,15 @@ func initializeTemplate() (*template.Template, error) {
 		return &template.Template{}, err
 	}
 	// Inject the operator image URI from the pod's env variables
-	operator_image, varPresent := os.LookupEnv("OPERATOR_IMAGE")
+	operatorImage, varPresent := os.LookupEnv("OPERATOR_IMAGE")
 	if !varPresent {
 		err := goerror.New("Operator image environment variable not found")
 		log.Error(err, "Error: no operator image found for job template")
 		return &template.Template{}, err
 	}
-	// TODO: make this a normal template parameter instead. This is ugly but works
-	str := strings.Replace(string(text), "THIS_STRING_WILL_BE_REPLACED_BUT_DONT_CHANGE_IT", operator_image, 1)
+	// TODO: make these normal template parameters instead. This is ugly but works
+	str := strings.Replace(string(text), "THIS_STRING_WILL_BE_REPLACED_BUT_DONT_CHANGE_IT", operatorImage, 1)
+	str = strings.Replace(str, "MUST_GATHER_IMAGE_DONT_CHANGE", clusterVersion, 1)
 	jobTemplate, err := template.New("MustGatherJob").Parse(str)
 	if err != nil {
 		log.Error(err, "Error parsing template", "template", str)
@@ -342,12 +331,6 @@ func (r *MustGatherReconciler) SetupWithManager(mgr ctrl.Manager) error {
 
 func (r *MustGatherReconciler) IsInitialized(instance *mustgatherv1alpha1.MustGather) bool {
 	initialized := true
-	imageSet := strset.New(instance.Spec.MustGatherImages...)
-	if !imageSet.Has(defaultMustGatherImage) {
-		imageSet.Add(defaultMustGatherImage)
-		instance.Spec.MustGatherImages = imageSet.List()
-		initialized = false
-	}
 
 	if instance.Spec.ServiceAccountRef.Name == "" {
 		instance.Spec.ServiceAccountRef.Name = "default"
@@ -385,8 +368,12 @@ func (r *MustGatherReconciler) addFinalizer(reqLogger logr.Logger, m *mustgather
 }
 
 func (r *MustGatherReconciler) getJobFromInstance(instance *mustgatherv1alpha1.MustGather) (*unstructured.Unstructured, error) {
-	var err error
-	jobTemplate, err = initializeTemplate()
+	version, err := r.getClusterVersionForJobTemplate("version")
+	if err != nil {
+		return nil, fmt.Errorf("failed to get cluster version for job template: %w", err)
+	}
+
+	jobTemplate, err = initializeTemplate(version)
 	if err != nil {
 		log.Error(err, "unable to initialize job template")
 		return &unstructured.Unstructured{}, err
@@ -399,6 +386,28 @@ func (r *MustGatherReconciler) getJobFromInstance(instance *mustgatherv1alpha1.M
 	return unstructuredJob, nil
 }
 
+func (r *MustGatherReconciler) getClusterVersionForJobTemplate(clusterVersionName string) (string, error) {
+	clusterVersion := &configv1.ClusterVersion{}
+	err := r.GetClient().Get(context.TODO(), types.NamespacedName{Name: clusterVersionName}, clusterVersion)
+	if err != nil {
+		return "", fmt.Errorf("unable to get clusterversion '%v': %w", clusterVersionName, err)
+	}
+
+	var version string
+	for _, history := range clusterVersion.Status.History {
+		if history.State == "Completed" {
+			version = history.Version
+			break
+		}
+	}
+	if version == "" {
+		return "", goerror.New("unable to determine cluster version from status history")
+	}
+
+	parsedVersion, _ := semver.New(version)
+	return fmt.Sprintf("%v.%v", parsedVersion.Major, parsedVersion.Minor), nil
+}
+
 // contains is a helper function for finalizer
 func contains(list []string, s string) bool {
 	for _, v := range list {

controllers/mustgather/mustgather_controller_test.go

@@ -2,7 +2,9 @@ package mustgather
 
 import (
 	"context"
+	configv1 "github.com/openshift/api/config/v1"
 	"os"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"testing"
 
 	mustgatherv1alpha1 "github.com/openshift/must-gather-operator/api/v1alpha1"
@@ -79,7 +81,6 @@ func createMustGatherObject() *mustgatherv1alpha1.MustGather {
 			ServiceAccountRef: corev1.LocalObjectReference{
 				Name: "",
 			},
-			MustGatherImages: []string{"quay.io/openshift/origin-must-gather:latest"},
 		},
 	}
 }
@@ -100,3 +101,95 @@ func createMustGatherSecretObject() *corev1.Secret {
 		},
 	}
 }
+
+func generateFakeClient(objs ...client.Object) (client.Client, error) {
+	s := runtime.NewScheme()
+	if err := configv1.AddToScheme(s); err != nil {
+		return nil, err
+	}
+	return fake.NewClientBuilder().WithScheme(s).WithObjects(objs...).Build(), nil
+}
+
+func TestMustGatherReconciler_getClusterVersionForJobTemplate(t *testing.T) {
+	tests := []struct {
+		name               string
+		clusterVersionObj  *configv1.ClusterVersion
+		clusterVersionName string
+		want               string
+		wantErr            bool
+	}{
+		{
+			name: "clusterVersion doesn't exist",
+			clusterVersionObj: &configv1.ClusterVersion{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+			},
+			clusterVersionName: "badName",
+			wantErr:            true,
+		},
+		{
+			name: "clusterVersion does not have any status history",
+			clusterVersionObj: &configv1.ClusterVersion{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+				Status:     configv1.ClusterVersionStatus{},
+			},
+			clusterVersionName: "cluster",
+			wantErr:            true,
+		},
+		{
+			name: "clusterVersion does not have a Completed status history",
+			clusterVersionObj: &configv1.ClusterVersion{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+				Status:     configv1.ClusterVersionStatus{History: []configv1.UpdateHistory{{State: "Foo"}}},
+			},
+			clusterVersionName: "cluster",
+			wantErr:            true,
+		},
+		{
+			name: "clusterVersion Completed status history does not have a version",
+			clusterVersionObj: &configv1.ClusterVersion{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+				Status:     configv1.ClusterVersionStatus{History: []configv1.UpdateHistory{{State: "Completed"}}},
+			},
+			clusterVersionName: "cluster",
+			wantErr:            true,
+		},
+		{
+			name: "clusterVersion is a standard version in x.y.z format",
+			clusterVersionObj: &configv1.ClusterVersion{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+				Status:     configv1.ClusterVersionStatus{History: []configv1.UpdateHistory{{State: "Completed", Version: "1.2.3"}}},
+			},
+			clusterVersionName: "cluster",
+			want:               "1.2",
+		},
+		{
+			name: "clusterVersion contains a build identifier",
+			clusterVersionObj: &configv1.ClusterVersion{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+				Status:     configv1.ClusterVersionStatus{History: []configv1.UpdateHistory{{State: "Completed", Version: "1.2.3-rc.0"}}},
+			},
+			clusterVersionName: "cluster",
+			want:               "1.2",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			objects := []client.Object{tt.clusterVersionObj}
+			fakeClient, err := generateFakeClient(objects...)
+			if err != nil {
+				t.Errorf("failed to generate fake client")
+			}
+			r := &MustGatherReconciler{
+				ReconcilerBase: util.NewReconcilerBase(fakeClient, fakeClient.Scheme(), &rest.Config{}, &record.FakeRecorder{}, nil),
+			}
+			got, err := r.getClusterVersionForJobTemplate(tt.clusterVersionName)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("getClusterVersionForJobTemplate() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if got != tt.want {
+				t.Errorf("getClusterVersionForJobTemplate() got = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

deploy/02_must-gather-operator.ClusterRole.yaml

@@ -3,6 +3,15 @@ apiVersion: rbac.authorization.k8s.io/v1
 metadata:
   name: must-gather-operator
 rules:
+  # cluster version
+  - apiGroups:
+      - "config.openshift.io"
+    resources:
+      - clusterversions
+      - clusterversions/status
+    verbs:
+      - list
+      - get
   # leader election
   - apiGroups:
       - ""

deploy/crds/managed.openshift.io_mustgathers.yaml

@@ -67,13 +67,6 @@ spec:
                   A flag to specify if the upload user provided in the caseManagementAccountSecret is a RH internal user.
                   See documentation for further information.
                 type: boolean
-              mustGatherImages:
-                description: 'The list of must gather images to run, optional, it
-                  will default to: $DEFAULT_MUST_GATHER_IMAGE'
-                items:
-                  type: string
-                type: array
-                x-kubernetes-list-type: set
               mustGatherTimeout:
                 description: |-
                   A time limit for gather command to complete a floating point number with a suffix:

examples/mustgather_full.yaml

@@ -7,7 +7,4 @@ spec:
   caseManagementAccountSecretRef:
     name: case-management-creds
   serviceAccountRef:
-    name: must-gather-admin
-  mustGatherImages:
-  - quay.io/kubevirt/must-gather:latest
-  - quay.io/ocs-dev/ocs-must-gather
+    name: must-gather-admin

go.mod

@@ -3,6 +3,7 @@ module github.com/openshift/must-gather-operator
 go 1.21
 
 require (
+	github.com/blang/semver/v4 v4.0.0
 	github.com/go-logr/logr v1.2.4
 	github.com/onsi/ginkgo/v2 v2.11.0
 	github.com/onsi/gomega v1.27.10
@@ -12,7 +13,6 @@ require (
 	github.com/operator-framework/operator-lib v0.11.0
 	github.com/prometheus/client_golang v1.16.0
 	github.com/redhat-cop/operator-utils v1.3.7
-	github.com/scylladb/go-set v1.0.2
 	k8s.io/api v0.28.2
 	k8s.io/apimachinery v0.28.2
 	k8s.io/client-go v0.28.2