Repository navigation
Conversation
iamWHTWD
left a comment
iamWHTWD
left a comment
There was a problem hiding this comment.
I reviewed the chart and checker at 317ba889. The local test suite passed. The inline comments cover first-install Registry access, probe reliability, HTTPS and credential handling, and Helm upgrade behavior.
| podSelector: | ||
| matchLabels: | ||
| {{- toYaml .Values.registry.podSelector | nindent 6 }} | ||
| policyTypes: [Ingress] |
There was a problem hiding this comment.
[P1] Preserve existing Registry ingress on first install
If this is the first ingress NetworkPolicy selecting these Registry Pods, installing the chart makes them ingress-isolated. Existing clients such as BuildKit or direct push/pull clients can lose access while all five seed probes still pass. Could we make policy creation opt-in and require operators to verify that their existing clients are covered before enabling it? Please also reject an empty registry.ports list. It renders as ports: null, which allows matching seeds to reach every destination port.
| config = "".join(f"{key} = {json.dumps(value)}\n" for key, value in options.items()) | ||
| config += "".join(f"header = {json.dumps(key + ': ' + value)}\n" for key, value in (headers or {}).items()) | ||
| payload = config.encode() | ||
| command = ["/bin/sh", "-c", 'head -c "$1" | curl --config - --silent --show-error --include --write-out "\\n__AKERNEL_HTTP_STATUS__%{http_code}\\n" | base64', "registry-check", str(len(payload))] |
There was a problem hiding this comment.
[P2] Fail the check when curl fails
The pipeline reports base64's exit status and the checker only reads curl's HTTP status. A local response that sent HTTP 200 with a larger Content-Length than its body made curl exit 18, while the pipeline exited 0 and the checker decoded 200. If the received bytes match the expected digest, this can record a failed transfer as a successful probe. Please propagate and validate curl's exit status and add a regression test for this case.
| if status != 200: | ||
| raise CheckFailed("authenticated bootstrap read failed") | ||
| result = bootstrap_contents(body, descriptor) | ||
| status, _, body = http.get(url, auth, proxy=cfg.get("seedProxy", "http://127.0.0.1:4001")) |
There was a problem hiding this comment.
[P2] Account for Dragonfly's proxy certificate in the HTTPS probe
For an HTTPS Registry URL, curl uses CONNECT. Dragonfly terminates that TLS connection and presents its own certificate, which is self-signed when no proxy CA is configured. The README's HTTPS example only mentions trusting the Registry and token service CAs, so a healthy Registry can still fail this check. Please document and verify the proxy CA requirement with an HTTPS probe.
| raise ValueError("missing token") | ||
| except (ValueError, TypeError) as error: | ||
| raise CheckFailed("invalid pull token response") from error | ||
| auth = {"Authorization": "Bearer " + token, "Accept": "application/vnd.oci.image.manifest.v1+json, application/vnd.docker.distribution.manifest.v2+json"} |
There was a problem hiding this comment.
[P2] Keep the pull token out of seed logs
This bearer header is also sent through the Dragonfly proxy for the bootstrap request. Dragonfly v1.3.0 has INFO-level forwarding logs that include the full request, so the token can appear in seed logs even when the checker's own logs are redacted. Please require redaction or disable the relevant request logging, and verify the behavior with a dummy credential before promising that tokens stay out of logs.
| name: {{ .Release.Name }}-state | ||
| namespace: {{ .Release.Namespace }} | ||
| data: | ||
| state.json: '{}' |
There was a problem hiding this comment.
[P2] Let the Job own state.json
With Helm 4 server-side apply, Helm initially owns this field. Once a successful Job patches it, a later upgrade can conflict when Helm tries to restore {}. Could the chart create the ConfigMap without this key and let the Job write it? The checker already handles a missing data field.
| connection.update(timeout=1) | ||
| while connection.peek_stdout(): | ||
| chunk = connection.read_stdout() | ||
| size += len(chunk) |
There was a problem hiding this comment.
[P2] Fit the allowed probe size within the Job's memory limit
This buffers base64 output before joining and decoding it. In a local simulation, five concurrent 20 MiB responses reached about 494 MiB peak RSS. The chart caps the container at 256 MiB even though each response can be up to 32 MiB. Please align the response cap, concurrency, and memory limit so an allowed probe cannot OOM the checker.
|
Reviewed head The companion-release design is useful: keeping Registry Deployment/PVC ownership separate, ANDing namespace and Pod selectors, restricting exec to configured Pod names, and advancing success state only after stable verification are sensible boundaries. The existing inline review tracks other ingress, transport, credential and operational concerns. I reproduced two additional compatibility issues to address alongside those threads before merging. R1 — [P2] Support the configured seed count rather than requiring exactly fiveBoth the chart and Reproduction:
Please accept a nonempty, duplicate-free configured list, require every listed seed to be present/Ready and to pass verification, and generate the same name-scoped exec RBAC from that list. Five can remain an example/default profile; installing a connectivity guard should not require expanding an existing three-seed deployment. Add positive three-/five-seed cases and negative missing, unready and duplicate-name coverage. R2 — [P2] Handle valid Registry blob redirectsThe curl invocation does not follow redirects, and the bootstrap read treats a non-200 response as a failed check. Blob redirects are part of the Distribution API, and the OCI specification allows redirects while restricting Authorization forwarding across host boundaries. A healthy redirecting/object-storage-backed Registry can therefore produce permanently failed guard Jobs. I exercised the actual
Please support bounded redirects with an explicit credential-forwarding policy and retain final digest/size validation. Also parse the final response body correctly: simply adding Validation and scopeThe existing 18 checker/chart tests, These were isolated local fixtures, not a live CNI/Kubernetes-exec or production rollout qualification. The reported five-seed production results do not cover a three-seed topology or a redirecting Registry. Please rerun the affected tests and CI on the resulting head after the fixes. Non-blocking follow-ups: add build/minimal-start coverage for the checker image and pinned dependencies, and distinguish proxy cache-hit verification from evidence of a fresh origin fetch. |
Registry relocation can block Dragonfly image preparation when ingress relies only on CIDRs. Add a companion Helm release that verifies authenticated Nydus reads from every configured seed without owning Registry storage, resources or placement.
Behavior:
Validation:
Deployment status: this review updates code, not production runtime images. The existing namespace/Pod ingress rule remains installed. The newly added production checker CronJob is suspended because current Dragonfly INFO logging is not credential-safe; no Registry or Dragonfly rollout was performed.