diff --git a/.fullsend/harness/review.yaml b/.fullsend/harness/review.yaml new file mode 100644 index 00000000..af4c1b1f --- /dev/null +++ b/.fullsend/harness/review.yaml @@ -0,0 +1,55 @@ +# .fullsend/harness/review.yaml +# +# Composes the upstream fullsend review harness (pinned to agents v0.38.0) and +# adds the local coderabbit-review skill as a complementary finding source. +# v0.38.0 review harness includes all built-in dimensions (correctness, protected +# paths, intent/coherence, challenger pass). This harness extends it to also +# ingest CodeRabbit findings via GitHub comments (S3 pattern) for synthesis. +# +# CodeRabbit findings are mapped with coderabbit-* category prefixes so the +# synthesizer can dedupe and attribute them properly. The built-in review +# dimensions remain authoritative for the verdict. +# +# Trigger: Inherits from base harness (no override needed - /fs-review works +# automatically for generated PRs per https://fullsend.sh/docs/guides/user/bugfix-workflow#slash-commands) +base: https://raw.githubusercontent.com/fullsend-ai/agents/48511880eaea5ef01f80b69ba4f228147611db33/harness/review.yaml#sha256=260bf4939f52ef173911d3d788f85bd62b26802f09711d8dcdcce8dbb24c5f04 + +allowed_remote_resources: [https://raw.githubusercontent.com/fullsend-ai/agents/] + +# Extend base skills with repo-specific coderabbit-review skill +skills: + - skills/coderabbit-review + +# Note: Required env vars should be set before running fullsend: +# export PRIOR_REVIEW_SHA="" +# export PRIOR_REVIEW_PROVENANCE="none" +# export REPO_FULL_NAME="openshift/ocm-agent-operator" +# export PR_NUMBER="123" +# export GH_TOKEN="$(gh auth token)" +# +# S2 (CLI mode) - CodeRabbit API key configuration: +# The CODERABBIT_API_KEY must be set as a CI secret in your pipeline +# (OpenShift Prow, Tekton, GitHub Actions, etc.) and exposed to the +# runner-side pre-script that generates coderabbit-findings.json. +# NEVER inject this secret into the sandbox - only the runner-side +# pre-script should have access to it. +# +# Example Tekton secret: +# apiVersion: v1 +# kind: Secret +# metadata: +# name: coderabbit-api-key +# data: +# api-key: +# +# Example pre-script invocation in CI: +# export CODERABBIT_API_KEY="$(cat /path/to/secret)" +# export CODERABBIT_MODE="cli" +# .fullsend/skills/coderabbit-review/scripts/run-coderabbit.sh "$PR_NUMBER" \ +# > /tmp/workspace/coderabbit-findings.json +# +# S2 (CLI mode) host_files mapping for CodeRabbit findings: +host_files: + - src: /tmp/workspace/coderabbit-findings.json + dest: /sandbox/workspace/coderabbit-findings.json + optional: true \ No newline at end of file diff --git a/.fullsend/skills/coderabbit-review/SKILL.md b/.fullsend/skills/coderabbit-review/SKILL.md new file mode 100644 index 00000000..f4ee11cd --- /dev/null +++ b/.fullsend/skills/coderabbit-review/SKILL.md @@ -0,0 +1,107 @@ +--- +name: coderabbit-review +description: >- + Ingest CodeRabbit findings for the current PR and map them into FullSend + review findings for synthesis. Use during /fs-review after the built-in + dimension sub-agents return, as a complementary (not replacement) source. +--- + +# CodeRabbit Review Skill + +This skill adds CodeRabbit's AI review as an **extra finding source** for the +FullSend review agent. It does **not** replace `code-review` / `pr-review` — +those own protected-path checks, intent/coherence, the challenger pass, and the +JSON the post-script consumes. This skill only *gathers* CodeRabbit findings and +*maps* them into the same finding shape so the orchestrator can synthesise them. + +> Naming: this skill is intentionally named `coderabbit-review` (a novel name). +> A repo skill named `code-review` or `pr-review` would be shadowed by the +> built-in and never invoked (Personal > Project precedence). + +## When to use + +- During `/fs-review` on a pull request, after the built-in review dimensions + have produced their findings, to fold in CodeRabbit's findings. +- Not for local pre-push (`code-review`) unless CodeRabbit results are already + available for the branch. + +## Sources (in priority order) + +The CodeRabbit CLI **cannot** run inside the review sandbox (no `coderabbit` +binary, no `curl`, `api.coderabbit.ai` is not in the network allowlist, and no +`CODERABBIT_API_KEY` is injected). So this skill never invokes the CLI directly. +Instead it reads findings that already exist: + +1. **Injected file (S2, preferred for production):** if + `/sandbox/workspace/coderabbit-findings.json` exists, read it. A runner-side + pre-script produced it outside the sandbox; the API key never enters the + sandbox. + + **CI Setup for S2 mode:** + - Store `CODERABBIT_API_KEY` as a secret in your CI system (OpenShift Prow, + Tekton, GitHub Actions, etc.) + - In your CI pipeline, before the review harness runs, execute the pre-script: + ```bash + export CODERABBIT_API_KEY="$(cat /path/to/secret)" + export CODERABBIT_MODE="cli" + .fullsend/skills/coderabbit-review/scripts/run-coderabbit.sh "$PR_NUMBER" \ + > /tmp/workspace/coderabbit-findings.json + ``` + - The `host_files` mapping in `review.yaml` copies this into the sandbox + - **Never** pass the API key into the sandbox environment + +2. **GitHub ingest (S3, spike default):** otherwise run + `scripts/run-coderabbit.sh `, which uses the read-only `gh` client + already available to the review agent to pull CodeRabbit's existing PR review + comments. No extra network, binary, or secret required. + +If neither source yields findings, emit a short informational note and continue. +**Do not fail the whole review because CodeRabbit was unavailable.** + +## Step 1: Gather CodeRabbit findings + +```bash +# Prefer the injected file; fall back to GitHub ingest. +if [ -f /sandbox/workspace/coderabbit-findings.json ]; then + cat /sandbox/workspace/coderabbit-findings.json +else + scripts/run-coderabbit.sh "$PR_NUMBER" +fi +``` + +The script emits a JSON array of `{source, path, line, body, url}` objects. + +## Step 2: Map into FullSend review findings + +For each CodeRabbit item, produce a finding object with these fields: + +- `severity` — map CodeRabbit's severity to the review scale; default to a + low/`info` severity when CodeRabbit does not state one. +- `category` — prefix with `coderabbit-` (e.g. `coderabbit-correctness`, + `coderabbit-style`) so the challenger / synthesiser can dedupe against the + built-in dimensions. +- `file`, `line` — from the CodeRabbit comment (`null` for PR-level comments). +- `description` — CodeRabbit's finding text. +- `remediation` — CodeRabbit's suggested change, if present. +- `url` — link back to the CodeRabbit comment for human traceability. + +## Step 3: Respect repo path exclusions + +`.coderabbit.yaml` already excludes `boilerplate/**`, `hack/**`, `vendor/**`, +`**/testdata/**`, and generated `**/zz_generated.*.go`. Drop any CodeRabbit +finding whose `path` matches those globs so the review does not re-flag +generated or vendored code. + +## Constraints + +- **Do not** write `agent-result.json` / `review-result.json` — `pr-review` is + the sole producer of the schema-valid verdict. This skill only contributes + findings for synthesis. +- **Never** log, echo, or commit `CODERABBIT_API_KEY` or any token. +- Treat CodeRabbit findings as advisory input, not gate decisions — the + built-in dimensions and challenger remain authoritative. + +## Related + +- `scripts/run-coderabbit.sh` — GitHub ingest (S3) and the runner-only CLI (S2) + mode, with sandbox caveats documented inline. \ No newline at end of file diff --git a/.fullsend/skills/coderabbit-review/scripts/run-coderabbit.sh b/.fullsend/skills/coderabbit-review/scripts/run-coderabbit.sh new file mode 100644 index 00000000..402b02f4 --- /dev/null +++ b/.fullsend/skills/coderabbit-review/scripts/run-coderabbit.sh @@ -0,0 +1,133 @@ +#!/usr/bin/env bash +# Collect CodeRabbit findings for a PR and emit them as JSON on stdout. +# +# Modes: +# comment (default, S3) - read CodeRabbit's existing PR review comments via +# `gh`. Sandbox-safe: uses only the read-only GitHub +# client already available to the review agent. No +# CodeRabbit credentials and no egress to +# coderabbit.ai. +# cli (S2) - run the CodeRabbit CLI for a fresh review. RUNNER +# ONLY. This will FAIL inside the FullSend review +# sandbox: no `coderabbit` binary, no `curl`, +# `api.coderabbit.ai` is not in the network allowlist, +# and CODERABBIT_API_KEY is not injected. Intended to +# run in a trusted pre-script whose output is copied +# into the sandbox via `host_files`. +# +# Usage: +# scripts/run-coderabbit.sh +# CODERABBIT_MODE=cli scripts/run-coderabbit.sh +set -euo pipefail + +# Emit empty JSON array and exit (for non-fatal failures in comment mode) +emit_empty() { + echo "[]" + exit 0 +} + +REPO="${CODERABBIT_REPO:-openshift/ocm-agent-operator}" +MODE="${CODERABBIT_MODE:-comment}" +BOT="${CODERABBIT_BOT:-coderabbitai}" + +die() { echo "error: $*" >&2; exit 1; } + +PR="${1:-}" +[ -n "$PR" ] || die "usage: run-coderabbit.sh " + +case "$MODE" in + comment) + # Sandbox-compatible: uses only gh and node (both allowlisted). + # Failures are non-fatal - emit empty array if CodeRabbit data unavailable. + command -v gh >/dev/null || { echo "warning: gh CLI not found" >&2; emit_empty; } + command -v node >/dev/null || { echo "warning: node not found" >&2; emit_empty; } + + # Get current PR head commit to filter stale comments. + head_sha=$(gh api "repos/${REPO}/pulls/${PR}" --jq '.head.sha' 2>/dev/null || true) + if [ -z "$head_sha" ]; then + echo "warning: failed to get PR head commit" >&2 + emit_empty + fi + + # Fetch review comments and issue comments (non-fatal on failure). + review_raw=$(gh api --paginate "repos/${REPO}/pulls/${PR}/comments" 2>/dev/null || echo "[]") + issue_raw=$(gh api --paginate "repos/${REPO}/issues/${PR}/comments" 2>/dev/null || echo "[]") + + # Use node to filter and transform comments (sandbox-compatible). + node -e " + const bot = process.argv[1].toLowerCase(); + const headSha = process.argv[2]; + const reviewRaw = JSON.parse(process.argv[3]); + const issueRaw = JSON.parse(process.argv[4]); + + // Filter review comments: exact bot login, current commit only. + const review = reviewRaw + .filter(c => c.user?.login?.toLowerCase() === bot) + .filter(c => c.commit_id === headSha) + .map(c => ({ + source: 'coderabbit', + path: c.path, + line: c.line ?? c.original_line, + body: c.body, + url: c.html_url, + commit_id: c.commit_id + })); + + // Filter issue comments: exact bot login. + const issue = issueRaw + .filter(c => c.user?.login?.toLowerCase() === bot) + .map(c => ({ + source: 'coderabbit', + path: null, + line: null, + body: c.body, + url: c.html_url + })); + + console.log(JSON.stringify([...review, ...issue])); + " "$BOT" "$head_sha" "$review_raw" "$issue_raw" + ;; + cli) + # RUNNER ONLY - see header. Blocked inside the review sandbox. + # CODERABBIT_API_KEY must be set as a CI secret and exposed to this pre-script. + # See SKILL.md and review.yaml for CI setup instructions. + command -v coderabbit >/dev/null || die "coderabbit CLI not found (runner-only; use MODE=comment in-sandbox)" + command -v node >/dev/null || die "node not found (needed to normalize CLI output)" + [ -n "${CODERABBIT_API_KEY:-}" ] || die "CODERABBIT_API_KEY not set (must be configured as CI secret; see SKILL.md)" + + # Get CLI output (try JSON format first, fall back to plain text parsing if needed). + # Adjust flags based on your installed CLI version's capabilities. + cli_output=$(coderabbit review --format json --pr "$PR" --api-key "$CODERABBIT_API_KEY" 2>/dev/null \ + || coderabbit review --plain --pr "$PR" --api-key "$CODERABBIT_API_KEY" 2>&1 \ + || die "coderabbit CLI failed") + + # Normalize CLI output to {source, path, line, body, url}[] format. + node -e " + const raw = process.argv[1]; + let findings = []; + + try { + // Try parsing as JSON first + const parsed = JSON.parse(raw); + findings = (Array.isArray(parsed) ? parsed : [parsed]) + .filter(f => f.file && f.line && f.message) + .map(f => ({ + source: 'coderabbit', + path: f.file || f.path || null, + line: f.line || null, + body: f.message || f.body || f.comment || '', + url: f.url || null + })); + } catch (e) { + // Plain text fallback: emit empty array (CLI output not parseable) + console.error('warning: CLI output not in expected JSON format', e.message); + findings = []; + } + + console.log(JSON.stringify(findings)); + " "$cli_output" + ;; + *) + die "unknown CODERABBIT_MODE: $MODE (expected 'comment' or 'cli')" + ;; +esac \ No newline at end of file diff --git a/api/v1alpha1/managedfleetnotification_types.go b/api/v1alpha1/managedfleetnotification_types.go index 218903ab..2150f031 100644 --- a/api/v1alpha1/managedfleetnotification_types.go +++ b/api/v1alpha1/managedfleetnotification_types.go @@ -39,7 +39,7 @@ type FleetNotification struct { // References useful for context or remediation - this could be links to documentation, KB articles, etc References []NotificationReferenceType `json:"references,omitempty"` - // +kubebuilder:validation:Enum={"Debug","Info","Warning","Major","Critical","Error","Fatal"} + // +kubebuilder:validation:Enum={"Debug","Info","Warning","Major","Critical","Error","Fatal","Important","Moderate","Low"} // Re-use the severity definitation in managednotification_types Severity NotificationSeverity `json:"severity"` diff --git a/api/v1alpha1/managednotification_types.go b/api/v1alpha1/managednotification_types.go index 2c4a37bd..82fa8807 100644 --- a/api/v1alpha1/managednotification_types.go +++ b/api/v1alpha1/managednotification_types.go @@ -28,12 +28,16 @@ type NotificationSeverity string const ( SeverityDebug NotificationSeverity = "Debug" - SeverityWarning NotificationSeverity = "Warning" - SeverityInfo NotificationSeverity = "Info" - SeverityMajor NotificationSeverity = "Major" + SeverityWarning NotificationSeverity = "Warning" // Deprecated: use SeverityModerate + SeverityInfo NotificationSeverity = "Info" // Deprecated: use SeverityLow + SeverityMajor NotificationSeverity = "Major" // Deprecated: use SeverityImportant SeverityCritical NotificationSeverity = "Critical" SeverityError NotificationSeverity = "Error" SeverityFatal NotificationSeverity = "Fatal" + + SeverityImportant NotificationSeverity = "Important" + SeverityModerate NotificationSeverity = "Moderate" + SeverityLow NotificationSeverity = "Low" ) // +kubebuilder:validation:Pattern=`^https?:\/\/.+$` @@ -59,7 +63,7 @@ type Notification struct { // References useful for context or remediation - this could be links to documentation, KB articles, etc References []NotificationReferenceType `json:"references,omitempty"` - // +kubebuilder:validation:Enum={"Debug","Info","Warning","Major","Critical","Error","Fatal"} + // +kubebuilder:validation:Enum={"Debug","Info","Warning","Major","Critical","Error","Fatal","Important","Moderate","Low"} // The severity of the Service Log notification Severity NotificationSeverity `json:"severity"` diff --git a/api/v1alpha1/managednotification_types_test.go b/api/v1alpha1/managednotification_types_test.go index e3abfe44..aee5ae44 100644 --- a/api/v1alpha1/managednotification_types_test.go +++ b/api/v1alpha1/managednotification_types_test.go @@ -37,7 +37,7 @@ var _ = Describe("OCMAgent Controller", func() { Summary: "Test Summary", ActiveDesc: "Test Firing", ResolvedDesc: "Test Resolved", - Severity: "Info", + Severity: "Low", ResendWait: 1, }, }, @@ -82,7 +82,7 @@ var _ = Describe("OCMAgent Controller", func() { Name: testNotificationNameWrb, Summary: "Test Summary", ActiveDesc: "Test Firing", - Severity: "Info", + Severity: "Low", ResendWait: 1, }, }, diff --git a/controllers/ocmagent/ocmagent_controller.go b/controllers/ocmagent/ocmagent_controller.go index 7a5c52e4..0798131f 100644 --- a/controllers/ocmagent/ocmagent_controller.go +++ b/controllers/ocmagent/ocmagent_controller.go @@ -74,13 +74,18 @@ func (r *OcmAgentReconciler) Reconcile(ctx context.Context, request reconcile.Re // Request object not found, could have been deleted after reconcile request. // Owned objects are automatically garbage collected. For additional cleanup logic use finalizers. // Return and don't requeue + reqLogger.V(2).Info("OCMAgent resource not found, marking as absent") localmetrics.UpdateMetricOcmAgentResourceAbsent() return reconcile.Result{}, nil } // Error reading the object - requeue the request. - reqLogger.Error(err, "Failed to retrieve OCMAgent. Will retry on next reconcile.") + reqLogger.Error(err, "Failed to retrieve OCMAgent from API server. Will retry on next reconcile.", + "namespace", request.Namespace, "name", request.Name) return reconcile.Result{}, err } + reqLogger.V(2).Info("Successfully retrieved OCMAgent resource", + "replicas", instance.Spec.Replicas, + "fleetMode", instance.Spec.FleetMode) localmetrics.ResetMetricOcmAgentResourceAbsent() oaohandler, err := r.OCMAgentHandlerBuilder.New() diff --git a/deploy/crds/ocmagent.managed.openshift.io_managedfleetnotifications.yaml b/deploy/crds/ocmagent.managed.openshift.io_managedfleetnotifications.yaml index b164191e..c4c740b6 100644 --- a/deploy/crds/ocmagent.managed.openshift.io_managedfleetnotifications.yaml +++ b/deploy/crds/ocmagent.managed.openshift.io_managedfleetnotifications.yaml @@ -83,6 +83,9 @@ spec: - Critical - Error - Fatal + - Important + - Moderate + - Low type: string summary: description: The summary line of the notification diff --git a/deploy/crds/ocmagent.managed.openshift.io_managednotifications.yaml b/deploy/crds/ocmagent.managed.openshift.io_managednotifications.yaml index c7dcca68..4cedcb0c 100644 --- a/deploy/crds/ocmagent.managed.openshift.io_managednotifications.yaml +++ b/deploy/crds/ocmagent.managed.openshift.io_managednotifications.yaml @@ -83,6 +83,9 @@ spec: - Critical - Error - Fatal + - Important + - Moderate + - Low type: string summary: description: The summary line of the Service Log notification diff --git a/deploy_pko/.test-fixtures/config-with-proxy/CustomResourceDefinition-managedfleetnotifications.ocmagent.managed.openshift.io.yaml b/deploy_pko/.test-fixtures/config-with-proxy/CustomResourceDefinition-managedfleetnotifications.ocmagent.managed.openshift.io.yaml index d97ddf54..92061ea7 100755 --- a/deploy_pko/.test-fixtures/config-with-proxy/CustomResourceDefinition-managedfleetnotifications.ocmagent.managed.openshift.io.yaml +++ b/deploy_pko/.test-fixtures/config-with-proxy/CustomResourceDefinition-managedfleetnotifications.ocmagent.managed.openshift.io.yaml @@ -77,6 +77,9 @@ spec: - Critical - Error - Fatal + - Important + - Moderate + - Low type: string summary: description: The summary line of the notification diff --git a/deploy_pko/.test-fixtures/config-with-proxy/CustomResourceDefinition-managednotifications.ocmagent.managed.openshift.io.yaml b/deploy_pko/.test-fixtures/config-with-proxy/CustomResourceDefinition-managednotifications.ocmagent.managed.openshift.io.yaml index 5310c34d..559b1925 100755 --- a/deploy_pko/.test-fixtures/config-with-proxy/CustomResourceDefinition-managednotifications.ocmagent.managed.openshift.io.yaml +++ b/deploy_pko/.test-fixtures/config-with-proxy/CustomResourceDefinition-managednotifications.ocmagent.managed.openshift.io.yaml @@ -77,6 +77,9 @@ spec: - Critical - Error - Fatal + - Important + - Moderate + - Low type: string summary: description: The summary line of the Service Log notification diff --git a/deploy_pko/CustomResourceDefinition-managedfleetnotifications.ocmagent.managed.openshift.io.yaml b/deploy_pko/CustomResourceDefinition-managedfleetnotifications.ocmagent.managed.openshift.io.yaml index d97ddf54..92061ea7 100644 --- a/deploy_pko/CustomResourceDefinition-managedfleetnotifications.ocmagent.managed.openshift.io.yaml +++ b/deploy_pko/CustomResourceDefinition-managedfleetnotifications.ocmagent.managed.openshift.io.yaml @@ -77,6 +77,9 @@ spec: - Critical - Error - Fatal + - Important + - Moderate + - Low type: string summary: description: The summary line of the notification diff --git a/deploy_pko/CustomResourceDefinition-managednotifications.ocmagent.managed.openshift.io.yaml b/deploy_pko/CustomResourceDefinition-managednotifications.ocmagent.managed.openshift.io.yaml index 5310c34d..559b1925 100644 --- a/deploy_pko/CustomResourceDefinition-managednotifications.ocmagent.managed.openshift.io.yaml +++ b/deploy_pko/CustomResourceDefinition-managednotifications.ocmagent.managed.openshift.io.yaml @@ -77,6 +77,9 @@ spec: - Critical - Error - Fatal + - Important + - Moderate + - Low type: string summary: description: The summary line of the Service Log notification diff --git a/pkg/consts/ocmagenthandler/ocmagenthandler.go b/pkg/consts/ocmagenthandler/ocmagenthandler.go index 14559783..e299cee1 100644 --- a/pkg/consts/ocmagenthandler/ocmagenthandler.go +++ b/pkg/consts/ocmagenthandler/ocmagenthandler.go @@ -20,6 +20,8 @@ const ( OCMAgentOBONetworkPolicySuffix = "-allow-obo-alertmanager" // OCMAgentMUONetworkPolicySuffix is the name of the network policy to restrict OA for MUO OCMAgentMUONetworkPolicySuffix = "-allow-muo-communication" + // OCMAgentPrometheusNetworkPolicySuffix is the name of the network policy to allow Prometheus metrics scraping + OCMAgentPrometheusNetworkPolicySuffix = "-allow-prometheus-metrics" // OCMAgentPortName is the name of the OCM Agent service port used in the OCM Agent Deployment OCMAgentPortName = "ocm-agent" // OCMAgentPort is the container port number used by the agent for exposing its services @@ -89,6 +91,10 @@ const ( // Verified via: oc get ns observatorium-mst-production -> NotFound (on both MC and SC) NamespaceRHOBS = "rhobs-alertmanager" NamespaceOBO = "openshift-observability-operator" + // NamespacePrometheus is a dispatch key, not a literal k8s namespace: Prometheus runs in + // NamespaceMonitorng (openshift-monitoring). This key is used to create a separate + // NetworkPolicy allowing prometheus-k8s pods to scrape metrics on port 8383. + NamespacePrometheus = "prometheus-k8s" // AlertmanagerPodLabelKey/Value identifies the Alertmanager StatefulSet pods in openshift-monitoring AlertmanagerPodLabelKey = "alertmanager" @@ -105,6 +111,10 @@ const ( // Verified via: oc get po -n openshift-observability-operator -l alertmanager=hypershift-monitoring-stack OBOPodLabelKey = "alertmanager" OBOPodLabelValue = "hypershift-monitoring-stack" + // PrometheusPodLabelKey/Value identifies the Prometheus pods in openshift-monitoring. + // Verified via: oc get po -n openshift-monitoring -l app.kubernetes.io/name=prometheus + PrometheusPodLabelKey = "app.kubernetes.io/name" + PrometheusPodLabelValue = "prometheus" ) var ( diff --git a/pkg/ocmagenthandler/ocmagenthandler_networkpolicy.go b/pkg/ocmagenthandler/ocmagenthandler_networkpolicy.go index b407554f..bec07c0d 100644 --- a/pkg/ocmagenthandler/ocmagenthandler_networkpolicy.go +++ b/pkg/ocmagenthandler/ocmagenthandler_networkpolicy.go @@ -28,6 +28,8 @@ func buildNetworkPolicyName(ocmAgent ocmagentv1alpha1.OcmAgent, namespace string namespacedName = oah.BuildNamespacedName(ocmAgent.Name + oah.OCMAgentMUONetworkPolicySuffix) case oah.NamespaceOBO: namespacedName = oah.BuildNamespacedName(ocmAgent.Name + oah.OCMAgentOBONetworkPolicySuffix) + case oah.NamespacePrometheus: + namespacedName = oah.BuildNamespacedName(ocmAgent.Name + oah.OCMAgentPrometheusNetworkPolicySuffix) } return namespacedName @@ -57,6 +59,10 @@ func callerPodSelector(namespace string) (*metav1.LabelSelector, error) { return &metav1.LabelSelector{ MatchLabels: map[string]string{oah.OBOPodLabelKey: oah.OBOPodLabelValue}, }, nil + case oah.NamespacePrometheus: + return &metav1.LabelSelector{ + MatchLabels: map[string]string{oah.PrometheusPodLabelKey: oah.PrometheusPodLabelValue}, + }, nil default: return nil, fmt.Errorf("callerPodSelector: no pod selector defined for namespace %q", namespace) } @@ -67,10 +73,14 @@ func callerPodSelector(namespace string) (*metav1.LabelSelector, error) { // Alertmanager runs in NamespaceOBO alongside the OBO Alertmanager, not in a namespace of its // own (oah.NamespaceRHOBS is a dispatch key, not a literal namespace - see its doc comment). func callerNamespace(namespace string) string { - if namespace == oah.NamespaceRHOBS { + switch namespace { + case oah.NamespaceRHOBS: return oah.NamespaceOBO + case oah.NamespacePrometheus: + return oah.NamespaceMonitorng + default: + return namespace } - return namespace } func buildNetworkPolicy(ocmAgent ocmagentv1alpha1.OcmAgent, namespace string) (netv1.NetworkPolicy, error) { @@ -115,9 +125,9 @@ func buildNetworkPolicy(ocmAgent ocmagentv1alpha1.OcmAgent, namespace string) (n func (o *ocmAgentHandler) ensureAllNetworkPolicies(ctx context.Context, ocmAgent ocmagentv1alpha1.OcmAgent) error { var namespaces []string if ocmAgent.Spec.FleetMode { - namespaces = append(namespaces, oah.NamespaceMonitorng, oah.NamespaceRHOBS, oah.NamespaceOBO) + namespaces = append(namespaces, oah.NamespaceMonitorng, oah.NamespaceRHOBS, oah.NamespaceOBO, oah.NamespacePrometheus) } else { - namespaces = append(namespaces, oah.NamespaceMonitorng, oah.NamespaceMUO) + namespaces = append(namespaces, oah.NamespaceMonitorng, oah.NamespaceMUO, oah.NamespacePrometheus) } for _, ns := range namespaces { err := o.ensureNetworkPolicy(ctx, ocmAgent, ns) @@ -184,9 +194,9 @@ func (o *ocmAgentHandler) ensureNetworkPolicy(ctx context.Context, ocmAgent ocma func (o *ocmAgentHandler) ensureAllNetworkPoliciesDeleted(ctx context.Context, ocmAgent ocmagentv1alpha1.OcmAgent) error { var namespaces []string if ocmAgent.Spec.FleetMode { - namespaces = append(namespaces, oah.NamespaceMonitorng, oah.NamespaceRHOBS, oah.NamespaceOBO) + namespaces = append(namespaces, oah.NamespaceMonitorng, oah.NamespaceRHOBS, oah.NamespaceOBO, oah.NamespacePrometheus) } else { - namespaces = append(namespaces, oah.NamespaceMonitorng, oah.NamespaceMUO) + namespaces = append(namespaces, oah.NamespaceMonitorng, oah.NamespaceMUO, oah.NamespacePrometheus) } for _, ns := range namespaces { err := o.ensureNetworkPolicyDeleted(ctx, ocmAgent, ns) diff --git a/pkg/ocmagenthandler/ocmagenthandler_networkpolicy_test.go b/pkg/ocmagenthandler/ocmagenthandler_networkpolicy_test.go index a2b62dab..bca692dd 100644 --- a/pkg/ocmagenthandler/ocmagenthandler_networkpolicy_test.go +++ b/pkg/ocmagenthandler/ocmagenthandler_networkpolicy_test.go @@ -129,6 +129,27 @@ var _ = Describe("OCM Agent NetworkPolicy Handler", func() { }) }) + Context("for the Prometheus dispatch key", func() { + BeforeEach(func() { + testNamespace = oah.NamespacePrometheus + var err error + networkPolicy, err = buildNetworkPolicy(testOcmAgent, testNamespace) + Expect(err).To(BeNil()) + }) + + It("Should restrict ingress to Prometheus pods only", func() { + podSelector := networkPolicy.Spec.Ingress[0].From[0].PodSelector + Expect(podSelector).NotTo(BeNil()) + Expect(podSelector.MatchLabels).To(HaveKeyWithValue(oah.PrometheusPodLabelKey, oah.PrometheusPodLabelValue)) + }) + + It("Should scope ingress to the monitoring namespace, since NamespacePrometheus is a dispatch key", func() { + nsSelector := networkPolicy.Spec.Ingress[0].From[0].NamespaceSelector + Expect(nsSelector).NotTo(BeNil()) + Expect(nsSelector.MatchLabels).To(HaveKeyWithValue("kubernetes.io/metadata.name", oah.NamespaceMonitorng)) + }) + }) + Context("for an unrecognized namespace", func() { It("returns an error instead of silently falling back to a namespace-wide policy", func() { _, err := buildNetworkPolicy(testOcmAgent, "some-other-namespace") @@ -226,17 +247,17 @@ var _ = Describe("OCM Agent NetworkPolicy Handler", func() { Context("ensure all the required networkpolicies created", func() { When("creating a non-fleet ocm-agent", func() { - It("should have the 2 networkpolicies created", func() { - mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Times(2) - mockClient.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any()).MinTimes(2) + It("should have the 3 networkpolicies created", func() { + mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Times(3) + mockClient.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any()).MinTimes(3) err := testOcmAgentHandler.ensureAllNetworkPolicies(testconst.Context, testOcmAgent) Expect(err).To(BeNil()) }) }) When("creating a fleet ocm-agent", func() { - It("should have the 3 networkpolicies created", func() { - mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Times(3) - mockClient.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any()).Times(3) + It("should have the 4 networkpolicies created", func() { + mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Times(4) + mockClient.EXPECT().Update(gomock.Any(), gomock.Any(), gomock.Any()).Times(4) err := testOcmAgentHandler.ensureAllNetworkPolicies(testconst.Context, testFleetOcmAgent) Expect(err).To(BeNil()) }) diff --git a/pkg/util/namespace/namespace.go b/pkg/util/namespace/namespace.go index 7f912cb2..2c1e5194 100644 --- a/pkg/util/namespace/namespace.go +++ b/pkg/util/namespace/namespace.go @@ -3,6 +3,7 @@ package namespace import ( "fmt" "os" + "strings" ) // GetOperatorNamespace retrieves the operator namespace from the running environment or error if unavailable @@ -14,3 +15,51 @@ func GetOperatorNamespace() (string, error) { } return ns, nil } + +// ValidateNamespace checks if a namespace name follows Kubernetes naming conventions +func ValidateNamespace(ns string) error { + if ns == "" { + return fmt.Errorf("namespace cannot be empty") + } + + if len(ns) > 63 { + return fmt.Errorf("namespace length cannot exceed 63 characters") + } + + // Check if namespace contains only lowercase alphanumeric characters and hyphens + for _, char := range ns { + if !((char >= 'a' && char <= 'z') || (char >= '0' && char <= '9') || char == '-') { + return fmt.Errorf("namespace contains invalid character: %c", char) + } + } + + // Namespace cannot start or end with hyphen + if strings.HasPrefix(ns, "-") || strings.HasSuffix(ns, "-") { + return fmt.Errorf("namespace cannot start or end with hyphen") + } + + return nil +} + +// IsSystemNamespace checks if a namespace is a Kubernetes system namespace +func IsSystemNamespace(ns string) bool { + systemNamespaces := []string{ + "kube-system", + "kube-public", + "kube-node-lease", + "default", + } + + for _, sysNs := range systemNamespaces { + if ns == sysNs { + return true + } + } + + // Check for openshift system namespaces + if strings.HasPrefix(ns, "openshift-") { + return true + } + + return false +} diff --git a/pkg/util/namespace/namespace_test.go b/pkg/util/namespace/namespace_test.go new file mode 100644 index 00000000..89d84926 --- /dev/null +++ b/pkg/util/namespace/namespace_test.go @@ -0,0 +1,119 @@ +package namespace + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestNamespace(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Namespace Suite") +} + +var _ = Describe("ValidateNamespace", func() { + Context("when namespace name is valid", func() { + It("should accept lowercase alphanumeric names", func() { + err := ValidateNamespace("my-namespace") + Expect(err).ToNot(HaveOccurred()) + }) + + It("should accept names with numbers", func() { + err := ValidateNamespace("namespace-123") + Expect(err).ToNot(HaveOccurred()) + }) + + It("should accept single character names", func() { + err := ValidateNamespace("a") + Expect(err).ToNot(HaveOccurred()) + }) + }) + + Context("when namespace name is invalid", func() { + It("should reject empty namespace", func() { + err := ValidateNamespace("") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("cannot be empty")) + }) + + It("should reject namespace longer than 63 characters", func() { + longName := "this-is-a-very-long-namespace-name-that-exceeds-the-maximum-length-of-63-characters" + err := ValidateNamespace(longName) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("cannot exceed 63 characters")) + }) + + It("should reject namespace with uppercase letters", func() { + err := ValidateNamespace("MyNamespace") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("invalid character")) + }) + + It("should reject namespace starting with hyphen", func() { + err := ValidateNamespace("-namespace") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("cannot start or end with hyphen")) + }) + + It("should reject namespace ending with hyphen", func() { + err := ValidateNamespace("namespace-") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("cannot start or end with hyphen")) + }) + + It("should reject namespace with special characters", func() { + err := ValidateNamespace("namespace_test") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("invalid character")) + }) + }) +}) + +var _ = Describe("IsSystemNamespace", func() { + Context("when checking Kubernetes system namespaces", func() { + It("should identify kube-system as system namespace", func() { + result := IsSystemNamespace("kube-system") + Expect(result).To(BeTrue()) + }) + + It("should identify kube-public as system namespace", func() { + result := IsSystemNamespace("kube-public") + Expect(result).To(BeTrue()) + }) + + It("should identify kube-node-lease as system namespace", func() { + result := IsSystemNamespace("kube-node-lease") + Expect(result).To(BeTrue()) + }) + + It("should identify default as system namespace", func() { + result := IsSystemNamespace("default") + Expect(result).To(BeTrue()) + }) + }) + + Context("when checking OpenShift system namespaces", func() { + It("should identify openshift-prefixed namespaces as system", func() { + result := IsSystemNamespace("openshift-monitoring") + Expect(result).To(BeTrue()) + }) + + It("should identify openshift-console as system namespace", func() { + result := IsSystemNamespace("openshift-console") + Expect(result).To(BeTrue()) + }) + }) + + Context("when checking user namespaces", func() { + It("should not identify regular namespace as system", func() { + result := IsSystemNamespace("my-application") + Expect(result).To(BeFalse()) + }) + + It("should not identify namespace with openshift in middle", func() { + result := IsSystemNamespace("my-openshift-app") + Expect(result).To(BeFalse()) + }) + }) +}) diff --git a/test/deploy/60_ocmagent.ManagedFleetNotification.yaml b/test/deploy/60_ocmagent.ManagedFleetNotification.yaml index 677cac14..b9bfb0de 100644 --- a/test/deploy/60_ocmagent.ManagedFleetNotification.yaml +++ b/test/deploy/60_ocmagent.ManagedFleetNotification.yaml @@ -9,5 +9,5 @@ spec: summary: 'Action Required: Test Notification' notificationMessage: |- This is a test notification for hypershift hosted clusters. - severity: Warning - resendWait: 60 \ No newline at end of file + severity: Moderate + resendWait: 60 diff --git a/test/deploy/60_ocmagent.ManagedNotification.yaml b/test/deploy/60_ocmagent.ManagedNotification.yaml index 72ba991b..b9cbc676 100644 --- a/test/deploy/60_ocmagent.ManagedNotification.yaml +++ b/test/deploy/60_ocmagent.ManagedNotification.yaml @@ -11,5 +11,5 @@ spec: resendWait: 24 resolvedBody: |- This is a test notification when an alert is resolved. - severity: Info - summary: 'Action Required: Test Notification' \ No newline at end of file + severity: Low + summary: 'Action Required: Test Notification' diff --git a/test/e2e/ocm_agent_operator_tests.go b/test/e2e/ocm_agent_operator_tests.go index 17606264..cfe104b8 100644 --- a/test/e2e/ocm_agent_operator_tests.go +++ b/test/e2e/ocm_agent_operator_tests.go @@ -687,7 +687,7 @@ var _ = ginkgo.Describe("ocm-agent-operator", ginkgo.Ordered, func() { "name": "test-notification-e2e", "summary": "E2E Test MFN No Controller", "notificationMessage": "Testing MFN has no controller behavior", - "severity": "Info", + "severity": "Low", "resendWait": 1, }, }, @@ -712,7 +712,7 @@ var _ = ginkgo.Describe("ocm-agent-operator", ginkgo.Ordered, func() { spec := baseline.Object["spec"].(map[string]interface{}) fleetNotif := spec["fleetNotification"].(map[string]interface{}) Expect(fleetNotif["name"]).To(Equal("test-notification-e2e")) - Expect(fleetNotif["severity"]).To(Equal("Info")) + Expect(fleetNotif["severity"]).To(Equal("Low")) Expect(fleetNotif["resendWait"]).To(Equal(int64(1))) ginkgo.By("monitoring MFN for no controller activity over time")