SANDBOX-1561 | feature: debug builds for the host operator #1228
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MikelAlejoBR 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 |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds an env-driven override for the registration-service command, exposes it in the deployment template, and groups related constants. Introduces debug build and podman image/push debug targets, and extends the Dockerfile with prod/debug multi-stage flows including Delve-enabled debug stages. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Controller as Controller\n(getVars)
participant Env as Environment\n(REGISTRATION_SERVICE_COMMAND)
participant Template as Template\n(manifest vars)
participant K8s as Kubernetes API\n(Deployment)
participant Pod as Pod/Container\n(container start)
Controller->>Env: read REGISTRATION_SERVICE_COMMAND
Env-->>Controller: value or empty
Controller->>Template: set REGISTRATION_SERVICE_COMMAND var (env value or default)
Template->>K8s: apply Deployment manifest (command from var)
K8s->>Pod: create Pod with container command
Pod-->>K8s: Pod running with resolved command
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @build/bin/entrypoint:
- Around line 12-17: The Delve invocation in the debug branch should pass
operator arguments after a `--` so dlv doesn't consume operator flags and the
non-debug exec should quote `${OPERATOR}`; modify the debug branch to use `exec
/usr/local/bin/dlv ... -- "${OPERATOR}" "$@"` (i.e., add `--` and pass quoted
OPERATOR then "$@") and change the else branch to `exec "${OPERATOR}" "$@"` to
ensure proper quoting.
In @build/Dockerfile.debug:
- Around line 28-30: The LABEL instructions in the Dockerfile.debug use invalid
syntax with spaces around the equals sign (the lines containing LABEL maintainer
= "KubeSaw <[email protected]>" and LABEL author = "KubeSaw
<[email protected]>"); update those LABEL directives to valid Docker syntax
by removing spaces around '=' (e.g., maintainer="..." and author="...") or
convert them to the JSON array LABEL form so Docker parses the key/value pairs
correctly.
In @deploy/templates/registration-service/registration-service.yaml:
- Around line 98-105: Add a description to the COMMAND parameter documenting
that it must be a JSON array of strings (e.g., '["registration-service"]')
rather than a plain string; update the parameter definition for COMMAND to
include a descriptive "description" field explaining the required array format
and giving an example so consumers do not pass a single string like
"registration-service --foo" which would produce an invalid containers[].command
value when rendered.
In @make/podman.mk:
- Around line 13-17: The debug image target currently tags and pushes to the
same IMAGE variable, risking overwriting the release image; change the debug
build/push flow (targets podman-image-debug and podman-push-debug) to use a
separate tag (e.g., IMAGE_DEBUG or derive from IMAGE by appending "-debug") when
building and when pushing so the Delve-enabled image is stored under a distinct
name and never replaces the normal IMAGE.
🧹 Nitpick comments (3)
make/go.mk (1)
23-35: Consider ensuring$(OUT_DIR)/binexists forbuild-debug(parity withbuild).
If directory creation is currently an implicit side-effect of$(OUT_DIR)/operator,build-debug: generatemay fail in a clean workspace.make/podman.mk (1)
13-17: Declarepodman-image-debugas.PHONY(consistency).
Keeps behavior consistent if a file namedpodman-image-debugever appears.build/Dockerfile.debug (1)
1-21: Debug image likely needs extra runtime privileges; also consider multi-arch.Delve typically requires
SYS_PTRACE(and may be blocked by restrictive seccomp); make sure the debug deployment path sets the neededsecurityContextand that port50000is reachable (port-forward/service). Separately,linux-amd64hardcoding will break arm64 builds (common for local dev).Also applies to: 53-53
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
build/Dockerfile.debugbuild/bin/entrypointcontrollers/toolchainconfig/env.gocontrollers/toolchainconfig/toolchainconfig_controller.godeploy/templates/registration-service/registration-service.yamlmake/go.mkmake/podman.mk
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/toolchainconfig/toolchainconfig_controller.go (1)
controllers/toolchainconfig/env.go (1)
RegistrationServiceCommandEnvKey(5-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test with Coverage
🔇 Additional comments (1)
controllers/toolchainconfig/env.go (1)
3-6: New env key constant looks good.
Clear naming and colocated with the existing registration-service env key.
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources-host-op
branch
2 times, most recently
from
face830 to
2226d60
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@build/Dockerfile`:
- Line 28: Fix the typo in the Dockerfile comment: change "debgugging" to
"debugging" in the comment string that currently reads "# when setting up
breakpoints when debgugging." so the comment reads "# when setting up
breakpoints when debugging."
- Line 5: Fix the typo in the top-of-file comment in the Dockerfile: change
"setps" to "steps" in the comment line that currently reads "# Default
production build setps for the operator." so it correctly reads "# Default
production build steps for the operator."; this is purely a comment edit in the
Dockerfile header.
🧹 Nitpick comments (3)
build/Dockerfile (3)
44-46: Consider verifying the Go archive checksum for supply chain security.The Go tarball is downloaded without checksum verification. While this is for debug builds only, adding verification would harden against potential MITM or CDN compromise.
Proposed fix with checksum verification
-# Download and extract Golang. -RUN curl --location --output "/tmp/${GOLANG_VERSION}.linux-amd64.tar.gz" "https://go.dev/dl/${GOLANG_VERSION}.linux-amd64.tar.gz" \ +# Download Golang and verify its checksum. +RUN curl --location --output "/tmp/${GOLANG_VERSION}.linux-amd64.tar.gz" "https://go.dev/dl/${GOLANG_VERSION}.linux-amd64.tar.gz" \ + && curl --location --silent "https://go.dev/dl/${GOLANG_VERSION}.linux-amd64.tar.gz.sha256" -o "/tmp/go.sha256" \ + && echo "$(cat /tmp/go.sha256) /tmp/${GOLANG_VERSION}.linux-amd64.tar.gz" | sha256sum --check \ && tar --directory /usr/local --extract --file "/tmp/${GOLANG_VERSION}.linux-amd64.tar.gz" \ && rm --force "/tmp/${GOLANG_VERSION}.linux-amd64.tar.gz"
44-44: Architecture is hardcoded tolinux-amd64.This limits debug builds to x86_64. If ARM support (e.g., Apple Silicon, ARM-based clusters) is needed in the future, consider parameterizing the architecture.
Proposed fix to support multiple architectures
Add an ARG for architecture:
ARG GOLANG_VERSION +ARG TARGETARCH=amd64Then use it in the download URL:
-RUN curl --location --output "/tmp/${GOLANG_VERSION}.linux-amd64.tar.gz" "https://go.dev/dl/${GOLANG_VERSION}.linux-amd64.tar.gz" \ - && tar --directory /usr/local --extract --file "/tmp/${GOLANG_VERSION}.linux-amd64.tar.gz" \ - && rm --force "/tmp/${GOLANG_VERSION}.linux-amd64.tar.gz" +RUN curl --location --output "/tmp/${GOLANG_VERSION}.linux-${TARGETARCH}.tar.gz" "https://go.dev/dl/${GOLANG_VERSION}.linux-${TARGETARCH}.tar.gz" \ + && tar --directory /usr/local --extract --file "/tmp/${GOLANG_VERSION}.linux-${TARGETARCH}.tar.gz" \ + && rm --force "/tmp/${GOLANG_VERSION}.linux-${TARGETARCH}.tar.gz"
52-52: Delve installed with@latestmay cause non-reproducible builds.Using
@latestmeans the Delve version can change between builds. For debug tooling this is often acceptable, but pinning a version would improve reproducibility if needed.
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources-host-op
branch
from
2226d60 to
1e2e7ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@build/Dockerfile`:
- Around line 31-53: Pin Delve to a specific release instead of using `@latest`
and verify the downloaded Go tarball checksum before extraction: change the go
install invocation (GOBIN=/tmp/bin go install
github.com/go-delve/delve/cmd/dlv@latest) to use a fixed tag (e.g., `@v1.26.0`),
fetch the corresponding SHA256 for the ARG GOLANG_VERSION tarball from go.dev/dl
(or a vetted checksum mapping), download the checksum file or inline expected
checksum, compute and compare the SHA256 of
"/tmp/${GOLANG_VERSION}.linux-amd64.tar.gz" and abort on mismatch, and only then
extract and remove the tarball; keep references to ARG GOLANG_VERSION, the
curl+tar RUN block, and the GOBIN=/tmp/bin go install command when making these
changes.
♻️ Duplicate comments (3)
make/podman.mk (1)
13-17: Avoid pushing debug builds under the release image tag.Debug targets still build/push
${IMAGE}, so a Delve-enabled image can overwrite the release tag. Use a dedicated debug tag.🛠️ Suggested fix (separate debug image tag)
IMAGE_TAG ?= ${GIT_COMMIT_ID_SHORT} IMAGE ?= ${TARGET_REGISTRY}/${QUAY_NAMESPACE}/${GO_PACKAGE_REPO_NAME}:${IMAGE_TAG} +DEBUG_IMAGE_TAG ?= ${IMAGE_TAG}-debug +DEBUG_IMAGE ?= ${TARGET_REGISTRY}/${QUAY_NAMESPACE}/${GO_PACKAGE_REPO_NAME}:${DEBUG_IMAGE_TAG} @@ podman-image-debug: build-debug - $(Q) podman build --platform "${IMAGE_PLATFORM}" --build-arg BUILD_TYPE='debug' --build-arg GOLANG_VERSION="$$(go version | awk '{print $$3}')" --file build/Dockerfile.debug --tag "${IMAGE}" . + $(Q) podman build --platform "${IMAGE_PLATFORM}" --build-arg BUILD_TYPE='debug' --build-arg GOLANG_VERSION="$$(go version | awk '{print $$3}')" --file build/Dockerfile.debug --tag "${DEBUG_IMAGE}" . @@ podman-push-debug: check-namespace podman-image-debug - $(Q)podman push ${IMAGE} + $(Q)podman push ${DEBUG_IMAGE}Also applies to: 23-27
controllers/toolchainconfig/toolchainconfig_controller.go (1)
155-162: NormalizeREGISTRATION_SERVICE_COMMANDto a JSON array before templating.Passing a raw string can render an invalid
command:value. Accept a JSON array or wrap a single token.🛠️ Suggested fix (accept JSON array or single token)
import ( "context" + "encoding/json" "fmt" "os" "time" @@ - command := os.Getenv(RegistrationServiceCommandEnvKey) - if command != "" { - vars["COMMAND"] = command + raw := os.Getenv(RegistrationServiceCommandEnvKey) + if raw != "" { + var parsed []string + if err := json.Unmarshal([]byte(raw), &parsed); err == nil { + vars["COMMAND"] = raw + } else { + b, _ := json.Marshal([]string{raw}) + vars["COMMAND"] = string(b) + } } else { vars["COMMAND"] = `["registration-service"]` }deploy/templates/registration-service/registration-service.yaml (1)
104-104: Document the required array format forCOMMAND.Without a description, users may pass a scalar string and render an invalid
containers[].command.Kubernetes PodSpec container "command" field required type (array of strings) and OpenShift Template parameter substitution for array fields🛠️ Suggested fix (add description)
- name: COMMAND + description: >- + Container command as a JSON/YAML array string, e.g. ["registration-service"] or + ["dlv","exec","/usr/local/bin/registration-service","--","--flag"]. + Must evaluate to a list of strings (not a scalar). value: '["registration-service"]'Also applies to: 273-274
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources-host-op
branch
2 times, most recently
from
4ad51d6 to
5df8bf1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@build/Dockerfile`:
- Around line 34-47: The curl download hard-codes "linux-amd64" so builds fail
on arm64; update the Dockerfile to accept an architecture arg and use it in the
download filename: add an ARG (e.g., GOLANG_ARCH or use the automatic TARGETARCH
build-arg) and replace the hard-coded "linux-amd64" in the RUN curl/tar command
with "${GOLANG_ARCH}" (or the chosen arg) so the downloaded archive name becomes
"/tmp/${GOLANG_VERSION}.${GOLANG_ARCH}.tar.gz" and the tar command uses the same
variable.
c87b8d3 to
65c99ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@deploy/templates/registration-service/registration-service.yaml`:
- Around line 273-277: The description for the parameter
REGISTRATION_SERVICE_COMMAND has awkward grammar and should be tightened for
clarity; update the descriptive text associated with
REGISTRATION_SERVICE_COMMAND to use correct subject-verb agreement and simpler
phrasing (e.g., describe that it is the command used to launch the registration
service container, must be specified as a YAML array, and give the example
format ["registration-service", "arg1", ...]) while keeping the existing example
value '["registration-service"]'.
e3a129b to
601db4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@make/podman.mk`:
- Around line 14-17: Add a .PHONY declaration for the podman-image-debug make
target and update its Dockerfile reference: declare podman-image-debug as phony
via a .PHONY line, and change the build file argument from
build/Dockerfile.debug to build/Dockerfile (the Dockerfile already honors
BUILD_TYPE='debug'), keeping the existing build flags and variables in the
podman-image-debug rule.
66275f6 to
62a84bb
Compare
In order to be able to debug the operator live in the local cluster, we need the binary to be executed with Delve instead, and we need it to be built without any code optimizations or minimization so that the breakpoints work. For that purpose, a new Dockerfile is added which can build the image that way, and the corresponding Make targets make it easy to kick off the whole process with a single command. The Makefile targets also enable the "toolchain-e2e" repository to easily build the "debug" image. Also, when Developer Sandbox is deployed locally, usually the registration service gets managed by the "ToolChain config controller", which launches it with a specific command. In order to be able to launch the registration service with Delve, a few minor modifications are made so that that launch command can be overridden. SANDBOX-1561
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources-host-op
branch
from
62a84bb to
d198e5f
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1228 +/- ##
==========================================
- Coverage 70.15% 70.12% -0.04%
==========================================
Files 84 84
Lines 6565 6570 +5
==========================================
+ Hits 4606 4607 +1
- Misses 1747 1749 +2
- Partials 212 214 +2
🚀 New features to boost your workflow:
|
1 participant
In order to be able to debug the operator live in the local cluster, we need the binary to be executed with Delve instead, and we need it to be built without any code optimizations or minimization so that the breakpoints work.
For that purpose, a new Dockerfile is added which can build the image that way, and the corresponding Make targets make it easy to kick off the whole process with a single command.
The Makefile targets also enable the "toolchain-e2e" repository to easily build the "debug" image.
Also, when Developer Sandbox is deployed locally, usually the registration service gets managed by the "ToolChain config controller", which launches it with a specific command. In order to be able to launch the registration service with Delve, a few minor modifications are made so that that launch command can be overridden.
Jira ticket
[SANDBOX-1561]
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.