ESO-174: Harden RBAC by adding resourceNames #50
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@chiragkyal: This pull request references ESO-174 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In 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. |
openshift-ci
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chiragkyal The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
It looks like the resourceNames might not help as expected with ClusterRole, because of limitations with create, list, and watch verbs.
xref: https://kubernetes.io/docs/reference/access-authn-authz/rbac/
Note:
You cannot restrict create or deletecollection requests by their resource name. For create, this limitation is because the name of the new object may not be known at authorization time. If you restrict list or watch by resourceName, clients must include a metadata.name field selector in their list or watch request that matches the specified resourceName in order to be authorized. For example, kubectl get configmaps --field-selector=metadata.name=my-configmap
openshift-merge-robot
added
the
needs-rebase
chiragkyal
force-pushed
the
harden-rbac
branch
4 times, most recently
from
67af8cc to
3b4e815
Compare
openshift-merge-robot
removed
the
needs-rebase
Signed-off-by: chiragkyal <[email protected]>
Signed-off-by: chiragkyal <[email protected]>
Signed-off-by: chiragkyal <[email protected]>
chiragkyal
force-pushed
the
harden-rbac
branch
from
3b4e815 to
c4b6625
Compare
|
Signed-off-by: chiragkyal <[email protected]> Remove resourceNames from role and rolebinding Signed-off-by: chiragkyal <[email protected]>
chiragkyal
force-pushed
the
harden-rbac
branch
from
dffa051 to
3f502a3
Compare
|
Looks like |
Signed-off-by: chiragkyal <[email protected]>
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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 kubernetes-sigs/prow repository. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds a validate-rbac Makefile target and script to verify RBAC resourceNames against assets/CRDs; wires it into verify. Centralizes operator-sdk installation via a new script. Refines RBAC kubebuilder annotations and propagates those narrower, name-scoped rules to ClusterRole and CSV; updates CSV timestamp. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Make as make verify
participant VR as validate-rbac.sh
participant Src as Kubebuilder Annotations
participant As as Assets (bindata/…)
participant Crd as CRDs (config/crd/bases)
Dev->>Make: run verify
Make->>VR: invoke validate-rbac
VR->>Src: read RBAC resourceNames
VR->>As: extract names (Deployments, Webhooks, …)
VR->>Crd: extract CRD names
VR->>VR: compare sets per resource type
alt any mismatch
VR-->>Make: exit 1
else all match
VR-->>Make: exit 0
end
sequenceDiagram
autonumber
participant Dev as Developer
participant Make as make setup (implicit)
participant OS as hack/operator-sdk.sh
participant GH as Releases CDN
Dev->>Make: needs operator-sdk
Make->>OS: run operator-sdk.sh [OUTPUT, VERSION]
OS->>OS: detect GOOS/GOARCH
OS->>GH: download operator-sdk_${GOOS}_${GOARCH}
GH-->>OS: binary
OS->>OS: install to OUTPUT and chmod +x
OS-->>Make: path to operator-sdk
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches
🧪 Generate unit tests
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 |
|
@chiragkyal: This pull request references ESO-174 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (6)
hack/operator-sdk.sh (2)
8-9: Avoid hard dependency on Go for OS/ARCH detection (add uname fallback).This improves developer ergonomics on minimal environments.
-GOOS=$(go env GOOS) -GOARCH=$(go env GOARCH) +if command -v go >/dev/null 2>&1; then + GOOS=$(go env GOOS) + GOARCH=$(go env GOARCH) +else + GOOS=$(uname -s | tr '[:upper:]' '[:lower:]') + case "$(uname -m)" in + x86_64) GOARCH=amd64 ;; + aarch64|arm64) GOARCH=arm64 ;; + ppc64le) GOARCH=ppc64le ;; + s390x) GOARCH=s390x ;; + *) echo "Unsupported architecture $(uname -m)"; exit 1 ;; + esac +fi
29-31: Add SHA256 checksum verification after the curl downloadAfter the
curl --location -o "${BIN_PATH}" …step, detectsha256sum, fetch the release’schecksums.txt, and verify the downloaded binary:if command -v sha256sum >/dev/null 2>&1; then sumfile="${TEMPDIR}/checksums.txt" curl -fSL -o "${sumfile}" "${OPERATOR_SDK_DL_URL}/checksums.txt" grep "operator-sdk_${GOOS}_${GOARCH}$" "${sumfile}" | sha256sum -c - else echo "sha256sum not found; skipping checksum verification" >&2 fipkg/controller/external_secrets/controller.go (1)
111-111: Consider scoping CRD update/patch by resourceNames.If the operator only updates its own CRDs (e.g., externalsecrets.external-secrets.io, secretstores.external-secrets.io, clustersecretstores.external-secrets.io, clusterexternalsecrets.external-secrets.io, pushsecrets.external-secrets.io, clusterpushsecrets.external-secrets.io), restrict update/patch to those names to reduce blast radius.
Example:
-// +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list;watch;update;patch +// +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list;watch +// +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=update;patch,resourceNames=externalsecrets.external-secrets.io;secretstores.external-secrets.io;clustersecretstores.external-secrets.io;clusterexternalsecrets.external-secrets.io;pushsecrets.external-secrets.io;clusterpushsecrets.external-secrets.ioPlease adjust the list to the exact CRD names present in this repo.
bundle/manifests/external-secrets-operator.clusterserviceversion.yaml (1)
207-207: Nit: createdAt should be auto-generated by bundle tooling.Ensure this isn’t maintained manually to avoid drift.
hack/validate-rbac-resourcenames.sh (2)
120-126: Make Roles/RoleBindings checks optional to reduce brittleness.These are often generated or not annotated via kubebuilder; consider gating with flags to avoid false negatives in CI.
55-61: Comparison edge cases: ensure stable sort/locale.Set LC_ALL=C to keep comm deterministic across environments.
- local missing_in_kb + local missing_in_kb + export LC_ALL=C
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (6)
Makefile(3 hunks)bundle/manifests/external-secrets-operator.clusterserviceversion.yaml(3 hunks)config/rbac/role.yaml(2 hunks)hack/operator-sdk.sh(1 hunks)hack/validate-rbac-resourcenames.sh(1 hunks)pkg/controller/external_secrets/controller.go(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
hack/operator-sdk.sh
[warning] 14-14: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 19-19: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 24-24: In POSIX sh, &> is undefined.
(SC3020)
hack/validate-rbac-resourcenames.sh
[warning] 24-24: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames.
(SC2038)
[warning] 34-34: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames.
(SC2038)
🔇 Additional comments (7)
pkg/controller/external_secrets/controller.go (2)
100-102: Good split of ValidatingWebhookConfigurations RBAC with resourceNames.Scoping create/update/patch to specific webhook names while keeping get/list/watch broad is appropriate.
103-105: Remove the patch-verb recommendation
This controller never callsclient.Patchorcontrollerutil.CreateOrPatch/CreateOrUpdateon those Deployment resources, so addingpatchto the RBACverbsis unnecessary.Likely an incorrect or invalid review comment.
Makefile (3)
121-124: Nice addition: validate-rbac target + help text.This guards drift between annotations and rendered assets.
287-289: Centralizing operator-sdk install via script is cleaner.Given the script now handles OS/ARCH detection, this reduces Makefile noise. Ensure the script fixes from this review land to keep POSIX compatibility.
372-372: verify pipeline now includes validate-rbac.LGTM. Please confirm validate-rbac is deterministic and doesn’t require network to avoid CI flakiness.
config/rbac/role.yaml (1)
73-74: LGTM: add list/watch for apps/deployments.Keeping list/watch broad is correct; name-scoped list/watch would require clients to use field selectors.
bundle/manifests/external-secrets-operator.clusterserviceversion.yaml (1)
411-413: LGTM: list/watch for apps/deployments stay broad.Correct approach given field-selector requirement for name-scoped list/watch.
| - get | ||
| - list | ||
| - watch |
There was a problem hiding this comment.
Add global “create” for ValidatingWebhookConfigurations.
Otherwise the operator cannot create the webhook objects; resourceNames cannot scope create.
resources:
- validatingwebhookconfigurations
verbs:
+ - create
- get
- list
- watch📝 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.
| - get | |
| - list | |
| - watch | |
| resources: | |
| - validatingwebhookconfigurations | |
| verbs: | |
| - create | |
| - get | |
| - list | |
| - watch |
🤖 Prompt for AI Agents
In bundle/manifests/external-secrets-operator.clusterserviceversion.yaml around
lines 382-384, the RBAC rule for ValidatingWebhookConfigurations only includes
get/list/watch which prevents the operator from creating webhook objects; add
the "create" verb to that rule (ensure the rule targets the
admissionregistration.k8s.io API group and the validatingwebhookconfigurations
resource) so the operator can create webhook resources cluster-wide.
| - admissionregistration.k8s.io | ||
| resourceNames: | ||
| - externalsecret-validate | ||
| - secretstore-validate | ||
| resources: | ||
| - validatingwebhookconfigurations | ||
| verbs: | ||
| - create | ||
| - patch | ||
| - update |
There was a problem hiding this comment.
Remove “create” from the resourceNames-scoped webhook rule.
This does not authorize create and gives a false sense of scoping.
verbs:
- - create
- patch
- update📝 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.
| - admissionregistration.k8s.io | |
| resourceNames: | |
| - externalsecret-validate | |
| - secretstore-validate | |
| resources: | |
| - validatingwebhookconfigurations | |
| verbs: | |
| - create | |
| - patch | |
| - update | |
| - admissionregistration.k8s.io | |
| resourceNames: | |
| - externalsecret-validate | |
| - secretstore-validate | |
| resources: | |
| - validatingwebhookconfigurations | |
| verbs: | |
| - patch | |
| - update |
🤖 Prompt for AI Agents
In bundle/manifests/external-secrets-operator.clusterserviceversion.yaml around
lines 386 to 395, the webhook rule that is scoped to specific resourceNames
incorrectly includes the "create" verb which does not actually authorize create
for resourceNames-scoped rules; remove the "create" entry from the verbs list so
only valid verbs (e.g., patch, update) remain for that rule, and verify the rule
now correctly reflects the intended scoping.
| - get | ||
| - list | ||
| - watch | ||
| - apiGroups: |
There was a problem hiding this comment.
Create on ValidatingWebhookConfigurations must be global (resourceNames cannot scope create).
A rule with resourceNames never matches create (name unknown at auth time). Without a global create on validatingwebhookconfigurations, the operator won’t be able to create the webhooks.
Apply:
resources:
- validatingwebhookconfigurations
verbs:
+ - create
- get
- list
- watch🤖 Prompt for AI Agents
config/rbac/role.yaml around lines 44-47: the current rule scopes create via
resourceNames which never matches CREATE (name unknown), so add a global create
for ValidatingWebhookConfigurations; update the rule so the verbs array includes
"create" at the top-level (not scoped by resourceNames) and ensure the rule
targets the validatingwebhookconfigurations resource in the
admissionregistration.k8s.io API group, or split into two rules where one global
rule grants create for validatingwebhookconfigurations and the other rule keeps
the resourceNames-scoped verbs for get/list/watch if needed.
| - admissionregistration.k8s.io | ||
| resourceNames: | ||
| - externalsecret-validate | ||
| - secretstore-validate | ||
| resources: | ||
| - validatingwebhookconfigurations | ||
| verbs: | ||
| - create | ||
| - patch |
There was a problem hiding this comment.
Remove “create” from the resourceNames-scoped webhook rule.
Create cannot be name-scoped; this rule won’t authorize create anyway and is misleading.
verbs:
- - create
- patch
- updateCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In config/rbac/role.yaml around lines 48-56, the webhook RBAC rule that is
scoped to resourceNames incorrectly includes the "create" verb (name-scoped
rules cannot authorize create); remove the "create" entry from the verbs list
for the resourceNames-scoped validatingwebhookconfigurations rule so only valid
verbs (e.g., patch) remain.
| if [[ "$GOOS" != "linux" && "$GOOS" != "darwin" ]]; then | ||
| echo "Unsupported OS $GOOS" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [[ "$GOARCH" != "amd64" && "$GOARCH" != "arm64" && "$GOARCH" != "ppc64le" && "$GOARCH" != "s390x" ]]; then | ||
| echo "Unsupported architecture $GOARCH" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Fix bashisms under /bin/sh (will fail on dash).
The script uses [[ … ]] and &> which are bash-only. With the current shebang (/bin/sh), this breaks on many systems.
Apply:
-if [[ "$GOOS" != "linux" && "$GOOS" != "darwin" ]]; then
- echo "Unsupported OS $GOOS"
- exit 1
-fi
+case "$GOOS" in
+ linux|darwin) ;;
+ *) echo "Unsupported OS $GOOS"; exit 1 ;;
+esac
-
-if [[ "$GOARCH" != "amd64" && "$GOARCH" != "arm64" && "$GOARCH" != "ppc64le" && "$GOARCH" != "s390x" ]]; then
- echo "Unsupported architecture $GOARCH"
- exit 1
-fi
+case "$GOOS/$GOARCH" in
+ linux/amd64|linux/arm64|linux/ppc64le|linux/s390x|darwin/amd64|darwin/arm64) ;;
+ *) echo "Unsupported architecture $GOARCH for $GOOS"; exit 1 ;;
+esac
-
-command -v curl &> /dev/null || { echo "can't find curl command" && exit 1; }
+command -v curl >/dev/null 2>&1 || { echo "can't find curl command" >&2; exit 1; }Also applies to: 24-24
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 14-14: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 19-19: In POSIX sh, [[ ]] is undefined.
(SC3010)
🤖 Prompt for AI Agents
In hack/operator-sdk.sh around lines 14-22 (and also line 24) replace bash-only
constructs so the script is POSIX /bin/sh compatible: change the [[ ... ]] tests
to POSIX [ ... ] (or use a case statement) with proper spacing and use -o or
separate tests combined with &&/|| as needed, and replace any &> redirects with
the POSIX equivalent >file 2>&1 (or >/dev/stderr as appropriate); ensure the
shebang stays /bin/sh and update both the OS and GOARCH checks (and the
occurrence at line 24) accordingly.
| TEMPDIR=$(mktemp -d) | ||
| BIN_PATH="${TEMPDIR}/${BIN_ARCH}" | ||
|
|
||
| echo "> downloading binary" | ||
| curl --location -o "${BIN_PATH}" "${OPERATOR_SDK_DL_URL}/operator-sdk_${GOOS}_${GOARCH}" | ||
|
|
||
| echo "> installing binary" | ||
| mv "${BIN_PATH}" "${OUTPUT_PATH}" | ||
| chmod +x "${OUTPUT_PATH}" | ||
| rm -rf "${TEMPDIR}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden download/install and guarantee cleanup.
Add trap for tempdir cleanup, fail on HTTP errors, ensure destination dir exists, and use install(1) atomically.
TEMPDIR=$(mktemp -d)
BIN_PATH="${TEMPDIR}/${BIN_ARCH}"
+trap 'rm -rf "${TEMPDIR}"' EXIT
echo "> downloading binary"
-curl --location -o "${BIN_PATH}" "${OPERATOR_SDK_DL_URL}/operator-sdk_${GOOS}_${GOARCH}"
+curl -fSL -o "${BIN_PATH}" "${OPERATOR_SDK_DL_URL}/operator-sdk_${GOOS}_${GOARCH}"
echo "> installing binary"
-mv "${BIN_PATH}" "${OUTPUT_PATH}"
-chmod +x "${OUTPUT_PATH}"
-rm -rf "${TEMPDIR}"
+mkdir -p "$(dirname "${OUTPUT_PATH}")"
+install -m 0755 "${BIN_PATH}" "${OUTPUT_PATH}"📝 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.
| TEMPDIR=$(mktemp -d) | |
| BIN_PATH="${TEMPDIR}/${BIN_ARCH}" | |
| echo "> downloading binary" | |
| curl --location -o "${BIN_PATH}" "${OPERATOR_SDK_DL_URL}/operator-sdk_${GOOS}_${GOARCH}" | |
| echo "> installing binary" | |
| mv "${BIN_PATH}" "${OUTPUT_PATH}" | |
| chmod +x "${OUTPUT_PATH}" | |
| rm -rf "${TEMPDIR}" | |
| TEMPDIR=$(mktemp -d) | |
| BIN_PATH="${TEMPDIR}/${BIN_ARCH}" | |
| trap 'rm -rf "${TEMPDIR}"' EXIT | |
| echo "> downloading binary" | |
| curl -fSL -o "${BIN_PATH}" "${OPERATOR_SDK_DL_URL}/operator-sdk_${GOOS}_${GOARCH}" | |
| echo "> installing binary" | |
| mkdir -p "$(dirname "${OUTPUT_PATH}")" | |
| install -m 0755 "${BIN_PATH}" "${OUTPUT_PATH}" |
🤖 Prompt for AI Agents
In hack/operator-sdk.sh around lines 26 to 35, make the download/install more
robust: create the temp dir with mktemp -d as now but register a trap 'trap "rm
-rf \"$TEMPDIR\"" EXIT' immediately after to guarantee cleanup; make curl fail
on HTTP errors and show errors by using --fail --show-error --location --output
(or -fSLo) so the script exits on non-2xx responses; ensure the destination
directory exists with 'mkdir -p "$(dirname "$OUTPUT_PATH")"' before installing;
replace mv+chmod with an atomic install(1) call like 'install -m 0755
"$BIN_PATH" "$OUTPUT_PATH"' (which sets executable bit and moves atomically) and
remove the explicit chmod and mv; also run the script with strict failure flags
(set -euo pipefail) if not already present to abort on errors.
| grep -E "^\s*//\s*\+kubebuilder:rbac:.*resources=${resource_type}.*resourceNames=" \ | ||
| "${PROJECT_ROOT}/pkg/controller/external_secrets/controller.go" | \ | ||
| sed -E 's/.*resourceNames=([^,]*).*/\1/' | \ | ||
| tr ';' '\n' | sort -u |
There was a problem hiding this comment.
Regex uses ‘\s’ which grep -E doesn’t support; extractor will return empty.
Use POSIX classes.
- grep -E "^\s*//\s*\+kubebuilder:rbac:.*resources=${resource_type}.*resourceNames=" \
+ grep -E "^[[:space:]]*//[[:space:]]*\+kubebuilder:rbac:.*resources=${resource_type}.*resourceNames=" \
"${PROJECT_ROOT}/pkg/controller/external_secrets/controller.go" | \📝 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.
| grep -E "^\s*//\s*\+kubebuilder:rbac:.*resources=${resource_type}.*resourceNames=" \ | |
| "${PROJECT_ROOT}/pkg/controller/external_secrets/controller.go" | \ | |
| sed -E 's/.*resourceNames=([^,]*).*/\1/' | \ | |
| tr ';' '\n' | sort -u | |
| grep -E "^[[:space:]]*//[[:space:]]*\+kubebuilder:rbac:.*resources=${resource_type}.*resourceNames=" \ | |
| "${PROJECT_ROOT}/pkg/controller/external_secrets/controller.go" | \ | |
| sed -E 's/.*resourceNames=([^,]*).*/\1/' | \ | |
| tr ';' '\n' | sort -u |
🤖 Prompt for AI Agents
In hack/validate-rbac-resourcenames.sh around lines 15 to 18, the grep -E
pattern uses \s (not supported by grep -E) causing no matches; replace
backslash-s with the POSIX character class [:space:] (e.g. use [[:space:]]* in
place of \s*) so the regex correctly matches whitespace, leaving the rest of the
pipeline (sed/tr/ sort) unchanged.
| find "${PROJECT_ROOT}/bindata/external-secrets/resources" "${PROJECT_ROOT}/bindata/external-secrets" -name "*.yml" -exec grep -l "kind: ${pattern}" {} \; 2>/dev/null | \ | ||
| xargs grep -h "^ name:" | \ | ||
| awk '{print $2}' | sort -u | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle .yaml files and filenames with spaces; avoid SC2038.
Use -exec … + and include both extensions; match indentation-agnostic “name:”.
-extract_asset_names() {
+extract_asset_names() {
local pattern="$1"
- find "${PROJECT_ROOT}/bindata/external-secrets/resources" "${PROJECT_ROOT}/bindata/external-secrets" -name "*.yml" -exec grep -l "kind: ${pattern}" {} \; 2>/dev/null | \
- xargs grep -h "^ name:" | \
- awk '{print $2}' | sort -u
+ find "${PROJECT_ROOT}/bindata/external-secrets/resources" "${PROJECT_ROOT}/bindata/external-secrets" \
+ -type f \( -name "*.yml" -o -name "*.yaml" \) -exec grep -l "kind: ${pattern}" {} + 2>/dev/null | \
+ xargs -r grep -hE '^[[:space:]]{2}name:' | \
+ awk '{print $2}' | sort -u
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 24-24: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames.
(SC2038)
🤖 Prompt for AI Agents
In hack/validate-rbac-resourcenames.sh around lines 24-27, the find/grep
pipeline only matches .yml, breaks on filenames with spaces, and is
indentation-sensitive; update the find invocation to include both "*.yml" and
"*.yaml" (using -name A -o -name B), use -exec ... + or -print0 combined with
xargs -0 to safely handle spaces (avoid plain xargs to address SC2038), and
change the final grep to match "name:" with optional leading whitespace (e.g.,
use a regex like '^\s*name:') so it is indentation-agnostic before extracting
the resource name.
| crd_names=$(find "${PROJECT_ROOT}/config/crd/bases" -name "*.yml" -o -name "*.yaml" 2>/dev/null | \ | ||
| xargs grep -l "kind: CustomResourceDefinition" 2>/dev/null | \ | ||
| xargs grep -h "^ name:" 2>/dev/null | \ | ||
| awk '{print $2}') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same robustness fix for CRD discovery; avoid xargs on unsanitized filenames.
- crd_names=$(find "${PROJECT_ROOT}/config/crd/bases" -name "*.yml" -o -name "*.yaml" 2>/dev/null | \
- xargs grep -l "kind: CustomResourceDefinition" 2>/dev/null | \
- xargs grep -h "^ name:" 2>/dev/null | \
- awk '{print $2}')
+ crd_names=$(find "${PROJECT_ROOT}/config/crd/bases" -type f \( -name "*.yml" -o -name "*.yaml" \) \
+ -exec grep -l "kind: CustomResourceDefinition" {} + 2>/dev/null | \
+ xargs -r grep -hE '^[[:space:]]{2}name:' 2>/dev/null | \
+ awk '{print $2}')📝 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.
| crd_names=$(find "${PROJECT_ROOT}/config/crd/bases" -name "*.yml" -o -name "*.yaml" 2>/dev/null | \ | |
| xargs grep -l "kind: CustomResourceDefinition" 2>/dev/null | \ | |
| xargs grep -h "^ name:" 2>/dev/null | \ | |
| awk '{print $2}') | |
| crd_names=$(find "${PROJECT_ROOT}/config/crd/bases" -type f \( -name "*.yml" -o -name "*.yaml" \) \ | |
| -exec grep -l "kind: CustomResourceDefinition" {} + 2>/dev/null | \ | |
| xargs -r grep -hE '^[[:space:]]{2}name:' 2>/dev/null | \ | |
| awk '{print $2}') |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 34-34: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames.
(SC2038)
🤖 Prompt for AI Agents
In hack/validate-rbac-resourcenames.sh around lines 34 to 37, the CRD discovery
pipeline uses xargs on unsanitized filenames which breaks on spaces/special
chars; change the pipeline to use null-delimited paths (find ... -print0) and
xargs -0 for the grep steps or replace the pipeline with a safe while-read loop
that reads -d '' from find -print0 and runs grep -h "^ name:" on each file,
collecting the second field as before so filenames with spaces/newlines are
handled correctly.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@chiragkyal: This pull request references ESO-174 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In 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. |
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)
config/rbac/role.yaml (1)
1-235: Removecreatefrom name-scoped RBAC rules
The rules scoped byresourceNamesin both config/rbac/role.yaml and bundle/manifests/external-secrets-operator.clusterserviceversion.yaml still includecreate; drop that verb.
♻️ Duplicate comments (12)
hack/operator-sdk.sh (2)
26-35: Harden download/install and guarantee cleanup.Trap tempdir, fail on HTTP errors, ensure destination dir exists, and install atomically.
TEMPDIR=$(mktemp -d) BIN_PATH="${TEMPDIR}/${BIN_ARCH}" +trap 'rm -rf "${TEMPDIR}"' EXIT echo "> downloading binary" -curl --location -o "${BIN_PATH}" "${OPERATOR_SDK_DL_URL}/operator-sdk_${GOOS}_${GOARCH}" +curl -fSL -o "${BIN_PATH}" "${OPERATOR_SDK_DL_URL}/operator-sdk_${GOOS}_${GOARCH}" echo "> installing binary" -mv "${BIN_PATH}" "${OUTPUT_PATH}" -chmod +x "${OUTPUT_PATH}" -rm -rf "${TEMPDIR}" +mkdir -p "$(dirname "${OUTPUT_PATH}")" +install -m 0755 "${BIN_PATH}" "${OUTPUT_PATH}"
14-24: Fix bashisms under /bin/sh (breaks on dash) and use POSIX redirection.[[ ... ]] and &> are bash-only with a /bin/sh shebang. Replace with POSIX case/redirects.
-if [[ "$GOOS" != "linux" && "$GOOS" != "darwin" ]]; then - echo "Unsupported OS $GOOS" - exit 1 -fi +case "$GOOS" in + linux|darwin) ;; + *) echo "Unsupported OS $GOOS"; exit 1 ;; +esac - -if [[ "$GOARCH" != "amd64" && "$GOARCH" != "arm64" && "$GOARCH" != "ppc64le" && "$GOARCH" != "s390x" ]]; then - echo "Unsupported architecture $GOARCH" - exit 1 -fi +case "$GOARCH" in + amd64|arm64|ppc64le|s390x) ;; + *) echo "Unsupported architecture $GOARCH for $GOOS"; exit 1 ;; +esac - -command -v curl &> /dev/null || { echo "can't find curl command" && exit 1; } +command -v curl >/dev/null 2>&1 || { echo "can't find curl command" >&2; exit 1; }config/rbac/role.yaml (4)
40-47: Webhook: add global create (name-scoped rules cannot authorize create).Without global create, the operator cannot create ValidatingWebhookConfigurations.
resources: - validatingwebhookconfigurations verbs: + - create - get - list - watch
69-75: Deployments: add global create or creation will be denied.Create cannot be restricted by resourceNames.
resources: - deployments verbs: + - create - list - watch
48-57: Remove create from resourceNames-scoped webhook rule.Create is ineffective here and misleading.
verbs: - - create - patch - update
75-88: Remove create from resourceNames-scoped deployments rule.Keep get/update here; rely on the global create (or a namespaced Role).
resources: - deployments verbs: - - create - get - updatebundle/manifests/external-secrets-operator.clusterserviceversion.yaml (3)
378-384: Add global create for ValidatingWebhookConfigurations.Create cannot be scoped by resourceNames. Without a global create here, the operator cannot create the webhook objects.
Apply:
verbs: + - create - get - list - watch
407-426: Deployments: name-scoped create in ClusterRole is ineffective; grant create via namespaced Role (CSV.permissions) or broaden create.Create cannot be restricted by resourceNames. Either:
- Preferred: move create/get/update for the named Deployments under spec.install.spec.permissions (namespaced Role) for the operator SA; keep list/watch in clusterPermissions, or
- Quick unblock: add a global create for apps/deployments in the non-name-scoped rule.
Quick unblock diff:
- apiGroups: - apps resources: - deployments verbs: + - create - list - watchRecommended (illustrative, to be added under spec.install.spec.permissions):
- rules: - apiGroups: ["apps"] resources: ["deployments"] verbs: ["create","get","update","patch"] resourceNames: - bitwarden-sdk-server - external-secrets - external-secrets-cert-controller - external-secrets-webhook serviceAccountName: external-secrets-operator-controller-manager
386-395: Remove create from the resourceNames-scoped webhook rule.Create with resourceNames does not authorize creation; keeping it is misleading.
Apply:
verbs: - - create - patch - updatehack/validate-rbac-resourcenames.sh (3)
34-39: Same robustness for CRD discovery; avoid unsafe xargs and operator precedence pitfalls.Group -name predicates, prefer -exec … + and xargs -r.
Apply:
- crd_names=$(find "${PROJECT_ROOT}/config/crd/bases" -name "*.yml" -o -name "*.yaml" 2>/dev/null | \ - xargs grep -l "kind: CustomResourceDefinition" 2>/dev/null | \ - xargs grep -h "^ name:" 2>/dev/null | \ - awk '{print $2}') + crd_names=$(find "${PROJECT_ROOT}/config/crd/bases" -type f \( -name "*.yml" -o -name "*.yaml" \) \ + -exec grep -l "kind: CustomResourceDefinition" {} + 2>/dev/null | \ + xargs -r grep -hE '^[[:space:]]{2}name:' 2>/dev/null | \ + awk '{print $2}')
15-18: Fix grep regex: \s is unsupported by grep -E, extractor returns empty.Use POSIX classes so matches are found; current pattern risks silent false negatives.
Apply:
- grep -E "^\s*//\s*\+kubebuilder:rbac:.*resources=${resource_type}.*resourceNames=" \ + grep -E "^[[:space:]]*//[[:space:]]*\+kubebuilder:rbac:.*resources=${resource_type}.*resourceNames=" \ "${PROJECT_ROOT}/pkg/controller/external_secrets/controller.go" | \
22-27: Harden asset discovery: handle .yaml, filenames with spaces, and missing dirs.Current pipeline is brittle and may fail under set -e. Use -exec … + and guard dirs.
Apply:
extract_asset_names() { local pattern="$1" - find "${PROJECT_ROOT}/bindata/external-secrets/resources" "${PROJECT_ROOT}/bindata/external-secrets" -name "*.yml" -exec grep -l "kind: ${pattern}" {} \; 2>/dev/null | \ - xargs grep -h "^ name:" | \ - awk '{print $2}' | sort -u + local roots=() + [[ -d "${PROJECT_ROOT}/bindata/external-secrets/resources" ]] && roots+=("${PROJECT_ROOT}/bindata/external-secrets/resources") + [[ -d "${PROJECT_ROOT}/bindata/external-secrets" ]] && roots+=("${PROJECT_ROOT}/bindata/external-secrets") + if ((${#roots[@]}==0)); then + return 0 + fi + find "${roots[@]}" -type f \( -name "*.yml" -o -name "*.yaml" \) -exec grep -l "kind: ${pattern}" {} + 2>/dev/null | \ + xargs -r grep -hE '^[[:space:]]{2}name:' | \ + awk '{print $2}' | sort -u }
🧹 Nitpick comments (5)
hack/operator-sdk.sh (2)
3-9: Make script more robust without assuming Go is installed.Optional: add -u and fallback to uname if go is unavailable.
-set -e +set -eu @@ -GOOS=$(go env GOOS) -GOARCH=$(go env GOARCH) +if command -v go >/dev/null 2>&1; then + GOOS=$(go env GOOS) + GOARCH=$(go env GOARCH) +else + # POSIX fallbacks + GOOS=$(uname -s | tr '[:upper:]' '[:lower:]') + GOARCH=$(uname -m) + case "$GOARCH" in + x86_64) GOARCH=amd64 ;; + aarch64|arm64) GOARCH=arm64 ;; + ppc64le|s390x) : ;; # already correct + *) echo "Unsupported uname -m: $GOARCH" >&2; exit 1 ;; + esac +fi
29-30: Optional: verify release checksum to prevent supply-chain issues.Download the checksums file and validate before install.
echo "> downloading binary" -curl --location -o "${BIN_PATH}" "${OPERATOR_SDK_DL_URL}/operator-sdk_${GOOS}_${GOARCH}" +curl -fSL -o "${BIN_PATH}" "${OPERATOR_SDK_DL_URL}/operator-sdk_${GOOS}_${GOARCH}" +curl -fSL -o "${BIN_PATH}.sha256" "${OPERATOR_SDK_DL_URL}/checksums.txt" +grep "operator-sdk_${GOOS}_${GOARCH}\$" "${BIN_PATH}.sha256" | sha256sum -c - || { echo "checksum verification failed" >&2; exit 1; }config/rbac/role.yaml (1)
69-88: Optional hardening: move Deployments/Webhooks create to a namespaced Role bound to the operator SA.This limits cluster-wide create while preserving function via CSV “permissions”.
Example Role (apply via CSV permissions):
apiVersion: rbac.authorization.k8s.io/v1 kind: Role metadata: name: eso-deployer namespace: external-secrets-operator rules: - apiGroups: ["apps"] resources: ["deployments"] verbs: ["create","get","update","patch"] resourceNames: - bitwarden-sdk-server - external-secrets - external-secrets-cert-controller - external-secrets-webhook - apiGroups: ["admissionregistration.k8s.io"] resources: ["validatingwebhookconfigurations"] verbs: ["create","update","patch"] resourceNames: ["externalsecret-validate","secretstore-validate"] # note: create not enforced by resourceNames; the Role scopes by namespace onlyhack/validate-rbac-resourcenames.sh (2)
55-80: Consider failing on extras as well, or emit a non-zero exit to catch stale annotations.Currently only “missing” triggers failure. Stale resourceNames can linger undetected.
Apply:
- if [[ -n "${extra_in_kb}" ]]; then + if [[ -n "${extra_in_kb}" ]]; then echo "Extra in kubebuilder annotations (might be outdated):" echo "${extra_in_kb}" | sed 's/^/ /' + has_errors=true fi
15-18: Order-agnostic match for resources/resourceNames (optional).If annotation fields are reordered, the current regex may miss matches.
Apply:
- grep -E "^[[:space:]]*//[[:space:]]*\+kubebuilder:rbac:.*resources=${resource_type}.*resourceNames=" \ + grep -E "^[[:space:]]*//[[:space:]]*\+kubebuilder:rbac:.*(resources=${resource_type}.*resourceNames=|resourceNames=.*resources=${resource_type})" \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (6)
Makefile(3 hunks)bundle/manifests/external-secrets-operator.clusterserviceversion.yaml(3 hunks)config/rbac/role.yaml(2 hunks)hack/operator-sdk.sh(1 hunks)hack/validate-rbac-resourcenames.sh(1 hunks)pkg/controller/external_secrets/controller.go(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
hack/validate-rbac-resourcenames.sh
[warning] 24-24: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames.
(SC2038)
[warning] 34-34: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames.
(SC2038)
hack/operator-sdk.sh
[warning] 14-14: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 19-19: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 24-24: In POSIX sh, &> is undefined.
(SC3020)
🔇 Additional comments (5)
pkg/controller/external_secrets/controller.go (1)
111-111: CRD verbs tightened — looks good.Dropping create/delete for CRDs aligns with OLM owning CRDs.
bundle/manifests/external-secrets-operator.clusterserviceversion.yaml (1)
397-405: CRD writes limited to get/list/patch/update/watch — LGTM.Dropping create/delete for CRDs aligns with OLM owning CRD lifecycle while allowing the operator to patch/update as needed.
Makefile (3)
121-124: New validate-rbac target — LGTM.Good addition to surface RBAC/resourceNames drift in CI.
372-372: Wire validate-rbac into verify — LGTM.This ensures validation runs by default.
287-289: Centralize operator-sdk install — LGTM.Using hack/operator-sdk.sh improves reproducibility.
| // +kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=validatingwebhookconfigurations,verbs=get;list;watch | ||
| // +kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=validatingwebhookconfigurations,verbs=create;update;patch,resourceNames=externalsecret-validate;secretstore-validate | ||
| // +kubebuilder:rbac:groups="",resources=events;secrets;services;serviceaccounts,verbs=get;list;watch;create;update;delete;patch |
There was a problem hiding this comment.
Webhook RBAC: add global create; remove create from name-scoped rule.
Create cannot be authorized with resourceNames. Without a global create, webhook creation will fail and escalate checks will also fail.
-// +kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=validatingwebhookconfigurations,verbs=get;list;watch
-// +kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=validatingwebhookconfigurations,verbs=create;update;patch,resourceNames=externalsecret-validate;secretstore-validate
+// +kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=validatingwebhookconfigurations,verbs=create;get;list;watch
+// +kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=validatingwebhookconfigurations,verbs=update;patch,resourceNames=externalsecret-validate;secretstore-validate🤖 Prompt for AI Agents
In pkg/controller/external_secrets/controller.go around lines 100 to 102, the
RBAC rules declare create on validatingwebhookconfigurations only with
resourceNames which is invalid because create cannot be scoped by resourceNames;
add a separate global rule granting create on validatingwebhookconfigurations
(verbs=create) and remove create from the name-scoped RBAC line so the webhook
can be created successfully while keeping name-scoped update/patch as needed.
| // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=list;watch | ||
| // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;create;update,resourceNames=external-secrets;external-secrets-cert-controller;external-secrets-webhook;bitwarden-sdk-server | ||
| // +kubebuilder:rbac:groups=cert-manager.io,resources=certificates;clusterissuers;issuers,verbs=get;list;watch;create;update |
There was a problem hiding this comment.
Deployments RBAC: create cannot be name-scoped; add global create or move to a namespaced Role.
As-is, operator won't be able to create its Deployments. Quick fix below; longer-term consider moving create to a namespaced Role bound to the operator SA.
-// +kubebuilder:rbac:groups=apps,resources=deployments,verbs=list;watch
-// +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;create;update,resourceNames=external-secrets;external-secrets-cert-controller;external-secrets-webhook;bitwarden-sdk-server
+// +kubebuilder:rbac:groups=apps,resources=deployments,verbs=create;list;watch
+// +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;update,resourceNames=external-secrets;external-secrets-cert-controller;external-secrets-webhook;bitwarden-sdk-serverAlternative (recommended hardening): move create/get/update for these resourceNames into a namespaced Role (CSV permissions) instead of ClusterRole.
📝 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.
| // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=list;watch | |
| // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;create;update,resourceNames=external-secrets;external-secrets-cert-controller;external-secrets-webhook;bitwarden-sdk-server | |
| // +kubebuilder:rbac:groups=cert-manager.io,resources=certificates;clusterissuers;issuers,verbs=get;list;watch;create;update | |
| // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=create;list;watch | |
| // +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;update,resourceNames=external-secrets;external-secrets-cert-controller;external-secrets-webhook;bitwarden-sdk-server | |
| // +kubebuilder:rbac:groups=cert-manager.io,resources=certificates;clusterissuers;issuers,verbs=get;list;watch;create;update |
🤖 Prompt for AI Agents
In pkg/controller/external_secrets/controller.go around lines 103-105, the RBAC
kubebuilder comment scopes the create verb to specific resourceNames which is
invalid for create; fix by removing resourceNames from the create rule or by
moving the create permission into a namespaced Role bound to the operator
ServiceAccount. Concretely, update the RBAC annotation so ClusterRole grants
create (and other cluster-scoped verbs) on deployments without resourceNames, or
split responsibilities: keep get/update for the specific resourceNames in a Role
(namespaced) bound to the SA and grant create globally in the ClusterRole.
|
@chiragkyal: The following tests failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
/hold
Summary by CodeRabbit