Conversation
There was a problem hiding this comment.
Pull request overview
Adds a Helm-chart configurable “dynamic Kubernetes query” capability for Chao/Gremlin by introducing a feature flag and an RBAC allowlist that extends the existing gremlin-watcher ClusterRole with additional read-only permissions when enabled.
Changes:
- Add
chao.features.dynamicQueryvalues (flag + default allowlist) to control dynamic query behavior. - Extend the
gremlin-watcherClusterRole rules and Chao container args when the feature is enabled. - Add Helm template validations and helm-unittest coverage; document the feature and defaults in the chart README.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
gremlin/values.yaml |
Introduces chao.features.dynamicQuery and a default RBAC allowlist. |
gremlin/tests/notes_test.yaml |
Adds template validation tests for dynamic query allowlist/verbs constraints. |
gremlin/templates/chao-service-account.yaml |
Conditionally appends allowlist RBAC rules to the gremlin-watcher ClusterRole. |
gremlin/templates/chao-deployment.yaml |
Conditionally enables Chao’s -dynamic_query flag. |
gremlin/templates/_validation.tpl |
Adds validations for allowlist presence and verb restrictions when enabled. |
gremlin/README.md |
Documents dynamic queries, configuration keys, and default allowlist intent. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # ClusterRole bound to Chao's service account is extended with the read-only rules | ||
| # described by `allowlist`, and Chao is told the feature is on. | ||
| dynamicQuery: | ||
| enabled: false |
There was a problem hiding this comment.
should we enable this by default?
There was a problem hiding this comment.
We've decided to have it off by default for now while we let customers decide to onboard themselves, then we can flip it to default true later. Chatted with Phil about it
There was a problem hiding this comment.
Off by default means SAs and AEs can't demo it without a config change, and on large deployments that's a multi-week ask. Customers don't read release notes. Ship it on by default and note the change in the release notes.
@gremsam thoughts?
| # those two; any other verb (including `watch` and the `"*"` wildcard) fails the install. Chao therefore | ||
| # cannot maintain a watch over these resources and must poll them. | ||
| allowlist: | ||
| # How workloads are wired together, what they consume, and what has happened to them. |
There was a problem hiding this comment.
for my own understanding how do we handle it at run time when customers don't grant us these permissions? Does the permission error get sent back to the chat to expose to the customer?
There was a problem hiding this comment.
Yes, we'll give back an access denied. It verifies permissions first before executing a query.
For the upcoming Dynamic K8s Query feature, add an option to enable it, and it's expanded set of read capabilities.
K8s doesn't have a deny list, only adding allows, so we're just adding some more basic constructs to the list instead of a wildcard by default.