operator: support multiple interfaces - #2537
Conversation
1ffef80 to
c41b453
Compare
|
@cubic-ai-dev |
@AndrewChubatiuk I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 102 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
b1666b4 to
9d3ebd8
Compare
There was a problem hiding this comment.
All reported issues were addressed across 107 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
9d3ebd8 to
893d6f1
Compare
There was a problem hiding this comment.
1 issue found across 109 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="test/e2e/vlcluster_test.go">
<violation number="1" location="test/e2e/vlcluster_test.go:51">
P2: This PR's headline feature is HTTPListeners-based multiple-interface support, but the VLCluster e2e test only adapts existing entries to the new StandardAppsParams nesting and never exercises HTTPListeners. Add an e2e entry that configures HTTPListeners on VLInsert/VLSelect/VLStorage and verifies the generated services, ports, and probe behavior, so the new feature gets integration coverage instead of only unit-level factory coverage.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
893d6f1 to
a5ade65
Compare
left a comment
•
There was a problem hiding this comment.
All reported issues were addressed across 109 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
8761494 to
94d9e11
Compare
left a comment
•
There was a problem hiding this comment.
All reported issues were addressed across 109 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
94d9e11 to
fbbded6
Compare
left a comment
•
There was a problem hiding this comment.
All reported issues were addressed across 109 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 6 unresolved issues already reported by Cubic.
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
fbbded6 to
c47519c
Compare
left a comment
•
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 109 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 6 unresolved issues already reported by Cubic.
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
c47519c to
515567d
Compare
left a comment
•
There was a problem hiding this comment.
All reported issues were addressed across 109 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
515567d to
4b31bef
Compare
4b31bef to
7da86ba
Compare
left a comment
•
There was a problem hiding this comment.
1 issue found across 109 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="api/operator/v1alpha1/vmdistributed_types.go">
<violation number="1" location="api/operator/v1alpha1/vmdistributed_types.go:577">
P1: When HTTP listener settings are split between `zoneCommon` and a zone, these checks can accept a conflict that exists only after the specs are merged. Validate the merged VMAgent spec before building it, so `httpListeners` and inherited `extraArgs` cannot produce mismatched process and service ports.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
| } | ||
| } | ||
| if err := zone.VMAgent.Spec.Validate(); err != nil { |
There was a problem hiding this comment.
P1: When HTTP listener settings are split between zoneCommon and a zone, these checks can accept a conflict that exists only after the specs are merged. Validate the merged VMAgent spec before building it, so httpListeners and inherited extraArgs cannot produce mismatched process and service ports.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/operator/v1alpha1/vmdistributed_types.go, line 577:
<comment>When HTTP listener settings are split between `zoneCommon` and a zone, these checks can accept a conflict that exists only after the specs are merged. Validate the merged VMAgent spec before building it, so `httpListeners` and inherited `extraArgs` cannot produce mismatched process and service ports.</comment>
<file context>
@@ -574,6 +574,12 @@ func (cr *VMDistributed) Validate() error {
}
}
}
+ if err := zone.VMAgent.Spec.Validate(); err != nil {
+ return fmt.Errorf("spec.zones[%d].vmagent.spec: %w", i, err)
+ }
</file context>
7da86ba to
9df0541
Compare
left a comment
•
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 114 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 7 unresolved issues already reported by Cubic.
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
9df0541 to
1bfb645
Compare
left a comment
•
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 115 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 6 unresolved issues already reported by Cubic.
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
1bfb645 to
b76b86a
Compare
left a comment
•
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 115 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 4 unresolved issues already reported by Cubic.
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
b76b86a to
a309458
Compare
left a comment
•
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 116 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
a309458 to
4bb2da9
Compare
left a comment
•
There was a problem hiding this comment.
1 issue found across 116 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="api/operator/v1/common.go">
<violation number="1" location="api/operator/v1/common.go:60">
P2: Changing this exported method from `Validate(httpPort string)` to a two-argument signature breaks external Go callers using the existing API. Preserve the one-argument method and add a listener-aware helper, or use a backward-compatible variadic form.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| func (g *OTLPGRPCSpec) Validate(httpPort string) error { | ||
| // Validate checks that ListenPort doesn't collide with httpPort or any configured HTTPListener, | ||
| // and that no HTTPListener is named OTLPGRPCPortName. | ||
| func (g *OTLPGRPCSpec) Validate(httpPort string, listeners []vmv1beta1.HTTPListener) error { |
There was a problem hiding this comment.
P2: Changing this exported method from Validate(httpPort string) to a two-argument signature breaks external Go callers using the existing API. Preserve the one-argument method and add a listener-aware helper, or use a backward-compatible variadic form.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/operator/v1/common.go, line 60:
<comment>Changing this exported method from `Validate(httpPort string)` to a two-argument signature breaks external Go callers using the existing API. Preserve the one-argument method and add a listener-aware helper, or use a backward-compatible variadic form.</comment>
<file context>
@@ -51,18 +55,40 @@ type OTLPGRPCSpec struct {
-func (g *OTLPGRPCSpec) Validate(httpPort string) error {
+// Validate checks that ListenPort doesn't collide with httpPort or any configured HTTPListener,
+// and that no HTTPListener is named OTLPGRPCPortName.
+func (g *OTLPGRPCSpec) Validate(httpPort string, listeners []vmv1beta1.HTTPListener) error {
if g == nil {
return nil
</file context>
4bb2da9 to
4cd198e
Compare
left a comment
•
There was a problem hiding this comment.
3 issues found across 116 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="api/operator/v1alpha1/vmdistributed_types.go">
<violation number="1" location="api/operator/v1alpha1/vmdistributed_types.go:577">
P2: When common and zone agent settings split the HTTP listener configuration, these independent validations miss conflicts introduced by `MergeSpecs`. Validate the merged common-plus-zone spec, or explicitly reject cross-layer `httpListeners`/HTTP-related `extraArgs` combinations before reconciliation.</violation>
</file>
<file name="api/operator/v1alpha1/vldistributed_types.go">
<violation number="1" location="api/operator/v1alpha1/vldistributed_types.go:534">
P2: When common and zone agent settings split the legacy `extraArgs["httpListenAddr"]` and `httpListeners` fields, these independent checks accept an invalid merged VLAgent. Validate the merged agent specification, or reject conflicts across the common and zone scopes before reconciliation.</violation>
</file>
<file name="internal/controller/operator/factory/vlcluster/vlcluster_test.go">
<violation number="1" location="internal/controller/operator/factory/vlcluster/vlcluster_test.go:78">
P2: This PR introduces multiple-interface (HTTPListeners) support for the VL components, but this test file only migrates the embedded struct and adds no case exercising the new behavior (multiple listeners, per-listener TLS/proxy protocol, listener-derived container/service ports). Add a test that sets HTTPListeners on VLInsert/VLSelect/VLStorage and asserts the generated args, ports, and service ports.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| @@ -75,18 +75,24 @@ func TestCreateOrUpdate(t *testing.T) { | |||
| }, | |||
There was a problem hiding this comment.
P2: This PR introduces multiple-interface (HTTPListeners) support for the VL components, but this test file only migrates the embedded struct and adds no case exercising the new behavior (multiple listeners, per-listener TLS/proxy protocol, listener-derived container/service ports). Add a test that sets HTTPListeners on VLInsert/VLSelect/VLStorage and asserts the generated args, ports, and service ports.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/controller/operator/factory/vlcluster/vlcluster_test.go, line 78:
<comment>This PR introduces multiple-interface (HTTPListeners) support for the VL components, but this test file only migrates the embedded struct and adds no case exercising the new behavior (multiple listeners, per-listener TLS/proxy protocol, listener-derived container/service ports). Add a test that sets HTTPListeners on VLInsert/VLSelect/VLStorage and asserts the generated args, ports, and service ports.</comment>
<file context>
@@ -75,18 +75,24 @@ func TestCreateOrUpdate(t *testing.T) {
VLInsert: &vmv1.VLInsert{
- CommonAppsParams: vmv1beta1.CommonAppsParams{
- ReplicaCount: ptr.To(int32(2)),
+ StandardAppsParams: vmv1beta1.StandardAppsParams{
+ CommonAppsParams: vmv1beta1.CommonAppsParams{
+ ReplicaCount: ptr.To(int32(2)),
</file context>
4cd198e to
0447450
Compare
left a comment
•
There was a problem hiding this comment.
2 existing issues remain and 3 new issues found across 117 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="api/operator/v1/vlagent_types.go">
<violation number="1" location="api/operator/v1/vlagent_types.go:475">
P2: When every VLAgent HTTP listener uses proxy protocol, `GetListeners` causes the scrape builder to skip those listeners and then re-add the primary port through its empty-endpoint fallback. Prometheus will scrape a proxy-protocol endpoint without sending the required protocol and self-scraping will fail; keep the endpoint list empty when listener enumeration matched but all listeners were skipped, or make the fallback conditional on unsupported listener enumeration.</violation>
</file>
<file name="internal/controller/operator/factory/vmauth/vmauth.go">
<violation number="1" location="internal/controller/operator/factory/vmauth/vmauth.go:577">
P2: When VMAuth has multiple `httpListeners`, the self-scrape omits every secondary interface, so metrics exposed only there are never collected. Make VMAuth implement the listener lookup used by `build.VMServiceScrape`, or generate the listener endpoints in this path.</violation>
</file>
<file name="api/operator/v1beta1/vmagent_types.go">
<violation number="1" location="api/operator/v1beta1/vmagent_types.go:602">
P2: When every configured VMAgent listener uses `useProxyProtocol`, DaemonSet self-scraping still creates an ordinary HTTP endpoint after the listener loop skips all listeners. That endpoint cannot scrape the proxy-protocol port; preserve the configured-listener state so the fallback does not add a legacy endpoint when all listeners were intentionally skipped.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 4 unresolved issues already reported by Cubic.
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
|
|
||
| // GetListeners implements build.listenersEnumerator interface | ||
| func (cr *VLAgent) GetListeners() []vmv1beta1.HTTPListener { |
There was a problem hiding this comment.
P2: When every VLAgent HTTP listener uses proxy protocol, GetListeners causes the scrape builder to skip those listeners and then re-add the primary port through its empty-endpoint fallback. Prometheus will scrape a proxy-protocol endpoint without sending the required protocol and self-scraping will fail; keep the endpoint list empty when listener enumeration matched but all listeners were skipped, or make the fallback conditional on unsupported listener enumeration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/operator/v1/vlagent_types.go, line 475:
<comment>When every VLAgent HTTP listener uses proxy protocol, `GetListeners` causes the scrape builder to skip those listeners and then re-add the primary port through its empty-endpoint fallback. Prometheus will scrape a proxy-protocol endpoint without sending the required protocol and self-scraping will fail; keep the endpoint list empty when listener enumeration matched but all listeners were skipped, or make the fallback conditional on unsupported listener enumeration.</comment>
<file context>
@@ -460,7 +463,22 @@ func (cr *VLAgent) GetMetricsPath() string {
+}
+
+// GetListeners implements build.listenersEnumerator interface
+func (cr *VLAgent) GetListeners() []vmv1beta1.HTTPListener {
+ return cr.Spec.GetListeners()
+}
</file context>
|
|
||
| // GetListeners implements build.listenersEnumerator interface | ||
| func (cr *VMAgent) GetListeners() []HTTPListener { | ||
| return cr.Spec.GetListeners() |
There was a problem hiding this comment.
P2: When every configured VMAgent listener uses useProxyProtocol, DaemonSet self-scraping still creates an ordinary HTTP endpoint after the listener loop skips all listeners. That endpoint cannot scrape the proxy-protocol port; preserve the configured-listener state so the fallback does not add a legacy endpoint when all listeners were intentionally skipped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At api/operator/v1beta1/vmagent_types.go, line 602:
<comment>When every configured VMAgent listener uses `useProxyProtocol`, DaemonSet self-scraping still creates an ordinary HTTP endpoint after the listener loop skips all listeners. That endpoint cannot scrape the proxy-protocol port; preserve the configured-listener state so the fallback does not add a legacy endpoint when all listeners were intentionally skipped.</comment>
<file context>
@@ -586,7 +589,22 @@ func (cr *VMAgent) GetMetricsPath() string {
+
+// GetListeners implements build.listenersEnumerator interface
+func (cr *VMAgent) GetListeners() []HTTPListener {
+ return cr.Spec.GetListeners()
+}
+
</file context>
|
|
||
| func setInternalSvcPort(cr *vmv1beta1.VMAuth) func(svc *corev1.Service) { | ||
| return func(svc *corev1.Service) { | ||
| build.AddHTTPListenerPortsToService(svc, cr.Spec.HTTPListeners) |
There was a problem hiding this comment.
P2: When VMAuth has multiple httpListeners, the self-scrape omits every secondary interface, so metrics exposed only there are never collected. Make VMAuth implement the listener lookup used by build.VMServiceScrape, or generate the listener endpoints in this path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/controller/operator/factory/vmauth/vmauth.go, line 577:
<comment>When VMAuth has multiple `httpListeners`, the self-scrape omits every secondary interface, so metrics exposed only there are never collected. Make VMAuth implement the listener lookup used by `build.VMServiceScrape`, or generate the listener endpoints in this path.</comment>
<file context>
@@ -577,6 +574,7 @@ func buildIngressConfig(cr *vmv1beta1.VMAuth) *networkingv1.Ingress {
func setInternalSvcPort(cr *vmv1beta1.VMAuth) func(svc *corev1.Service) {
return func(svc *corev1.Service) {
+ build.AddHTTPListenerPortsToService(svc, cr.Spec.HTTPListeners)
if len(cr.Spec.InternalListenPort) > 0 {
p := intstr.Parse(cr.Spec.InternalListenPort)
</file context>
0447450 to
db18020
Compare
fixes #2346