Deploy ziti-management and enable Ziti - #153
Conversation
Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
Clean, well-structured PR that follows existing patterns closely. All five spec items from #152 are addressed:
- ✅ Variables in
variables.tf— chart version, image tag, DB password, PVC size - ✅ Locals — resolved image tag, chart name, DB values, service values
- ✅
ziti_management_dbArgoCD application (sync-wave 8,wait = true, timeouts) - ✅
ziti_managementArgoCD application (sync-wave 17, dynamic automated sync) - ✅ Gateway updated with
depends_onand Ziti Helm values
One minor comment on PVC size default/description consistency. No blocking issues.
Summary
Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
Verified the fix: PVC default updated to 5Gi and description aligned to the established pattern. Previous comment resolved. LGTM — ready to merge.
Summary
Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
Verified the updated approach. Ziti configuration moved from custom gateway Helm values (zitiEnabled, zitiManagementGrpcTarget) to the standard env list (ZITI_ENABLED, ZITI_MANAGEMENT_GRPC_TARGET). This aligns with the established pattern used by all other services (agents, users, ziti-management, etc.). The gateway block is cleanly restored to its original three fields. LGTM.
Summary
Test & Lint Summary
|
Summary
Test & Lint Summary
|
The enrollment step was creating the platform namespace before Terraform, causing "namespaces platform already exists" error. Move the step to after stack:platform so the namespace already exists, and remove the redundant kubectl create namespace call.
The container image uses a named user (app) which causes CreateContainerConfigError with runAsNonRoot. Set explicit runAsUser=100 and runAsGroup=101 matching other services.
ziti-management pod needs to reach the Ziti controller management API at ziti-mgmt.<domain>. Without a CoreDNS rewrite, this resolves to 127.0.0.1 (loopback) inside the cluster, causing immediate connection refused and CrashLoopBackOff. Add rewrite rule mapping ziti-mgmt.<domain> to the controller management service in the ziti namespace.
Helm env array override replaces defaults entirely. The chart defaults include ZITI_CERT_FILE, ZITI_KEY_FILE, and ZITI_CA_FILE which are required for config.FromEnv(). Without them the container exits immediately with "ZITI_CERT_FILE must be set".
|
CI is now green ✅ after fixing three issues:
Requesting fresh review as previous approvals were dismissed by the new commits. |
noa-lucent
left a comment
There was a problem hiding this comment.
Re-reviewed after the four CI-fix commits. All changes look correct and well-integrated:
- Enrollment step (
apply.sh) — Correctly placed afterstack:platform, idempotent (ziti-certsexistence check), proper cleanup with trap/unset pattern. One trailing whitespace nit noted. - Security context (
ziti_management_values) —runAsUser=100/runAsGroup=101with full hardening (readOnlyRootFilesystem,drop ALL, seccomp) follows the established pattern for other services in this file. - CoreDNS rewrite (
stacks/deps/main.tf) —ziti-mgmtrewrite added with clear comment; existing block comment updated to reflect the change. - Cert env vars (
ZITI_CERT_FILE,ZITI_KEY_FILE,ZITI_CA_FILE) — Correctly added toziti_management_values. - OpenZiti CLI install (
.github/workflows/bootstrap.yml) — Consistent with the existing unpinned curl-to-bash pattern used for k3d.
All five spec items from #152 remain correctly addressed. The CI fixes are appropriate and don't introduce any structural concerns.
LGTM — one nit on trailing blank lines, not blocking.
The gateway code defaults to "ziti-management:50051" which matches what we were setting explicitly. No need to pass it.
noa-lucent
left a comment
There was a problem hiding this comment.
Verified the new commit: ZITI_MANAGEMENT_GRPC_TARGET correctly removed from the gateway env list since the Go service defaults to ziti-management:50051. The gateway now passes only ZITI_ENABLED=true, which is clean and sufficient.
Previous nit (trailing blank lines in apply.sh) resolved — not blocking.
All #152 requirements remain fully addressed. LGTM.
- Update ziti-management chart to v0.2.0 (adds startup self-enrollment) - Add PVC persistence, enrollment JWT secret mount, Recreate strategy - Change cert paths from /etc/ziti/ to /var/lib/ziti/ (writable PVC) - Add ZITI_ENROLLMENT_JWT_FILE env var - Create enrollment JWT secret in platform namespace via ziti remote state - Remove enroll-ziti-management step from apply.sh - Remove 'Install OpenZiti CLI' step from CI workflow
Self-enrollment update pushedSwitched Added:
Removed:
|
noa-lucent
left a comment
There was a problem hiding this comment.
Verified the self-enrollment refactor. This is a clean improvement over the previous shell-based approach:
- Removed:
enroll-ziti-managementstep inapply.sh(52 lines), OpenZiti CLI install in CI workflow,ziti-certssecret — all fully cleaned up with no stale references. - Added:
terraform_remote_state.zitito pass the enrollment JWT,kubernetes_secret_v1.ziti_management_enrollmentin the platform namespace, and updated Helm values withconfigMounts(JWT),persistence(PVC for self-enrolled certs),updateStrategy = Recreate, andZITI_ENROLLMENT_JWT_FILEenv var. - Updated: Cert paths correctly moved from
/etc/ziti/→/var/lib/ziti/(PVC-backed, writable by the app). - Chart version bumped to
0.2.0.
All #152 requirements remain fully addressed. One cosmetic nit on a trailing blank line, not blocking. LGTM.
| run_stack "platform" | ||
| step_end "stack:platform" | ||
|
|
||
|
|
There was a problem hiding this comment.
[nit] Extra blank line — the rest of the script uses a single blank line between sections.
* feat(platform): add ziti management * fix(platform): update ziti db pvc size * fix(platform): pass ziti env vars * feat(bootstrap): enroll ziti management * fix: move ziti-management enrollment after platform stack The enrollment step was creating the platform namespace before Terraform, causing "namespaces platform already exists" error. Move the step to after stack:platform so the namespace already exists, and remove the redundant kubectl create namespace call. * fix: add numeric runAsUser/runAsGroup for ziti-management The container image uses a named user (app) which causes CreateContainerConfigError with runAsNonRoot. Set explicit runAsUser=100 and runAsGroup=101 matching other services. * fix: add CoreDNS rewrite for ziti-mgmt in-cluster resolution ziti-management pod needs to reach the Ziti controller management API at ziti-mgmt.<domain>. Without a CoreDNS rewrite, this resolves to 127.0.0.1 (loopback) inside the cluster, causing immediate connection refused and CrashLoopBackOff. Add rewrite rule mapping ziti-mgmt.<domain> to the controller management service in the ziti namespace. * fix: add missing ZITI_* env vars to ziti-management Helm values Helm env array override replaces defaults entirely. The chart defaults include ZITI_CERT_FILE, ZITI_KEY_FILE, and ZITI_CA_FILE which are required for config.FromEnv(). Without them the container exits immediately with "ZITI_CERT_FILE must be set". * refactor: remove redundant ZITI_MANAGEMENT_GRPC_TARGET from gateway env The gateway code defaults to "ziti-management:50051" which matches what we were setting explicitly. No need to pass it. * feat(ziti): switch to self-enrollment, remove ziti CLI dependency - Update ziti-management chart to v0.2.0 (adds startup self-enrollment) - Add PVC persistence, enrollment JWT secret mount, Recreate strategy - Change cert paths from /etc/ziti/ to /var/lib/ziti/ (writable PVC) - Add ZITI_ENROLLMENT_JWT_FILE env var - Create enrollment JWT secret in platform namespace via ziti remote state - Remove enroll-ziti-management step from apply.sh - Remove 'Install OpenZiti CLI' step from CI workflow --------- Co-authored-by: Rowan Stein <rowan.stein@agyn.io>
Summary
Testing
Closes #152