Conversation
There was a problem hiding this comment.
5 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/api.md">
<violation number="1" location="docs/api.md:2230">
P3: The new `NamespacedName` field descriptions are misleadingly CRD-specific; they should describe generic Kubernetes object references to match the section definition.</violation>
</file>
<file name="api/operator/v1beta1/vmuser_types.go">
<violation number="1" location="api/operator/v1beta1/vmuser_types.go:163">
P1: `crd.objects` entries are not validated for empty name/namespace, so invalid objects can be accepted and then fail later at reconcile time.</violation>
</file>
<file name="internal/controller/operator/factory/vmauth/vmusers_config.go">
<violation number="1" location="internal/controller/operator/factory/vmauth/vmusers_config.go:320">
P1: Cache-hit handling returns from `updateCache` too early, skipping remaining CRD objects in the same ref.</violation>
</file>
<file name="docs/CHANGELOG.md">
<violation number="1" location="docs/CHANGELOG.md:21">
P3: The new changelog sentence is unclear due to duplicated wording ("multiple referencing multiple"), which reduces documentation quality and can confuse users about the feature behavior.</violation>
<violation number="2" location="docs/CHANGELOG.md:21">
P1: Custom agent: **Changelog Review Agent**
Changelog entry is missing the mandatory issue/PR reference required by the changelog structure (Required structure §4: References).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
docs/CHANGELOG.md
Outdated
| * FEATURE: [vmsingle](https://docs.victoriametrics.com/operator/resources/vmsingle/): VMSingle reuses vmagent implementation to allow scraping and relabelling. See [#1694](https://github.com/VictoriaMetrics/operator/issues/1694) | ||
| * FEATURE: [vmoperator](https://docs.victoriametrics.com/operator/): perform statefulset pods deletion instead of eviction when maxUnavailable set to 100%, which is important for [minimum downtime strategy](https://docs.victoriametrics.com/victoriametrics/cluster-victoriametrics/#minimum-downtime-strategy). See [#1706](https://github.com/VictoriaMetrics/operator/issues/1706). | ||
| * FEATURE: [vmuser](https://docs.victoriametrics.com/operator/resources/vmuser/): support `targetRefs[*].crds` to place multiple CRD objects behind a single backend | ||
| * FEATURE: [vmuser](https://docs.victoriametrics.com/operator/resources/vmuser/): support multiple referencing multiple targets of same kind at `targetRefs[*].crd.objects`. |
There was a problem hiding this comment.
P1: Custom agent: Changelog Review Agent
Changelog entry is missing the mandatory issue/PR reference required by the changelog structure (Required structure §4: References).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/CHANGELOG.md, line 21:
<comment>Changelog entry is missing the mandatory issue/PR reference required by the changelog structure (Required structure §4: References).</comment>
<file context>
@@ -18,7 +18,7 @@ aliases:
* FEATURE: [vmsingle](https://docs.victoriametrics.com/operator/resources/vmsingle/): VMSingle reuses vmagent implementation to allow scraping and relabelling. See [#1694](https://github.com/VictoriaMetrics/operator/issues/1694)
* FEATURE: [vmoperator](https://docs.victoriametrics.com/operator/): perform statefulset pods deletion instead of eviction when maxUnavailable set to 100%, which is important for [minimum downtime strategy](https://docs.victoriametrics.com/victoriametrics/cluster-victoriametrics/#minimum-downtime-strategy). See [#1706](https://github.com/VictoriaMetrics/operator/issues/1706).
-* FEATURE: [vmuser](https://docs.victoriametrics.com/operator/resources/vmuser/): support `targetRefs[*].crds` to place multiple CRD objects behind a single backend
+* FEATURE: [vmuser](https://docs.victoriametrics.com/operator/resources/vmuser/): support multiple referencing multiple targets of same kind at `targetRefs[*].crd.objects`.
* BUGFIX: [vmoperator](https://docs.victoriametrics.com/operator/): VMPodScrape for VLAgent and VMAgent now uses the correct port; previously it used the wrong port and could cause scrape failures. See [#1887](https://github.com/VictoriaMetrics/operator/issues/1887).
</file context>
e9139f1 to
582ddb5
Compare
There was a problem hiding this comment.
5 issues found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/controller/operator/factory/vmauth/vmusers_config.go">
<violation number="1" location="internal/controller/operator/factory/vmauth/vmusers_config.go:105">
P1: Default backend merges are applied to range-variable copies, so merged TargetRef values are not persisted for config generation.</violation>
</file>
<file name="config/crd/overlay/crd.yaml">
<violation number="1" location="config/crd/overlay/crd.yaml:25437">
P1: `crd.objects` is introduced, but schema validation still requires `crd.name` and `crd.namespace`, preventing object-list-only configurations.</violation>
</file>
<file name="docs/api.md">
<violation number="1" location="docs/api.md:2961">
P2: `TargetRef.name` is documented as required, but it is only required for `spec.defaultTargetRefs` and optional in regular `targetRefs`.</violation>
<violation number="2" location="docs/api.md:3729">
P2: `VMAuth.spec.defaultTargetRefs` is documented as required, but the API defines it as optional.</violation>
</file>
<file name="api/operator/v1beta1/vmauth_types.go">
<violation number="1" location="api/operator/v1beta1/vmauth_types.go:453">
P2: Default targetRef retry validation uses the wrong global retry source, causing incorrect acceptance/rejection of configs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
d1862b3 to
587bec3
Compare
…ds for a single backend
587bec3 to
62450f2
Compare
| // CRD describes existing operator's CRD objects, | ||
| // operator generates access url based on CRD params. | ||
| // +optional | ||
| CRDs []CRDRef `json:"crds,omitempty"` |
There was a problem hiding this comment.
I don't think this ever made it to a released version, right?
There was a problem hiding this comment.
it was in 0.69 rc
| replicaCount: 1 | ||
| unauthorizedUserAccessSpec: | ||
| targetRefs: | ||
| - name: read |
There was a problem hiding this comment.
Are these hardcoded names in VMDistributed? We should add those to the docs
There was a problem hiding this comment.
right, added to VMAuth spec.defaultTargetRefs property, which allows to add predefined backends, that can be referenced in unanauthorized user section and VMUser as well. for VMDistributed this list has two items with read and write names
let's mentioned it in docs.
this change is kind a breaking, as it requires existing VMDistributed users to update CR manuallly, but suppose it's fine since CRD is in alpha
There was a problem hiding this comment.
added authorization section to VMDistributed docs and section about predefined backends to VMAuth
| // MergeDeep merges an override object into a base one. | ||
| // Fields present in the override will overwrite corresponding fields in the base. | ||
| func MergeDeep[T comparable](base, override T) error { | ||
| func MergeDeep[T comparable](base, override T, reverse bool) error { |
There was a problem hiding this comment.
Why not swap it on call time?
There was a problem hiding this comment.
before in merge result was stored to base and in case of repeated parameters override had higher priority, with this change still want to store merge result to base, but change priorities
There was a problem hiding this comment.
ah, yes, we mutate the vars here.
b8e7327 to
b36cdd3
Compare
…, disabled unauthorized access in VMdistributed by default
b36cdd3 to
047667b
Compare
this PR introduces
targetRefs[*].crds, which was basically needed for vmdistributed, now proposing to addtargetsRefs[*].crd.objectsinstead. multiple objects of a single kind most likely have same path prefixes and endpoints, which is important to get predictable response while querying given backend.additionally given below changes fix issue #1840:
Summary by cubic
VMUser now supports multiple same‑kind targets per backend via
targetRefs[*].crd.objects. VMAuth addsspec.defaultTargetRefsso VMUser and unauthorized access can reference sharedread/writebackends by name; VMDistributed predefines these and disables unauthorized access by default.targetRefs[*].crdswithtargetRefs[*].crd.objects; keepcrd.kindand list each object as{name, namespace}.crd.nameandcrd.namespacestill work.targetRefs[*].nameto referenceVMAuth.spec.defaultTargetRefs; inline fields override defaults.UnauthorizedUserAccessSpec.targetRefsalso supports names.crdorstaticper TargetRef; CRD schema, examples, docs, and controller/tests are updated.Written for commit 047667b. Summary will update on new commits.