NO-ISSUE: adds kubeconform kube-linter and pre-push hooks to osac-operator#218
NO-ISSUE: adds kubeconform kube-linter and pre-push hooks to osac-operator#218wgordon17 wants to merge 6 commits into
Conversation
|
@wgordon17: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
|
Warning Review limit reached
More reviews will be available in 47 minutes and 25 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughAdds CI manifest validation, kube-linter config, pre-push hooks, Makefile targets to install kubeconform/kube-linter, refines golangci exclusions, updates tests to use t.Setenv, and documents hook installation. ChangesKubernetes Validation and Developer Workflow Tooling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Security & Risk: Low–Moderate — the changes add manifest validation but intentionally exclude some kube-linter checks (e.g., Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
642f04f to
0f97c03
Compare
a89b558 to
f38d5e5
Compare
bcd8dc9 to
0325ee6
Compare
0325ee6 to
f3e0abe
Compare
|
/retest |
f3e0abe to
0bef91c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/check-pull-request.yaml:
- Line 66: The CI uses a mutable branch URL for the CRD schema (the
schema-location
'https://raw.githubusercontent.com/datreeio/CRDs-catalog/main/{{.Group}}/{{.ResourceKind}}_{{.ResourceAPIVersion}}.json');
change that to a pinned immutable ref by replacing "main" with a commit SHA or a
release tag (e.g.,
'https://raw.githubusercontent.com/datreeio/CRDs-catalog/<COMMIT_OR_TAG>/{{.Group}}/{{.ResourceKind}}_{{.ResourceAPIVersion}}.json'),
or make the ref configurable via an input/env var so the workflow can be updated
to a specific commit/tag and ensure reproducible kubeconform validations.
In `@helpers/getenvwithdefault_test.go`:
- Around line 33-35: The test currently skips calling t.Setenv when tt.envValue
== "", so the "string with empty env" case is exercising an unset variable
instead of an explicitly empty value; change the test loop in
helpers/getenvwithdefault_test.go to always call t.Setenv(tt.envVar,
tt.envValue) (remove the if-check) so empty string values are actually set, and
if you still need to test a missing-variable case add a distinct test case
(e.g., name "missing env") that leaves the variable unset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 447dc006-2e75-451e-93dc-e79268efd9ce
📒 Files selected for processing (8)
.github/workflows/check-pull-request.yaml.golangci.yml.kube-linter.yaml.pre-commit-config.yamlCLAUDE.mdMakefilehelpers/getenvwithdefault_test.gointernal/consoleproxy/console_test.go
- Bumps kubeconform kubernetes-version from 1.31.0 to 1.34.0 for OpenShift 4.21 - Fixes empty-env test case to actually set env var to empty string Generated-By: Claude Code (Anthropic) Signed-off-by: Will Gordon <wgordon@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
helpers/getenvwithdefault_test.go (1)
171-188: ⚡ Quick winConsider splitting type-specific validation tests into subtests.
The current pattern uses
t.Setenvfor the initial value andos.Setenvfor the override within the same test. While functional, splitting into subtests would be more idiomatic and clearer.♻️ Example refactor for TestGetEnvWithDefault_StringValidation
func TestGetEnvWithDefault_StringValidation(t *testing.T) { - t.Setenv("TEST_MODE", "production") - validModes := func(v string) bool { return v == "development" || v == "staging" || v == "production" } - result := GetEnvWithDefault("TEST_MODE", "development", validModes) - if result != "production" { - t.Errorf("expected production, got %v", result) - } + t.Run("valid value", func(t *testing.T) { + t.Setenv("TEST_MODE", "production") + result := GetEnvWithDefault("TEST_MODE", "development", validModes) + if result != "production" { + t.Errorf("expected production, got %v", result) + } + }) - os.Setenv("TEST_MODE", "invalid") //nolint:errcheck // overriding within t.Setenv-managed scope - result = GetEnvWithDefault("TEST_MODE", "development", validModes) - if result != "development" { - t.Errorf("expected default development, got %v", result) - } + t.Run("invalid value", func(t *testing.T) { + t.Setenv("TEST_MODE", "invalid") + result := GetEnvWithDefault("TEST_MODE", "development", validModes) + if result != "development" { + t.Errorf("expected default development, got %v", result) + } + }) }Apply the same pattern to
TestGetEnvWithDefault_BoolValidation,TestGetEnvWithDefault_Float64Validation, andTestGetEnvWithDefault_DurationValidation.Also applies to: 190-206, 208-224, 226-243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@helpers/getenvwithdefault_test.go` around lines 171 - 188, Split each type-specific test into subtests using t.Run so each scenario uses its own t.Setenv-scoped environment instead of mixing t.Setenv and os.Setenv; for TestGetEnvWithDefault_StringValidation, create subtests (e.g., "valid mode" and "invalid mode") and call t.Setenv("TEST_MODE", ...) inside each subtest, then call GetEnvWithDefault and assert results, removing the os.Setenv override; apply the same pattern to TestGetEnvWithDefault_BoolValidation, TestGetEnvWithDefault_Float64Validation, and TestGetEnvWithDefault_DurationValidation to ensure isolated env setup and clearer test structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@helpers/getenvwithdefault_test.go`:
- Around line 171-188: Split each type-specific test into subtests using t.Run
so each scenario uses its own t.Setenv-scoped environment instead of mixing
t.Setenv and os.Setenv; for TestGetEnvWithDefault_StringValidation, create
subtests (e.g., "valid mode" and "invalid mode") and call t.Setenv("TEST_MODE",
...) inside each subtest, then call GetEnvWithDefault and assert results,
removing the os.Setenv override; apply the same pattern to
TestGetEnvWithDefault_BoolValidation, TestGetEnvWithDefault_Float64Validation,
and TestGetEnvWithDefault_DurationValidation to ensure isolated env setup and
clearer test structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bb06e220-ed88-4fb1-8658-557d6115b306
📒 Files selected for processing (2)
.github/workflows/check-pull-request.yamlhelpers/getenvwithdefault_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/check-pull-request.yaml
- Bumps kubeconform kubernetes-version from 1.31.0 to 1.34.0 for OpenShift 4.21 - Fixes empty-env test case to actually set env var to empty string Generated-By: Claude Code (Anthropic) Signed-off-by: Will Gordon <wgordon@redhat.com>
01f5871 to
dfa5703
Compare
5aff764 to
7df8088
Compare
- Bumps kubeconform kubernetes-version from 1.31.0 to 1.34.0 for OpenShift 4.21 - Fixes empty-env test case to actually set env var to empty string Generated-By: Claude Code (Anthropic) Signed-off-by: Will Gordon <wgordon@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/check-pull-request.yaml (1)
22-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConstrain
check-generated-codeto read-only token permissions.Risk: major. This job inherits the repository default
GITHUB_TOKENscope, while the sibling job is already locked down tocontents: read.check-generated-codeonly needs checkout and diff access, so leaving broader defaults here increases blast radius for PR-controlled generation steps. As per coding guidelines, "Least privilege: minimize GITHUB_TOKEN permissions".Suggested fix
check-generated-code: name: Check generated code runs-on: ubuntu-latest + permissions: + contents: read steps:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/check-pull-request.yaml around lines 22 - 40, The check-generated-code job is using the default GITHUB_TOKEN scope; restrict it to read-only by adding a permissions block to the job (e.g., permissions: contents: read) so the checkout/diff steps (actions/checkout, bufbuild/buf-action, the git diff and make commands under the job and the "Check controller-gen output is up to date" step) only have read access; update the check-generated-code job definition to include that permissions entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.golangci.yml:
- Line 40: The path pattern in .golangci.yml uses an unanchored regex
"(internal/|pkg/)" which can match substrings anywhere; update the pattern to
anchor it (e.g., "^(internal/|pkg/)") so it only matches top-level internal/ or
pkg/ directories; apply the same anchoring change to the other similar path
patterns in the file (the other occurrences of "(internal/|pkg/)") to keep
matching behavior consistent for the linter configuration.
- Around line 33-48: The repo enables the errcheck linter but test files aren't
excluded, so unhandled error returns in *_test.go will be flagged; either fix
the tests to explicitly handle or check returned errors (update test
functions/assertions in test files) or, if intentionally ignoring errors, add a
targeted exclusion for errcheck under exclusions.rules (mirror the existing
dupl/_test\.go entry) or apply per-case //nolint:errcheck comments; reference
the errcheck linter name, the exclusions.rules section in .golangci.yml, and the
_test\.go path pattern when making the change.
In @.kube-linter.yaml:
- Around line 3-44: The current .kube-linter.yaml indiscriminately disables
checks via checks.exclude (e.g., wildcard-in-rules, no-read-only-root-fs,
non-isolated-pod), which applies cluster-wide; instead narrow these exceptions
by moving them from global checks.exclude into kind/object-scoped suppressions
(use skipKinds/rejectKinds or customChecks.scope.objectKinds for the specific
checks) for only the operator manager Deployment/ClusterRole objects, or split
lint steps so one kube-linter run uses a permissive config only against the
operator overlay while another run keeps strict defaults for the rest of the
kustomize build; update references to checks.exclude and the named checks
(wildcard-in-rules, no-read-only-root-fs, non-isolated-pod, etc.) accordingly.
In @.pre-commit-config.yaml:
- Around line 32-37: The pre-push hook "kustomize-build" assumes bin/kustomize
exists; update the hook's entry so it first invokes the Makefile target that
installs the kustomize binary and then runs the build (i.e., replace the current
entry command with a two-step command that runs the Makefile target that
installs kustomize—e.g., make <install-kustomize-target>—and then runs
bin/kustomize build config/default > /dev/null), keeping the hook id
"kustomize-build" and the stages unchanged.
- Around line 39-44: The pre-push hook "check-generated" currently runs "make
manifests generate" then diffs only config/rbac/role.yaml, config/crd/ and
api/v1alpha1/zz_generated.deepcopy.go; update the hook's entry (the entry string
under id: check-generated) to also verify Helm CRD sync by running "make
check-helm-crds" (or "make helm-crds" then verifying) and include the Helm
templates directory in the git diff (charts/operator-crds/templates/) so the
final command looks like: run "make manifests generate && make check-helm-crds
&& git diff --exit-code config/crd/ config/rbac/role.yaml
charts/operator-crds/templates/ api/v1alpha1/zz_generated.deepcopy.go" (adjust
spacing/quoting to match existing bash -c usage).
In `@AGENTS.md`:
- Around line 105-119: The documentation currently shows "pre-commit run
--all-files" but omits the manual command to run pre-push hooks; update the Hook
Installation section to add the pre-push validation command (e.g., run
pre-commit with --hook-type pre-push and --all-files to execute kustomize-build
and check-generated) and include a short example that mirrors the existing
commit-stage example so developers can manually validate
`.pre-commit-config.yaml` pre-push hooks like kustomize-build and
check-generated.
In `@Makefile`:
- Around line 365-371: Remove the leftover merge-conflict markers (<<<<<<<,
|||||||, =======, >>>>>>>) from the Makefile block used by the go-install-tool
target and ensure the GOTOOLCHAIN assignment is a single valid shell/Make
assignment; replace the conflicted lines with one consistent line such as using
the intended variable (e.g., GOTOOLCHAIN=$(GOTOOLCHAIN) GOBIN=$(LOCALBIN) go
install $${package} or GOTOOLCHAIN=auto GOBIN=$(LOCALBIN) go install
$${package}) so targets that bootstrap Go tools like controller-gen, kustomize,
and kubeconform run without syntax errors.
---
Outside diff comments:
In @.github/workflows/check-pull-request.yaml:
- Around line 22-40: The check-generated-code job is using the default
GITHUB_TOKEN scope; restrict it to read-only by adding a permissions block to
the job (e.g., permissions: contents: read) so the checkout/diff steps
(actions/checkout, bufbuild/buf-action, the git diff and make commands under the
job and the "Check controller-gen output is up to date" step) only have read
access; update the check-generated-code job definition to include that
permissions entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: fc621a9b-2404-4699-b3cb-632cca27fb6a
📒 Files selected for processing (8)
.github/workflows/check-pull-request.yaml.golangci.yml.kube-linter.yaml.pre-commit-config.yamlAGENTS.mdMakefilehelpers/getenvwithdefault_test.gointernal/consoleproxy/subresource_test.go
💤 Files with no reviewable changes (1)
- internal/consoleproxy/subresource_test.go
| - linters: | ||
| - lll | ||
| path: api/ | ||
| path: (internal/|pkg/) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider anchoring the path regex for precision.
The unanchored pattern (internal/|pkg/) will match any path containing those substrings, including potentially vendor/.../internal/ or nested test fixtures. While vendor is likely excluded via the global paths section, anchoring the pattern (e.g., ^(internal/|pkg/)) would make the intent clearer and avoid unintended matches.
Risk: Low. May inadvertently exclude the lll linter from unintended paths, but lll is cosmetic and poses no security risk.
📐 Suggested refinement
- path: (internal/|pkg/)
+ path: ^(internal/|pkg/)Apply similar anchoring to lines 44 and 48 if you want strict path matching.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| path: (internal/|pkg/) | |
| path: ^(internal/|pkg/) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.golangci.yml at line 40, The path pattern in .golangci.yml uses an
unanchored regex "(internal/|pkg/)" which can match substrings anywhere; update
the pattern to anchor it (e.g., "^(internal/|pkg/)") so it only matches
top-level internal/ or pkg/ directories; apply the same anchoring change to the
other similar path patterns in the file (the other occurrences of
"(internal/|pkg/)") to keep matching behavior consistent for the linter
configuration.
| exclude: | ||
| # The manager image uses `controller:latest` as a placeholder in config/manager/manager.yaml. | ||
| # The real image tag is injected by kustomize edit set image at build/deploy time. | ||
| - latest-tag | ||
|
|
||
| # The manager binds its health endpoints on :8081 via --health-probe-bind-address=:8081. | ||
| # Kubernetes does not require containerPort declarations for probes to function. | ||
| # Declaring the port is cosmetic; omitting it is the upstream operator-sdk scaffold default. | ||
| - liveness-port | ||
| - readiness-port | ||
|
|
||
| # The operator manager needs a writable filesystem (Go runtime tmp, controller-runtime caches). | ||
| # readOnlyRootFilesystem is not set in the upstream operator-sdk scaffold. | ||
| - no-read-only-root-fs | ||
|
|
||
| # Admin ClusterRoles intentionally use wildcard verbs — they are aggregated admin roles | ||
| # intended for human operators, not the controller itself. | ||
| - wildcard-in-rules | ||
|
|
||
| # Operator-sdk scaffold does not set dnsConfig. Standard cluster DNS is sufficient. | ||
| - dnsconfig-options | ||
|
|
||
| # Controller-manager runs as a single replica by design (leader-election for HA). | ||
| # Multiple replicas compete via lease, so minimum-three-replicas is inapplicable. | ||
| - minimum-three-replicas | ||
|
|
||
| # Operator pods do not declare node affinity; scheduling is left to the cluster default. | ||
| - no-node-affinity | ||
|
|
||
| # operator-sdk scaffold does not set updateStrategy on the Deployment explicitly. | ||
| # Kubernetes defaults to RollingUpdate, so this check produces a false positive. | ||
| - no-rolling-update-strategy | ||
|
|
||
| # NetworkPolicy is managed externally (osac-installer kustomize overlays). | ||
| - non-isolated-pod | ||
|
|
||
| # Ownership annotations and labels are not part of the operator-sdk scaffold convention. | ||
| - required-annotation-email | ||
| - required-label-owner | ||
|
|
||
| # Deployment has no explicit restartPolicy; Kubernetes defaults to Always for Deployments. | ||
| - restart-policy |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== .kube-linter.yaml =="
ls -la .kube-linter.yaml || true
nl -ba .kube-linter.yaml || true
echo
echo "== Search for kube-linter config usage =="
rg -n "kube-linter|kube_linter|kube-linter\.yaml|--config" -S . || true
echo
echo "== Search for other kube-linter configs =="
fd -a "kube-linter.*\.ya?ml" . || true
fd -a ".kube-linter.*\.ya?ml" . || trueRepository: osac-project/osac-operator
Length of output: 698
🌐 Web query:
kube-linter config checks.exclude scope global vs resource-specific yaml schema
💡 Result:
In kube-linter (KubeLinter), exclude behavior is defined at two different “scopes”, depending on what you mean by “scope global vs resource-specific”: 1) Global (check selection) via checks.exclude (YAML config) - The checks section supports include/exclude lists of check names, and exclude “always takes precedence” over include: if a check is present in exclude, KubeLinter skips it even if it appears in include too. This is configuration for which checks run at all, not for which Kubernetes object types they apply to. [1] 2) Resource-specific behavior via customChecks.scope - For custom checks, you can constrain what Kubernetes object kinds the check applies to using customChecks[].scope.objectKinds (for example, restricting a template-based check to Deployment-like kinds). This is how you get resource-specific applicability. [1] - Note: the provided docs snippet is about objectKinds scoping for customChecks, not about checks.exclude scoping to specific resource kinds. [1] What this means for your “global vs resource-specific yaml schema” question - checks.exclude is global in the sense that it excludes checks by name from consideration during linting (it doesn’t express a YAML schema that targets only certain resource kinds). [1] - To make a check apply only to certain resources, you use customChecks.scope.objectKinds (or the equivalent scope fields supported by that template), which is part of the custom check schema rather than the checks.exclude schema. [1] Practical implication - If you want “run check X everywhere except resource kind Y”, KubeLinter’s config model (per the documented schema) doesn’t provide that exact split using checks.exclude alone; instead you would typically model the resource restriction in customChecks.scope, and/or use include/exclude at the check-name level for broad enable/disable. [1]
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== .kube-linter.yaml (numbered) =="
# nl may not exist; use cat -n instead
cat -n .kube-linter.yaml
echo
echo "== Makefile: how kube-linter is run =="
rg -n --hidden "kube-linter|KUBE_LINTER|--config|--config-file|config" Makefile
echo
echo "== Search for --config usage in repo =="
rg -n --hidden --glob='*{yml,yaml,go,sh,Makefile,txt,md}' --glob='!**/vendor/**' "kube-linter.*--config|--config.*kube-linter|kube-linter.*config" .Repository: osac-project/osac-operator
Length of output: 5236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== check-pull-request.yaml kube-linter invocation =="
nl -ba .github/workflows/check-pull-request.yaml | sed -n '40,110p'
echo
echo "== Any other kube-linter invocations =="
rg -n "kube-linter.*lint|--config .kube-linter.yaml|kube-linter" .github/workflows -S || trueRepository: osac-project/osac-operator
Length of output: 184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== check-pull-request.yaml kube-linter invocation =="
cat -n .github/workflows/check-pull-request.yaml | sed -n '50,95p'
echo
echo "== Show how RENDERED is produced =="
cat -n .github/workflows/check-pull-request.yaml | sed -n '1,60p'
echo
echo "== Search for any other kube-linter lint calls =="
rg -n "kube-linter lint|stackrox/kube-linter|--config \.kube-linter\.yaml" .github/workflows -S || trueRepository: osac-project/osac-operator
Length of output: 3311
🌐 Web query:
kube-linter checks.exclude scope object kinds built-in checks exclude by kind
💡 Result:
KubeLinter’s “exclude” is for excluding checks by check name (not for excluding individual built-in checks by the Kubernetes object kind they evaluate). The documented configuration keys are checks.include and checks.exclude, where exclude disables specific checks by name and always takes precedence if the same check appears in include and exclude [1]. As for excluding by object kind, KubeLinter’s built-in check templates reference template parameters skipKinds and rejectKinds, which are “resource kinds to ignore/reject during validation” (i.e., an in-check mechanism to skip certain resource kinds while the check runs) [2]. Those parameters are not the same as checks.exclude, and they only apply to checks/templates that define and use them. So, in practice: 1) Use checks.exclude to skip entire checks (by name) across all object kinds [1]. 2) If a specific check supports kind-based filtering, use the corresponding template parameter(s) (skipKinds / rejectKinds) for that check’s configuration; otherwise there’s no generic “built-in exclude by kind” knob shown in the top-level docs [2].
Citations:
- 1: https://github.com/stackrox/kube-linter/blob/main/docs/configuring-kubelinter.md
- 2: https://github.com/stackrox/kube-linter/blob/main/docs/generated/templates.md
🌐 Web query:
kube-linter check "wildcard-in-rules" "no-read-only-root-fs" "non-isolated-pod"
💡 Result:
KubeLinter includes several built-in checks designed to identify security misconfigurations and enforce best practices in Kubernetes manifests [1][2][3]. The specific checks you mentioned are described below: no-read-only-root-fs Description: This check identifies containers that are running without a read-only root filesystem [1]. Running with a writable root filesystem can allow an attacker who gains shell access to modify container binaries or inject malicious files [4]. Remediation: Set the readOnlyRootFilesystem field to true within the container's securityContext [1][3]. Status: This check is typically enabled by default [1]. non-isolated-pod Description: This check alerts on deployment-like objects (such as Pods) that are not selected by any NetworkPolicy [1]. Without a NetworkPolicy, pods may be exposed to unnecessary or unauthorized traffic from other pods or namespaces [1]. Remediation: Isolate the pod by defining a NetworkPolicy that restricts traffic to and from the pod [1]. Status: This check is generally not enabled by default [1]. wildcard-in-rules Description: This check flags the use of wildcards (e.g., *) within Role or ClusterRole rules [1]. As noted in the CIS Kubernetes Benchmark (5.1.3), using wildcards is not optimal for security because it may inadvertently grant excessive access to new resources, such as custom resource definitions (CRDs), as they are added to the Kubernetes API [1]. Remediation: Replace wildcard entries in roles and clusterroles with specific object names or actions whenever possible [1]. Status: This check is generally not enabled by default [1]. You can list all available checks in KubeLinter by running the command kube-linter checks list [5][6]. Many of these checks are configurable, allowing you to enable or disable them based on your specific security policies [5][3].
Citations:
- 1: https://github.com/stackrox/kube-linter/blob/main/docs/generated/checks.md
- 2: https://docs.kubelinter.io/
- 3: https://github.com/stackrox/kube-linter
- 4: https://dev.to/vijaykanth-devops/8-kubernetes-security-misconfigurations-that-make-it-to-production-and-how-to-fix-them-439l
- 5: https://github.com/pacopeng/paco-acs-demo
- 6: https://www.redhat.com/en/blog/what-is-kubelinter
Narrow kube-linter checks.exclude to avoid disabling security checks across the entire CI lint input.
.kube-linter.yaml disables security-focused built-in checks (e.g., wildcard-in-rules, no-read-only-root-fs, non-isolated-pod) via checks.exclude, and CI applies this config to the full rendered output from config/default (kustomize build config/default). That makes the exceptions effectively check-wide for every object in that bundle, so later RBAC/workload hardening regressions can slip through without detection.
- Use kind/object-scoped suppression instead of check-wide excludes (e.g., template
skipKinds/rejectKindswhere supported, orcustomChecks.scope.objectKinds). - If kind-scoped filtering isn’t available for a check, split kube-linter runs/configs so the strict checks remain enabled for everything except the explicitly intended resources.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.kube-linter.yaml around lines 3 - 44, The current .kube-linter.yaml
indiscriminately disables checks via checks.exclude (e.g., wildcard-in-rules,
no-read-only-root-fs, non-isolated-pod), which applies cluster-wide; instead
narrow these exceptions by moving them from global checks.exclude into
kind/object-scoped suppressions (use skipKinds/rejectKinds or
customChecks.scope.objectKinds for the specific checks) for only the operator
manager Deployment/ClusterRole objects, or split lint steps so one kube-linter
run uses a permissive config only against the operator overlay while another run
keeps strict defaults for the rest of the kustomize build; update references to
checks.exclude and the named checks (wildcard-in-rules, no-read-only-root-fs,
non-isolated-pod, etc.) accordingly.
| - id: kustomize-build | ||
| name: validate kustomize build | ||
| entry: bash -c 'bin/kustomize build config/default > /dev/null' | ||
| language: system | ||
| pass_filenames: false | ||
| stages: [pre-push] |
There was a problem hiding this comment.
Bootstrap kustomize inside the pre-push hook.
Risk: major. This hook assumes bin/kustomize already exists, but make install-hooks only installs hooks, not tool binaries. On a clean clone that makes pre-push fail before validation even starts. Reuse the Makefile target that installs the binary first.
Suggested fix
- id: kustomize-build
name: validate kustomize build
- entry: bash -c 'bin/kustomize build config/default > /dev/null'
+ entry: bash -c 'make kustomize && bin/kustomize build config/default > /dev/null'
language: system
pass_filenames: false
stages: [pre-push]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.pre-commit-config.yaml around lines 32 - 37, The pre-push hook
"kustomize-build" assumes bin/kustomize exists; update the hook's entry so it
first invokes the Makefile target that installs the kustomize binary and then
runs the build (i.e., replace the current entry command with a two-step command
that runs the Makefile target that installs kustomize—e.g., make
<install-kustomize-target>—and then runs bin/kustomize build config/default >
/dev/null), keeping the hook id "kustomize-build" and the stages unchanged.
| - id: check-generated | ||
| name: check generated code is up to date | ||
| entry: bash -c 'make manifests generate && git diff --exit-code config/crd/ config/rbac/role.yaml api/v1alpha1/zz_generated.deepcopy.go' | ||
| language: system | ||
| pass_filenames: false | ||
| stages: [pre-push] |
There was a problem hiding this comment.
Include Helm CRD sync in the generated-file gate.
Risk: major. This hook regenerates CRDs but never verifies charts/operator-crds/templates/, so a CRD change can pass pre-push with stale Helm templates. Fold make check-helm-crds into this gate so the chart stays coupled to the generated CRDs. Based on learnings, "Always run make helm-crds after regenerating CRDs, or run make check-helm-crds to verify sync".
Suggested fix
- id: check-generated
name: check generated code is up to date
- entry: bash -c 'make manifests generate && git diff --exit-code config/crd/ config/rbac/role.yaml api/v1alpha1/zz_generated.deepcopy.go'
+ entry: bash -c 'make manifests generate generate check-helm-crds && git diff --exit-code config/crd/ config/rbac/role.yaml api/v1alpha1/zz_generated.deepcopy.go'
language: system
pass_filenames: false
stages: [pre-push]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.pre-commit-config.yaml around lines 39 - 44, The pre-push hook
"check-generated" currently runs "make manifests generate" then diffs only
config/rbac/role.yaml, config/crd/ and api/v1alpha1/zz_generated.deepcopy.go;
update the hook's entry (the entry string under id: check-generated) to also
verify Helm CRD sync by running "make check-helm-crds" (or "make helm-crds" then
verifying) and include the Helm templates directory in the git diff
(charts/operator-crds/templates/) so the final command looks like: run "make
manifests generate && make check-helm-crds && git diff --exit-code config/crd/
config/rbac/role.yaml charts/operator-crds/templates/
api/v1alpha1/zz_generated.deepcopy.go" (adjust spacing/quoting to match existing
bash -c usage).
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
- Bumps kubeconform kubernetes-version from 1.31.0 to 1.34.0 for OpenShift 4.21 - Fixes empty-env test case to actually set env var to empty string Generated-By: Claude Code (Anthropic) Signed-off-by: Will Gordon <wgordon@redhat.com>
Generated-By: Claude Code (Anthropic) Signed-off-by: Will Gordon <wgordon@redhat.com>
Generated-By: Claude Code (Anthropic) Signed-off-by: Will Gordon <wgordon@redhat.com>
Generated-By: Claude Code (Anthropic) Signed-off-by: Will Gordon <wgordon@redhat.com>
04bee3b to
5875c6a
Compare
Generated-By: Claude Code (Anthropic) Signed-off-by: Will Gordon <wgordon@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/check-pull-request.yaml (1)
22-40:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffLock down
check-generated-codejob permissions.Risk: major. This job inherits default workflow permissions (write to contents, issues, PRs, etc.) but only performs read-only validation. If a compromised dependency in the toolchain (buf, controller-gen, or any transitive Go module) executes malicious code, it gains write access to the repository.
Impact: Supply-chain attack surface. The job needs zero permissions beyond code checkout.
🔒 Restrict to read-only
check-generated-code: name: Check generated code runs-on: ubuntu-latest + permissions: + contents: read steps:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/check-pull-request.yaml around lines 22 - 40, The check-generated-code job currently runs with default workflow permissions; restrict it to read-only by adding a permissions block on the job named check-generated-code (e.g., permissions: contents: read) so the job only has the minimal permission required to checkout and read the repository; update the job definition for check-generated-code to include this permissions entry (targeting the job identifier check-generated-code) so buf, controller-gen, and Go tooling cannot obtain write access during this validation step.
♻️ Duplicate comments (3)
.pre-commit-config.yaml (2)
32-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBootstrap kustomize binary in pre-push hook.
Risk: major. This hook assumes
bin/kustomizeexists, butmake install-hooksonly installs hook scripts, not tool binaries. On a fresh clone, the first pre-push fails before validation starts.Impact: Hook cannot validate manifests, breaking the safety gate. Developers must manually run
make kustomizefirst or encounter cryptic failures.⚙️ Bootstrap binary before validation
- id: kustomize-build name: validate kustomize build - entry: bash -c 'bin/kustomize build config/default > /dev/null' + entry: bash -c 'make kustomize && bin/kustomize build config/default > /dev/null' language: system pass_filenames: false stages: [pre-push]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.pre-commit-config.yaml around lines 32 - 37, The pre-push hook "kustomize-build" runs "bin/kustomize" but doesn't ensure that binary exists; update the hook (id: kustomize-build, entry: bash -c 'bin/kustomize build config/default > /dev/null') to first bootstrap the kustomize binary when missing (e.g., check for bin/kustomize and run the project's install step such as "make kustomize" or an equivalent bootstrap target), then run the existing build command so fresh clones won't fail before validation.
39-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify Helm CRD sync in pre-push hook.
Risk: major. This hook regenerates
config/crd/but doesn't verifycharts/operator-crds/templates/stays in sync. CRD changes pass pre-push with stale Helm templates, breaking Helm deployments.Impact: Helm chart ships outdated CRDs, causing runtime schema mismatches and workload failures. Based on learnings, "Always run
make helm-crdsafter regenerating CRDs, or runmake check-helm-crdsto verify sync."🎯 Enforce Helm CRD freshness
- id: check-generated name: check generated code is up to date - entry: bash -c 'make manifests generate && git diff --exit-code config/crd/ config/rbac/role.yaml api/v1alpha1/zz_generated.deepcopy.go' + entry: bash -c 'make manifests generate check-helm-crds && git diff --exit-code config/crd/ config/rbac/role.yaml charts/operator-crds/templates/ api/v1alpha1/zz_generated.deepcopy.go' language: system pass_filenames: false stages: [pre-push]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.pre-commit-config.yaml around lines 39 - 44, The pre-push hook "check-generated" only regenerates config/crd/ via "make manifests generate" but doesn't ensure charts/operator-crds/templates/ is updated; update the hook (id: check-generated) so that after running "make manifests generate" it also runs "make helm-crds" (or "make check-helm-crds" to verify) and fail if Helm CRDs are out of sync; specifically modify the entry that currently runs "make manifests generate && git diff --exit-code config/crd/ config/rbac/role.yaml api/v1alpha1/zz_generated.deepcopy.go" to include running "make helm-crds" or "make check-helm-crds" and ensure the git diff check covers charts/operator-crds/templates/ as well, so check-generated will catch stale Helm templates.Source: Learnings
AGENTS.md (1)
106-115:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument manual pre-push hook validation.
Risk: minor. The Hook Installation section shows how to install pre-push hooks but not how to manually test them. Developers might not discover they can run
pre-commit run --hook-stage pre-push --all-filesto validate without pushing.Impact: Reduced developer ergonomics - validation requires actual push attempts instead of local testing.
📖 Add manual validation guidance
### Hook Installation Two hook types must be installed separately: ```bash pre-commit install # commit-stage hooks (lint, whitespace, etc.) pre-commit install --hook-type pre-push # push-stage hooks (kustomize-build, check-generated)Or use
make install-hooksto install both at once.
+
+To manually test hooks without committing/pushing:
+
+bash +pre-commit run --all-files # commit-stage hooks +pre-commit run --hook-stage pre-push --all-files # pre-push hooks +</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@AGENTS.mdaround lines 106 - 115, Add explicit manual validation
instructions to the "Hook Installation" section: explain how to run commit-stage
hooks locally using the command pre-commit run --all-files and how to run
pre-push hooks locally using pre-commit run --hook-stage pre-push --all-files,
and insert these two lines beneath the existing install examples (and optionally
mention make install-hooks remains available).</details> <!-- cr-comment:v1:f08b47e2847b0d7cc3596d57 --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In @.github/workflows/check-pull-request.yaml:
- Line 48: The checkout step currently uses actions/checkout@v6 which by default
persists repo credentials; modify the checkout step (the actions/checkout@v6
usage) to disable credential persistence by adding the with:
persist-credentials: false option so credentials are not stored in .git/config
before running go install or other third-party tooling.In @.kube-linter.yaml:
- Around line 1-44: The kube-linter config currently excludes many checks
globally under the exclude list in .kube-linter.yaml for operator-sdk scaffold
defaults; ensure the kustomize overlay at config/default only renders the
operator Deployment and RBAC (no extra workloads/samples) so these global
exclusions stay safe, and if you plan to include additional resources later,
replace broad exclusions with resource-scoped exclusions using skipKinds or
customChecks.scope (referencing the exclude block and keys
skipKinds/customChecks.scope) to target only the operator Deployment and
specific RBAC kinds.
Outside diff comments:
In @.github/workflows/check-pull-request.yaml:
- Around line 22-40: The check-generated-code job currently runs with default
workflow permissions; restrict it to read-only by adding a permissions block on
the job named check-generated-code (e.g., permissions: contents: read) so the
job only has the minimal permission required to checkout and read the
repository; update the job definition for check-generated-code to include this
permissions entry (targeting the job identifier check-generated-code) so buf,
controller-gen, and Go tooling cannot obtain write access during this validation
step.
Duplicate comments:
In @.pre-commit-config.yaml:
- Around line 32-37: The pre-push hook "kustomize-build" runs "bin/kustomize"
but doesn't ensure that binary exists; update the hook (id: kustomize-build,
entry: bash -c 'bin/kustomize build config/default > /dev/null') to first
bootstrap the kustomize binary when missing (e.g., check for bin/kustomize and
run the project's install step such as "make kustomize" or an equivalent
bootstrap target), then run the existing build command so fresh clones won't
fail before validation.- Around line 39-44: The pre-push hook "check-generated" only regenerates
config/crd/ via "make manifests generate" but doesn't ensure
charts/operator-crds/templates/ is updated; update the hook (id:
check-generated) so that after running "make manifests generate" it also runs
"make helm-crds" (or "make check-helm-crds" to verify) and fail if Helm CRDs are
out of sync; specifically modify the entry that currently runs "make manifests
generate && git diff --exit-code config/crd/ config/rbac/role.yaml
api/v1alpha1/zz_generated.deepcopy.go" to include running "make helm-crds" or
"make check-helm-crds" and ensure the git diff check covers
charts/operator-crds/templates/ as well, so check-generated will catch stale
Helm templates.In
@AGENTS.md:
- Around line 106-115: Add explicit manual validation instructions to the "Hook
Installation" section: explain how to run commit-stage hooks locally using the
command pre-commit run --all-files and how to run pre-push hooks locally using
pre-commit run --hook-stage pre-push --all-files, and insert these two lines
beneath the existing install examples (and optionally mention make install-hooks
remains available).</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Repository: osac-project/coderabbit/.coderabbit.yaml **Review profile**: ASSERTIVE **Plan**: Enterprise **Run ID**: `e0c80a1f-5963-4d9f-bbba-a7396c807377` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 7df8088a98cd603dffcc678241e6e1208c4a7ccb and 5875c6a70b699fe1e8c3b61380ef268997538e1a. </details> <details> <summary>📒 Files selected for processing (8)</summary> * `.github/workflows/check-pull-request.yaml` * `.golangci.yml` * `.kube-linter.yaml` * `.pre-commit-config.yaml` * `AGENTS.md` * `Makefile` * `helpers/getenvwithdefault_test.go` * `internal/consoleproxy/subresource_test.go` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| permissions: | ||
| contents: read | ||
| steps: | ||
| - uses: actions/checkout@v6 |
There was a problem hiding this comment.
Prevent credential persistence in checkout.
Risk: major. actions/checkout@v6 defaults to persisting Git credentials in .git/config. Later steps install and execute third-party Go binaries (kubeconform, kube-linter) via go install, which pulls transitive dependencies. If any binary or dependency is compromised, persisted credentials grant write access to the repository.
Impact: Repository credentials available to untrusted toolchain code.
🔒 Disable credential persistence
- uses: actions/checkout@v6
+ with:
+ persist-credentials: false🧰 Tools
🪛 zizmor (1.25.2)
[warning] 48-48: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 48-48: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/check-pull-request.yaml at line 48, The checkout step
currently uses actions/checkout@v6 which by default persists repo credentials;
modify the checkout step (the actions/checkout@v6 usage) to disable credential
persistence by adding the with: persist-credentials: false option so credentials
are not stored in .git/config before running go install or other third-party
tooling.
| checks: | ||
| addAllBuiltIn: true | ||
| exclude: | ||
| # The manager image uses `controller:latest` as a placeholder in config/manager/manager.yaml. | ||
| # The real image tag is injected by kustomize edit set image at build/deploy time. | ||
| - latest-tag | ||
|
|
||
| # The manager binds its health endpoints on :8081 via --health-probe-bind-address=:8081. | ||
| # Kubernetes does not require containerPort declarations for probes to function. | ||
| # Declaring the port is cosmetic; omitting it is the upstream operator-sdk scaffold default. | ||
| - liveness-port | ||
| - readiness-port | ||
|
|
||
| # The operator manager needs a writable filesystem (Go runtime tmp, controller-runtime caches). | ||
| # readOnlyRootFilesystem is not set in the upstream operator-sdk scaffold. | ||
| - no-read-only-root-fs | ||
|
|
||
| # Admin ClusterRoles intentionally use wildcard verbs — they are aggregated admin roles | ||
| # intended for human operators, not the controller itself. | ||
| - wildcard-in-rules | ||
|
|
||
| # Operator-sdk scaffold does not set dnsConfig. Standard cluster DNS is sufficient. | ||
| - dnsconfig-options | ||
|
|
||
| # Controller-manager runs as a single replica by design (leader-election for HA). | ||
| # Multiple replicas compete via lease, so minimum-three-replicas is inapplicable. | ||
| - minimum-three-replicas | ||
|
|
||
| # Operator pods do not declare node affinity; scheduling is left to the cluster default. | ||
| - no-node-affinity | ||
|
|
||
| # operator-sdk scaffold does not set updateStrategy on the Deployment explicitly. | ||
| # Kubernetes defaults to RollingUpdate, so this check produces a false positive. | ||
| - no-rolling-update-strategy | ||
|
|
||
| # NetworkPolicy is managed externally (osac-installer kustomize overlays). | ||
| - non-isolated-pod | ||
|
|
||
| # Ownership annotations and labels are not part of the operator-sdk scaffold convention. | ||
| - required-annotation-email | ||
| - required-label-owner | ||
|
|
||
| # Deployment has no explicit restartPolicy; Kubernetes defaults to Always for Deployments. | ||
| - restart-policy |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
kube-linter config scoped to operator deployment.
The excluded checks align with operator-sdk scaffold defaults and are justified for the operator's own deployment (writable filesystem for Go runtime/caches, single replica with leader-election, external NetworkPolicy management, etc.).
Ensure config/default renders only the operator deployment + RBAC. If additional workloads or sample resources are included in future kustomize overlays linted with this config, resource-scoped exclusions (via skipKinds/customChecks.scope) would be more precise.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.kube-linter.yaml around lines 1 - 44, The kube-linter config currently
excludes many checks globally under the exclude list in .kube-linter.yaml for
operator-sdk scaffold defaults; ensure the kustomize overlay at config/default
only renders the operator Deployment and RBAC (no extra workloads/samples) so
these global exclusions stay safe, and if you plan to include additional
resources later, replace broad exclusions with resource-scoped exclusions using
skipKinds or customChecks.scope (referencing the exclude block and keys
skipKinds/customChecks.scope) to target only the operator Deployment and
specific RBAC kinds.
|
/retest |
amej
left a comment
There was a problem hiding this comment.
I trust that you will address the comments by others and the CodeRabbit.
lgtm.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: amej, wgordon17 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@wgordon17: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Summary
Summary by CodeRabbit
Chores
Documentation
Tests