-
Notifications
You must be signed in to change notification settings - Fork 55
OSAC-1460: Wire dispatcher into controllers behind feature gate #126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
bebaa92
bda68d5
c47f3d6
4bb5842
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| --- | ||
| # cudn_net delegates SecurityGroup enforcement to the standalone network_policy | ||
| # role (NetworkPolicy-based), which is reusable across any K8s-based NetworkClass. | ||
| # See README.md for the rationale. | ||
|
|
||
| - name: Delegate SecurityGroup creation to network_policy role | ||
| ansible.builtin.include_role: | ||
| name: osac.templates.network_policy | ||
| tasks_from: create_security_group |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| --- | ||
| # cudn_net delegates SecurityGroup enforcement to the standalone network_policy | ||
| # role (NetworkPolicy-based), which is reusable across any K8s-based NetworkClass. | ||
| # See README.md for the rationale. | ||
|
|
||
| - name: Delegate SecurityGroup deletion to network_policy role | ||
| ansible.builtin.include_role: | ||
| name: osac.templates.network_policy | ||
| tasks_from: delete_security_group |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,7 +68,7 @@ tenants: [] | |
| # The operator discovers managers by selecting ConfigMaps with labels | ||
| # osac.openshift.io/network-fabric-manager or osac.openshift.io/network-k8s-manager. | ||
| networkManagers: | ||
| enabled: false | ||
| enabled: true | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] scope-mismatch PR title says 'behind feature gate' but networkManagers.enabled is flipped from false to true. While controller behavior is still gated by runtime prerequisites (gRPC + namespace), the infrastructure change enables the two-manager model by default for qualifying deployments. Suggested fix: Either revert the default to false or update PR title/description to accurately describe enablement. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] feature-gate-default-change The networkManagers.enabled default is changed from false to true, making it active for all deployments on upgrade. The new cudn_net fabric manager entry is also enabled: true, so upgrading creates a new ConfigMap and triggers NetworkClass capability reconciliation. While the downstream impact is limited (read-reconcile loop, no unsafe mutations), changing a Helm default warrants documentation. Suggested fix: Document in the PR description or release notes that this is an intentional default change, or keep false as the default with environment-specific overrides. |
||
| # capabilitiesSyncInterval controls how often the operator recomputes NetworkClass | ||
| # capabilities from the fabric/k8s manager ConfigMaps (in addition to reacting | ||
| # immediately to ConfigMap changes). | ||
|
|
@@ -80,4 +80,11 @@ networkManagers: | |
| Netris SDN controller for physical fabric management. | ||
| Manages VLAN/VxLAN segments, ACLs, public IP allocation, and NAT gateways. | ||
| capabilities: "ipv4" | ||
| cudn_net: | ||
| enabled: true | ||
| description: >- | ||
| CUDN-based isolated networking (ClusterUserDefinedNetwork). Self-contained | ||
| VirtualNetwork/Subnet provisioning with no separate physical fabric to | ||
| bridge into — used as the platform default NetworkClass. | ||
| capabilities: "ipv4,ipv6,dualStack" | ||
| k8sManagers: {} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| /* | ||
| 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" | ||
| "errors" | ||
| "fmt" | ||
|
|
||
| "github.com/osac-project/osac/osac-operator/pkg/dispatcher" | ||
| ) | ||
|
|
||
| // resolveImplementationStrategy determines the value a networking controller should | ||
| // write into osacImplementationStrategyAnnotation for AAP playbook selection. | ||
| // | ||
| // When resolver is configured (non-nil, i.e. the gRPC connection and networking | ||
| // namespace needed for manager discovery are set up) and networkClassID is non-empty, | ||
| // it resolves the NetworkClass's fabric manager via the dispatcher package (the | ||
| // "dispatcher path") and returns the resolved manager's name. If the NetworkClass has | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] doc-style Doc comment has orphaned line break mid-sentence splitting 'Any other resolution error'. Suggested fix: Join the broken sentence. |
||
| // neither a fabricManager nor a k8sManager set yet (dispatcher.ErrNoManagerConfigured), | ||
| // it falls back to legacyStrategy (the "implementation_strategy annotation path"). Any | ||
| // other resolution error (e.g. a fabricManager referencing an unregistered manager | ||
| // ConfigMap) is returned to the caller as a real reconcile error, since that indicates | ||
| // a misconfiguration rather than an expected pre-migration state. | ||
| // | ||
| // When resolver is nil or networkClassID is empty, dispatch is skipped entirely and | ||
| // legacyStrategy is returned unchanged — this is the behavior for deployments without | ||
| // the two-manager model configured, or resources using the platform-default | ||
| // NetworkClass (which has no ID to resolve against). | ||
| func resolveImplementationStrategy( | ||
| ctx context.Context, | ||
| resolver *dispatcher.Resolver, | ||
| kind string, | ||
| networkClassID string, | ||
| legacyStrategy string, | ||
| ) (string, error) { | ||
| if resolver == nil || networkClassID == "" { | ||
| return legacyStrategy, nil | ||
| } | ||
|
|
||
| plan, err := dispatcher.NewDispatcher(resolver).Dispatch(ctx, kind, networkClassID) | ||
| switch { | ||
| case err == nil: | ||
| target := plan.FabricTarget() | ||
| if target == nil { | ||
| // Defensive: every entry in the dispatch table includes the fabric role, | ||
| // so this should not happen in practice. | ||
| return legacyStrategy, nil | ||
| } | ||
| return target.Manager.Name, nil | ||
| case errors.Is(err, dispatcher.ErrNoManagerConfigured): | ||
| return legacyStrategy, nil | ||
| default: | ||
| return "", fmt.Errorf("resolving dispatch plan for %s (networkClass %q): %w", kind, networkClassID, err) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ import ( | |
| mcreconcile "sigs.k8s.io/multicluster-runtime/pkg/reconcile" | ||
|
|
||
| "github.com/osac-project/osac/osac-operator/api/v1alpha1" | ||
| "github.com/osac-project/osac/osac-operator/pkg/dispatcher" | ||
| "github.com/osac-project/osac/osac-operator/pkg/provisioning" | ||
| ) | ||
|
|
||
|
|
@@ -53,6 +54,10 @@ type SecurityGroupReconciler struct { | |
| StatusPollInterval time.Duration | ||
| MaxJobHistory int | ||
| targetCluster mc.ClusterName | ||
| // Resolver resolves a NetworkClass to its registered managers. Nil when the | ||
| // two-manager model isn't configured (no gRPC connection / networking namespace), | ||
| // in which case the controller always uses the legacy implementation-strategy path. | ||
| Resolver *dispatcher.Resolver | ||
| } | ||
|
|
||
| // NewSecurityGroupReconciler creates a new reconciler for SecurityGroup resources. | ||
|
|
@@ -63,6 +68,7 @@ func NewSecurityGroupReconciler( | |
| statusPollInterval time.Duration, | ||
| maxJobHistory int, | ||
| targetCluster mc.ClusterName, | ||
| resolver *dispatcher.Resolver, | ||
| ) *SecurityGroupReconciler { | ||
| if mgr == nil { | ||
| panic("mgr must not be nil") | ||
|
|
@@ -83,6 +89,7 @@ func NewSecurityGroupReconciler( | |
| StatusPollInterval: statusPollInterval, | ||
| MaxJobHistory: maxJobHistory, | ||
| targetCluster: targetCluster, | ||
| Resolver: resolver, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -149,10 +156,36 @@ func (r *SecurityGroupReconciler) handleUpdate(ctx context.Context, sg *v1alpha1 | |
| sg.Status.Phase = v1alpha1.SecurityGroupPhaseProgressing | ||
| } | ||
|
|
||
| // Read implementation strategy from spec (set by fulfillment-service), fall back to default | ||
| implementationStrategy := sg.Spec.ImplementationStrategy | ||
| if implementationStrategy == "" { | ||
| implementationStrategy = defaultSecurityGroupImplementationStrategy | ||
| // Look up the parent VirtualNetwork's NetworkClass to check whether it has a | ||
| // fabricManager registered (dispatcher path). | ||
| var networkClassID string | ||
| vnetList := &v1alpha1.VirtualNetworkList{} | ||
| if err := r.List(ctx, vnetList, | ||
| client.InNamespace(sg.Namespace), | ||
| client.MatchingLabels{osacVirtualNetworkIDLabel: sg.Spec.VirtualNetwork}, | ||
| ); err != nil { | ||
| return ctrl.Result{}, err | ||
| } else if len(vnetList.Items) > 1 { | ||
| return ctrl.Result{}, fmt.Errorf( | ||
| "expected exactly one parent VirtualNetwork with uuid %q but found %d", | ||
| sg.Spec.VirtualNetwork, len(vnetList.Items)) | ||
| } else if len(vnetList.Items) == 1 { | ||
| networkClassID = vnetList.Items[0].Spec.NetworkClass | ||
| } else { | ||
| log.Info("parent VirtualNetwork not found, using legacy implementation strategy", "uuid", sg.Spec.VirtualNetwork) | ||
| } | ||
|
|
||
| // Read implementation strategy from spec (set by fulfillment-service), fall back to | ||
| // default. This is the legacy value; resolveImplementationStrategy below only uses | ||
| // it when the dispatcher path isn't available (see doc comment). | ||
| legacyStrategy := sg.Spec.ImplementationStrategy | ||
| if legacyStrategy == "" { | ||
| legacyStrategy = defaultSecurityGroupImplementationStrategy | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] logic-error SecurityGroup controller silently falls back to legacy strategy when multiple parent VirtualNetworks match the UUID label (len > 1), while the Subnet controller errors. This masks data corruption and could provision the SecurityGroup against the wrong NetworkClass. Suggested fix: Add an ambiguity guard matching the Subnet controller pattern — error on len > 1. |
||
| } | ||
|
|
||
| implementationStrategy, err := resolveImplementationStrategy(ctx, r.Resolver, "SecurityGroup", networkClassID, legacyStrategy) | ||
| if err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| // Add implementation-strategy annotation if not present or different | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[medium] scope-creep
Removing k8s_manager: cudn_localnet changes NetworkClass registration metadata beyond dispatcher wiring scope. Could conflict with existing records if fulfillment-service enforces k8s_manager immutability (NC-VAL-07).
Suggested fix: Confirm removal won't conflict with existing NetworkClass records. Consider splitting into OSAC-1511.