Repository navigation
Conversation
|
/ok to test c5afa06 |
c5afa06 to
cb8ae8b
Compare
|
Label |
|
/ok to test cb8ae8b |
cb8ae8b to
5efe3e8
Compare
|
@krishicks I pushed a new commit solving a conflict with |
|
/ok to test 5efe3e8 |
46d70d8 to
cfae1b6
Compare
|
/ok to test cfae1b6 |
|
I added this to to 0.1.1 milestone as we're freezing what goes into 0.1.0. For this to actually land in 0.1.1 it would need to be implemented in a backwards-compatible way. Failing that this would need to be pushed to 0.2.0 which is the next release where breaking changes can get in. |
cfae1b6 to
df2e85c
Compare
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
Signed-off-by: Gaizka Menendez Hernandez <gmenende@redhat.com>
4e1bcdb to
4329258
Compare
|
Rebased onto latest main and conflicts are resolved. @krishicks could I have the test suite re-executed with |
|
I did some analysis as the goal is to have this rolled in a backwards compatible manner. Posting my analysis: Compat check against 0.1.x: legacy server.oidc.* without role names loses RBAC on upgrade: I rendered the chart at the merge-base (b8ffe52) and at this PR's head (de1451b) with the same legacy ci/values-*.yaml overlays, then compared the parsed Regression: a values file that sets server.oidc.issuer and audience but no role names (for example The old template emitted roles_claim, admin_role and user_role only when non-empty (gateway-config.yaml L163-171), so the gateway fell back to realm_access.roles / openshell-admin / openshell-user. With explicit empty strings: Config-file values replace defaulted CLI args ( Repro: Suggested fix: in the legacy OIDC translation, add roles_claim, admin_role and user_role only when the legacy value is non-empty, as the old template did and as this PR already does for the other optional fields. Please also add a Helm test: legacy server.oidc.issuer only → gateway.toml has no admin_role, user_role or roles_claim. Another test could pin that an explicit schema-v2 admin_role = "" is still honoured. |
| {{- $_ := set $config "openshell.drivers.kubernetes.managed_ssh_ingress" (dict "enabled" .Values.networkPolicy.enabled "gateway_namespace" .Release.Namespace "gateway_pod_selector" (dict "app.kubernetes.io/name" (include "openshell.name" .) "app.kubernetes.io/instance" .Release.Name)) -}} | ||
| {{- end -}} | ||
| {{- $legacyOidc := get $legacyServer "oidc" | default dict -}} | ||
| {{- if and (get $legacyOidc "issuer") (not (hasKey $config "openshell.gateway.oidc")) -}} |
There was a problem hiding this comment.
Please fill missing fields from legacy values instead of replacing whole tables. Migrating only OIDC issuer/audience drops existing scope and role settings; partial OTLP/OCSF migration drops the required endpoint/path. The Vault compatibility path also overwrites explicit new settings. This breaks the documented per-field migration behavior and can weaken access checks or prevent startup.
| openshell.drivers.kubernetes: | ||
| image_pull_secrets: | ||
| - e2e-regcred | ||
| workspace_mode: managed |
There was a problem hiding this comment.
update workspaceSecretSourceNames to use the effective Kubernetes configuration. This overlay moves managed mode and image-pull Secrets into gatewayConfig, but the helper still reads legacy defaults and omits the source-Secret Role/Binding. The gateway then lacks permission to read its TLS or registry Secrets, causing sandbox creation to fail with Forbidden. Operator mode has the same TLS issue.
| {{- if hasKey $credentialSecretsRbac "create" -}} | ||
| {{- $createRbac = get $credentialSecretsRbac "create" -}} | ||
| {{- end -}} | ||
| {{- if and (eq (include "openshell.credentialDriverEnabled" (list . "kubernetes-secrets")) "true") $createRbac }} |
There was a problem hiding this comment.
Please use credentialDriverEnabled in credential-secrets-namespace.yaml too. Selecting kubernetes-secrets through gatewayConfig creates its Role and Binding, but namespace creation still requires the old enabled flag. A fresh installation with createNamespace: true therefore renders RBAC in a namespace the chart does not create.
| {{- $legacyServer := .Values.server | default dict -}} | ||
| {{- $gateway := get $config "openshell.gateway" | default dict -}} | ||
| {{- if not (hasKey $gateway "name") -}}{{- $_ := set $gateway "name" (get $legacyServer "name" | default (include "openshell.fullname" .)) -}}{{- end -}} | ||
| {{- if not (hasKey $gateway "bind_address") -}}{{- $_ := set $gateway "bind_address" (printf "0.0.0.0:%v" .Values.service.port) -}}{{- end -}} |
There was a problem hiding this comment.
derive listener ports from the chart-owned Service settings, or reject conflicting values. Setting runtime ports to 19080/19081/19090 leaves the Service and probes targeting 8080/8081/9090. The TOML is valid, but the gateway becomes unreachable and fails health checks.
| {{- if not (hasKey $gateway $runtimeKey) -}}{{- $_ := set $gateway $runtimeKey (get $legacyServer $legacyKey) -}}{{- end -}} | ||
| {{- end -}} | ||
| {{- if not (hasKey $gateway "compute_driver") -}}{{- $_ := set $gateway "compute_driver" "kubernetes" -}}{{- end -}} | ||
| {{- if and .Values.certManager.enabled .Values.certManager.serverDnsNames (not (hasKey $gateway "server_sans")) -}} |
There was a problem hiding this comment.
preserve the pkiInitJob.serverDnsNames fallback when deriving server_sans. An existing installation using *.apps.example.com still generates that certificate, but its runtime SANs now disappear because this branch only handles cert-manager. Existing sandbox service URLs then lose their wildcard routing domain.
| {{- end -}} | ||
| {{- if and (get $legacyServer "enableUserNamespaces") (not (hasKey $kubernetes "enable_user_namespaces")) -}}{{- $_ := set $kubernetes "enable_user_namespaces" true -}}{{- end -}} | ||
| {{- if and (get $legacyServer "hostGatewayIP") (not (hasKey $kubernetes "host_gateway_ip")) -}}{{- $_ := set $kubernetes "host_gateway_ip" (get $legacyServer "hostGatewayIP") -}}{{- end -}} | ||
| {{- if not (hasKey $kubernetes "namespace") -}}{{- $_ := set $kubernetes "namespace" (include "openshell.sandboxNamespace" .) -}}{{- end -}} |
There was a problem hiding this comment.
keep the runtime namespace and service_account_name aligned with chart-created resources. In shared mode, gatewayConfig can select a different namespace/account while the ServiceAccount, Role, RoleBinding and NetworkPolicy still use the old chart inputs. Sandboxes then target accounts and permissions the chart did not create.
| supervisor.image.tag: '{{.IMAGE_TAG_openshell_supervisor}}' | ||
| sandboxRuntime.image.repository: '{{.IMAGE_REPO_openshell_sandbox}}' | ||
| sandboxRuntime.image.tag: '{{.IMAGE_TAG_openshell_sandbox}}' | ||
| gatewayConfig.openshell\\.drivers\\.kubernetes.supervisor_image: '{{.IMAGE_REPO_openshell_supervisor}}:{{.IMAGE_TAG_openshell_supervisor}}' |
There was a problem hiding this comment.
remove this redundant override, or use one literal backslash before each dot. This plain YAML key contains two backslashes, which Skaffold forwards to Helm. Helm creates an unexpected TOML root instead of updating the Kubernetes driver table, and the gateway rejects the configuration.
| | `OPENSHELL_OIDC_SCOPES_CLAIM` | Dot-separated claim path containing scopes. Empty disables scope enforcement. | Empty | | ||
|
|
||
| For Helm deployments, set the same values under `server.oidc`: | ||
| For Helm deployments, set the same values in `gatewayConfig` under the |
There was a problem hiding this comment.
change the following instructions to use jwks_allowed_origins and dangerously_allow_insecure_http, and update the matching access-control page. The example now uses gatewayConfig, where field names pass directly to the Rust parser. Following the existing camelCase instructions causes an unknown-field error and prevents startup.
| # with test-only helpers enabled. | ||
| "cargo test --workspace --exclude openshell-server", | ||
| "cargo test -p openshell-server --features test-support", | ||
| "cargo nextest run --config-file .config/nextest.toml --manifest-path examples/supervisor-middleware-content-guard/Cargo.toml", |
There was a problem hiding this comment.
Please restore this example test command, or move its removal into a separate PR with an explanation. The content-guard example has its own Cargo workspace, so the remaining commands do not run its 16 tests. This removes coverage from local test:rust/ci and is unrelated to gatewayConfig. Branch CI still runs the suite separately.
|
Optional but, IMO we should split this PR |
Summary
Migrate the Helm chart from field-by-field
gateway.tomlconstruction to the schema-v2gatewayConfigboundary while preserving the non-secret ConfigMap boundary and Helm-owned Secret, volume, and resource wiring.Related Issue
Closes #3060.
Compatibility
This implementation is backwards-compatible for 0.1.x:
sandboxRuntime,supervisor,upstreamProxy, and Kubernetes driver aliases for existing values files.gatewayConfigis authoritative whenever both the schema-v2 field and its legacy alias are supplied.gatewayConfig.Changes
Testing
mise run cimise run helm:test(170 gateway-chart tests and 5 workspace-chart tests)mise run helm:lintacross chart overlaysChecklist
Signed-off-bytrailers.