Skip to content
This repository was archived by the owner on Sep 9, 2026. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .claude/rules/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@ Config via environment variables from a Secret (see `config/samples/osac-config-
- `OSAC_CLUSTER_ORDER_NAMESPACE`, `OSAC_COMPUTE_INSTANCE_NAMESPACE`
- `OSAC_TENANT_NAMESPACE`, `OSAC_NETWORKING_NAMESPACE`

## Networking Managers

- `OSAC_NETWORK_CLASS_SYNC_INTERVAL` — periodic full resync interval for NetworkClass
capabilities intersection, in addition to the immediate resync triggered by manager
ConfigMap changes (default: 5m). Only active when the fulfillment-service gRPC
connection is configured.
Comment on lines +27 to +28

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Document the namespace prerequisite too.

This entry states the sync is "Only active when the fulfillment-service gRPC connection is configured." The actual gate in cmd/main.go also requires a non-empty networking namespace (grpcConn != nil && networkingNamespace != ""). Mention both prerequisites so operators do not troubleshoot a silently inactive sync when only the gRPC connection is set.

📝 Proposed doc fix
 - `OSAC_NETWORK_CLASS_SYNC_INTERVAL` — periodic full resync interval for NetworkClass
   capabilities intersection, in addition to the immediate resync triggered by manager
-  ConfigMap changes (default: 5m). Only active when the fulfillment-service gRPC
-  connection is configured.
+  ConfigMap changes (default: 5m). Only active when the fulfillment-service gRPC
+  connection and `OSAC_NETWORKING_NAMESPACE` are both configured.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
ConfigMap changes (default: 5m). Only active when the fulfillment-service gRPC
connection is configured.
ConfigMap changes (default: 5m). Only active when the fulfillment-service gRPC
connection and `OSAC_NETWORKING_NAMESPACE` are both configured.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.claude/rules/configuration.md around lines 27 - 28, Update the ConfigMap
changes entry to state that synchronization is active only when both the
fulfillment-service gRPC connection is configured and the networking namespace
is non-empty, matching the gate in cmd/main.go.


## Controller Enable Flags

- `OSAC_ENABLE_CLUSTER_CONTROLLER` / `--enable-cluster-controller`
Expand Down
4 changes: 4 additions & 0 deletions charts/operator/templates/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ spec:
- name: OSAC_AAP_TEMPLATE_PREFIX
value: {{ .Values.aap.templatePrefix | quote }}
{{- end }}
{{- if .Values.networkManagers.capabilitiesSyncInterval }}
- name: OSAC_NETWORK_CLASS_SYNC_INTERVAL
value: {{ .Values.networkManagers.capabilitiesSyncInterval | quote }}
{{- end }}
- name: OSAC_FULFILLMENT_SERVER_ADDRESS
value: {{ .Values.fulfillment.serverAddress | default "fulfillment-internal-api:8001" | quote }}
- name: OSAC_FULFILLMENT_TOKEN_FILE
Expand Down
4 changes: 2 additions & 2 deletions charts/operator/templates/network-managers.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ metadata:
namespace: {{ $.Release.Namespace }}
labels:
{{- include "osac-operator.labels" $ | nindent 4 }}
osac.openshift.io/network/fabric-manager: "true"
osac.openshift.io/network-fabric-manager: "true"
data:
name: {{ $name | quote }}
{{- if $mgr.description }}
Expand All @@ -48,7 +48,7 @@ metadata:
namespace: {{ $.Release.Namespace }}
labels:
{{- include "osac-operator.labels" $ | nindent 4 }}
osac.openshift.io/network/k8s-manager: "true"
osac.openshift.io/network-k8s-manager: "true"
data:
name: {{ $name | quote }}
{{- if $mgr.description }}
Expand Down
6 changes: 5 additions & 1 deletion charts/operator/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,13 @@ tenants: []
# --- Network Manager Registration ---
# ConfigMaps that register networking managers for the two-manager model.
# The operator discovers managers by selecting ConfigMaps with labels
# osac.openshift.io/network/fabric-manager or osac.openshift.io/network/k8s-manager.
# osac.openshift.io/network-fabric-manager or osac.openshift.io/network-k8s-manager.
networkManagers:
enabled: false
# capabilitiesSyncInterval controls how often the operator recomputes NetworkClass
# capabilities from the fabric/k8s manager ConfigMaps (in addition to reacting
# immediately to ConfigMap changes).
capabilitiesSyncInterval: "5m"
fabricManagers:
netris:
enabled: false
Expand Down
44 changes: 44 additions & 0 deletions cmd/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ import (
"github.com/osac-project/osac-operator/internal/controller"
"github.com/osac-project/osac-operator/internal/migrations"
"github.com/osac-project/osac-operator/pkg/aap"
"github.com/osac-project/osac-operator/pkg/dispatcher"
"github.com/osac-project/osac-operator/pkg/networkmanager"
"github.com/osac-project/osac-operator/pkg/provisioning"
// +kubebuilder:scaffold:imports
)
Expand Down Expand Up @@ -102,6 +104,9 @@ const (
// Job history configuration
envMaxJobHistory = "OSAC_MAX_JOB_HISTORY"

// NetworkClass capabilities sync configuration
envNetworkClassSyncInterval = "OSAC_NETWORK_CLASS_SYNC_INTERVAL"

// Tenant configuration
envTenantNamespace = "OSAC_TENANT_NAMESPACE"

Expand All @@ -117,6 +122,10 @@ const (
envEnableBareMetalInstanceController = "OSAC_ENABLE_BAREMETAL_INSTANCE_CONTROLLER"

remoteClusterName = "remote"

// defaultNetworkClassSyncInterval is the fallback periodic resync interval for
// NetworkClass capabilities when OSAC_NETWORK_CLASS_SYNC_INTERVAL is unset.
defaultNetworkClassSyncInterval = 5 * time.Minute
)

// controllerFlags holds the enable flags for all controllers.
Expand Down Expand Up @@ -495,6 +504,14 @@ func setupNetworkingControllers(
return fmt.Errorf("externalip attachment provider: %w", err)
}

if grpcConn != nil && networkingNamespace != "" {
if err := setupNetworkClassCapabilitiesController(
mgr, localMgr, grpcConn, networkingNamespace,
); err != nil {
return err
}
}

if err := setupVirtualNetworkControllers(
mgr, localMgr, grpcConn, networkingNamespace,
networkingProvider, statusPollInterval, maxJobHistory, targetCluster,
Expand Down Expand Up @@ -541,6 +558,33 @@ func setupNetworkingControllers(
return nil
}

// setupNetworkClassCapabilitiesController registers the NetworkClass capabilities
// reconciler (ConfigMap-triggered) and its periodic resync runnable. Both require the
// gRPC connection and a manager resolver built from ConfigMap-based discovery, so this
// is only called when grpcConn is set and a networking namespace is configured.
func setupNetworkClassCapabilitiesController(
mgr mcmanager.Manager, localMgr ctrl.Manager, grpcConn *grpc.ClientConn, networkingNamespace string,
) error {
disc, err := networkmanager.NewDiscovery(localMgr.GetClient(), networkingNamespace)
if err != nil {
return fmt.Errorf("network manager discovery: %w", err)
}
resolver := dispatcher.NewResolver(privatev1.NewNetworkClassesClient(grpcConn), disc)

ncReconciler := controller.NewNetworkClassCapabilitiesReconciler(
privatev1.NewNetworkClassesClient(grpcConn), resolver, networkingNamespace,
)
Comment on lines +572 to +576

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reuse a single NetworkClassesClient instance.

privatev1.NewNetworkClassesClient(grpcConn) is called twice: once for resolver and once for ncReconciler. Both wrap the same grpcConn, so a single client instance can be shared.

♻️ Proposed refactor
-	resolver := dispatcher.NewResolver(privatev1.NewNetworkClassesClient(grpcConn), disc)
+	ncClient := privatev1.NewNetworkClassesClient(grpcConn)
+	resolver := dispatcher.NewResolver(ncClient, disc)
 
 	ncReconciler := controller.NewNetworkClassCapabilitiesReconciler(
-		privatev1.NewNetworkClassesClient(grpcConn), resolver, networkingNamespace,
+		ncClient, resolver, networkingNamespace,
 	)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
resolver := dispatcher.NewResolver(privatev1.NewNetworkClassesClient(grpcConn), disc)
ncReconciler := controller.NewNetworkClassCapabilitiesReconciler(
privatev1.NewNetworkClassesClient(grpcConn), resolver, networkingNamespace,
)
ncClient := privatev1.NewNetworkClassesClient(grpcConn)
resolver := dispatcher.NewResolver(ncClient, disc)
ncReconciler := controller.NewNetworkClassCapabilitiesReconciler(
ncClient, resolver, networkingNamespace,
)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/main.go` around lines 572 - 576, Instantiate
privatev1.NewNetworkClassesClient(grpcConn) once before creating the resolver,
assign it to a shared client variable, and pass that same instance to both
dispatcher.NewResolver and controller.NewNetworkClassCapabilitiesReconciler.

if err := ncReconciler.SetupWithManager(mgr); err != nil {
return fmt.Errorf("networkclass capabilities controller: %w", err)
}

syncInterval := helpers.GetEnvWithDefault(envNetworkClassSyncInterval, defaultNetworkClassSyncInterval)
if err := localMgr.Add(controller.NewNetworkClassCapabilitiesSyncRunnable(ncReconciler, syncInterval)); err != nil {
return fmt.Errorf("networkclass capabilities sync runnable: %w", err)
}
Comment on lines +581 to +584

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Validate that the sync interval is positive before use.

helpers.GetEnvWithDefault(envNetworkClassSyncInterval, defaultNetworkClassSyncInterval) accepts a validator argument, but none is passed here. time.ParseDuration accepts "0s" or negative durations like "-5m" as valid parses, so an operator setting OSAC_NETWORK_CLASS_SYNC_INTERVAL to zero or negative reaches NewNetworkClassCapabilitiesSyncRunnable unchecked. time.NewTicker panics for a non-positive duration in Start() (internal/controller/networkclass_capabilities_controller.go, line 254), crashing the manager process from a single misconfigured env var.

Add a positive-duration validator to the GetEnvWithDefault call.

🛡️ Proposed fix
-	syncInterval := helpers.GetEnvWithDefault(envNetworkClassSyncInterval, defaultNetworkClassSyncInterval)
+	syncInterval := helpers.GetEnvWithDefault(
+		envNetworkClassSyncInterval, defaultNetworkClassSyncInterval,
+		func(d time.Duration) bool { return d > 0 },
+	)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
syncInterval := helpers.GetEnvWithDefault(envNetworkClassSyncInterval, defaultNetworkClassSyncInterval)
if err := localMgr.Add(controller.NewNetworkClassCapabilitiesSyncRunnable(ncReconciler, syncInterval)); err != nil {
return fmt.Errorf("networkclass capabilities sync runnable: %w", err)
}
syncInterval := helpers.GetEnvWithDefault(
envNetworkClassSyncInterval, defaultNetworkClassSyncInterval,
func(d time.Duration) bool { return d > 0 },
)
if err := localMgr.Add(controller.NewNetworkClassCapabilitiesSyncRunnable(ncReconciler, syncInterval)); err != nil {
return fmt.Errorf("networkclass capabilities sync runnable: %w", err)
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/main.go` around lines 581 - 584, Update the GetEnvWithDefault call for
envNetworkClassSyncInterval to pass a validator that accepts only strictly
positive durations, rejecting zero and negative values before syncInterval
reaches NewNetworkClassCapabilitiesSyncRunnable. Reuse the existing duration
parsing/validation conventions or helper available in the codebase.

return nil
}

func setupVirtualNetworkControllers(
mgr mcmanager.Manager, localMgr ctrl.Manager, grpcConn *grpc.ClientConn,
networkingNamespace string, provider provisioning.ProvisioningProvider,
Expand Down
4 changes: 2 additions & 2 deletions config/samples/network-manager-fabric-netris.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@
# virtual network segments, security groups, external IPs, and NAT gateways.
#
# The operator discovers fabric managers by selecting ConfigMaps with the label
# osac.openshift.io/network/fabric-manager. The data.name value is referenced
# osac.openshift.io/network-fabric-manager. The data.name value is referenced
# by NetworkClass.fabricManager.
apiVersion: v1
kind: ConfigMap
metadata:
name: osac-network-fabric-manager-netris
namespace: osac
labels:
osac.openshift.io/network/fabric-manager: "true"
osac.openshift.io/network-fabric-manager: "true"
data:
# Unique identifier for this manager. Must match the fabricManager field
# on NetworkClass resources that use this backend.
Expand Down
118 changes: 118 additions & 0 deletions internal/controller/dispatcher_resolver_helpers_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/*
Copyright 2026.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package controller

import (
"context"
"fmt"

"google.golang.org/grpc"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

privatev1 "github.com/osac-project/osac-operator/internal/api/osac/private/v1"
"github.com/osac-project/osac-operator/pkg/networkmanager"
)

// stubNetworkClassesClient is a minimal test double for privatev1.NetworkClassesClient
// used by controller tests exercising manager-resolution paths (e.g. the OSAC-1755
// capabilities reconciler). Embedding the interface (nil) satisfies methods that
// aren't stubbed for a given test, which are not expected to be called.
type stubNetworkClassesClient struct {
privatev1.NetworkClassesClient
getFunc func(ctx context.Context, in *privatev1.NetworkClassesGetRequest, opts ...grpc.CallOption) (*privatev1.NetworkClassesGetResponse, error)
listFunc func(ctx context.Context, in *privatev1.NetworkClassesListRequest, opts ...grpc.CallOption) (*privatev1.NetworkClassesListResponse, error)
updateFunc func(ctx context.Context, in *privatev1.NetworkClassesUpdateRequest, opts ...grpc.CallOption) (*privatev1.NetworkClassesUpdateResponse, error)
}

func (s *stubNetworkClassesClient) Get(
ctx context.Context, in *privatev1.NetworkClassesGetRequest, opts ...grpc.CallOption,
) (*privatev1.NetworkClassesGetResponse, error) {
if s.getFunc != nil {
return s.getFunc(ctx, in, opts...)
}
return s.NetworkClassesClient.Get(ctx, in, opts...)
}

func (s *stubNetworkClassesClient) List(
ctx context.Context, in *privatev1.NetworkClassesListRequest, opts ...grpc.CallOption,
) (*privatev1.NetworkClassesListResponse, error) {
if s.listFunc != nil {
return s.listFunc(ctx, in, opts...)
}
return s.NetworkClassesClient.List(ctx, in, opts...)
}

func (s *stubNetworkClassesClient) Update(
ctx context.Context, in *privatev1.NetworkClassesUpdateRequest, opts ...grpc.CallOption,
) (*privatev1.NetworkClassesUpdateResponse, error) {
if s.updateFunc != nil {
return s.updateFunc(ctx, in, opts...)
}
return s.NetworkClassesClient.Update(ctx, in, opts...)
}

// newListingNetworkClassClient returns a stub NetworkClassesClient whose List method
// returns the given NetworkClasses and whose Get method finds one by ID among them.
// Update appends to the captured updates slice so tests can assert on what was saved.
func newListingNetworkClassClient(
items []*privatev1.NetworkClass, updates *[]*privatev1.NetworkClass,
) *stubNetworkClassesClient {
return &stubNetworkClassesClient{
listFunc: func(_ context.Context, _ *privatev1.NetworkClassesListRequest, _ ...grpc.CallOption) (*privatev1.NetworkClassesListResponse, error) {
return &privatev1.NetworkClassesListResponse{Items: items}, nil
},
getFunc: func(_ context.Context, in *privatev1.NetworkClassesGetRequest, _ ...grpc.CallOption) (*privatev1.NetworkClassesGetResponse, error) {
for _, item := range items {
if item.GetId() == in.GetId() {
return &privatev1.NetworkClassesGetResponse{Object: item}, nil
}
}
return nil, fmt.Errorf("network class %q not found", in.GetId())
},
updateFunc: func(_ context.Context, in *privatev1.NetworkClassesUpdateRequest, _ ...grpc.CallOption) (*privatev1.NetworkClassesUpdateResponse, error) {
*updates = append(*updates, in.GetObject())
return &privatev1.NetworkClassesUpdateResponse{Object: in.GetObject()}, nil
},
}
}

// newFabricManagerConfigMap builds a fabric-manager registration ConfigMap for tests
// that exercise dispatcher-backed manager discovery.
func newFabricManagerConfigMap(name, namespace, managerName string) *corev1.ConfigMap {
return newManagerConfigMap(name, namespace, managerName, networkmanager.LabelFabricManager, "ipv4")
}

// newK8sManagerConfigMap builds a k8s-manager registration ConfigMap for tests that
// exercise dispatcher-backed manager discovery.
func newK8sManagerConfigMap(name, namespace, managerName, capabilities string) *corev1.ConfigMap {
return newManagerConfigMap(name, namespace, managerName, networkmanager.LabelK8sManager, capabilities)
}

func newManagerConfigMap(name, namespace, managerName, labelKey, capabilities string) *corev1.ConfigMap {
return &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace,
Labels: map[string]string{labelKey: "true"},
},
Data: map[string]string{
"name": managerName,
"capabilities": capabilities,
},
}
}
Loading
Loading