Skip to content

feat(embedded): add network SP to environment-agent - #34

Open
gpb88 wants to merge 1 commit into
dcm-project:mainfrom
gpb88:import-network-sp
Open

feat(embedded): add network SP to environment-agent#34
gpb88 wants to merge 1 commit into
dcm-project:mainfrom
gpb88:import-network-sp

Conversation

@gpb88

@gpb88 gpb88 commented Sep 7, 2026

Copy link
Copy Markdown

Implements network service provider enabled via
AGENT_EMBEDDED_SPS=network and exposed at embedded://network endpoint.

  • Define network/v1alpha1 API spec with ports, routing levels, and Kubernetes provider hints
  • Implement Kubernetes store with create, get, list, and delete operations plus label-based filtering
  • Add validation layer for routing levels, node ports, and service type constraints
  • Wire HTTP handler and health checks into embedded server bundles
  • Add NATS monitoring for LoadBalancer Services publishing status events to dcm.network when external IPs are assigned (PENDING → READY → DELETED)
  • Implement StatusMonitor with Service informer, debouncer (500ms), and graceful shutdown

Include unit and integration tests covering Service lifecycle, error cases and monitoring

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Add embedded Kubernetes network service provider

✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

AI Description

• Adds embedded network lifecycle management backed by labeled Kubernetes Services.
• Publishes debounced PENDING, READY, and DELETED network status events through NATS.
• Defines network API generation, validation, health checks, and comprehensive lifecycle tests.
Diagram

sequenceDiagram
    actor DCM
    participant Router as Embedded Router
    participant Handler as Network Handler
    participant Validator
    participant Store as Network Store
    participant K8s as Kubernetes API
    participant Monitor as Status Monitor
    participant NATS
    DCM->>Router: Create or delete
    Router->>Handler: Route network request
    Handler->>Validator: Validate specification
    Validator-->>Handler: Validated request
    Handler->>Store: Execute lifecycle operation
    Store->>K8s: Manage Service
    K8s-->>Monitor: Emit Service event
    Monitor->>Monitor: Map and debounce status
    Monitor->>NATS: Publish CloudEvent
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Deploy a standalone network provider
  • ➕ Provides independent scaling and failure isolation.
  • ➕ Allows provider-specific deployment and release cadence.
  • ➖ Adds operational and deployment overhead.
  • ➖ Duplicates connectivity and lifecycle integration already available in the agent.
  • ➖ Does not satisfy the embedded provider objective as directly.
2. Poll Kubernetes Service status
  • ➕ Avoids informer lifecycle and debounce concurrency.
  • ➕ Can be simpler for very small deployments.
  • ➖ Increases Kubernetes API load and status latency.
  • ➖ Can miss short-lived transitions.
  • ➖ Requires polling interval and reconciliation trade-offs.

Recommendation: Keep the embedded repository plus informer-based monitoring approach. It matches the existing embedded-provider model, uses Kubernetes-native event delivery, and isolates transport behind interfaces; a standalone provider or polling loop would add operational cost or weaken status timeliness.

Files changed (47) +4264 / -1

Enhancement (27) +2750 / -0
openapi.yamlDefine the network v1alpha1 API contract +513/-0

Define the network v1alpha1 API contract

• Defines network CRUD, health, pagination, status, ports, routing levels, Kubernetes hints, and RFC 7807 errors. Models Kubernetes Services as AEP-compatible network resources.

api/network/v1alpha1/openapi.yaml

paths.goDerive the network create path from OpenAPI +19/-0

Derive the network create path from OpenAPI

• Adds a helper that locates the first POST path in the embedded network specification instead of hardcoding it.

api/network/v1alpha1/paths.go

spec.gen.goEmbed the generated network OpenAPI specification +167/-0

Embed the generated network OpenAPI specification

• Adds generated helpers for decoding, caching, and loading the network API specification.

api/network/v1alpha1/spec.gen.go

types.gen.goGenerate network API models and enums +440/-0

Generate network API models and enums

• Adds generated Go types for networks, ports, Kubernetes hints and state, statuses, health, pagination, and errors.

api/network/v1alpha1/types.gen.go

embedded.goRegister the network embedded provider bundle +31/-0

Register the network embedded provider bundle

• Adds network setup, request routing, health checks, startup, shutdown, and rollback cleanup to the aggregate embedded-provider lifecycle.

internal/embedded/embedded.go

handler.goImplement embedded network lifecycle handling +109/-0

Implement embedded network lifecycle handling

• Parses wrapped or plain network specifications, validates create requests, invokes the repository, and maps domain errors to HTTP provider responses.

internal/embedded/network/handler.go

health.goAdapt Kubernetes health checks for the agent +21/-0

Adapt Kubernetes health checks for the agent

• Wraps network repository connectivity checks as an embedded health monitor checker.

internal/embedded/network/health.go

setup.goConstruct and manage the embedded network bundle +71/-0

Construct and manage the embedded network bundle

• Enables the provider through AGENT_EMBEDDED_SPS, loads configuration, wires its app, handler, and checker, and exposes lifecycle methods.

internal/embedded/network/setup.go

app.goWire the network store, monitor, and publisher +117/-0

Wire the network store, monitor, and publisher

• Builds Kubernetes-backed network dependencies and optionally enables NATS status monitoring. Integrates monitor startup and publisher cleanup with the shared application lifecycle.

internal/openshift/network/app/app.go

labels.goDefine DCM network labeling conventions +46/-0

Define DCM network labeling conventions

• Adds reserved labels and selector helpers used to identify and filter DCM-managed network Services.

internal/openshift/network/dcm/labels.go

client.goCreate the Kubernetes network client +33/-0

Create the Kubernetes network client

• Builds a client from kubeconfig or in-cluster credentials and applies a three-second request timeout.

internal/openshift/network/kubernetes/client.go

convert.goConvert network resources and Kubernetes Services +250/-0

Convert network resources and Kubernetes Services

• Maps API specifications into ClusterIP, NodePort, or LoadBalancer Services and reconstructs API responses. Derives runtime state, external addresses, user labels, ports, and network status.

internal/openshift/network/kubernetes/convert.go

labels.goAdd managed-Service label helpers +27/-0

Add managed-Service label helpers

• Wraps DCM selectors, verifies Service ownership, and merges labels while preserving reserved DCM values.

internal/openshift/network/kubernetes/labels.go

store.goImplement the Kubernetes network store foundation +61/-0

Implement the Kubernetes network store foundation

• Defines the repository implementation, Kubernetes connectivity health check, and label-based unique Service lookup.

internal/openshift/network/kubernetes/store.go

store_create.goCreate Kubernetes Services from network specifications +69/-0

Create Kubernetes Services from network specifications

• Validates routing and NodePort constraints, applies protected labels, creates Services, and maps Kubernetes errors into domain errors.

internal/openshift/network/kubernetes/store_create.go

store_delete.goDelete managed network Services +23/-0

Delete managed network Services

• Resolves a network by its DCM instance label and deletes the corresponding Kubernetes Service idempotently.

internal/openshift/network/kubernetes/store_delete.go

store_get.goRetrieve networks from Kubernetes Services +17/-0

Retrieve networks from Kubernetes Services

• Finds a managed Service by network identifier and converts it into the network API representation.

internal/openshift/network/kubernetes/store_get.go

store_list.goList managed networks with Kubernetes pagination +82/-0

List managed networks with Kubernetes pagination

• Filters Services by DCM labels, normalizes page limits, reuses Kubernetes continue tokens, and maps invalid tokens to domain errors.

internal/openshift/network/kubernetes/store_list.go

debouncer.goDebounce per-network status events +162/-0

Debounce per-network status events

• Coalesces rapid events by network identifier, safely handles concurrent publication, and flushes the latest pending state during shutdown.

internal/openshift/network/monitoring/debouncer.go

event.goBuild network status CloudEvents +42/-0

Build network status CloudEvents

• Serializes network status transitions as CloudEvents with provider source, network event type, subject, identifier, status, and message.

internal/openshift/network/monitoring/event.go

indexer.goExtract network identifiers from Kubernetes objects +13/-0

Extract network identifiers from Kubernetes objects

• Adds a shared helper for reading DCM instance identifiers from object labels.

internal/openshift/network/monitoring/indexer.go

monitor.goMonitor Kubernetes Services and publish status changes +223/-0

Monitor Kubernetes Services and publish status changes

• Runs a label-filtered Service informer, maps lifecycle events to statuses, suppresses duplicates, debounces changes, retries publication, and flushes during shutdown.

internal/openshift/network/monitoring/monitor.go

publisher.goPublish network status events through NATS +81/-0

Publish network status events through NATS

• Defines the status publisher abstraction and a reconnecting NATS implementation targeting dcm.network.

internal/openshift/network/monitoring/publisher.go

errors.goDefine network repository domain errors +38/-0

Define network repository domain errors

• Adds typed not-found, conflict, and invalid-argument errors for consistent transport-level mapping.

internal/openshift/network/store/errors.go

health.goDefine the network health-check contract +9/-0

Define the network health-check contract

• Adds an abstraction for checking backing infrastructure connectivity.

internal/openshift/network/store/health.go

repository.goDefine the network repository interface +18/-0

Define the network repository interface

• Specifies create, get, list, delete, and health operations for network persistence implementations.

internal/openshift/network/store/repository.go

validate.goValidate embedded network create requests +68/-0

Validate embedded network create requests

• Enforces AEP-122 identifiers, reserved paths and labels, the network service type, metadata names, and non-empty ports.

internal/openshift/network/validate/validate.go

Tests (14) +1432 / -0
paths_test.goVerify network POST path discovery +22/-0

Verify network POST path discovery

• Confirms the embedded specification resolves the create endpoint as /api/v1alpha1/networks.

api/network/v1alpha1/paths_test.go

handler_test.goTest embedded network request handling +174/-0

Test embedded network request handling

• Covers wrapped and plain creates, identifier validation, deletion, and HTTP mappings for repository errors.

internal/embedded/network/handler_test.go

setup_test.goTest network provider enablement +19/-0

Test network provider enablement

• Verifies that the network bundle is enabled only when network appears in the embedded provider list.

internal/embedded/network/setup_test.go

config_test.goAdd the network configuration test suite +13/-0

Add the network configuration test suite

• Registers the Ginkgo suite for network provider configuration tests.

internal/openshift/network/config/config_test.go

config_unit_test.goTest network configuration defaults and overrides +72/-0

Test network configuration defaults and overrides

• Covers agent-derived settings, namespace and provider-name overrides, defaults, and missing messaging configuration.

internal/openshift/network/config/config_unit_test.go

kubernetes_test.goAdd the Kubernetes store test suite +13/-0

Add the Kubernetes store test suite

• Registers the Ginkgo suite for Kubernetes network store tests.

internal/openshift/network/kubernetes/kubernetes_test.go

store_create_test.goTest Kubernetes Service creation scenarios +385/-0

Test Kubernetes Service creation scenarios

• Covers Service type inference, labels, ports, selectors, headless Services, routing restrictions, NodePort validation, and Kubernetes errors.

internal/openshift/network/kubernetes/store_create_test.go

store_delete_test.goTest managed network deletion +97/-0

Test managed network deletion

• Covers successful deletion, missing networks, and duplicate instance-label conflicts.

internal/openshift/network/kubernetes/store_delete_test.go

store_get_test.goTest network retrieval and status mapping +154/-0

Test network retrieval and status mapping

• Covers successful lookup, missing and duplicate resources, and READY or PENDING status derivation.

internal/openshift/network/kubernetes/store_get_test.go

store_health_test.goTest Kubernetes connectivity health checks +40/-0

Test Kubernetes connectivity health checks

• Verifies healthy discovery responses and propagation of simulated API discovery failures.

internal/openshift/network/kubernetes/store_health_test.go

store_list_test.goTest network listing and pagination +173/-0

Test network listing and pagination

• Covers filtered listing, empty results, continuation across pages, and invalid page-token handling.

internal/openshift/network/kubernetes/store_list_test.go

helpers_test.goAdd thread-safe monitoring test publishers +61/-0

Add thread-safe monitoring test publishers

• Provides mock publishers for recording events and testing cancellation-sensitive shutdown behavior.

internal/openshift/network/monitoring/helpers_test.go

informer_integration_test.goTest informer-driven network status transitions +196/-0

Test informer-driven network status transitions

• Exercises PENDING, READY, and DELETED publication for LoadBalancer Services and immediate READY publication for ClusterIP Services.

internal/openshift/network/monitoring/informer_integration_test.go

monitoring_test.goAdd the network monitoring test suite +13/-0

Add the network monitoring test suite

• Registers the Ginkgo suite for network status monitoring tests.

internal/openshift/network/monitoring/monitoring_test.go

Other (6) +82 / -1
MakefileAdd network API generation targets +16/-1

Add network API generation targets

• Adds oapi-codegen targets for network models and the embedded specification. Includes network generation in aggregate API and image-build workflows.

Makefile

spec.gen.cfgConfigure embedded network specification generation +5/-0

Configure embedded network specification generation

• Configures oapi-codegen to embed the complete network OpenAPI specification.

api/network/v1alpha1/spec.gen.cfg

types.gen.cfgConfigure network model generation +5/-0

Configure network model generation

• Configures oapi-codegen to generate all network API models without pruning.

api/network/v1alpha1/types.gen.cfg

config.goDefine embedded network provider configuration +36/-0

Define embedded network provider configuration

• Loads namespace, debounce, resync, and retry settings from environment variables while inheriting shared agent connectivity settings.

internal/openshift/network/config/config.go

config.goDefine Kubernetes network store settings +6/-0

Define Kubernetes network store settings

• Adds namespace configuration for Kubernetes-backed network operations.

internal/openshift/network/kubernetes/config.go

config.goDefine network status monitor settings +14/-0

Define network status monitor settings

• Adds namespace, debounce, resync, retry, and graceful-shutdown publishing configuration.

internal/openshift/network/monitoring/config.go

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (6) 📘 Rule violations (9) 📜 Skill insights (0)

Grey Divider


Action required

1. Shutdown can lose network status events 🐞 Bug ☼ Reliability
Description
NATSPublisher.Close closes the NATS connection without draining or flushing messages previously
queued by Publish. When the monitor flushes pending status changes during shutdown, the lifecycle
immediately closes the publisher afterward and those final notifications may never reach
subscribers.
Code

internal/openshift/network/monitoring/publisher.go[R77-80]

+// Close closes the underlying NATS connection.
+func (p *NATSPublisher) Close() error {
+	p.conn.Close()
+	return nil
Evidence
Publish returns immediately after conn.Publish, while Close calls conn.Close without any delivery
barrier. AppLifecycle stops the monitor and then closes registered resources, so there is no later
opportunity to flush the final events emitted by Debouncer.Stop.

internal/openshift/network/monitoring/publisher.go[59-80]
internal/openshift/network/monitoring/monitor.go[99-108]
internal/openshift/network/monitoring/debouncer.go[138-162]
internal/openshift/worker/worker.go[55-69]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The network status publisher closes its NATS connection without waiting for queued messages to be delivered, so shutdown-flushed status events can be lost.

## Issue Context
The worker lifecycle waits for the monitor to stop and then invokes the publisher's `Close`. `Publish` only queues through `conn.Publish`, while `Close` immediately closes the connection.

## Fix Focus Areas
- internal/openshift/network/monitoring/publisher.go[59-80]
- internal/openshift/network/app/app.go[104-117]
- internal/openshift/worker/worker.go[55-69]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Invalid routing requests create load balancers 🐞 Bug ≡ Correctness
Description
inferServiceType returns LoadBalancer for every non-nil routing_level except the separately
rejected application value, while JSON parsing and ValidateCreate do not enforce membership in
the declared enum. An embedded-endpoint request containing any unknown routing value therefore
passes validation and reaches Service construction as a load balancer rather than the advertised
invalid-argument path.
Code

internal/openshift/network/kubernetes/convert.go[R58-65]

+	if spec.RoutingLevel == nil {
+		if hasNodePorts {
+			return corev1.ServiceTypeNodePort
+		}
+		return corev1.ServiceTypeClusterIP
+	}
+
+	return corev1.ServiceTypeLoadBalancer
Evidence
The API schema limits routing_level to network and application, but generated Go enum types
can contain arbitrary decoded strings after JSON unmarshalling, and ValidateCreate checks service
type, name, ports, and labels without inspecting this field. The store rejects only application,
after which inferServiceType maps every other non-nil value to LoadBalancer, proving that an
unknown value reaches load-balancer Service creation.

api/network/v1alpha1/openapi.yaml[412-422]
internal/embedded/network/handler.go[74-92]
internal/openshift/network/validate/validate.go[31-67]
internal/openshift/network/kubernetes/store_create.go[13-19]
internal/openshift/network/kubernetes/convert.go[52-66]
api/network/v1alpha1/openapi.yaml[412-420]
internal/openshift/network/validate/validate.go[31-41]
internal/openshift/network/kubernetes/convert.go[52-65]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Reject routing levels outside the declared `network` and `application` values before service-type inference, so unknown values cannot be converted into LoadBalancer Services.

## Issue Context
Generated OpenAPI enum types do not enforce enum membership during JSON unmarshalling, and the embedded handler passes parsed specifications to `ValidateCreate` before the store builds the Service. Validate `RoutingLevel.Valid()` whenever the field is present, while retaining the explicit unsupported-`application` response.

## Fix Focus Areas
- internal/openshift/network/validate/validate.go[31-41]
- internal/openshift/network/kubernetes/store_create.go[13-19]
- internal/openshift/network/kubernetes/convert.go[52-66]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Application routing never reaches clients 🐞 Bug ≡ Correctness
Description
The API schema describes application routing as HTTP/HTTPS routing, but Create always returns an
invalid-argument error when that documented enum value is supplied. Any client selecting the
advertised application routing level receives a 400 response and no network resource is created.
Code

internal/openshift/network/kubernetes/store_create.go[R13-19]

+func (s *K8sNetworkStore) Create(ctx context.Context, spec v1alpha1.NetworkSpec, id string) (*v1alpha1.Network, error) {
+	if spec.RoutingLevel != nil && *spec.RoutingLevel == v1alpha1.NetworkSpecRoutingLevelApplication {
+		return nil, &store.InvalidArgumentError{
+			Field:   "routing_level",
+			Message: "application routing not supported",
+		}
+	}
Evidence
application is an allowed public enum value and is documented with routing semantics, yet the only
implementation path for that value explicitly rejects it. The conversion code otherwise only
distinguishes a missing routing level from a non-nil one, so there is no implementation that honors
the documented application behavior.

api/network/v1alpha1/openapi.yaml[412-422]
internal/openshift/network/kubernetes/store_create.go[13-19]
internal/openshift/network/kubernetes/convert.go[52-66]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Make the documented `application` routing level create its intended resource, or remove it from the public API schema until it is supported.

## Issue Context
The OpenAPI documentation explicitly presents `application` as HTTP/HTTPS routing, while the create path unconditionally rejects it.

## Fix Focus Areas
- api/network/v1alpha1/openapi.yaml[412-422]
- internal/openshift/network/kubernetes/store_create.go[13-19]
- internal/openshift/network/kubernetes/convert.go[52-66]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

4. Network failures lose operation context 📘 Rule violation ≡ Correctness
Description
findService, Delete, Get, mapListError, and CheckHealth return dependency errors directly
instead of wrapping them with operation context. When Kubernetes discovery, listing, lookup, or
deletion fails, the embedded handler receives an error chain that does not identify which store
operation or namespace failed.
Code

internal/openshift/network/kubernetes/store.go[R47-48]

+	if err != nil {
+		return nil, err
Evidence
Compliance rule 2788501 requires returned errors to be wrapped with fmt.Errorf and %w. The cited
store functions directly return errors from Kubernetes clients or subordinate store calls.

Rule 2788501: Wrap errors with context using fmt.Errorf and %w
internal/openshift/network/kubernetes/store.go[34-49]
internal/openshift/network/kubernetes/store_delete.go[11-22]
internal/openshift/network/kubernetes/store_get.go[9-13]
internal/openshift/network/kubernetes/store_list.go[66-82]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Several network store functions return dependency errors without adding operation context.

## Issue Context
Wrap underlying errors with `fmt.Errorf("context: %w", err)` while preserving typed errors that callers match with `errors.As`.

## Fix Focus Areas
- internal/openshift/network/kubernetes/store.go[34-49]
- internal/openshift/network/kubernetes/store_delete.go[11-22]
- internal/openshift/network/kubernetes/store_get.go[9-13]
- internal/openshift/network/kubernetes/store_list.go[66-82]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Invalid service errors lose their cause 📘 Rule violation ≡ Correctness
Description
Create formats a Kubernetes invalid-specification error into InvalidArgumentError.Message with
%v rather than retaining it through %w. When the API server rejects a Service, callers can no
longer inspect the underlying Kubernetes status error and receive only flattened text.
Code

internal/openshift/network/kubernetes/store_create.go[R59-62]

+			return nil, &store.InvalidArgumentError{
+				Field:   "spec",
+				Message: fmt.Sprintf("invalid service spec: %v", err),
+			}
Evidence
Rule 2788501 explicitly prohibits using %v for returned errors. The invalid Service branch embeds
err into a message with %v, and InvalidArgumentError has no cause or Unwrap method.

Rule 2788501: Wrap errors with context using fmt.Errorf and %w
internal/openshift/network/kubernetes/store_create.go[58-64]
internal/openshift/network/store/errors.go[30-38]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The invalid-Service branch flattens the Kubernetes error with `%v`, preventing error-chain inspection.

## Issue Context
Preserve `InvalidArgumentError` matching while retaining the Kubernetes cause through `%w` or an `Unwrap` implementation.

## Fix Focus Areas
- internal/openshift/network/kubernetes/store_create.go[58-64]
- internal/openshift/network/store/errors.go[30-38]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Health errors are logged too early 📘 Rule violation ◔ Observability
Description
CheckHealth writes a warning for a discovery failure and then returns that same error to its
caller. Whenever Kubernetes discovery fails, the intermediate store layer emits the failure before
the health-monitoring boundary decides how it should be handled or recorded.
Code

internal/openshift/network/kubernetes/store.go[R36-38]

+	if err != nil {
+		s.logger.Warn("kubernetes health check failed", "error", err)
+		return err
Evidence
Rule 2788508 prohibits intermediate functions from both logging and returning an error.
CheckHealth logs the discovery error on line 37 and returns it on line 38.

Rule 2788508: Log errors only at the handling boundary, not at intermediate layers
internal/openshift/network/kubernetes/store.go[33-40]
internal/embedded/network/health.go[14-20]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Kubernetes store logs a health-check error and also returns it.

## Issue Context
Intermediate layers should return a context-wrapped error; logging belongs at the boundary that consumes the failure.

## Fix Focus Areas
- internal/openshift/network/kubernetes/store.go[33-40]
- internal/embedded/network/health.go[14-20]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View medium (7)
7. Invalid dependencies fail after startup 📘 Rule violation ≡ Correctness
Description
NewK8sNetworkStore accepts nil client and logger values without panicking during construction.
A nil client reaches every Kubernetes operation, while a nil logger reaches failed health checks and
turns setup mistakes into delayed nil-pointer panics.
Code

internal/openshift/network/kubernetes/store.go[R23-24]

+func NewK8sNetworkStore(client kubernetes.Interface, cfg K8sConfig, logger *slog.Logger) *K8sNetworkStore {
+	return &K8sNetworkStore{
Evidence
Rule 2788523 requires constructors to panic when required dependencies are nil. The constructor
stores both dependencies without checks, and CheckHealth subsequently dereferences both.

Rule 2788523: Required constructor dependencies must panic on nil
internal/openshift/network/kubernetes/store.go[22-29]
internal/openshift/network/kubernetes/store.go[33-38]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The store constructor does not reject required nil dependencies.

## Issue Context
Both the Kubernetes client and logger are dereferenced by store methods and therefore must be validated during construction.

## Fix Focus Areas
- internal/openshift/network/kubernetes/store.go[22-29]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


8. Consumers do not own store contracts 📘 Rule violation ⌂ Architecture
Description
NetworkRepository is declared in the store package even though app and the embedded network
handler consume the repository behavior. Changes to consumer needs therefore alter a shared
provider-level contract instead of allowing each consumer to define the minimal behavior it
requires.
Code

internal/openshift/network/store/repository.go[R12-15]

+type NetworkRepository interface {
+	Create(ctx context.Context, spec v1alpha1.NetworkSpec, id string) (*v1alpha1.Network, error)
+	Get(ctx context.Context, networkID string) (*v1alpha1.Network, error)
+	List(ctx context.Context, maxPageSize int32, pageToken string) (*v1alpha1.NetworkList, error)
Evidence
Rule 2788525 requires interfaces to be declared where consumed. NetworkRepository is declared
under store, implemented under kubernetes, and held by app, while the embedded handler
independently defines a narrower consuming interface.

Rule 2788525: Define interfaces where consumed, not where implemented
internal/openshift/network/store/repository.go[10-18]
internal/openshift/network/app/app.go[16-29]
internal/embedded/network/handler.go[20-29]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The repository interface is declared in a standalone abstraction package rather than at its consumption site.

## Issue Context
Define minimal behavior-based interfaces in consuming packages and let the Kubernetes implementation satisfy them implicitly.

## Fix Focus Areas
- internal/openshift/network/store/repository.go[10-18]
- internal/openshift/network/app/app.go[16-29]
- internal/embedded/network/handler.go[20-29]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


9. One API test breaks suite conventions 📘 Rule violation ▣ Testability
Description
TestPostPath uses testing.T with Fatal and Errorf assertions instead of Ginkgo v2 blocks and
Gomega Expect assertions. Extending this API test now requires maintaining a separate test style
and failure-reporting pattern from the rest of the added network suites.
Code

api/network/v1alpha1/paths_test.go[R9-12]

+func TestPostPath(t *testing.T) {
+	got, err := v1alpha1.PostPath()
+	if err != nil {
+		t.Fatalf("PostPath() returned unexpected error: %v", err)
Evidence
Rule 2788537 requires new tests to use Ginkgo v2 and Gomega. The cited test is a direct testing.T
test whose assertions call t.Fatal and t.Errorf.

Rule 2788537: Use Ginkgo BDD structure with Describe/Context/It and Expect assertions
api/network/v1alpha1/paths_test.go[9-20]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The API path test uses standard `testing.T` assertions rather than the required Ginkgo and Gomega structure.

## Issue Context
Add or reuse a Ginkgo suite bootstrap and express the behavior with `Describe`, `Context` or `It`, and `Expect`.

## Fix Focus Areas
- api/network/v1alpha1/paths_test.go[1-22]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


10. Messaging integration stays unverified 📘 Rule violation ▣ Testability
Description
informer_integration_test.go injects mockStatusPublisher instead of constructing NATSPublisher
against an embedded NATS server. Its lifecycle scenarios only inspect an in-memory event slice, so
CloudEvent serialization, subject routing, connection behavior, and message delivery are not
exercised.
Code

internal/openshift/network/monitoring/informer_integration_test.go[R77-80]

+		BeforeEach(func() {
+			client = fake.NewClientset()
+			publisher = newMockPublisher()
+			logger = slog.New(slog.NewJSONHandler(io.Discard, nil))
Evidence
Rule 2788540 requires messaging integration tests to use an embedded NATS server rather than a mock
client or publisher. The integration test creates mockStatusPublisher, and the helper records
publications only in memory.

Rule 2788540: Use real NATS server for messaging integration tests, not mocks
internal/openshift/network/monitoring/informer_integration_test.go[67-83]
internal/openshift/network/monitoring/helpers_test.go[13-44]
internal/openshift/network/monitoring/publisher.go[34-75]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The monitoring integration test replaces NATS publishing with an in-memory mock.

## Issue Context
Start an embedded `nats-server`, construct the real publisher, subscribe to `dcm.network`, and assert on the delivered CloudEvents.

## Fix Focus Areas
- internal/openshift/network/monitoring/informer_integration_test.go[67-196]
- internal/openshift/network/monitoring/helpers_test.go[13-44]
- internal/openshift/network/monitoring/publisher.go[34-75]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


11. Service watches churn every 3 seconds 🐞 Bug ➹ Performance
Description
NewClient assigns a three-second timeout to the REST configuration later shared with the status
informer. Because informer watches are long-running requests, an enabled network provider repeatedly
terminates and re-establishes its Service watch instead of maintaining one connection.
Code

internal/openshift/network/kubernetes/client.go[27]

+	restCfg.Timeout = 3 * time.Second
Evidence
The network Kubernetes client uniquely sets restCfg.Timeout to three seconds, and App.New passes
that exact client into StatusMonitor. StatusMonitor uses it to create the shared Service informer
whose watch is expected to remain active until its context is cancelled.

internal/openshift/network/kubernetes/client.go[12-32]
internal/openshift/network/app/app.go[62-79]
internal/openshift/network/monitoring/monitor.go[56-70]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Kubernetes client used by the Service informer has a global three-second request timeout, forcing long-running watches to reconnect continuously.

## Issue Context
The same client is constructed in the network app and passed both to the repository and to `NewStatusMonitor`. Use context deadlines for bounded CRUD and health operations, or construct a separate informer client without a global timeout.

## Fix Focus Areas
- internal/openshift/network/kubernetes/client.go[12-32]
- internal/openshift/network/app/app.go[62-79]
- internal/openshift/network/monitoring/monitor.go[56-70]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


12. Requested node ports can be discarded 🐞 Bug ≡ Correctness
Description
Create verifies that every declared port has a matching node_ports key but never verifies that
every node_ports key names exactly one declared port. When a request contains an extra, duplicate,
or misspelled map key, buildService iterates only actual Service ports and silently drops that
requested node-port assignment while returning creation success.
Code

internal/openshift/network/kubernetes/store_create.go[R31-37]

+				// Validate that the port name exists in the node_ports map
+				if _, exists := (*nodePorts)[*port.Name]; !exists {
+					return nil, &store.InvalidArgumentError{
+						Field:   "ports",
+						Message: fmt.Sprintf("port name %q not found in node_ports map", *port.Name),
+					}
+				}
Evidence
The API defines node_ports as a map from port names to requested values, but Create only
iterates over spec.Ports to require matching map entries. buildService likewise iterates only
over generated Service ports and applies values for matching names, proving that surplus, unknown,
or otherwise unmatched node_ports keys are neither applied nor surfaced as errors.

api/network/v1alpha1/openapi.yaml[342-352]
internal/openshift/network/kubernetes/store_create.go[21-39]
internal/openshift/network/kubernetes/convert.go[36-43]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Validate that every `provider_hints.kubernetes.node_ports` map key corresponds to exactly one declared port name. Reject unknown, misspelled, duplicate, or surplus names instead of silently ignoring their requested node-port allocations.

## Issue Context
Current validation is one-directional: it checks that each declared service port has a map entry, but does not check map-to-port membership. Conversion then iterates only over generated Service ports, so unmatched map entries are discarded while creation reports success; add the reverse check before creating the Kubernetes Service.

## Fix Focus Areas
- internal/openshift/network/kubernetes/store_create.go[21-39]
- internal/openshift/network/kubernetes/convert.go[36-43]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


13. Consumers receive duplicate status events 🐞 Bug ☼ Reliability
Description
submitIfChanged writes lastSubmitted but compares new events only with lastPublished, which is
updated only after publishing succeeds. If another informer update with the same status arrives
during a slow or retried publication, the debouncer treats it as a newer version and publishes an
identical follow-up event.
Code

internal/openshift/network/monitoring/monitor.go[R177-186]

+func (m *StatusMonitor) submitIfChanged(debouncer *Debouncer, event StatusEvent) {
+	m.mu.Lock()
+	if prev, ok := m.lastPublished[event.InstanceID]; ok &&
+		prev.Status == event.Status && prev.Message == event.Message {
+		m.mu.Unlock()
+		return
+	}
+	m.lastSubmitted[event.InstanceID] = event
+	m.mu.Unlock()
+	debouncer.Submit(event.InstanceID, event)
Evidence
The monitor records queued events in lastSubmitted but never uses that map to suppress a
same-value update, and it does not update lastPublished until after Publish returns
successfully. The debouncer explicitly invokes another publication when an event version changes
while a callback is draining, so duplicate informer updates can become duplicate outbound events.

internal/openshift/network/monitoring/monitor.go[30-33]
internal/openshift/network/monitoring/monitor.go[175-187]
internal/openshift/network/monitoring/monitor.go[189-221]
internal/openshift/network/monitoring/debouncer.go[101-135]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Avoid submitting an identical status event while that event is already queued or being published, without preventing a later retry after publication failure.

## Issue Context
`lastSubmitted` exists specifically for pre-ack state but is never read; the debouncer schedules a follow-up whenever its version changes during a draining callback.

## Fix Focus Areas
- internal/openshift/network/monitoring/monitor.go[175-187]
- internal/openshift/network/monitoring/monitor.go[189-221]
- internal/openshift/network/monitoring/debouncer.go[101-135]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

14. Store field roles lack guidance 📘 Rule violation ⚙ Maintainability
Description
K8sNetworkStore places its required client, configuration, and infrastructure logger together
without category headers. A later maintainer cannot determine from the declaration which values are
mandatory dependencies versus configuration or infrastructure concerns.
Code

internal/openshift/network/kubernetes/store.go[R16-19]

+type K8sNetworkStore struct {
+	client kubernetes.Interface
+	cfg    K8sConfig
+	logger *slog.Logger
Evidence
Rule 2788534 requires struct fields to be grouped by category with comment headers. The store
declaration mixes a client dependency, configuration, and logger without those headers.

Rule 2788534: Struct fields should be grouped by category with comments
internal/openshift/network/kubernetes/store.go[16-20]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The store struct does not group fields by category with comments.

## Issue Context
Separate required dependencies, configuration, and infrastructure fields using concise category headers.

## Fix Focus Areas
- internal/openshift/network/kubernetes/store.go[16-20]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


15. Network behavior lacks real coverage 📘 Rule violation ▣ Testability
Description
The new Kubernetes CRUD suites construct fake.Clientset instances for straightforward create, get,
list, delete, and health operations rather than exercising the provider against a real API server.
Admission behavior, real pagination tokens, discovery, and Kubernetes validation can therefore
differ from the behavior asserted by these tests.
Code

internal/openshift/network/kubernetes/store_create_test.go[R31-34]

+	BeforeEach(func() {
+		client = fake.NewClientset()
+		logger = slog.New(slog.NewJSONHandler(io.Discard, nil))
+		s = k8sstore.NewK8sNetworkStore(client, k8sstore.K8sConfig{Namespace: "default"}, logger)
Evidence
Rule 2788542 recommends integration tests through real components and limits unit tests to isolated
non-trivial logic. The CRUD suites repeatedly use fake.NewClientset to test provider operations
that otherwise span the repository and Kubernetes API layers.

Rule 2788542: Prefer integration tests over unit tests
internal/openshift/network/kubernetes/store_create_test.go[23-35]
internal/openshift/network/kubernetes/store_get_test.go[19-31]
internal/openshift/network/kubernetes/store_list_test.go[23-35]
internal/openshift/network/kubernetes/store_delete_test.go[19-31]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Straightforward repository behavior is covered primarily through fake Kubernetes clients.

## Issue Context
Favor integration tests using a real test API server or cluster for CRUD, pagination, validation, and health behavior; retain unit tests only for isolated conversion logic.

## Fix Focus Areas
- internal/openshift/network/kubernetes/store_create_test.go[23-381]
- internal/openshift/network/kubernetes/store_get_test.go[19-154]
- internal/openshift/network/kubernetes/store_list_test.go[23-117]
- internal/openshift/network/kubernetes/store_delete_test.go[19-97]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 17 rules
Review mode: 🧠 Deep: This is a broad, behavior-heavy Kubernetes integration spanning API contracts, CRUD paths, validation, embedded wiring, asynchronous informer/debouncer concurrency, and NATS status publication, creating many independent opportunities for subtle defects.

Grey Divider

Tip of the day
💡 Did you know, you can copy the agent prompt from any finding and feed it to your IDE agent

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread internal/openshift/network/kubernetes/store.go Outdated
Comment thread internal/openshift/network/kubernetes/store_create.go Outdated
Comment thread internal/openshift/network/kubernetes/store.go Outdated
Comment thread internal/openshift/network/kubernetes/store.go
Comment thread internal/openshift/network/store/repository.go
Comment thread internal/openshift/network/kubernetes/client.go Outdated
Comment thread internal/openshift/network/kubernetes/convert.go
Comment thread internal/openshift/network/kubernetes/store_create.go
Comment thread internal/openshift/network/kubernetes/store_create.go
Comment thread internal/openshift/network/monitoring/monitor.go
@gpb88
gpb88 force-pushed the import-network-sp branch 2 times, most recently from 1ab27a2 to aa1da5c Compare September 7, 2026 12:29
Implements network service provider enabled via
AGENT_EMBEDDED_SPS=network and exposed at embedded://network endpoint.

- Define network/v1alpha1 API spec with ports, routing levels, and
  Kubernetes provider hints
- Implement Kubernetes store with create, get, list, and delete operations
  plus label-based filtering
- Add validation layer for routing levels, node ports, and service type
  constraints
- Wire HTTP handler and health checks into embedded server bundles
- Add NATS monitoring for LoadBalancer Services publishing status events
  to dcm.network when external IPs are assigned (PENDING → READY →
DELETED)
- Implement StatusMonitor with Service informer, debouncer (500ms), and
  graceful shutdown

Include unit and integration tests covering Service lifecycle, error
cases and monitoring

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Paweł Waresiak <pwaresia@redhat.com>
@gpb88
gpb88 force-pushed the import-network-sp branch from aa1da5c to e6afbb6 Compare September 7, 2026 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant