feat: add Kubernetes operator and instance settings YAML editor#7836
feat: add Kubernetes operator and instance settings YAML editor#7836rubenfiszel merged 22 commits intomainfrom
Conversation
Introduces a new `windmill-operator` crate that enables declarative management of Windmill instance configuration via a Kubernetes `WindmillInstance` CRD. The operator watches CRD resources and performs full declarative sync of global_settings and worker configs to the database, supporting GitOps workflows for instance-level configuration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @rubenfiszel's task in 2m 56s —— View job Review: Add Kubernetes operator for declarative instance configOverall: This is a well-structured first implementation of a Kubernetes operator for Windmill. The crate separation is clean, the CRD design is straightforward, and the reconciler follows standard kube-rs patterns. I have a few concerns around atomicity and operational edge cases. Issues Found1. Critical: Missing tracing initialization for operator subcommand ( The operator CLI path returns early before the tracing subscriber is initialized at line 758. All the 2. Important: No transaction wrapping for sync operations ( Both 3. Design: Multiple CRD instances can conflict ( The CRD is 4. Design: Protected settings list ( The hardcoded Positive Observations
Minor Notes
Testing InstructionsThis feature adds a Kubernetes operator mode to the Windmill binary. To test:
Note: This feature is not accessible through the Windmill web UI -- it is a separate CLI mode ( |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| - wmi | ||
| scope: Namespaced | ||
| versions: | ||
| - name: v1alpha1 |
There was a problem hiding this comment.
Ensure the CRD YAML manifest remains in sync with the CustomResource definition in code.
| tracing::info!("Synced global setting: {key}"); | ||
| } | ||
|
|
||
| // Delete settings not in desired (except protected keys) |
There was a problem hiding this comment.
Consider batching deletion queries for global settings and worker configs to improve performance with many keys.
| let generation = instance.metadata.generation.unwrap_or(0); | ||
|
|
||
| // Sync global settings | ||
| if let Err(e) = db_sync::sync_global_settings(&ctx.db, &instance.spec.global_settings).await { |
There was a problem hiding this comment.
Fixed requeue delays on errors; consider implementing exponential backoff to avoid constant rapid retries on persistent failures.
backend/Cargo.lock
Outdated
| "futures", | ||
| "k8s-openapi", | ||
| "kube", | ||
| "schemars 0.8.22", |
There was a problem hiding this comment.
Typo check: The dependency string "schemars 0.8.22" looks unusual as it combines the package name with a version number. Please verify if it is correctly formatted or if it should be just "schemars".
backend/src/main.rs
Outdated
| if sub_arg == "crd" { | ||
| use kube::CustomResourceExt; | ||
| let crd = windmill_operator::crd::WindmillInstance::crd(); | ||
| println!("{}", serde_yml::to_string(&crd).unwrap()); |
There was a problem hiding this comment.
Potential typo: You are using serde_yml::to_string here, which might be a typo if the intended crate is serde_yaml. Please verify if this is correct.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Fetch current settings from DB | ||
| let current_rows: Vec<(String,)> = sqlx::query_as("SELECT name FROM global_settings") | ||
| .fetch_all(db) | ||
| .await?; | ||
| let current_keys: HashSet<String> = current_rows.into_iter().map(|(name,)| name).collect(); | ||
|
|
||
| // Upsert desired settings | ||
| for (key, value) in desired { | ||
| sqlx::query( | ||
| "INSERT INTO global_settings (name, value) VALUES ($1, $2) \ | ||
| ON CONFLICT (name) DO UPDATE SET value = EXCLUDED.value, updated_at = now()", | ||
| ) | ||
| .bind(key) | ||
| .bind(value) | ||
| .execute(db) | ||
| .await?; | ||
| tracing::info!("Synced global setting: {key}"); | ||
| } | ||
|
|
||
| // Delete settings not in desired (except protected keys) | ||
| let desired_keys: HashSet<&str> = desired.keys().map(|s| s.as_str()).collect(); | ||
| for current_key in ¤t_keys { | ||
| if !desired_keys.contains(current_key.as_str()) | ||
| && !PROTECTED_SETTINGS.contains(¤t_key.as_str()) | ||
| { | ||
| sqlx::query("DELETE FROM global_settings WHERE name = $1") | ||
| .bind(current_key) | ||
| .execute(db) | ||
| .await?; | ||
| tracing::info!("Deleted global setting not in CRD: {current_key}"); | ||
| } | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
Bug risk: No transactional guarantee for the sync operation.
The upsert and delete operations are executed as individual queries without a transaction. If the operator crashes or the DB connection drops mid-sync, the state can be left partially applied -- e.g., some settings upserted but stale ones not yet deleted, or vice versa.
Consider wrapping the entire sync in a transaction:
| // Fetch current settings from DB | |
| let current_rows: Vec<(String,)> = sqlx::query_as("SELECT name FROM global_settings") | |
| .fetch_all(db) | |
| .await?; | |
| let current_keys: HashSet<String> = current_rows.into_iter().map(|(name,)| name).collect(); | |
| // Upsert desired settings | |
| for (key, value) in desired { | |
| sqlx::query( | |
| "INSERT INTO global_settings (name, value) VALUES ($1, $2) \ | |
| ON CONFLICT (name) DO UPDATE SET value = EXCLUDED.value, updated_at = now()", | |
| ) | |
| .bind(key) | |
| .bind(value) | |
| .execute(db) | |
| .await?; | |
| tracing::info!("Synced global setting: {key}"); | |
| } | |
| // Delete settings not in desired (except protected keys) | |
| let desired_keys: HashSet<&str> = desired.keys().map(|s| s.as_str()).collect(); | |
| for current_key in ¤t_keys { | |
| if !desired_keys.contains(current_key.as_str()) | |
| && !PROTECTED_SETTINGS.contains(¤t_key.as_str()) | |
| { | |
| sqlx::query("DELETE FROM global_settings WHERE name = $1") | |
| .bind(current_key) | |
| .execute(db) | |
| .await?; | |
| tracing::info!("Deleted global setting not in CRD: {current_key}"); | |
| } | |
| } | |
| Ok(()) | |
| let mut tx = db.begin().await?; | |
| // Fetch current settings from DB | |
| let current_rows: Vec<(String,)> = sqlx::query_as("SELECT name FROM global_settings") | |
| .fetch_all(&mut *tx) | |
| .await?; | |
| let current_keys: HashSet<String> = current_rows.into_iter().map(|(name,)| name).collect(); | |
| // Upsert desired settings | |
| for (key, value) in desired { | |
| sqlx::query( | |
| "INSERT INTO global_settings (name, value) VALUES ($1, $2) \ | |
| ON CONFLICT (name) DO UPDATE SET value = EXCLUDED.value, updated_at = now()", | |
| ) | |
| .bind(key) | |
| .bind(value) | |
| .execute(&mut *tx) | |
| .await?; | |
| tracing::info!("Synced global setting: {key}"); | |
| } | |
| // Delete settings not in desired (except protected keys) | |
| let desired_keys: HashSet<&str> = desired.keys().map(|s| s.as_str()).collect(); | |
| for current_key in ¤t_keys { | |
| if !desired_keys.contains(current_key.as_str()) | |
| && !PROTECTED_SETTINGS.contains(¤t_key.as_str()) | |
| { | |
| sqlx::query("DELETE FROM global_settings WHERE name = $1") | |
| .bind(current_key) | |
| .execute(&mut *tx) | |
| .await?; | |
| tracing::info!("Deleted global setting not in CRD: {current_key}"); | |
| } | |
| } | |
| tx.commit().await?; | |
| Ok(()) |
Same concern applies to sync_worker_configs below.
| const PROTECTED_SETTINGS: &[&str] = &[ | ||
| "ducklake_user_pg_pwd", | ||
| "ducklake_settings", | ||
| "custom_instance_pg_databases", | ||
| ]; |
There was a problem hiding this comment.
Consider expanding the protected list or making it configurable.
A few questions:
- Should
license_keybe protected? Accidentally deleting it via CRD could break the EE instance. - The list is hardcoded -- if new internal settings are added later, this list could become stale silently. Consider adding a comment documenting when/how this list should be updated, or alternatively using a naming convention (e.g., prefix-based) to auto-detect internal settings.
| async fn reconcile( | ||
| instance: Arc<WindmillInstance>, | ||
| ctx: Arc<Context>, | ||
| ) -> Result<Action, kube::Error> { |
There was a problem hiding this comment.
The return type Result<Action, kube::Error> means DB errors from db_sync are caught but converted to status updates. This is a reasonable pattern, but note that kube::Error is returned for status update failures -- meaning if the DB sync fails AND the status update also fails, the original DB error is swallowed and only the kube error propagates. Consider logging the original error more prominently or using a custom error type that wraps both.
| // Sync global settings | ||
| if let Err(e) = db_sync::sync_global_settings(&ctx.db, &instance.spec.global_settings).await { | ||
| tracing::error!("Failed to sync global settings for {name}: {e:#}"); | ||
| update_status( | ||
| &ctx.client, | ||
| &instance, | ||
| false, | ||
| format!("Error syncing global settings: {e}"), | ||
| generation, | ||
| ) | ||
| .await?; | ||
| // Requeue after 30s on error | ||
| return Ok(Action::requeue(Duration::from_secs(30))); | ||
| } |
There was a problem hiding this comment.
Partial sync issue: If sync_global_settings succeeds but sync_worker_configs fails, the global settings are persisted but the status is marked as failed. On the next reconciliation, sync_global_settings will re-run (no-op in practice since it's idempotent), which is fine. But the status message only mentions "worker configs" -- the user might not know global settings were actually applied. Consider noting which phase succeeded in the status message.
Also, since global_settings writes are not transactional (see db_sync comment), a partial global_settings write followed by a worker_configs failure could leave the system in an inconsistent state.
| // Verify the CRD is installed by attempting to list | ||
| let api: Api<WindmillInstance> = Api::all(client.clone()); | ||
| api.list(&Default::default()).await.map_err(|e| { | ||
| anyhow::anyhow!("Failed to list WindmillInstance CRDs. Is the CRD installed? Error: {e}") | ||
| })?; |
There was a problem hiding this comment.
Minor: CRD verification lists all resources across all namespaces. For large clusters with many WindmillInstance resources (unlikely but possible), this initial list call fetches all of them. This is standard for kube-rs controllers, but worth noting. An alternative would be a lighter check like a dry-run or API discovery call, though this is fine for v1alpha1.
| .await?; | ||
|
|
||
| // Periodic re-sync every 5 minutes for drift detection | ||
| Ok(Action::requeue(Duration::from_secs(300))) |
There was a problem hiding this comment.
5-minute re-sync interval: This is reasonable for drift detection, but worth documenting that this means the operator may take up to 5 minutes to detect out-of-band changes to global_settings or config tables. If someone modifies settings via the UI, the operator will overwrite them within 5 minutes. This is the expected behavior for a declarative operator, but it should be clearly documented for users.
| #[derive(CustomResource, Deserialize, Serialize, Clone, Debug, JsonSchema)] | ||
| #[kube( | ||
| group = "windmill.dev", | ||
| version = "v1alpha1", | ||
| kind = "WindmillInstance", | ||
| namespaced, | ||
| shortname = "wmi", | ||
| status = "WindmillInstanceStatus", | ||
| printcolumn = r#"{"name":"Synced","type":"string","jsonPath":".status.synced"}"#, | ||
| printcolumn = r#"{"name":"Last Synced","type":"date","jsonPath":".status.lastSyncedAt"}"#, | ||
| printcolumn = r#"{"name":"Age","type":"date","jsonPath":".metadata.creationTimestamp"}"# | ||
| )] |
There was a problem hiding this comment.
Nit: Consider adding doc attribute to the CRD derive.
The CRD is namespaced which means each namespace can have its own WindmillInstance. Since this operator connects to a single database, having multiple WindmillInstance resources across namespaces (or even within the same namespace) could cause conflicts -- they'd all try to sync to the same global_settings table and overwrite each other.
Consider either:
- Making the CRD cluster-scoped (remove
namespaced) since instance config is inherently global, or - Adding validation/documentation that only one
WindmillInstanceshould exist per database, or - Adding a label selector or name filter in the controller to only watch a specific instance
Deploying windmill with
|
| Latest commit: |
ce64a4a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://727c7d37.windmill.pages.dev |
| Branch Preview URL: | https://rf-iac.windmill.pages.dev |
| #[cfg(feature = "operator")] | ||
| "operator" => { | ||
| let sub_arg = std::env::args().nth(2).unwrap_or_default(); | ||
| if sub_arg == "crd" { | ||
| use kube::CustomResourceExt; | ||
| let crd = windmill_operator::crd::WindmillInstance::crd(); | ||
| println!("{}", serde_yml::to_string(&crd).unwrap()); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| println!("Starting Windmill Kubernetes operator..."); | ||
| println!("Connecting to database..."); | ||
| let db = windmill_common::initial_connection().await?; | ||
| println!("Database connected. Starting controller..."); | ||
| windmill_operator::run(db).await?; | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
Tracing setup is skipped for the operator subcommand. The operator returns early at line 644, before the tracing subscriber is initialized (line 758). This means the operator will only get println! output but none of the structured tracing::info!/tracing::error! calls in reconciler.rs and db_sync.rs will be visible.
Consider initializing tracing before the operator starts, similar to how it's done for the main flow. At minimum, a basic subscriber should be set up.
- 9 unit tests for CRD serialization, deserialization, metadata, and status field behavior - 15 integration tests for db_sync using #[sqlx::test] with full declarative sync coverage: upsert, delete, protected keys, idempotency, worker config prefix handling, and end-to-end sync Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| .expect("sync should succeed"); | ||
|
|
||
| for key in &protected_keys { | ||
| assert!( |
There was a problem hiding this comment.
The assert message using "Protected key {key} should not be deleted" won’t interpolate the variable. Consider using a proper format string, e.g. "Protected key {} should not be deleted", key.
| #[test] | ||
| fn crd_generation_produces_valid_yaml() { | ||
| let crd = WindmillInstance::crd(); | ||
| let yaml = serde_yml::to_string(&crd).expect("CRD should serialize to YAML"); |
There was a problem hiding this comment.
Using 'serde_yml' here is unusual; typically the crate is 'serde_yaml'. Verify if this naming is intentional.
backend/Cargo.lock
Outdated
| "schemars 0.8.22", | ||
| "serde", | ||
| "serde_json", | ||
| "serde_yml", |
There was a problem hiding this comment.
Typo alert: The crate name is specified as "serde_yml"; most likely you intended to use "serde_yaml". Please confirm and correct if necessary.
| schemars = "0.8" | ||
|
|
||
| [dev-dependencies] | ||
| serde_yml.workspace = true |
There was a problem hiding this comment.
Typo: It appears that the dependency name 'serde_yml' may be misspelled. Did you mean 'serde_yaml.workspace = true'? Please confirm the correct package name.
…idation GlobalSettings, SmtpSettings, IndexerSettings, and WorkerGroupConfig now have explicit typed fields with serde(flatten) catch-all for forward compatibility. The generated CRD YAML includes a full OpenAPI v3 schema that Kubernetes validates on kubectl apply. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| name: v1alpha1 | ||
| schema: | ||
| openAPIV3Schema: | ||
| description: Auto-generated derived type for WindmillInstanceSpec via `CustomResource` |
There was a problem hiding this comment.
Ensure the CRD description and properties remain in sync with the Rust docs for WindmillInstanceSpec.
backend/windmill-operator/src/crd.rs
Outdated
| pub extra: BTreeMap<String, serde_json::Value>, | ||
| } | ||
|
|
||
| impl GlobalSettings { |
There was a problem hiding this comment.
Review to_settings_map: flattened extra fields may override explicit fields if keys conflict.
| default: {} | ||
| description: Global settings to sync to the `global_settings` table. | ||
| properties: | ||
| app_workspaced_route: |
There was a problem hiding this comment.
Typo: The property name 'app_workspaced_route' may be misspelled. Did you mean 'app_workspace_route'?
| base_url: | ||
| nullable: true | ||
| type: string | ||
| bunfig_install_scopes: |
There was a problem hiding this comment.
Typo: The property name 'bunfig_install_scopes' looks like it might be a misspelling. Consider reviewing its intended spelling.
backend/windmill-operator/src/crd.rs
Outdated
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub monitor_logs_on_s3: Option<bool>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub app_workspaced_route: Option<bool>, |
There was a problem hiding this comment.
Typo in field name: "app_workspaced_route". Consider renaming it to "app_workspace_route" for clarity.
backend/windmill-operator/src/crd.rs
Outdated
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub npm_config_registry: Option<String>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub bunfig_install_scopes: Option<String>, |
There was a problem hiding this comment.
Possible typo in field name: "bunfig_install_scopes". Verify if this should be "bunfig_install_scopes" or something like "bun_config_install_scopes".
Replace most remaining serde_json::Value fields in WindmillInstance CRD with properly typed structs derived from the codebase: - oauths: BTreeMap<String, OAuthClient> - otel: OtelSettings - otel_tracing_proxy: OtelTracingProxySettings with ScriptLang enum - critical_error_channels: Vec<CriticalErrorChannel> (untagged enum) - critical_alerts_on_db_oversize: DbOversizeAlert - ducklake_settings: DucklakeSettings with nested catalog/storage types - custom_instance_pg_databases: CustomInstancePgDatabases - autoscaling (worker config): AutoscalingConfig with integration struct - custom_tags, default_tags_workspaces: Vec<String> - default_tags_per_workspace: bool Still opaque (serde_json::Value): object_store_cache_config (kube-core can't generate schemas for internally-tagged enums), secret_backend (EE-private), slack, teams (no clear struct definitions). Regenerated CRD YAML with full OpenAPI schema (352→703 lines). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| minimum: 0.0 | ||
| nullable: true | ||
| type: integer | ||
| integration: |
There was a problem hiding this comment.
Consider enforcing allowed values for the autoscaling integration's type field (e.g. 'script', 'dryrun', 'kubernetes') by using an enum or validation in the schema.
backend/windmill-operator/src/crd.rs
Outdated
| /// The `type` field selects the backend: `"script"`, `"dryrun"`, or `"kubernetes"`. | ||
| /// For `"script"`, `path` is required and `tag` is optional. | ||
| #[derive(Deserialize, Serialize, Clone, Debug, Default, JsonSchema)] | ||
| pub struct AutoscalingIntegration { |
There was a problem hiding this comment.
For AutoscalingIntegration, consider replacing the raw string field for integration_type with an enum to enforce valid backend types and reduce typos.
backend/windmill-operator/src/crd.rs
Outdated
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub teams: Option<serde_json::Value>, | ||
|
|
||
| /// Catch-all for settings not yet covered by typed fields. |
There was a problem hiding this comment.
The use of a flattened extra field in GlobalSettings is useful for forward compatibility; however, consider namespacing these keys to avoid collisions with future typed fields.
backend/windmill-operator/src/crd.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| fn autoscaling_script_integration_roundtrips() { |
There was a problem hiding this comment.
Excellent round-trip tests for autoscaling and ducklake settings. Consider adding tests for error cases (e.g. missing required fields in AutoscalingIntegration) to further ensure robustness.
backend/windmill-operator/src/crd.rs
Outdated
| Deno, | ||
| Go, | ||
| Bash, | ||
| Powershell, |
There was a problem hiding this comment.
Typo: In the 'ScriptLang' enum, consider using 'PowerShell' (with an uppercase S) instead of 'Powershell' for consistency with common naming conventions.
backend/windmill-operator/src/crd.rs
Outdated
| Mysql, | ||
| Bigquery, | ||
| Snowflake, | ||
| Graphql, |
There was a problem hiding this comment.
Typo: In the 'ScriptLang' enum, 'Graphql' may be intended as 'GraphQL' (with 'QL' capitalized). Please verify.
backend/windmill-operator/src/crd.rs
Outdated
| Bigquery, | ||
| Snowflake, | ||
| Graphql, | ||
| Nativets, |
There was a problem hiding this comment.
Typo: The enum variant 'Nativets' looks unusual. If this is meant to represent native TypeScript (or similar), please double-check the spelling.
backend/windmill-operator/src/crd.rs
Outdated
| // DuckLake | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| /// DuckLake catalog database settings. |
There was a problem hiding this comment.
Inconsistency: The comment refers to 'DuckLake' (with an uppercase L) while the struct names use 'Ducklake'. Consider standardizing the capitalization for clarity.
Move all typed settings (GlobalSettings, WorkerGroupConfig, etc.) from windmill-operator/crd.rs into windmill-common/instance_config.rs so both the API server and operator share a single source of truth. Add diff/apply logic (Merge mode for UI, Replace mode for operator) and InstanceConfig::from_db(). Add GET/PUT /settings/instance_config endpoints so the frontend loads all settings in 1 call instead of 42, and saves with a single bulk PUT. The backend handles the diff internally, running pre-write hooks for changed keys. Refactor windmill-operator/db_sync.rs to use the shared diff+apply functions and slim crd.rs down to the CRD wrapper with re-exports. Includes 32 unit tests and 30 integration tests covering serialization, diff logic, DB roundtrips, protected settings, and edge cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| } | ||
|
|
||
| let nvalues = JSON.parse(JSON.stringify(initialValues)) | ||
| if (nvalues['base_url'] == undefined) { |
There was a problem hiding this comment.
Duplicate base_url fallback: There are two consecutive "if (nvalues['base_url'] == undefined)" blocks (lines 73 and 79). Consider consolidating into one, or clarifying the intended fallback order.
| APP_WORKSPACED_ROUTE_SETTING => { | ||
| let serde_json::Value::Bool(workspaced_route) = value else { | ||
| return Err(error::Error::BadRequest(format!( | ||
| "{} setting Expected to be boolean", |
There was a problem hiding this comment.
Typo: In the error message, the word 'Expected' appears capitalized. Consider rephrasing to "{} setting expected to be boolean" for consistency.
|
|
||
| if !duplicate_app.is_empty() { | ||
| tracing::error!( | ||
| "Cannot disable {} setting as duplicate app with custom path were found: {:?}", |
There was a problem hiding this comment.
Typo: The error message reads "duplicate app with custom path were found". Consider rephrasing to "duplicate apps with custom paths were found" to fix the subject-verb agreement.
| pub critical_alert_mute_ui: Option<bool>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub monitor_logs_on_s3: Option<bool>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] |
There was a problem hiding this comment.
There's a possible typo with the field name app_workspaced_route. It may have been intended to be app_workspace_route.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| if (oauths && Object.keys(oauths).length > 0) { | ||
| obj['oauths'] = oauths | ||
| } | ||
| if (requirePreexistingUserForOauth) { |
There was a problem hiding this comment.
Consider always including the 'require_preexisting_user_for_oauth' key in the YAML output even when its value is false. Omitting it may be confusing when a user expects to see an explicit false flag.
| } | ||
| $values = parsed | ||
| yamlError = '' | ||
| return true |
There was a problem hiding this comment.
In the catch block of syncYamlToForm, consider not exposing raw exception details directly to the user. This may leak internal error information. A more generic error message or sanitizing the exception details might be preferable.
|
|
||
| {#if viewMode === 'yaml' && !hideTabs} | ||
| <div class="border rounded w-full" style="min-height: 600px;"> | ||
| {#await import('$lib/components/SimpleEditor.svelte')} |
There was a problem hiding this comment.
The dynamic import of SimpleEditor lacks a catch block. Consider adding a {:catch} handler to gracefully handle module load failures for better user feedback.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| </script> | ||
|
|
||
| <div class="pb-12"> | ||
| <!-- svelte-ignore a11y_label_has_associated_control --> |
There was a problem hiding this comment.
Removal of the {#if !hideTabs} check now always renders the toggle button group. Confirm that this change is intentional, as the hideTabs prop is no longer respected here.
| </ToggleButtonGroup> | ||
| </div> | ||
|
|
||
| {#if viewMode === 'yaml'} |
There was a problem hiding this comment.
The conditional for rendering the YAML editor is changed from {#if viewMode === 'yaml' && !hideTabs} to {#if viewMode === 'yaml'}. Verify that this behavior is desired, especially when hideTabs is true.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| @@ -15,7 +15,10 @@ | |||
| import AuthSettings from './AuthSettings.svelte' | |||
| import InstanceSetting from './InstanceSetting.svelte' | |||
| import { writable, type Writable } from 'svelte/store' | |||
There was a problem hiding this comment.
Replace ToggleButtonGroup/ToggleButton imports with a single Toggle import. Ensure the new Toggle supports all needed features.
| @@ -111,6 +108,12 @@ | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Update condition in saveSettings to use 'yamlMode' boolean instead of string 'viewMode'.
| @@ -249,11 +249,98 @@ | |||
| function openSmtpSettings() { | |||
| tab = 'SMTP' | |||
| } | |||
There was a problem hiding this comment.
Switch state variable from viewMode ('form'|'yaml') to a boolean yamlMode; verify all references update accordingly.
| sendUserToast('Invalid YAML: ' + e, true) | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactored handleYamlToggle to use a boolean 'checked' parameter; ensure sync logic (form ↔ YAML) works as expected.
| </script> | ||
|
|
||
| <div class="pb-12"> | ||
| <!-- svelte-ignore a11y_label_has_associated_control --> |
There was a problem hiding this comment.
Updated UI to use the Toggle component with an on:change event; confirm that the event emits a boolean (via e.detail) as expected.
| /> | ||
| </div> | ||
|
|
||
| {#if yamlMode} |
There was a problem hiding this comment.
Conditional rendering now checks yamlMode instead of viewMode; verify that the YAML editor displays only in YAML mode.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| let yamlCode = $state('') | ||
| let yamlEditor: SimpleEditor | undefined = $state(undefined) | ||
| let yamlError = $state('') | ||
| let showSensitive = $state(false) |
There was a problem hiding this comment.
Add a short comment explaining that 'showSensitive' toggles the display of actual sensitive values in the YAML editor.
| let yamlError = $state('') | ||
| let showSensitive = $state(false) | ||
|
|
||
| const SENSITIVE_UNCHANGED = '__SENSITIVE_AND_UNCHANGED__' |
There was a problem hiding this comment.
Document the purpose of the SENSITIVE_UNCHANGED constant and the sensitiveKeys set to clarify why these values are masked.
| .map((s) => s.key) | ||
| ) | ||
|
|
||
| function maskSensitive(obj: Record<string, any>): Record<string, any> { |
There was a problem hiding this comment.
In maskSensitive, note that only one level (with special handling for 'oauths') is masked. If nested sensitive fields are introduced later, consider making the masking recursive.
| } | ||
|
|
||
| function buildYamlObject(): Record<string, any> { | ||
| const obj: Record<string, any> = { ...$values } |
There was a problem hiding this comment.
The buildYamlObject function now returns either the raw object or a masked version based on 'showSensitive'. Ensure that all sensitive fields are included in sensitiveKeys.
| return false | ||
| } | ||
|
|
||
| // Restore sensitive values that were not changed |
There was a problem hiding this comment.
Restoring the 'oauths' provider secrets when the secret field equals the placeholder is good—just verify that this correctly handles cases where a user might intentionally edit these fields.
| } | ||
|
|
||
| // Restore unchanged sensitive settings | ||
| for (const key of sensitiveKeys) { |
There was a problem hiding this comment.
The loop restoring unchanged sensitive settings (using sensitiveKeys) always resets the field to the current $values. Confirm that this merge behavior is intended even if a user might try to clear a field.
| } | ||
| } | ||
|
|
||
| function handleShowSensitiveToggle(checked: boolean) { |
There was a problem hiding this comment.
The handleShowSensitiveToggle function syncs YAML to form before and after toggling. Consider handling potential YAML parsing errors here to avoid unexpected state if the YAML is invalid.
| </div> | ||
|
|
||
| {#if yamlMode} | ||
| <div class="flex items-center justify-end mb-2"> |
There was a problem hiding this comment.
The new UI toggle for 'Show sensitive values' in YAML mode looks well integrated. Ensure the label and accessibility attributes are sufficient for all users.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…exports Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| ]; | ||
|
|
||
| /// Internal settings that are never exposed via the API or included in config exports. | ||
| pub const HIDDEN_SETTINGS: &[&str] = &["uid", "rsa_keys", "jwt_secret", "min_keep_alive_version"]; |
There was a problem hiding this comment.
Added 'jwt_secret' and 'min_keep_alive_version' to HIDDEN_SETTINGS to ensure these sensitive keys are not exposed via API/config exports. Confirm related functions (e.g. from_db) correctly filter them.
|
|
||
| const SENSITIVE_UNCHANGED = '__SENSITIVE_AND_UNCHANGED__' | ||
|
|
||
| const sensitiveKeys: Set<string> = new Set( |
There was a problem hiding this comment.
Removed explicit inclusion of 'jwt_secret' from sensitiveKeys so that the UI masking stays in sync with backend hidden settings. The sensitive keys are now solely derived from settings definitions.
Allow sensitive fields (license_key, hub_api_secret, scim_token, smtp_password, OAuthClient.secret, custom PG user_pwd) to reference Kubernetes Secrets via the standard secretKeyRef pattern instead of inlining values as plaintext YAML. The reconciler resolves all refs by reading K8s Secrets before syncing to the database. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rubenfiszel
changed the title
# Conflicts: # CLAUDE.md # backend/Cargo.toml # backend/src/main.rs # backend/src/monitor.rs # backend/windmill-api-users/src/users.rs # backend/windmill-api-workspaces/src/workspaces.rs # backend/windmill-common/Cargo.toml # frontend/src/lib/components/InstanceSetting.svelte # frontend/src/lib/components/InstanceSettings.svelte # frontend/src/lib/components/SuperadminSettings.svelte # frontend/src/lib/components/SuperadminSettingsInner.svelte
Resolve merge conflicts from origin/main, fix duplicate UV_INDEX_STRATEGY_SETTING import, and add Playwright MCP testing instructions to CLAUDE.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/ai-fast re-review entirely this PR |
|
Claude finished @rubenfiszel's task in 2m 29s —— View job Re-review of PR #7836: Kubernetes operator and instance settings YAML editorI've read all 37 changed files in this PR. Here's a comprehensive review. Architecture AssessmentThe PR is well-structured with a clean separation of concerns:
The design decision to share diff/apply logic between the operator ( Issues1. Tracing not initialized for operator subcommand — The The Consider initializing a basic tracing subscriber at the top of 2. No transaction wrapping for sync operations —
This is particularly relevant for the operator's 3. Duplicated side-effect logic in The Consider refactoring 4. Both structs use 5. When the operator resolves secret refs before calling Currently this can't happen because Frontend Review6. YAML editor The 7. This calls function handleShowSensitiveToggle(checked: boolean) {
if (!syncYamlToForm()) return
showSensitive = checked
syncFormToYaml()
}8. Unsaved changes detection in YAML mode — In YAML mode, dirty state is tracked by comparing 9. Drawer close guard works well — The 10. YAML-excluded keys —
Operator Review11. Reconciler partial sync — If 12. CRD is namespaced but syncs to a global DB — Multiple 13. Secret caching in The cache is per-reconciliation (scoped to the 14. 5-minute drift detection interval —
Test CoverageThe test coverage is thorough:
Minor Notes
SummaryThis is a solid implementation. The shared
|
…fects Initialize tracing subscriber before early-return CLI paths (sync-config, operator) so tracing calls are not silently dropped. Refactor set_global_setting_internal to call run_setting_pre_write_hook instead of duplicating the side-effect logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Dumps the current instance config (global settings + worker configs) as YAML. Supports --output-file to write to a file instead of stdout. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/ai-fast re-review entirely this PR |
|
Claude finished @rubenfiszel's task in 3m 43s —— View job Comprehensive Re-review of PR #7836
Previous Review Issues — Status
ArchitectureThe overall architecture is well-designed:
The Issues Found1. The Consider checking the specific requested key before returning let decoded: BTreeMap<String, String> = data
.into_iter()
.map(|(k, v)| {
String::from_utf8(v.0)
.map(|s| (k.clone(), s))
.map_err(|_| ResolveError::InvalidUtf8 {
secret: secret_ref.name.clone(),
key: k,
})
})
.collect::<Result<_, _>>()?;2. No transaction wrapping in These functions execute individual queries on the pool without a transaction. If the operator or API server crashes mid-sync, the database can be left in a partially-applied state. This is particularly relevant for the operator's 3. function handleShowSensitiveToggle(checked: boolean) {
syncYamlToForm() // returns false on parse error
showSensitive = checked
syncFormToYaml()
}If the YAML is invalid (user mid-edit), function handleShowSensitiveToggle(checked: boolean) {
if (!syncYamlToForm()) return
showSensitive = checked
syncFormToYaml()
}4.
Positive Observations
Summary
Note: PR was merged during the review. These items can be addressed in follow-up PRs. | Branch: |
1 participant
Summary
Adds a Kubernetes operator for declarative instance configuration (IaC) and a YAML editor to the instance settings UI for bulk viewing/editing all settings.
Changes
Backend — Kubernetes Operator (
windmill-operatorcrate)windmill-operatorcrate with CRD (WindmillInstance), reconciler, and db_sync modulesserde_json::Value) for schema validationsecretKeyReffor sensitive fields (pulls values from Kubernetes Secrets)windmill operator(run controller) /windmill operator crd(print CRD YAML)operator(part ofee_coremeta-feature)Backend — Unified Instance Config
windmill-common/src/instance_config.rswith shared types, diff, and apply logicGET/PUT /api/settings/instance_configskip_worker_configsquery parameter on PUT to skip worker config processing from the UIjwt_secretandmin_keep_alive_versionhidden from API responses and config exportsautomate_username_creationdefault changed totrueto match UI defaultrun_setting_pre_write_hook) shared by per-key and bulk endpointsset_global_setting_internalrefactored to callrun_setting_pre_write_hook(deduplicated side-effect logic)Backend — CLI Fixes
sync-config,operator) sotracing::info!/tracing::error!calls are not silently droppedwindmill_operator::print_crd_yaml()helperinitial_connection()import path in operator branchCLI —
wmill instance get-configwmill instance get-configcommand to dump the current instance config as YAML-o, --output-file <file>to write to a file instead of stdoutGET /api/settings/instance_configvia the generated API clientFrontend — Instance Settings YAML Editor
/user/instance_settings?mode=full)__SENSITIVE_AND_UNCHANGED__) for passwords, tokens, secretsFrontend — Instance Settings Page (
/user/instance_settings)yamlModereset when switching from full mode back to wizard (fixes Back button)Frontend — Drawer Layout
Documentation & Dev Environment
CLAUDE.mdwithREMOTEenv var for local dev and Playwright MCP testing instructionsexamples/deploy/instance-config-iac/Test plan
npm run checkpasses (0 errors)cargo check --features operatorpasseswmill instance get-configoutputs valid YAMLGenerated with Claude Code