feat: generate OpenAPI schemas from a running openshift-apiserver instance#645
Open
ingvagabund wants to merge 3 commits into
Open
feat: generate OpenAPI schemas from a running openshift-apiserver instance#645ingvagabund wants to merge 3 commits into
ingvagabund wants to merge 3 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds scripts and Makefile wiring to boot a local control plane, generate OpenAPI v2/v3 discovery and per-group specs, persist JSON under api/openapi-spec and api/discovery, and verify artifacts are up to date. ChangesOpenAPI Specification Generation and Verification
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
api/openapi-spec/v3/apis__apps.openshift.io_openapi.json (1)
1-165: Consider defining security requirements for operations in the generation process.All API group specifications define a
BearerTokensecurity scheme but don't declare security requirements at the global level or on individual operations. While the info description notes that client certificates may be used without bearer tokens, OpenAPI best practice recommends explicitly declaring security requirements (using an empty array[]for operations that allow anonymous access).Since these are auto-generated specifications, this observation applies to the generation process rather than manual file edits.
🤖 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 `@api/openapi-spec/v3/apis__apps.openshift.io_openapi.json` around lines 1 - 165, The OpenAPI spec defines a BearerToken security scheme but never attaches security requirements; update the generator to emit security requirements (either globally or per-operation) so operations that require bearer tokens reference the "BearerToken" scheme and operations that allow anonymous access explicitly use an empty security array. Specifically, modify the OpenAPI generation logic that builds components.securitySchemes (symbol: "BearerToken") to also populate either top-level "security" or each operation's "security" (e.g., for operationId "getAppsOpenshiftIoAPIGroup") with [{"BearerToken": []}] for token-protected endpoints and [] for anonymous endpoints, ensuring the generator distinguishes endpoints that may use client certs versus those that require bearer tokens.
🤖 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 `@hack/update-openapi-spec.sh`:
- Line 116: Define a shared CURL_FLAGS variable with sensible retry and timeout
bounds (e.g. --retry, --retry-delay, --retry-connrefused, --max-time) and
replace the raw curl invocations so they include $CURL_FLAGS; update the curl
that downloads "${KUBE_APISERVER}" (current call: curl -fL --progress-bar -o
"${KUBE_APISERVER}" "${binary_url}") and the other curl calls in this script
(the calls referenced in the review at lines 124, 283, 290, 300, 307) to use the
shared CURL_FLAGS in addition to existing flags so all fetches have bounded
retries and timeouts.
- Line 115: The script update-openapi-spec.sh currently hardcodes binary_url to
"https://dl.k8s.io/${KUBE_VERSION}/bin/linux/amd64/kube-apiserver" (lines
referencing binary_url at the top and again around line 123), so add an explicit
OS/arch precheck near the start of the script that uses uname -s and uname -m to
detect the host platform, validates it against supported values (e.g., Linux and
x86_64/amd64), and on unsupported platforms prints a clear error and exits
non-zero; then use the detected OS/ARCH to build binary_url (or bail early) so
the script fails fast with a helpful message instead of assuming linux/amd64.
- Around line 115-126: The script downloads and makes executables
(KUBE_APISERVER via binary_url and ETCD via tarball_url into paths
KUBE_APISERVER and ETCD) without verifying integrity; add checksum/signature
validation steps: obtain expected checksums or signatures for KUBE_VERSION and
ETCD_VERSION (from a trusted source), download the corresponding .sha256/.sig
file or release assets, verify the downloaded files with sha256sum --check or
gpg --verify before chmod +x and moving into BIN_DIR/TMP_DIR, and fail the
script with an explicit error if verification fails; ensure you reference
binary_url/KUBE_APISERVER and tarball_url/ETCD handling in the same code paths
so verification occurs prior to executing or installing the artifacts.
---
Nitpick comments:
In `@api/openapi-spec/v3/apis__apps.openshift.io_openapi.json`:
- Around line 1-165: The OpenAPI spec defines a BearerToken security scheme but
never attaches security requirements; update the generator to emit security
requirements (either globally or per-operation) so operations that require
bearer tokens reference the "BearerToken" scheme and operations that allow
anonymous access explicitly use an empty security array. Specifically, modify
the OpenAPI generation logic that builds components.securitySchemes (symbol:
"BearerToken") to also populate either top-level "security" or each operation's
"security" (e.g., for operationId "getAppsOpenshiftIoAPIGroup") with
[{"BearerToken": []}] for token-protected endpoints and [] for anonymous
endpoints, ensuring the generator distinguishes endpoints that may use client
certs versus those that require bearer tokens.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 12e64598-e42b-4d4d-90ae-1ec14cded71a
📒 Files selected for processing (26)
Makefileapi/discovery/v3-discovery.jsonapi/openapi-spec/swagger.jsonapi/openapi-spec/v3/apis__apps.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__apps.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__authorization.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__authorization.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__build.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__build.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__image.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__image.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__project.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__project.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__quota.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__quota.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__route.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__route.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__security.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__security.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__template.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__template.openshift.io_openapi.jsonapi/openapi-spec/v3/apis_openapi.jsonapi/openapi-spec/v3/version__openshift_openapi.jsonapi/openapi-spec/v3/version_openapi.jsonhack/update-openapi-spec.shhack/verify-openapi-spec.sh
Comment on lines
+115
to
+126
| binary_url="https://dl.k8s.io/${KUBE_VERSION}/bin/linux/amd64/kube-apiserver" | ||
| curl -fL --progress-bar -o "${KUBE_APISERVER}" "${binary_url}" | ||
| chmod +x "${KUBE_APISERVER}" | ||
| echo "Downloaded kube-apiserver to ${KUBE_APISERVER}" | ||
|
|
||
| # Download etcd binary | ||
| echo "Downloading etcd ${ETCD_VERSION}..." | ||
| ETCD="${BIN_DIR}/etcd" | ||
| tarball_url="https://github.com/etcd-io/etcd/releases/download/${ETCD_VERSION}/etcd-${ETCD_VERSION}-linux-amd64.tar.gz" | ||
| curl -fL --progress-bar "${tarball_url}" | tar xz -C "${TMP_DIR}" "etcd-${ETCD_VERSION}-linux-amd64/etcd" --strip-components=1 | ||
| mv "${TMP_DIR}/etcd" "${ETCD}" | ||
| chmod +x "${ETCD}" |
There was a problem hiding this comment.
Verify binary integrity before executing downloaded artifacts.
Line 115 and Line 123 download executable binaries and run them without checksum/signature validation. This weakens supply-chain guarantees for the generation pipeline.
Suggested hardening
+KUBE_APISERVER_SHA256="${KUBE_APISERVER_SHA256:-}"
+ETCD_SHA256="${ETCD_SHA256:-}"
+
curl -fL --progress-bar -o "${KUBE_APISERVER}" "${binary_url}"
+if [[ -n "${KUBE_APISERVER_SHA256}" ]]; then
+ echo "${KUBE_APISERVER_SHA256} ${KUBE_APISERVER}" | sha256sum -c -
+fi
chmod +x "${KUBE_APISERVER}"
...
curl -fL --progress-bar "${tarball_url}" | tar xz -C "${TMP_DIR}" "etcd-${ETCD_VERSION}-linux-amd64/etcd" --strip-components=1
mv "${TMP_DIR}/etcd" "${ETCD}"
+if [[ -n "${ETCD_SHA256}" ]]; then
+ echo "${ETCD_SHA256} ${ETCD}" | sha256sum -c -
+fi
chmod +x "${ETCD}"🤖 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 `@hack/update-openapi-spec.sh` around lines 115 - 126, The script downloads and
makes executables (KUBE_APISERVER via binary_url and ETCD via tarball_url into
paths KUBE_APISERVER and ETCD) without verifying integrity; add
checksum/signature validation steps: obtain expected checksums or signatures for
KUBE_VERSION and ETCD_VERSION (from a trusted source), download the
corresponding .sha256/.sig file or release assets, verify the downloaded files
with sha256sum --check or gpg --verify before chmod +x and moving into
BIN_DIR/TMP_DIR, and fail the script with an explicit error if verification
fails; ensure you reference binary_url/KUBE_APISERVER and tarball_url/ETCD
handling in the same code paths so verification occurs prior to executing or
installing the artifacts.
| echo "Downloading kube-apiserver ${KUBE_VERSION}..." | ||
| KUBE_APISERVER="${BIN_DIR}/kube-apiserver" | ||
| binary_url="https://dl.k8s.io/${KUBE_VERSION}/bin/linux/amd64/kube-apiserver" | ||
| curl -fL --progress-bar -o "${KUBE_APISERVER}" "${binary_url}" |
There was a problem hiding this comment.
Add retry and timeout bounds to all curl calls.
Lines 116, 124, 283, 290, 300, and 307 use unbounded/default curl behavior. This can cause flaky or hanging runs in CI/local environments during transient network issues.
Suggested hardening
+CURL_FLAGS=(--fail --silent --show-error --location --connect-timeout 10 --max-time 120 --retry 5 --retry-delay 1 --retry-all-errors)
...
-curl -fL --progress-bar -o "${KUBE_APISERVER}" "${binary_url}"
+curl "${CURL_FLAGS[@]}" --progress-bar -o "${KUBE_APISERVER}" "${binary_url}"
...
-curl -fL --progress-bar "${tarball_url}" | tar xz -C "${TMP_DIR}" "etcd-${ETCD_VERSION}-linux-amd64/etcd" --strip-components=1
+curl "${CURL_FLAGS[@]}" --progress-bar "${tarball_url}" | tar xz -C "${TMP_DIR}" "etcd-${ETCD_VERSION}-linux-amd64/etcd" --strip-components=1Apply the same CURL_FLAGS to fetches in Lines 283/290/300/307.
Also applies to: 124-124, 283-283, 290-290, 300-300, 307-307
🤖 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 `@hack/update-openapi-spec.sh` at line 116, Define a shared CURL_FLAGS variable
with sensible retry and timeout bounds (e.g. --retry, --retry-delay,
--retry-connrefused, --max-time) and replace the raw curl invocations so they
include $CURL_FLAGS; update the curl that downloads "${KUBE_APISERVER}" (current
call: curl -fL --progress-bar -o "${KUBE_APISERVER}" "${binary_url}") and the
other curl calls in this script (the calls referenced in the review at lines
124, 283, 290, 300, 307) to use the shared CURL_FLAGS in addition to existing
flags so all fetches have bounded retries and timeouts.
ingvagabund
force-pushed
the
run-apiserver-gen-openapi-integration-test
branch
from
d4f9cdb to
859fce7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@api/openapi-spec/v3/apis__apps.openshift.io_openapi.json`:
- Around line 110-117: The OpenAPI document defines
components.securitySchemes.BearerToken but does not declare security
requirements, so the API appears unauthenticated; add a security requirement
that references "BearerToken" (either as a top-level "security":
[{"BearerToken": []}] to apply globally or add an operation-level "security":
[{"BearerToken": []}] entry for the /apis/apps.openshift.io/ path (the operation
that currently documents the 401 response) so the contract explicitly requires
the BearerToken header ("authorization") for that endpoint.
In `@api/openapi-spec/v3/apis__template.openshift.io_openapi.json`:
- Around line 110-163: The generated OpenAPI v3 specs declare
components.securitySchemes.BearerToken but never add a corresponding security
requirement, so add a normalization step in the generator
(hack/update-openapi-spec.sh) that, when writing JSON under
api/openapi-spec/v3/, injects a top-level "security": [ { "BearerToken": [] } ]
into each output file (or alternatively injects per-operation security arrays)
if components.securitySchemes.BearerToken exists and no security key is present;
update the script to detect the presence of
components.securitySchemes.BearerToken and add the security array before saving
each JSON file so client generators see the auth requirement.
In `@api/openapi-spec/v3/version__openshift_openapi.json`:
- Line 22: The OpenAPI file version__openshift_openapi.json is empty for "paths"
— locate the generator/task that emits version__openshift_openapi.json (search
project for the basename "version__openshift" and generator scripts that
reference OpenAPI outputs), compare its template/inputs with the generator that
produces version_openapi.json to see whether it should include the /version path
or only components/schemas, then either update the generator to include /version
paths (so version__openshift_openapi.json matches the intended spec) or
remove/mark the file as an intentional placeholder and regenerate; check build
scripts (any OpenAPI generation or CI job) and run the generator to confirm the
resulting file is correct.
In `@hack/update-openapi-spec.sh`:
- Around line 257-269: When wait_for_url for openshift-apiserver fails we
currently disable the trap before exiting which leaves etcd and kube-apiserver
running; update the failure path to invoke the existing cleanup routine (call
the script's cleanup function or kill the background PIDs such as ETCD_PID and
KUBE_APISERVER_PID) before printing logs and running "trap - EXIT SIGINT
SIGTERM", then preserve the temp dir and exit. Ensure you reference the same
identifiers used elsewhere in the script (wait_for_url, TMP_DIR, the cleanup
function name or ETCD_PID/KUBE_APISERVER_PID) so processes are terminated before
the trap is cleared.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ed184a3a-1fb8-4630-94a4-c38504ebbd13
📒 Files selected for processing (26)
Makefileapi/discovery/v3-discovery.jsonapi/openapi-spec/swagger.jsonapi/openapi-spec/v3/apis__apps.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__apps.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__authorization.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__authorization.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__build.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__build.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__image.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__image.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__project.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__project.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__quota.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__quota.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__route.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__route.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__security.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__security.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__template.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__template.openshift.io_openapi.jsonapi/openapi-spec/v3/apis_openapi.jsonapi/openapi-spec/v3/version__openshift_openapi.jsonapi/openapi-spec/v3/version_openapi.jsonhack/update-openapi-spec.shhack/verify-openapi-spec.sh
✅ Files skipped from review due to trivial changes (1)
- api/discovery/v3-discovery.json
Comment on lines
+257
to
+269
| # Wait for openshift-apiserver health | ||
| if ! wait_for_url "https://${OPENSHIFT_API_HOST}:${OPENSHIFT_API_PORT}/healthz" "openshift-apiserver" 120; then | ||
| echo "Full log saved to: ${TMP_DIR}/openshift-apiserver.log" | ||
| echo "" | ||
| echo "Last 50 lines of log:" | ||
| tail -50 "${TMP_DIR}/openshift-apiserver.log" || true | ||
| echo "" | ||
| echo "Temp directory preserved at: ${TMP_DIR}" | ||
| echo "To inspect:" | ||
| echo " cat ${TMP_DIR}/openshift-apiserver.log" | ||
| trap - EXIT SIGINT SIGTERM | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Orphaned processes remain running when health check fails.
When wait_for_url fails for openshift-apiserver, the trap is disabled (Line 267) to preserve the temp directory for debugging, but etcd and kube-apiserver processes are left running as orphans. The cleanup function is never called in this path.
Suggested fix: kill processes before disabling the trap
if ! wait_for_url "https://${OPENSHIFT_API_HOST}:${OPENSHIFT_API_PORT}/healthz" "openshift-apiserver" 120; then
echo "Full log saved to: ${TMP_DIR}/openshift-apiserver.log"
echo ""
echo "Last 50 lines of log:"
tail -50 "${TMP_DIR}/openshift-apiserver.log" || true
echo ""
echo "Temp directory preserved at: ${TMP_DIR}"
echo "To inspect:"
echo " cat ${TMP_DIR}/openshift-apiserver.log"
+ # Stop processes but preserve temp directory for debugging
+ if [[ -n "${OPENSHIFT_APISERVER_PID}" ]]; then
+ kill "${OPENSHIFT_APISERVER_PID}" 2>/dev/null || true
+ wait "${OPENSHIFT_APISERVER_PID}" 2>/dev/null || true
+ fi
+ if [[ -n "${KUBE_APISERVER_PID}" ]]; then
+ kill "${KUBE_APISERVER_PID}" 2>/dev/null || true
+ wait "${KUBE_APISERVER_PID}" 2>/dev/null || true
+ fi
+ if [[ -n "${ETCD_PID}" ]]; then
+ kill "${ETCD_PID}" 2>/dev/null || true
+ wait "${ETCD_PID}" 2>/dev/null || true
+ fi
trap - EXIT SIGINT SIGTERM
exit 1
fi🤖 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 `@hack/update-openapi-spec.sh` around lines 257 - 269, When wait_for_url for
openshift-apiserver fails we currently disable the trap before exiting which
leaves etcd and kube-apiserver running; update the failure path to invoke the
existing cleanup routine (call the script's cleanup function or kill the
background PIDs such as ETCD_PID and KUBE_APISERVER_PID) before printing logs
and running "trap - EXIT SIGINT SIGTERM", then preserve the temp dir and exit.
Ensure you reference the same identifiers used elsewhere in the script
(wait_for_url, TMP_DIR, the cleanup function name or
ETCD_PID/KUBE_APISERVER_PID) so processes are terminated before the trap is
cleared.
Member
Author
|
/retest-required |
ingvagabund
force-pushed
the
run-apiserver-gen-openapi-integration-test
branch
from
859fce7 to
68d26d4
Compare
Member
Author
|
/retest-required |
ingvagabund
force-pushed
the
run-apiserver-gen-openapi-integration-test
branch
from
68d26d4 to
eb49767
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 `@hack/update-openapi-spec.sh`:
- Around line 286-291: The rm -rf invocation is unsafe if DISCOVERY_ROOT_DIR is
empty; add a guard that fails fast when DISCOVERY_ROOT_DIR is unset or an empty
string (e.g., use Bash parameter-expansion that errors on empty variables or an
explicit if-check that exits with an error) before running rm -rf
"${DISCOVERY_ROOT_DIR}"/*, and keep the rest of the logic that creates the
directory and writes v3-discovery.json unchanged; reference the
DISCOVERY_ROOT_DIR variable and the rm -rf line to locate where to add the
check.
- Around line 294-297: The rm -rf command can expand to dangerous paths if
OPENAPI_ROOT_DIR is empty; modify the script around the mkdir -p and rm -rf
calls to guard against an empty OPENAPI_ROOT_DIR by validating the variable
before using it (for example, fail fast if OPENAPI_ROOT_DIR is unset/empty or
use shell parameter validation), and then use the validated variable when
invoking mkdir -p and rm -rf so the operations only run when OPENAPI_ROOT_DIR is
non-empty (refer to the OPENAPI_ROOT_DIR variable and the existing mkdir -p and
rm -rf "${OPENAPI_ROOT_DIR}"/v3/* usages to locate where to add the guard).
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3ca34840-a0a8-4a7a-8b43-3c3fe0f69a07
📒 Files selected for processing (26)
Makefileapi/discovery/v3-discovery.jsonapi/openapi-spec/swagger.jsonapi/openapi-spec/v3/apis__apps.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__apps.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__authorization.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__authorization.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__build.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__build.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__image.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__image.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__project.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__project.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__quota.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__quota.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__route.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__route.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__security.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__security.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__template.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__template.openshift.io_openapi.jsonapi/openapi-spec/v3/apis_openapi.jsonapi/openapi-spec/v3/version__openshift_openapi.jsonapi/openapi-spec/v3/version_openapi.jsonhack/update-openapi-spec.shhack/verify-openapi-spec.sh
Comment on lines
+286
to
+291
| echo "Updating ${DISCOVERY_ROOT_DIR}/v3-discovery.json" | ||
| mkdir -p "${DISCOVERY_ROOT_DIR}" | ||
| rm -rf "${DISCOVERY_ROOT_DIR}"/* | ||
| curl -w "\n" -kfsS "${base_url}/openapi/v3" \ | ||
| | jq -S '.paths |= with_entries(.value.serverRelativeURL |= sub("\\?hash=.*$"; ""))' \ | ||
| > "${DISCOVERY_ROOT_DIR}/v3-discovery.json" |
There was a problem hiding this comment.
Guard against empty variable in rm -rf to prevent catastrophic deletion.
If DISCOVERY_ROOT_DIR is accidentally set to an empty string, rm -rf "${DISCOVERY_ROOT_DIR}"/* would expand to rm -rf /*, potentially destroying the filesystem. Use parameter expansion to fail fast if the variable is empty.
Proposed fix
mkdir -p "${DISCOVERY_ROOT_DIR}"
-rm -rf "${DISCOVERY_ROOT_DIR}"/*
+rm -rf "${DISCOVERY_ROOT_DIR:?}"/*📝 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.
Suggested change
| echo "Updating ${DISCOVERY_ROOT_DIR}/v3-discovery.json" | |
| mkdir -p "${DISCOVERY_ROOT_DIR}" | |
| rm -rf "${DISCOVERY_ROOT_DIR}"/* | |
| curl -w "\n" -kfsS "${base_url}/openapi/v3" \ | |
| | jq -S '.paths |= with_entries(.value.serverRelativeURL |= sub("\\?hash=.*$"; ""))' \ | |
| > "${DISCOVERY_ROOT_DIR}/v3-discovery.json" | |
| echo "Updating ${DISCOVERY_ROOT_DIR}/v3-discovery.json" | |
| mkdir -p "${DISCOVERY_ROOT_DIR}" | |
| rm -rf "${DISCOVERY_ROOT_DIR:?}"/* | |
| curl -w "\n" -kfsS "${base_url}/openapi/v3" \ | |
| | jq -S '.paths |= with_entries(.value.serverRelativeURL |= sub("\\?hash=.*$"; ""))' \ | |
| > "${DISCOVERY_ROOT_DIR}/v3-discovery.json" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 288-288: Use "${var:?}" to ensure this never expands to /* .
(SC2115)
🤖 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 `@hack/update-openapi-spec.sh` around lines 286 - 291, The rm -rf invocation is
unsafe if DISCOVERY_ROOT_DIR is empty; add a guard that fails fast when
DISCOVERY_ROOT_DIR is unset or an empty string (e.g., use Bash
parameter-expansion that errors on empty variables or an explicit if-check that
exits with an error) before running rm -rf "${DISCOVERY_ROOT_DIR}"/*, and keep
the rest of the logic that creates the directory and writes v3-discovery.json
unchanged; reference the DISCOVERY_ROOT_DIR variable and the rm -rf line to
locate where to add the check.
Comment on lines
+294
to
+297
| echo "Updating ${OPENAPI_ROOT_DIR}/v3 for OpenAPI v3" | ||
| echo "" | ||
| mkdir -p "${OPENAPI_ROOT_DIR}/v3" | ||
| rm -rf "${OPENAPI_ROOT_DIR}"/v3/* || true |
There was a problem hiding this comment.
Same empty variable guard needed here.
Apply the same fix to prevent potential rm -rf /* if OPENAPI_ROOT_DIR is empty.
Proposed fix
mkdir -p "${OPENAPI_ROOT_DIR}/v3"
-rm -rf "${OPENAPI_ROOT_DIR}"/v3/* || true
+rm -rf "${OPENAPI_ROOT_DIR:?}"/v3/* || true🤖 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 `@hack/update-openapi-spec.sh` around lines 294 - 297, The rm -rf command can
expand to dangerous paths if OPENAPI_ROOT_DIR is empty; modify the script around
the mkdir -p and rm -rf calls to guard against an empty OPENAPI_ROOT_DIR by
validating the variable before using it (for example, fail fast if
OPENAPI_ROOT_DIR is unset/empty or use shell parameter validation), and then use
the validated variable when invoking mkdir -p and rm -rf so the operations only
run when OPENAPI_ROOT_DIR is non-empty (refer to the OPENAPI_ROOT_DIR variable
and the existing mkdir -p and rm -rf "${OPENAPI_ROOT_DIR}"/v3/* usages to locate
where to add the guard).
eb49767 to
0fbc4d9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
hack/update-openapi-spec.sh (1)
254-264:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTerminate background processes before disabling the exit trap.
On the health-check failure path, the trap is cleared and the script exits, so background
etcd/kube-apiserver/openshift-apiserverprocesses can be left running.Proposed fix
if ! wait_for_url "https://${OPENSHIFT_API_HOST}:${OPENSHIFT_API_PORT}/healthz" "openshift-apiserver" 120; then echo "Full log saved to: ${TMP_DIR}/openshift-apiserver.log" echo "" echo "Last 50 lines of log:" tail -50 "${TMP_DIR}/openshift-apiserver.log" || true echo "" echo "Temp directory preserved at: ${TMP_DIR}" echo "To inspect:" echo " cat ${TMP_DIR}/openshift-apiserver.log" + # Stop background processes but keep TMP_DIR for debugging. + if [[ -n "${OPENSHIFT_APISERVER_PID}" ]]; then + kill "${OPENSHIFT_APISERVER_PID}" 2>/dev/null || true + wait "${OPENSHIFT_APISERVER_PID}" 2>/dev/null || true + fi + if [[ -n "${KUBE_APISERVER_PID}" ]]; then + kill "${KUBE_APISERVER_PID}" 2>/dev/null || true + wait "${KUBE_APISERVER_PID}" 2>/dev/null || true + fi + if [[ -n "${ETCD_PID}" ]]; then + kill "${ETCD_PID}" 2>/dev/null || true + wait "${ETCD_PID}" 2>/dev/null || true + fi trap - EXIT SIGINT SIGTERM exit 1 fi🤖 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 `@hack/update-openapi-spec.sh` around lines 254 - 264, The health-check failure branch clears the trap and exits without stopping background services (etcd/kube-apiserver/openshift-apiserver), which can leave processes running; before resetting the trap and exiting in the wait_for_url failure block, call the existing cleanup/stop routine (or explicitly kill and wait on the background PIDs such as ${ETCD_PID}, ${KUBE_APISERVER_PID}, ${OPENSHIFT_APISERVER_PID}) to terminate those processes, then preserve logs as before, restore or clear the trap, and finally exit 1. Ensure you reference wait_for_url, TMP_DIR, trap - EXIT SIGINT SIGTERM and the background PID variables or cleanup function to locate where to add the termination logic.
🤖 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.
Duplicate comments:
In `@hack/update-openapi-spec.sh`:
- Around line 254-264: The health-check failure branch clears the trap and exits
without stopping background services (etcd/kube-apiserver/openshift-apiserver),
which can leave processes running; before resetting the trap and exiting in the
wait_for_url failure block, call the existing cleanup/stop routine (or
explicitly kill and wait on the background PIDs such as ${ETCD_PID},
${KUBE_APISERVER_PID}, ${OPENSHIFT_APISERVER_PID}) to terminate those processes,
then preserve logs as before, restore or clear the trap, and finally exit 1.
Ensure you reference wait_for_url, TMP_DIR, trap - EXIT SIGINT SIGTERM and the
background PID variables or cleanup function to locate where to add the
termination logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d2e06960-636a-4930-867d-864e968b30a5
📒 Files selected for processing (35)
Makefileapi/discovery/apis__apps.openshift.io__v1.jsonapi/discovery/apis__authorization.openshift.io__v1.jsonapi/discovery/apis__build.openshift.io__v1.jsonapi/discovery/apis__image.openshift.io__v1.jsonapi/discovery/apis__project.openshift.io__v1.jsonapi/discovery/apis__quota.openshift.io__v1.jsonapi/discovery/apis__route.openshift.io__v1.jsonapi/discovery/apis__security.openshift.io__v1.jsonapi/discovery/apis__template.openshift.io__v1.jsonapi/discovery/v3-discovery.jsonapi/openapi-spec/swagger.jsonapi/openapi-spec/v3/apis__apps.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__apps.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__authorization.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__authorization.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__build.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__build.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__image.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__image.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__project.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__project.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__quota.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__quota.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__route.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__route.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__security.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__security.openshift.io_openapi.jsonapi/openapi-spec/v3/apis__template.openshift.io__v1_openapi.jsonapi/openapi-spec/v3/apis__template.openshift.io_openapi.jsonapi/openapi-spec/v3/apis_openapi.jsonapi/openapi-spec/v3/version__openshift_openapi.jsonapi/openapi-spec/v3/version_openapi.jsonhack/update-openapi-spec.shhack/verify-openapi-spec.sh
✅ Files skipped from review due to trivial changes (7)
- api/discovery/apis__project.openshift.io__v1.json
- api/discovery/apis__security.openshift.io__v1.json
- api/discovery/apis__route.openshift.io__v1.json
- api/discovery/apis__template.openshift.io__v1.json
- api/discovery/apis__build.openshift.io__v1.json
- api/discovery/apis__image.openshift.io__v1.json
- api/discovery/apis__authorization.openshift.io__v1.json
ingvagabund
force-pushed
the
run-apiserver-gen-openapi-integration-test
branch
from
0fbc4d9 to
cb664d8
Compare
ingvagabund
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Comment on lines
+28
to
+30
| # Kubernetes version (must match vendored version in go.mod) | ||
| KUBE_VERSION="${KUBE_VERSION:-v1.34.1}" | ||
| ETCD_VERSION="${ETCD_VERSION:-v3.5.17}" |
There was a problem hiding this comment.
If they must match the version in go.mod, maybe something like:
KUBE_VERSION=$(go list -m -f '{{.Version}}' k8s.io/apiserver | awk '{ sub(/v0/, "v1"); print}')
ETCD_VERSION=$(go list -m -f '{{.Version}}' go.etcd.io/etcd/server/v3)is more appropriate?
Those will make sure that we are only using the versions of those dependencies that are specified in the go.mod file (ignoring any redirects).
Member
Author
There was a problem hiding this comment.
Good point. Yeah, this could work. Assuming k8s.io/apiserver and go.etcd.io/etcd/server/v3 will always get set to a valid version string. Which (for testing or debugging purposes) might not always be the case. Even though any fork of either of the repos should have the version tags present as well.
Member
Author
There was a problem hiding this comment.
Updated. Added checks for the version format.
Comment on lines
+267
to
+268
| echo "Waiting for OpenAPI aggregation to complete..." | ||
| sleep 10 |
There was a problem hiding this comment.
Is there a signal other than sleeping for 10 seconds for us to know when OpenAPI aggregation is complete?
Member
Author
There was a problem hiding this comment.
Code updated by pulling the endpoint
Contributor
|
@ingvagabund: all tests passed! 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Making the generated files look as close to https://github.com/kubernetes/kubernetes/tree/master/api as possible. The newly added
update-openapi-spec.shmirrors the same pattern from https://github.com/kubernetes/kubernetes/blob/master/hack/update-openapi-spec.sh. The same forverify-openapi-spec.sh.Currently:
Summary by CodeRabbit
New Features
Chores