Capture controller pod logs and status as part of artifacts

[caxu-rh]

• Add new functions for getting the pods which match a deployment's selector label
• Add new functions for getting logs from a pod's container(s)
  • This introduces a new dependency, k8s.io/client-go, since controller-runtime basically only covers CRUD operations on resources, not logs
• Add logic in DeployableByOlmCheck to use these functions and write as artifacts

[coveralls]

coverage: 83.62% (-0.2%) from 83.812%
when pulling d8482d9 on caxu-rh:fix-eet-4864
into c2622b1 on redhat-openshift-ecosystem:main.

[dcibot]

from change #1314:

• SUCCESS https://www.distributed-ci.io/jobs/60b148c5-e083-4596-9657-1326c5492894/jobStates

[acornett21]

Just a couple of questions.

[dcibot]

from change #1314:

• SUCCESS https://www.distributed-ci.io/jobs/330dcc42-4102-4506-b067-27e01a308386/jobStates

[github-actions]

Code Review by Gemini

Overall, this is a well-structured and valuable set of changes. The introduction of k8s.io/client-go for log retrieval is a necessary and appropriate step, and the integration with the existing openshiftClient and DeployableByOlmCheck is handled cleanly.

Here's a detailed review:

internal/openshift/openshift.go

1. openshiftClient struct and NewClient function:

   • Adherence to best practices / Idiomatic Go: Introducing K8sInterface kubernetes.Interface and passing it via NewClient is excellent dependency injection. It makes the openshiftClient more flexible and testable.
   • Suggestion: None, this is well done.

2. GetDeploymentPods function:

   • Potential Issue (Security/Performance/Correctness): The current implementation has a subtle but critical flaw if deployment.Spec.Selector.MatchLabels is empty.

     selectorLabels := deployment.Spec.Selector.MatchLabels
     // ...
     labelSelector := crclient.MatchingLabels{}
     maps.Copy(labelSelector, selectorLabels) // If selectorLabels is empty, labelSelector will be empty.
     // ...
     err = oe.Client.List(ctx, &podList, labelSelector) // If labelSelector is empty, this lists ALL pods in the namespace.

     When crclient.MatchingLabels{} is empty, controller-runtime's client.List operation will effectively list all pods in the target namespace (or even cluster-wide if the client is configured for it). This is almost certainly not the intended behavior for "Get pods of a Deployment". A deployment with an empty MatchLabels selector is unusual and typically invalid or not managing any pods.
   • Recommendation (Correctness/Performance):
     • If deployment.Spec.Selector.MatchLabels is empty, the function should either return an empty slice of pods or an error, as it's an ambiguous state for "pods of this deployment". Returning an empty slice is generally more graceful for a "get" operation.
     • Additionally, it's good practice to explicitly specify the namespace for List operations, even if the client might default to it.

     // GetDeploymentPods can return an ErrNotFound
     func (oe *openshiftClient) GetDeploymentPods(ctx context.Context, name string, namespace string) ([]corev1.Pod, error) {
     	logger := logr.FromContextOrDiscard(ctx)
     
     	deployment, err := oe.GetDeployment(ctx, name, namespace)
     	if err != nil {
     		return nil, err
     	}
     
     	selectorLabels := deployment.Spec.Selector.MatchLabels
     	if len(selectorLabels) == 0 {
     		// A deployment without selector labels is unusual and might not manage pods
     		// in the way expected. Returning an empty list is safer than
     		// potentially listing all pods in the namespace.
     		logger.V(log.TRC).Info("deployment has no selector labels defined, returning empty pod list", "namespace", namespace, "name", name)
     		return []corev1.Pod{}, nil
     	}
     
     	labelSelector := crclient.MatchingLabels{}
     	maps.Copy(labelSelector, selectorLabels)
     
     	podList := corev1.PodList{}
     	// Explicitly filter by namespace and labels.
     	err = oe.Client.List(ctx, &podList, labelSelector, crclient.InNamespace(namespace))
     	if err != nil {
     		return nil, fmt.Errorf("could not list pods matching label selector: %v", err)
     	}
     
     	return podList.Items, nil
     }

3. GetPod function:

   • Adherence to best practices / Idiomatic Go: Standard controller-runtime client usage, good error handling.
   • Suggestion: None.

4. getContainerLogs (private helper function):

   • Adherence to best practices / Idiomatic Go: Correctly uses client-go for log streaming. defer logs.Close() ensures resource cleanup. Error handling is robust.
   • Performance: Streaming directly to bytes.Buffer is efficient for in-memory collection.
   • Security: Accessing logs requires appropriate RBAC permissions for the service account used by the tool. This is an inherent requirement for log collection and not a vulnerability introduced by the code itself. The code correctly uses the provided K8sInterface.
   • Suggestion: None.

5. GetPodLogs function:

   • Adherence to best practices / Idiomatic Go: Comprehensive, covering InitContainers, Containers, and EphemeralContainers. The strategy of logging errors and continuing for individual container log failures is appropriate for artifact collection.
   • Performance: Collects all logs into bytes.Buffers in memory. For extremely verbose logs across many containers, this could consume significant memory. However, for typical operator controller logs, this should be acceptable. The DeployableByOlmCheck then writes these buffers to disk, which is a reasonable two-step process.
   • Suggestion: None.

internal/openshift/openshift_test.go

1. Test Setup:
   • Adherence to best practices: The switch to fakecr.NewClientBuilder and fakecg.NewClientset() for controller-runtime and client-go clients respectively is good for testing.
   • Test Coverage: The tests for GetDeploymentPods, GetPod, and GetPodLogs are well-written and cover various scenarios (existing, non-existent, no matching pods).
   • Observation: The fakecg.NewClientset() provides a default "fake logs" string for log streams, which is sufficient for testing the log retrieval mechanism's flow without needing complex mocking.
   • Suggestion: None, the tests are good.

internal/openshift/types.go

1. Client interface:
   • Adherence to best practices: Correctly extends the interface to expose the new functionality.
   • Suggestion: None.

internal/policy/operator/deployable_by_olm.go

1. DeployableByOlmCheck struct and initClient, initOpenShiftEngine functions:

   • Adherence to best practices / Idiomatic Go: The k8sClientset field and its initialization in initClient and subsequent passing to openshift.NewClient demonstrate good dependency management.
   • Suggestion: None.

2. cleanUp function:

   • Adherence to best practices / Idiomatic Go: This is where the new functionality is put to use. The logic for iterating deployments, then their pods, and then each container's status and logs, is well-structured.
   • Error Handling: Logging warnings for failures during artifact collection (e.g., unable to retrieve deployment pods or logs) and continuing is a good strategy, ensuring that as many artifacts as possible are collected.
   • Artifact Naming: The filenames [pod.Name]-PodStatus.json and podLogs-[pod.Name]-[container].json are clear and descriptive.
   • Security: As mentioned before, logs can contain sensitive information. The act of collecting them is a design choice of the tool, not a vulnerability in the code itself. Ensure that the context in which these artifacts are stored and accessed aligns with security policies.
   • Suggestion: None, this part is well implemented.

Summary of Key Feedback:

The most important suggestion is to address the behavior of GetDeploymentPods when deployment.Spec.Selector.MatchLabels is empty to prevent it from listing all pods in the namespace.

Apart from that, the changes are solid, follow best practices, and are well-tested.

[acornett21]

The new files should be added to the .gitignore

[dcibot]

from change #1314:

• SUCCESS https://www.distributed-ci.io/jobs/758548bb-eef9-43d8-8837-d45349f3fc8f/jobStates

[acornett21]

/test 4.20-e2e

[dcibot]

from change #1314:

• SUCCESS https://www.distributed-ci.io/jobs/c7d7bae7-35ea-4d91-a8cf-b4c94d17562c/jobStates

[dcibot]

from change #1314:

• SUCCESS https://www.distributed-ci.io/jobs/2faece92-e27c-40d3-af28-525c2e2707da/jobStates

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acornett21, caxu-rh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [acornett21]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment