Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -72,5 +72,19 @@
"source": "openshift:payload:cluster-version-operator",
"lifecycle": "blocking",
"environmentSelector": {}
},
{
"name": "[Jira:\"Cluster Version Operator\"] cluster-version-operator should not install resources annotated with release.openshift.io/delete=true",
"labels": {
"42543": {},
"Conformance": {},
"High": {}
},
"resources": {
"isolation": {}
},
"source": "openshift:payload:cluster-version-operator",
"lifecycle": "blocking",
"environmentSelector": {}
}
]
14 changes: 14 additions & 0 deletions pkg/cvo/external/dynamicclient/client.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package dynamicclient

import (
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/dynamic"
"k8s.io/client-go/rest"

internaldynamicclient "github.com/openshift/cluster-version-operator/pkg/cvo/internal/dynamicclient"
)

// New returns the resource client using a singleton factory
func New(config *rest.Config, gvk schema.GroupVersionKind, namespace string) (dynamic.ResourceInterface, error) {
return internaldynamicclient.New(config, gvk, namespace)
}
48 changes: 48 additions & 0 deletions test/cvo/cvo.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,18 @@ package cvo

import (
"context"
"fmt"
"strings"

g "github.com/onsi/ginkgo/v2"
o "github.com/onsi/gomega"

apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"

"github.com/openshift/cluster-version-operator/pkg/cvo/external/dynamicclient"
"github.com/openshift/cluster-version-operator/pkg/external"
"github.com/openshift/cluster-version-operator/test/oc"
ocapi "github.com/openshift/cluster-version-operator/test/oc/api"
Expand Down Expand Up @@ -99,4 +103,48 @@ var _ = g.Describe(`[Jira:"Cluster Version Operator"] cluster-version-operator`,
sccAnnotation := cvoPod.Annotations["openshift.io/scc"]
o.Expect(sccAnnotation).To(o.Equal("hostaccess"), "Expected the annotation 'openshift.io/scc annotation' on pod %s to have the value 'hostaccess', but got %s", cvoPod.Name, sccAnnotation)
})

g.It(`should not install resources annotated with release.openshift.io/delete=true`, g.Label("Conformance", "High", "42543"), func() {
ctx := context.Background()
err := util.SkipIfHypershift(ctx, restCfg)
o.Expect(err).NotTo(o.HaveOccurred(), "Failed to determine if cluster is HyperShift")
err = util.SkipIfMicroshift(ctx, restCfg)
o.Expect(err).NotTo(o.HaveOccurred(), "Failed to determine if cluster is MicroShift")

pods, err := util.GetPodsByNamespace(ctx, kubeClient, external.DefaultCVONamespace, map[string]string{"k8s-app": "cluster-version-operator"})
o.Expect(err).NotTo(o.HaveOccurred())
o.Expect(pods).NotTo(o.BeEmpty(), "Expected at least one CVO pod")

Comment on lines +114 to +117
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Guard pod lookup before indexing and target a runnable CVO pod.

Line 118 dereferences pods[0] without validating list length, and the current selection may pick a non-running pod, causing panic/flaky exec failures.

Suggested fix
 	pods, err := util.GetPodsByNamespace(ctx, kubeClient, external.DefaultCVONamespace, map[string]string{"k8s-app": "cluster-version-operator"})
 	o.Expect(err).NotTo(o.HaveOccurred())
-	logger.Info("CVO pod found", "name", pods[0].Name)
+	o.Expect(pods).NotTo(o.BeEmpty(), "Expected at least one CVO pod")
+
+	cvoPod := pods[0]
+	o.Expect(cvoPod.Status.Phase).To(o.Equal(corev1.PodRunning), "Expected selected CVO pod to be Running")
+	logger.Info("CVO pod found", "name", cvoPod.Name)
@@
-	files, err := util.ListFilesInPodContainer(ctx, restCfg, command, external.DefaultCVONamespace, pods[0].Name, "cluster-version-operator")
+	files, err := util.ListFilesInPodContainer(ctx, restCfg, command, external.DefaultCVONamespace, cvoPod.Name, "cluster-version-operator")

Also applies to: 123-124

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/cvo/cvo.go` around lines 116 - 119, The code dereferences pods[0] after
util.GetPodsByNamespace without checking the slice length and may pick a
non-running CVO causing panics or flaky execs; update the logic around pods (the
variable returned from GetPodsByNamespace in test/cvo/cvo.go) to first assert
len(pods) > 0 and return/test-fail if empty, then choose a pod in Running state
(e.g., inspect pod.Status.Phase or pod.Status.Conditions) rather than assuming
pods[0]; replace direct pods[0] usage (and the same pattern later around the
subsequent pod access) with the guarded selection of a runnable pod and handle
the no-runnable-pod case cleanly.

annotation := "release.openshift.io/delete"
manifestDir := "/release-manifests/"
command := []string{"find", manifestDir, "-iname", "*.yaml", "-exec", "grep", "-l", annotation, "{}", ";"}
files, err := util.ListFilesInPodContainer(ctx, restCfg, command, external.DefaultCVONamespace, pods[0].Name, "cluster-version-operator")
o.Expect(err).NotTo(o.HaveOccurred())
o.Expect(files).ToNot(o.BeEmpty(), "Expected to find files in manifests directory of CVO pod, but found none")

for _, f := range files {
fileContent, err := util.GetFileContentInPodContainer(ctx, restCfg, external.DefaultCVONamespace, pods[0].Name, "cluster-version-operator", f)
o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("Failed to get content of file %s in CVO pod", f))
o.Expect(fileContent).ToNot(o.BeEmpty(), fmt.Sprintf("Expected to get content of file %s in CVO pod, but got empty content", f))

if !strings.Contains(fileContent, annotation) {
continue
}

manifest, err := util.ParseManifest(fileContent)
o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("Failed to parse manifest content of file %s in CVO pod", f))

ann := manifest.Obj.GetAnnotations()
if ann[annotation] != "true" {
continue
}
Comment on lines +134 to +140
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

This checks only one document per YAML file and can miss deleted resources.

util.ParseManifest(fileContent) returns a single manifest, but release-manifest files may contain multiple YAML documents. If release.openshift.io/delete=true appears in a later document, the test can incorrectly pass.

Suggested fix direction
-			manifest, err := util.ParseManifest(fileContent)
-			o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("Failed to parse manifest content of file %s in CVO pod", f))
-
-			ann := manifest.Obj.GetAnnotations()
-			if ann[annotation] != "true" {
-				continue
-			}
-			// check object absence ...
+			manifests, err := util.ParseManifests(fileContent) // parse all docs
+			o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("Failed to parse manifest content of file %s in CVO pod", f))
+			for _, manifest := range manifests {
+				ann := manifest.Obj.GetAnnotations()
+				if ann[annotation] != "true" {
+					continue
+				}
+				// check object absence ...
+			}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/cvo/cvo.go` around lines 136 - 142, The test currently parses only the
first YAML document via util.ParseManifest(fileContent) and then checks
manifest.Obj.GetAnnotations() for the annotation variable, which misses later
documents; change the logic to iterate over all YAML documents in fileContent
(either by using a multi-doc parser such as util.ParseManifests if available, or
by splitting on the YAML document separator and calling util.ParseManifest for
each document), and for each parsed manifest check
manifest.Obj.GetAnnotations()[annotation] == "true" and handle accordingly so
deleted resources in later documents are detected.

logger.Info("Checking file: ", f, ", GVK:", manifest.GVK.String(), ", Name: ", manifest.Obj.GetName(), ", Namespace: ", manifest.Obj.GetNamespace())
client, err := dynamicclient.New(restCfg, manifest.GVK, manifest.Obj.GetNamespace())
o.Expect(err).NotTo(o.HaveOccurred())
_, err = client.Get(ctx, manifest.Obj.GetName(), metav1.GetOptions{})
o.Expect(apierrors.IsNotFound(err)).To(o.BeTrue(),
fmt.Sprintf("The deleted manifest should not be installed, but actually installed: manifest: %s %s in namespace %s from file %q, error: %v",
manifest.GVK, manifest.Obj.GetName(), manifest.Obj.GetNamespace(), f, err))
}
})
})
3 changes: 2 additions & 1 deletion test/oc/api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import (
)

type ReleaseExtractOptions struct {
To string
To string
AuthFile string
}

type VersionOptions struct {
Expand Down
3 changes: 3 additions & 0 deletions test/oc/cli/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ func NewOCCli(o api.Options) (api.OC, error) {

func (c *client) AdmReleaseExtract(o api.ReleaseExtractOptions) error {
args := []string{"adm", "release", "extract", fmt.Sprintf("--to=%s", o.To)}
if o.AuthFile != "" {
args = append(args, fmt.Sprintf("--registry-config=%s", o.AuthFile))
}
_, err := c.executor.Run(args...)
if err != nil {
return err
Expand Down
133 changes: 133 additions & 0 deletions test/util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,21 @@ import (
"bytes"
"context"
"fmt"
"io"
"os"
"strings"
"time"

g "github.com/onsi/ginkgo/v2"
o "github.com/onsi/gomega"
"github.com/pkg/errors"

corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/apimachinery/pkg/util/yaml"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
Expand All @@ -23,6 +27,7 @@ import (

configv1 "github.com/openshift/api/config/v1"
clientconfigv1 "github.com/openshift/client-go/config/clientset/versioned"
libmanifest "github.com/openshift/library-go/pkg/manifest"

"github.com/openshift/cluster-version-operator/pkg/external"
)
Expand Down Expand Up @@ -103,6 +108,134 @@ func SkipIfMicroshift(ctx context.Context, restConfig *rest.Config) error {
return nil
}

// GetAuthFile retrieves the auth file from the specified secret and writes it to a temporary file, returning the file path.
// e.g.: ns="openshift-config", secretName="pull-secret", key=".dockerconfigjson"
func GetAuthFile(ctx context.Context, client kubernetes.Interface, ns string, secretName string, key string) (filePath string, err error) {
sec, err := client.CoreV1().Secrets(ns).Get(ctx, secretName, metav1.GetOptions{})
if err != nil {
return "", err
}
secretData, ok := sec.Data[key]
if !ok {
return "", fmt.Errorf("auth key not found in secret %s/%s", ns, secretName)
}
f, err := os.CreateTemp("", "ota-*")
if err != nil {
return "", fmt.Errorf("error creating temp file: %v", err)
}
defer func() {
_ = f.Close()
}()
if _, err := f.Write(secretData); err != nil {
_ = os.Remove(f.Name())
return "", fmt.Errorf("error writing to temp file: %v", err)
}
return f.Name(), nil
}

// GetPodsByNamespace retrieves the list of pods in the specified namespace with the given label selector.
func GetPodsByNamespace(ctx context.Context, client kubernetes.Interface, namespace string, labelSelector map[string]string) ([]corev1.Pod, error) {
podList, err := client.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{
LabelSelector: labels.FormatLabels(labelSelector),
})
if err != nil {
return nil, fmt.Errorf("error listing pods in namespace %s with label selector %v: %v", namespace, labelSelector, err)
}
return podList.Items, nil
}

// ListFilesInPodContainer executes the given command in the specified container of a pod and returns the output as a list of strings.
func ListFilesInPodContainer(ctx context.Context, restConfig *rest.Config, command []string, namespace string, podName string, containerName string) ([]string, error) {
kubeClient, err := GetKubeClient(restConfig)
if err != nil {
return nil, err
}
results := []string{}
req := kubeClient.CoreV1().RESTClient().Post().
Resource("pods").
Name(podName).
Namespace(namespace).
SubResource("exec").
VersionedParams(&corev1.PodExecOptions{
Command: command,
Container: containerName,
Stdout: true,
Stderr: true,
}, scheme.ParameterCodec)
exec, err := remotecommand.NewSPDYExecutor(restConfig, "POST", req.URL())
if err != nil {
return nil, fmt.Errorf("error creating executor for pod %s/%s: %v", namespace, podName, err)
}
var stdoutBuf, stderrBuf bytes.Buffer
err = exec.StreamWithContext(ctx, remotecommand.StreamOptions{
Stdout: &stdoutBuf,
Stderr: &stderrBuf,
})
if err != nil {
return nil, fmt.Errorf("error executing command in pod %s/%s: %v, stderr: %s", namespace, podName, err, stderrBuf.String())
}
files := strings.Split(stdoutBuf.String(), "\n")
for _, file := range files {
if file == "" {
continue
}
results = append(results, file)
}
return results, nil
}

// GetFileContentInPodContainer executes the command to read the content of a file in the specified container of a pod and returns the content as a string.
func GetFileContentInPodContainer(ctx context.Context, restConfig *rest.Config, namespace string, podName string, containerName string, filePath string) (string, error) {
kubeClient, err := GetKubeClient(restConfig)
if err != nil {
return "", err
}
command := []string{"cat", filePath}
req := kubeClient.CoreV1().RESTClient().Post().
Resource("pods").
Name(podName).
Namespace(namespace).
SubResource("exec").
VersionedParams(&corev1.PodExecOptions{
Command: command,
Container: containerName,
Stdout: true,
Stderr: true,
}, scheme.ParameterCodec)
exec, err := remotecommand.NewSPDYExecutor(restConfig, "POST", req.URL())
if err != nil {
return "", fmt.Errorf("error creating executor for pod %s/%s: %v", namespace, podName, err)
}
var stdoutBuf, stderrBuf bytes.Buffer
err = exec.StreamWithContext(ctx, remotecommand.StreamOptions{
Stdout: &stdoutBuf,
Stderr: &stderrBuf,
})
if err != nil {
return "", fmt.Errorf("error executing command in pod %s/%s: %v, stderr: %s", namespace, podName, err, stderrBuf.String())
}
return stdoutBuf.String(), nil
}

// ParseManifest parses the given file content as a Kubernetes manifest and returns a Manifest object.
func ParseManifest(fileContent string) (libmanifest.Manifest, error) {
d := yaml.NewYAMLOrJSONDecoder(strings.NewReader(fileContent), 1024)
for {
m := libmanifest.Manifest{}
if err := d.Decode(&m); err != nil {
if err == io.EOF {
return m, nil
}
return m, errors.Wrapf(err, "error parsing")
}
m.Raw = bytes.TrimSpace(m.Raw)
if len(m.Raw) == 0 || bytes.Equal(m.Raw, []byte("null")) {
continue
}
return m, nil
}
}

// GetRestConfig loads the Kubernetes REST configuration from KUBECONFIG environment variable.
func GetRestConfig() (*rest.Config, error) {
return clientcmd.NewNonInteractiveDeferredLoadingClientConfig(clientcmd.NewDefaultClientConfigLoadingRules(), &clientcmd.ConfigOverrides{}).ClientConfig()
Expand Down