From 83ff55584820df4c59e1585e70a6a5183919c1d0 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Thu, 27 Aug 2026 13:31:08 -0500 Subject: [PATCH 1/4] feat: read locations behind a source flag Compute reads two location facts: the cities a project may place workloads in, and the location a cell serves. Both are served today by network-services-operator and are moving to the locations service, which projects a Location into a project's control plane alongside the legacy LocationBinding. A new locationSource config field selects which group is read. It defaults to NetworkServices, so a deployment that does not set it reads exactly what it reads today. It governs reads only; nothing compute writes changes with it. Key changes: - Add internal/locations, which resolves placement locations and serving locations from either group and collapses the three duplicated city-code lookups into one call - Read the locations service through unstructured: its module path does not yet resolve, so a typed dependency cannot be pinned. A kind that is not installed reads as no locations rather than failing the caller - Route the workload webhook, the workload reconciler and the deployment reconciler through the accessor, including the serving location watch - Reject an unknown source when the server config loads, rather than on every reconcile --- cmd/main.go | 7 +- cmd/main_test.go | 34 +++ config/components/controller_rbac/role.yaml | 9 + internal/config/config.go | 14 ++ internal/config/config_test.go | 29 +++ internal/config/zz_generated.defaults.go | 1 + internal/controller/location_source_test.go | 158 ++++++++++++ internal/controller/workload_controller.go | 28 +-- .../controller/workload_controller_test.go | 22 +- .../workloaddeployment_controller.go | 34 ++- internal/locations/locations.go | 234 ++++++++++++++++++ internal/locations/locations_test.go | 212 ++++++++++++++++ internal/webhook/v1alpha/workload_webhook.go | 53 ++-- 13 files changed, 771 insertions(+), 64 deletions(-) create mode 100644 internal/controller/location_source_test.go create mode 100644 internal/locations/locations.go create mode 100644 internal/locations/locations_test.go diff --git a/cmd/main.go b/cmd/main.go index cb59e59e..e79e61a3 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -350,6 +350,7 @@ func main() { if enableManagementControllers { if err = (&controller.WorkloadReconciler{ NetworkingEnabled: features.FeatureGate.Enabled(features.NetworkingIntegration), + LocationSource: serverConfig.LocationSource, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Workload") os.Exit(1) @@ -374,6 +375,7 @@ func main() { } if err = (&controller.WorkloadDeploymentReconciler{ NetworkingEnabled: features.FeatureGate.Enabled(features.NetworkingIntegration), + LocationSource: serverConfig.LocationSource, }).SetupWithManager(mgr, wdOpts); err != nil { setupLog.Error(err, "unable to create controller", "controller", "WorkloadDeployment") os.Exit(1) @@ -457,7 +459,7 @@ func main() { } if serverConfig.WebhookServer != nil { - if err = computev1alphawebhooks.SetupWorkloadWebhookWithManager(mgr); err != nil { + if err = computev1alphawebhooks.SetupWorkloadWebhookWithManager(mgr, serverConfig.LocationSource); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "Workload") os.Exit(1) } @@ -803,6 +805,9 @@ func loadServerConfig(path string) (config.WorkloadOperator, error) { if err := runtime.DecodeInto(codecs.UniversalDecoder(), configData, &serverConfig); err != nil { return serverConfig, fmt.Errorf("unable to decode server config: %w", err) } + if _, err := serverConfig.LocationSource.Resolve(); err != nil { + return serverConfig, fmt.Errorf("invalid server config: %w", err) + } return serverConfig, nil } diff --git a/cmd/main_test.go b/cmd/main_test.go index c0053469..466e961a 100644 --- a/cmd/main_test.go +++ b/cmd/main_test.go @@ -3,10 +3,15 @@ package main import ( + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" multiclusterproviders "go.miloapis.com/milo/pkg/multicluster-runtime" + + "go.datum.net/compute/internal/locations" ) // TestComputeWatchProviderClaims is the #171 guard: quota enforcement (and thus @@ -21,3 +26,32 @@ func TestComputeWatchProviderClaims(t *testing.T) { assert.False(t, computeWatchProviderClaims(multiclusterproviders.ProviderKind), "non-milo modes disable quota") } + +// TestLoadServerConfig_LocationSource covers the startup guard: a config that +// names an unknown location source fails to load rather than surfacing later +// on every reconcile. +func TestLoadServerConfig_LocationSource(t *testing.T) { + write := func(t *testing.T, body string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "config.yaml") + require.NoError(t, os.WriteFile(path, []byte(body), 0o600)) + return path + } + + const header = `apiVersion: apiserver.config.datumapis.com/v1alpha1 +kind: WorkloadOperator +metricsServer: + bindAddress: "0" +` + + cfg, err := loadServerConfig(write(t, header)) + require.NoError(t, err) + assert.Equal(t, locations.SourceNetworkServices, cfg.LocationSource) + + cfg, err = loadServerConfig(write(t, header+"locationSource: Locations\n")) + require.NoError(t, err) + assert.Equal(t, locations.SourceLocations, cfg.LocationSource) + + _, err = loadServerConfig(write(t, header+"locationSource: Nonsense\n")) + require.Error(t, err) +} diff --git a/config/components/controller_rbac/role.yaml b/config/components/controller_rbac/role.yaml index acedcc22..0d6d6ed3 100644 --- a/config/components/controller_rbac/role.yaml +++ b/config/components/controller_rbac/role.yaml @@ -76,6 +76,15 @@ rules: verbs: - create - patch +- apiGroups: + - locations.miloapis.com + resources: + - locations + - servinglocations + verbs: + - get + - list + - watch - apiGroups: - networking.datumapis.com resources: diff --git a/internal/config/config.go b/internal/config/config.go index 46bebe59..4b832b50 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -19,6 +19,7 @@ import ( metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" "sigs.k8s.io/controller-runtime/pkg/webhook" + "go.datum.net/compute/internal/locations" multiclusterproviders "go.miloapis.com/milo/pkg/multicluster-runtime" ) @@ -42,6 +43,19 @@ type WorkloadOperator struct { // ReferencedData configures the ReferencedDataController. ReferencedData ReferencedDataConfig `json:"referencedData,omitempty"` + + // LocationSource names the API group locations are read from. Use + // "NetworkServices" for networking.datumapis.com LocationBindings and + // ServingLocations, or "Locations" for the dedicated locations.miloapis.com + // service. It governs reads only; nothing about what compute writes changes + // with it. Defaults to "NetworkServices". + LocationSource locations.Source `json:"locationSource,omitempty"` +} + +func SetDefaults_WorkloadOperator(obj *WorkloadOperator) { + if obj.LocationSource == "" { + obj.LocationSource = locations.SourceNetworkServices + } } // +k8s:deepcopy-gen=true diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 5a7a3cee..61c4ca33 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -7,6 +7,8 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" + + "go.datum.net/compute/internal/locations" ) func decode(t *testing.T, data string) *WorkloadOperator { @@ -122,3 +124,30 @@ users: t.Error("QuotaRestConfig() = nil, want non-nil when file exists") } } + +// TestLocationSource_DefaultsToNetworkServices is the safety property behind +// the flag: a config that does not set it reads what it reads today. +func TestLocationSource_DefaultsToNetworkServices(t *testing.T) { + cfg := decode(t, ` +apiVersion: apiserver.config.datumapis.com/v1alpha1 +kind: WorkloadOperator +metricsServer: + bindAddress: "0" +`) + if cfg.LocationSource != locations.SourceNetworkServices { + t.Errorf("LocationSource = %q, want %q", cfg.LocationSource, locations.SourceNetworkServices) + } +} + +func TestLocationSource_Explicit(t *testing.T) { + cfg := decode(t, ` +apiVersion: apiserver.config.datumapis.com/v1alpha1 +kind: WorkloadOperator +metricsServer: + bindAddress: "0" +locationSource: Locations +`) + if cfg.LocationSource != locations.SourceLocations { + t.Errorf("LocationSource = %q, want %q", cfg.LocationSource, locations.SourceLocations) + } +} diff --git a/internal/config/zz_generated.defaults.go b/internal/config/zz_generated.defaults.go index fed95896..0f8a363b 100644 --- a/internal/config/zz_generated.defaults.go +++ b/internal/config/zz_generated.defaults.go @@ -18,6 +18,7 @@ func RegisterDefaults(scheme *runtime.Scheme) error { } func SetObjectDefaults_WorkloadOperator(in *WorkloadOperator) { + SetDefaults_WorkloadOperator(in) SetDefaults_MetricsServerConfig(&in.MetricsServer) SetDefaults_TLSConfig(&in.MetricsServer.TLS) if in.WebhookServer != nil { diff --git a/internal/controller/location_source_test.go b/internal/controller/location_source_test.go new file mode 100644 index 00000000..9e7ccf91 --- /dev/null +++ b/internal/controller/location_source_test.go @@ -0,0 +1,158 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package controller + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + computev1alpha "go.datum.net/compute/api/v1alpha" + "go.datum.net/compute/internal/locations" +) + +var locationsServiceGroupVersion = schema.GroupVersion{Group: "locations.miloapis.com", Version: "v1alpha1"} + +// newLocationsServiceScheme returns the networking scheme with the locations +// service kinds registered as unstructured, which is how compute reads them. +func newLocationsServiceScheme() *runtime.Scheme { + s := newNetworkingScheme() + for _, kind := range []string{"Location", "ServingLocation"} { + s.AddKnownTypeWithName(locationsServiceGroupVersion.WithKind(kind), &unstructured.Unstructured{}) + s.AddKnownTypeWithName(locationsServiceGroupVersion.WithKind(kind+"List"), &unstructured.UnstructuredList{}) + } + return s +} + +func newLocationsServiceObject(kind, name, cityCode string) *unstructured.Unstructured { + object := &unstructured.Unstructured{Object: map[string]any{ + "metadata": map[string]any{"name": name}, + "spec": map[string]any{ + "topology": map[string]any{locations.TopologyCityCodeKey: cityCode}, + }, + }} + object.SetGroupVersionKind(locationsServiceGroupVersion.WithKind(kind)) + return object +} + +// TestGetDeploymentsForWorkload_LocationsSource verifies that a workload placed +// in a city is deployed there when the city is only known to the locations +// service, which is what the Locations source reads. +func TestGetDeploymentsForWorkload_LocationsSource(t *testing.T) { + t.Parallel() + + workload := &computev1alpha.Workload{ + ObjectMeta: metav1.ObjectMeta{ + Name: rdTestWorkloadName, + Namespace: testDefaultNamespace, + UID: types.UID("workload-uid"), + }, + Spec: computev1alpha.WorkloadSpec{ + Placements: []computev1alpha.WorkloadPlacement{ + { + Name: testDefaultPlacement, + CityCodes: []string{locTestCityCode}, + ScaleSettings: computev1alpha.HorizontalScaleSettings{ + MinReplicas: 1, + }, + }, + }, + }, + } + + cl := fake.NewClientBuilder(). + WithScheme(newLocationsServiceScheme()). + WithObjects(newLocationsServiceObject("Location", "dfw", locTestCityCode)). + WithIndex(&computev1alpha.WorkloadDeployment{}, deploymentWorkloadUIDIndex, deploymentWorkloadUIDIndexFunc). + Build() + + r := &WorkloadReconciler{LocationSource: locations.SourceLocations} + + desired, orphaned, err := r.getDeploymentsForWorkload(context.Background(), cl, workload) + require.NoError(t, err) + require.Empty(t, orphaned) + require.Len(t, desired, 1) + assert.Equal(t, locTestCityCode, desired[0].Spec.CityCode) +} + +// TestGetDeploymentsForWorkload_LocationsSourceIgnoresBindings verifies the +// sources do not leak into each other: LocationBindings are invisible to a +// deployment reading the locations service. +func TestGetDeploymentsForWorkload_LocationsSourceIgnoresBindings(t *testing.T) { + t.Parallel() + + workload := &computev1alpha.Workload{ + ObjectMeta: metav1.ObjectMeta{ + Name: rdTestWorkloadName, + Namespace: testDefaultNamespace, + UID: types.UID("workload-uid"), + }, + } + + cl := fake.NewClientBuilder(). + WithScheme(newLocationsServiceScheme()). + WithObjects(newTestLocationBinding("dfw", locTestCityCode)). + WithIndex(&computev1alpha.WorkloadDeployment{}, deploymentWorkloadUIDIndex, deploymentWorkloadUIDIndexFunc). + Build() + + r := &WorkloadReconciler{LocationSource: locations.SourceLocations} + + _, _, err := r.getDeploymentsForWorkload(context.Background(), cl, workload) + require.ErrorContains(t, err, "no locations are registered with the system") +} + +// TestResolveLocation_LocationsSource verifies a cell reads its serving +// location from the locations service when that source is selected. +func TestResolveLocation_LocationsSource(t *testing.T) { + t.Parallel() + + const locationName = "loc-dfw-1" + + cl := fake.NewClientBuilder(). + WithScheme(newLocationsServiceScheme()). + WithObjects( + newLocationsServiceObject("ServingLocation", locationName, locTestCityCode), + // The network services copy must be ignored by this source. + newTestServingLocation("nso-"+locationName, locTestOtherCityCode), + ). + Build() + + deployment := newLocationTestDeployment("test-wd") + + r := &WorkloadDeploymentReconciler{LocationSource: locations.SourceLocations} + result, err := r.resolveLocation(context.Background(), cl) + require.NoError(t, err) + result.evaluate(deployment) + + require.NotNil(t, result.reference) + assert.Equal(t, locationName, result.reference.Name) + assert.Empty(t, result.reason) + assert.False(t, result.blocked) +} + +// TestResolveLocation_NetworkServicesSourceIgnoresLocationsService is the +// safety property behind the flag: a deployment that does not set it reads +// exactly what it reads today. +func TestResolveLocation_NetworkServicesSourceIgnoresLocationsService(t *testing.T) { + t.Parallel() + + cl := fake.NewClientBuilder(). + WithScheme(newLocationsServiceScheme()). + WithObjects(newLocationsServiceObject("ServingLocation", "loc-dfw-1", locTestCityCode)). + Build() + + r := &WorkloadDeploymentReconciler{} + result, err := r.resolveLocation(context.Background(), cl) + require.NoError(t, err) + + assert.Nil(t, result.reference) + assert.Equal(t, computev1alpha.WorkloadDeploymentReasonNoMatchingLocation, result.reason) +} diff --git a/internal/controller/workload_controller.go b/internal/controller/workload_controller.go index c9d8ac98..6bba2365 100644 --- a/internal/controller/workload_controller.go +++ b/internal/controller/workload_controller.go @@ -31,6 +31,7 @@ import ( mcreconcile "sigs.k8s.io/multicluster-runtime/pkg/reconcile" computev1alpha "go.datum.net/compute/api/v1alpha" + "go.datum.net/compute/internal/locations" networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" ) @@ -49,12 +50,18 @@ type WorkloadReconciler struct { // on control planes without the integration, and engaging a watch against a // missing kind wedges the manager. NetworkingEnabled bool + + // LocationSource selects the API group placement locations are read from. + // The zero value reads network services, which is what every deployment + // does today. + LocationSource locations.Source } // +kubebuilder:rbac:groups=compute.datumapis.com,resources=workloads,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=compute.datumapis.com,resources=workloads/status,verbs=get;update;patch // +kubebuilder:rbac:groups=compute.datumapis.com,resources=workloads/finalizers,verbs=update // +kubebuilder:rbac:groups=networking.datumapis.com,resources=networks,verbs=get;list;watch +// +kubebuilder:rbac:groups=locations.miloapis.com,resources=locations,verbs=get;list;watch func (r *WorkloadReconciler) Reconcile(ctx context.Context, req mcreconcile.Request) (ctrl.Result, error) { logger := log.FromContext(ctx) @@ -446,28 +453,21 @@ func (r *WorkloadReconciler) getDeploymentsForWorkload( existingDeployments.Insert(deployment.Name) } - var locations networkingv1alpha.LocationBindingList - if err := upstreamClient.List(ctx, &locations); err != nil { - return nil, nil, fmt.Errorf("failed to list location bindings: %w", err) + placementLocations, err := locations.ListPlacementLocations(ctx, upstreamClient, r.LocationSource) + if err != nil { + return nil, nil, err } - if len(locations.Items) == 0 { + if len(placementLocations) == 0 { return nil, nil, fmt.Errorf("no locations are registered with the system") } + cityCodes := locations.CityCodes(placementLocations) + // Remember this: namespace, name, err := cache.SplitMetaNamespaceKey(key) for _, placement := range workload.Spec.Placements { for _, cityCode := range placement.CityCodes { - foundLocation := false - for _, location := range locations.Items { - locationCityCode, ok := location.Spec.Topology[networkingv1alpha.TopologyCityCodeKey] - if ok && cityCode == locationCityCode { - foundLocation = true - break - } - } - - if !foundLocation { + if !cityCodes.Has(cityCode) { // TODO(jreese) update status condition on placement if no locations are // found. continue diff --git a/internal/controller/workload_controller_test.go b/internal/controller/workload_controller_test.go index d8dbef80..c5a8e5dd 100644 --- a/internal/controller/workload_controller_test.go +++ b/internal/controller/workload_controller_test.go @@ -19,6 +19,18 @@ import ( networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" ) +// newTestLocationBinding builds the projection a project control plane holds +// today for a location it may place workloads at. +func newTestLocationBinding(name, cityCode string) *networkingv1alpha.LocationBinding { + return &networkingv1alpha.LocationBinding{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: networkingv1alpha.LocationBindingSpec{ + LocationRef: corev1.LocalObjectReference{Name: name}, + Topology: map[string]string{networkingv1alpha.TopologyCityCodeKey: cityCode}, + }, + } +} + // makeWorkload builds a Workload with the given generation for use in // reconcileWorkloadStatus unit tests. func makeWorkload(generation int64) *computev1alpha.Workload { @@ -87,7 +99,7 @@ func TestGetDeploymentsForWorkload_InitializesReplicas(t *testing.T) { workload := &computev1alpha.Workload{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-workload", + Name: rdTestWorkloadName, Namespace: testDefaultNamespace, UID: types.UID("workload-uid"), }, @@ -103,13 +115,7 @@ func TestGetDeploymentsForWorkload_InitializesReplicas(t *testing.T) { }, }, } - location := &networkingv1alpha.LocationBinding{ - ObjectMeta: metav1.ObjectMeta{Name: "dfw"}, - Spec: networkingv1alpha.LocationBindingSpec{ - LocationRef: corev1.LocalObjectReference{Name: "dfw"}, - Topology: map[string]string{"topology.datum.net/city-code": "DFW"}, - }, - } + location := newTestLocationBinding("dfw", "DFW") s := newNetworkingScheme() cl := fake.NewClientBuilder(). diff --git a/internal/controller/workloaddeployment_controller.go b/internal/controller/workloaddeployment_controller.go index 06bc90b8..329e2d9d 100644 --- a/internal/controller/workloaddeployment_controller.go +++ b/internal/controller/workloaddeployment_controller.go @@ -31,6 +31,7 @@ import ( mcreconcile "sigs.k8s.io/multicluster-runtime/pkg/reconcile" computev1alpha "go.datum.net/compute/api/v1alpha" + "go.datum.net/compute/internal/locations" networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" "go.datum.net/compute/internal/controller/instancecontrol" @@ -60,6 +61,11 @@ type WorkloadDeploymentReconciler struct { // When true, new Instances whose template references ConfigMaps or Secrets // receive the ReferencedData scheduling gate at creation time. enableReferencedDataGate bool + + // LocationSource selects the API group the cell's serving location is read + // from. The zero value reads network services, which is what every + // deployment does today. + LocationSource locations.Source } func effectiveDesiredReplicas(deployment *computev1alpha.WorkloadDeployment) int32 { @@ -81,6 +87,7 @@ func workloadDeploymentPodSelector(deployment *computev1alpha.WorkloadDeployment // +kubebuilder:rbac:groups=compute.datumapis.com,resources=workloaddeployments/status,verbs=get;update;patch // +kubebuilder:rbac:groups=compute.datumapis.com,resources=workloaddeployments/finalizers,verbs=update // +kubebuilder:rbac:groups=networking.datumapis.com,resources=servinglocations,verbs=get;list;watch +// +kubebuilder:rbac:groups=locations.miloapis.com,resources=servinglocations,verbs=get;list;watch // +kubebuilder:rbac:groups=networking.datumapis.com,resources=networkinterfaceclaims,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=networking.datumapis.com,resources=networkinterfaces,verbs=get;list;watch // The management-mode WorkloadReconciler watches Networks. Declare the grant as @@ -639,7 +646,7 @@ type servingLocationResult struct { // servingLocation is the single ServingLocation the cell was delivered, or // nil when it was delivered none or more than one. - servingLocation *networkingv1alpha.ServingLocation + servingLocation *locations.ServingLocation // reason and message are the Available condition the deployment should // report while the location is unusable. Both are empty once it is usable. @@ -662,14 +669,14 @@ func (r *WorkloadDeploymentReconciler) resolveLocation( ctx context.Context, c client.Client, ) (servingLocationResult, error) { - var servingLocations networkingv1alpha.ServingLocationList - if err := c.List(ctx, &servingLocations); err != nil { - return servingLocationResult{}, fmt.Errorf("failed to list serving locations: %w", err) + servingLocations, err := locations.ListServingLocations(ctx, c, r.LocationSource) + if err != nil { + return servingLocationResult{}, err } - if len(servingLocations.Items) > 1 { - names := make([]string, 0, len(servingLocations.Items)) - for _, servingLocation := range servingLocations.Items { + if len(servingLocations) > 1 { + names := make([]string, 0, len(servingLocations)) + for _, servingLocation := range servingLocations { names = append(names, servingLocation.Name) } slices.Sort(names) @@ -682,7 +689,7 @@ func (r *WorkloadDeploymentReconciler) resolveLocation( }, nil } - if len(servingLocations.Items) == 0 { + if len(servingLocations) == 0 { // Not an error: a cell that has not been identified yet still runs // workloads, it just cannot tell them where they are. log.FromContext(ctx).V(1).Info("cell has no serving location, waiting") @@ -690,11 +697,11 @@ func (r *WorkloadDeploymentReconciler) resolveLocation( return servingLocationResult{ reason: computev1alpha.WorkloadDeploymentReasonNoMatchingLocation, message: fmt.Sprintf("This cell has not been told which location it serves; it needs the %s cluster label, or its location has not reached it yet", - networkingv1alpha.ServingLocationTopologyLabel), + locations.ServingLocationTopologyLabel), }, nil } - servingLocation := &servingLocations.Items[0] + servingLocation := &servingLocations[0] // A ServingLocation takes the name of the Location it was copied from, and // Location is cluster scoped, so the reference carries a name and no @@ -919,6 +926,11 @@ func (r *WorkloadDeploymentReconciler) SetupWithManager(mgr mcmanager.Manager, o // On cells without network-services-operator these watches would log spurious // errors for missing CRDs. if r.NetworkingEnabled { + servingLocationObject, err := locations.ServingLocationObject(r.LocationSource) + if err != nil { + return err + } + b = b. // A claim becoming bound and allocated is what releases an instance's // Network gate, and nothing else wakes this reconciler for it. The claim @@ -933,7 +945,7 @@ func (r *WorkloadDeploymentReconciler) SetupWithManager(mgr mcmanager.Manager, o // without any other wake-up event, and the reconciler does not poll. // Watching ServingLocations re-reconciles those deployments as soon as // the cell learns where it is, so Status.Location is filled in. - Watches(&networkingv1alpha.ServingLocation{}, func(clusterName multicluster.ClusterName, cl cluster.Cluster) handler.TypedEventHandler[client.Object, mcreconcile.Request] { + Watches(servingLocationObject, func(clusterName multicluster.ClusterName, cl cluster.Cluster) handler.TypedEventHandler[client.Object, mcreconcile.Request] { return handler.TypedEnqueueRequestsFromMapFunc(func(ctx context.Context, _ client.Object) []mcreconcile.Request { return enqueueWorkloadDeploymentsForServingLocation(ctx, cl.GetClient(), clusterName) }) diff --git a/internal/locations/locations.go b/internal/locations/locations.go new file mode 100644 index 00000000..a7912fb3 --- /dev/null +++ b/internal/locations/locations.go @@ -0,0 +1,234 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +// Package locations reads the two location facts compute depends on: which +// cities a project may place workloads in, and which location a cell serves. +// +// Both are served today by network-services-operator and are moving to the +// locations service. Which one is read is selected per deployment by Source, +// so a control plane that has not been migrated keeps reading the types it +// already has. +package locations + +import ( + "context" + "fmt" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + apimeta "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/sets" + "sigs.k8s.io/controller-runtime/pkg/client" + + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +const ( + // TopologyCityCodeKey is the topology key holding a location's city. + TopologyCityCodeKey = "topology.datum.net/city-code" + + // ServingLocationTopologyLabel is the cluster label a cell carries to claim + // the location it serves. + ServingLocationTopologyLabel = "topology.datum.net/location" +) + +// Source names the API group locations are read from. +type Source string + +const ( + // SourceNetworkServices reads networking.datumapis.com LocationBindings and + // ServingLocations. This is what every deployment reads today. + SourceNetworkServices Source = "NetworkServices" + + // SourceLocations reads locations.miloapis.com Locations and + // ServingLocations, served by the locations service. + SourceLocations Source = "Locations" +) + +var ( + locationGVK = schema.GroupVersionKind{ + Group: "locations.miloapis.com", + Version: "v1alpha1", + Kind: "Location", + } + + servingLocationGVK = schema.GroupVersionKind{ + Group: "locations.miloapis.com", + Version: "v1alpha1", + Kind: "ServingLocation", + } +) + +// Resolve reports which source to read. An unset source reads network +// services, matching the config default. +func (s Source) Resolve() (Source, error) { + switch s { + case "", SourceNetworkServices: + return SourceNetworkServices, nil + case SourceLocations: + return SourceLocations, nil + default: + return "", fmt.Errorf("unknown location source %q, want %q or %q", s, SourceNetworkServices, SourceLocations) + } +} + +// PlacementLocation is a location a project may place workloads at. +type PlacementLocation struct { + Name string + Topology map[string]string +} + +// CityCode returns the city the location serves, and whether it declares one. +func (l PlacementLocation) CityCode() (string, bool) { + code, ok := l.Topology[TopologyCityCodeKey] + return code, ok +} + +// ServingLocation is the location a cell has been told it serves. +type ServingLocation struct { + Name string + Topology map[string]string +} + +// CityCode returns the city the cell sits in. +func (l ServingLocation) CityCode() string { + return l.Topology[TopologyCityCodeKey] +} + +// ListPlacementLocations returns the locations a project may place workloads +// at, read from the project's control plane. +func ListPlacementLocations(ctx context.Context, c client.Client, source Source) ([]PlacementLocation, error) { + resolved, err := source.Resolve() + if err != nil { + return nil, err + } + + if resolved == SourceNetworkServices { + var bindings networkingv1alpha.LocationBindingList + if err := c.List(ctx, &bindings); err != nil { + return nil, fmt.Errorf("failed to list location bindings: %w", err) + } + + locations := make([]PlacementLocation, 0, len(bindings.Items)) + for _, binding := range bindings.Items { + locations = append(locations, PlacementLocation{ + Name: binding.Name, + Topology: binding.Spec.Topology, + }) + } + return locations, nil + } + + items, err := listUnstructured(ctx, c, locationGVK) + if err != nil { + return nil, fmt.Errorf("failed to list locations: %w", err) + } + + locations := make([]PlacementLocation, 0, len(items)) + for _, item := range items { + topology, err := topologyOf(item) + if err != nil { + return nil, err + } + locations = append(locations, PlacementLocation{ + Name: item.GetName(), + Topology: topology, + }) + } + return locations, nil +} + +// ListServingLocations returns the locations delivered to a cell. +func ListServingLocations(ctx context.Context, c client.Client, source Source) ([]ServingLocation, error) { + resolved, err := source.Resolve() + if err != nil { + return nil, err + } + + if resolved == SourceNetworkServices { + var list networkingv1alpha.ServingLocationList + if err := c.List(ctx, &list); err != nil { + return nil, fmt.Errorf("failed to list serving locations: %w", err) + } + + locations := make([]ServingLocation, 0, len(list.Items)) + for _, item := range list.Items { + locations = append(locations, ServingLocation{ + Name: item.Name, + Topology: item.Spec.Topology, + }) + } + return locations, nil + } + + items, err := listUnstructured(ctx, c, servingLocationGVK) + if err != nil { + return nil, fmt.Errorf("failed to list serving locations: %w", err) + } + + locations := make([]ServingLocation, 0, len(items)) + for _, item := range items { + topology, err := topologyOf(item) + if err != nil { + return nil, err + } + locations = append(locations, ServingLocation{ + Name: item.GetName(), + Topology: topology, + }) + } + return locations, nil +} + +// ServingLocationObject returns the object a controller watches to learn that +// a cell has been told where it sits. +func ServingLocationObject(source Source) (client.Object, error) { + resolved, err := source.Resolve() + if err != nil { + return nil, err + } + + if resolved == SourceNetworkServices { + return &networkingv1alpha.ServingLocation{}, nil + } + + object := &unstructured.Unstructured{} + object.SetGroupVersionKind(servingLocationGVK) + return object, nil +} + +// CityCodes returns the cities the given locations serve. +func CityCodes(locations []PlacementLocation) sets.Set[string] { + codes := sets.Set[string]{} + for _, location := range locations { + if code, ok := location.CityCode(); ok { + codes.Insert(code) + } + } + return codes +} + +// listUnstructured lists a kind that may not be installed. A control plane is +// only expected to serve the kinds its consumers read, so a kind that is not +// there reads as empty rather than failing the caller. +func listUnstructured(ctx context.Context, c client.Client, gvk schema.GroupVersionKind) ([]unstructured.Unstructured, error) { + var list unstructured.UnstructuredList + list.SetGroupVersionKind(gvk.GroupVersion().WithKind(gvk.Kind + "List")) + + if err := c.List(ctx, &list); err != nil { + if apimeta.IsNoMatchError(err) || apierrors.IsNotFound(err) { + return nil, nil + } + return nil, err + } + + return list.Items, nil +} + +func topologyOf(object unstructured.Unstructured) (map[string]string, error) { + topology, _, err := unstructured.NestedStringMap(object.Object, "spec", "topology") + if err != nil { + return nil, fmt.Errorf("failed to read the topology of location %q: %w", object.GetName(), err) + } + return topology, nil +} diff --git a/internal/locations/locations_test.go b/internal/locations/locations_test.go new file mode 100644 index 00000000..5c2d59bb --- /dev/null +++ b/internal/locations/locations_test.go @@ -0,0 +1,212 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package locations + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +const ( + testCityCode = "DFW" + testOtherCityCode = "ORD" +) + +func testScheme(t *testing.T) *runtime.Scheme { + t.Helper() + + s := runtime.NewScheme() + require.NoError(t, networkingv1alpha.AddToScheme(s)) + + for _, gvk := range []schema.GroupVersionKind{locationGVK, servingLocationGVK} { + s.AddKnownTypeWithName(gvk, &unstructured.Unstructured{}) + s.AddKnownTypeWithName(gvk.GroupVersion().WithKind(gvk.Kind+"List"), &unstructured.UnstructuredList{}) + } + + return s +} + +func newBinding(name, cityCode string) *networkingv1alpha.LocationBinding { + return &networkingv1alpha.LocationBinding{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: networkingv1alpha.LocationBindingSpec{ + LocationRef: corev1.LocalObjectReference{Name: name}, + Topology: map[string]string{TopologyCityCodeKey: cityCode}, + }, + } +} + +func newUnstructuredLocation(gvk schema.GroupVersionKind, name string, topology map[string]any) *unstructured.Unstructured { + object := &unstructured.Unstructured{Object: map[string]any{ + "metadata": map[string]any{"name": name}, + "spec": map[string]any{"topology": topology}, + }} + object.SetGroupVersionKind(gvk) + return object +} + +func cityTopology(cityCode string) map[string]any { + return map[string]any{TopologyCityCodeKey: cityCode} +} + +func TestSourceResolve(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + source Source + want Source + wantOK bool + }{ + {source: "", want: SourceNetworkServices, wantOK: true}, + {source: SourceNetworkServices, want: SourceNetworkServices, wantOK: true}, + {source: SourceLocations, want: SourceLocations, wantOK: true}, + {source: "Nonsense"}, + } { + resolved, err := tc.source.Resolve() + if !tc.wantOK { + require.Error(t, err) + continue + } + require.NoError(t, err) + assert.Equal(t, tc.want, resolved) + } +} + +func TestListPlacementLocations_NetworkServices(t *testing.T) { + t.Parallel() + + cl := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects( + newBinding("dfw", testCityCode), + newBinding("ord", testOtherCityCode), + // A binding with no city code contributes no placement city. + &networkingv1alpha.LocationBinding{ObjectMeta: metav1.ObjectMeta{Name: "nowhere"}}, + // The Locations source must not be read when network services is + // selected. + newUnstructuredLocation(locationGVK, "lhr", cityTopology("LHR")), + ). + Build() + + found, err := ListPlacementLocations(context.Background(), cl, SourceNetworkServices) + require.NoError(t, err) + require.Len(t, found, 3) + assert.ElementsMatch(t, []string{testCityCode, testOtherCityCode}, CityCodes(found).UnsortedList()) +} + +func TestListPlacementLocations_Locations(t *testing.T) { + t.Parallel() + + cl := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects( + newUnstructuredLocation(locationGVK, "dfw", cityTopology(testCityCode)), + newUnstructuredLocation(locationGVK, "ord", cityTopology(testOtherCityCode)), + // The network services source must not be read when the locations + // service is selected. + newBinding("lhr", "LHR"), + ). + Build() + + found, err := ListPlacementLocations(context.Background(), cl, SourceLocations) + require.NoError(t, err) + require.Len(t, found, 2) + assert.ElementsMatch(t, []string{testCityCode, testOtherCityCode}, CityCodes(found).UnsortedList()) +} + +func TestListPlacementLocations_UnknownSource(t *testing.T) { + t.Parallel() + + cl := fake.NewClientBuilder().WithScheme(testScheme(t)).Build() + + _, err := ListPlacementLocations(context.Background(), cl, "Nonsense") + require.Error(t, err) +} + +// TestListPlacementLocations_KindNotInstalled covers a control plane that +// serves only the kinds its consumers read: the locations service kinds are +// absent, which must read as no locations rather than failing the caller. +func TestListPlacementLocations_KindNotInstalled(t *testing.T) { + t.Parallel() + + cl := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithInterceptorFuncs(interceptor.Funcs{ + List: func(_ context.Context, _ client.WithWatch, _ client.ObjectList, _ ...client.ListOption) error { + return &apimeta.NoKindMatchError{GroupKind: locationGVK.GroupKind()} + }, + }). + Build() + + found, err := ListPlacementLocations(context.Background(), cl, SourceLocations) + require.NoError(t, err) + assert.Empty(t, found) +} + +func TestListServingLocations(t *testing.T) { + t.Parallel() + + cl := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects( + &networkingv1alpha.ServingLocation{ + ObjectMeta: metav1.ObjectMeta{Name: "nso-dfw"}, + Spec: networkingv1alpha.ServingLocationSpec{ + Topology: map[string]string{TopologyCityCodeKey: testCityCode}, + }, + }, + newUnstructuredLocation(servingLocationGVK, "locations-ord", cityTopology(testOtherCityCode)), + ). + Build() + + ctx := context.Background() + + fromNetworkServices, err := ListServingLocations(ctx, cl, SourceNetworkServices) + require.NoError(t, err) + require.Len(t, fromNetworkServices, 1) + assert.Equal(t, "nso-dfw", fromNetworkServices[0].Name) + assert.Equal(t, testCityCode, fromNetworkServices[0].CityCode()) + + fromLocations, err := ListServingLocations(ctx, cl, SourceLocations) + require.NoError(t, err) + require.Len(t, fromLocations, 1) + assert.Equal(t, "locations-ord", fromLocations[0].Name) + assert.Equal(t, testOtherCityCode, fromLocations[0].CityCode()) + + // An unset source reads what every deployment reads today. + fromDefault, err := ListServingLocations(ctx, cl, "") + require.NoError(t, err) + assert.Equal(t, fromNetworkServices, fromDefault) +} + +func TestServingLocationObject(t *testing.T) { + t.Parallel() + + for _, source := range []Source{"", SourceNetworkServices} { + object, err := ServingLocationObject(source) + require.NoError(t, err) + assert.IsType(t, &networkingv1alpha.ServingLocation{}, object) + } + + object, err := ServingLocationObject(SourceLocations) + require.NoError(t, err) + require.IsType(t, &unstructured.Unstructured{}, object) + assert.Equal(t, servingLocationGVK, object.GetObjectKind().GroupVersionKind()) + + _, err = ServingLocationObject("Nonsense") + require.Error(t, err) +} diff --git a/internal/webhook/v1alpha/workload_webhook.go b/internal/webhook/v1alpha/workload_webhook.go index e8926b60..7a07680b 100644 --- a/internal/webhook/v1alpha/workload_webhook.go +++ b/internal/webhook/v1alpha/workload_webhook.go @@ -3,28 +3,28 @@ package webhook import ( "context" - "fmt" - "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/util/sets" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" mcmanager "sigs.k8s.io/multicluster-runtime/pkg/manager" "sigs.k8s.io/multicluster-runtime/pkg/multicluster" computev1alpha "go.datum.net/compute/api/v1alpha" + "go.datum.net/compute/internal/locations" "go.datum.net/compute/internal/validation" computewebhook "go.datum.net/compute/internal/webhook" - networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" ) // SetupWorkloadWebhookWithManager will setup the manager to manage workload // webhooks -func SetupWorkloadWebhookWithManager(mgr mcmanager.Manager) error { +func SetupWorkloadWebhookWithManager(mgr mcmanager.Manager, locationSource locations.Source) error { webhook := &workloadWebhook{ - mgr: mgr, + mgr: mgr, + locationSource: locationSource, } return ctrl.NewWebhookManagedBy(mgr.GetLocalManager(), &computev1alpha.Workload{}). @@ -36,7 +36,16 @@ func SetupWorkloadWebhookWithManager(mgr mcmanager.Manager) error { // +kubebuilder:webhook:path=/mutate-compute-datumapis-com-v1alpha-workload,mutating=true,failurePolicy=fail,sideEffects=None,groups=compute.datumapis.com,resources=workloads,verbs=create;update,versions=v1alpha,name=mworkload.kb.io,admissionReviewVersions=v1 type workloadWebhook struct { - mgr mcmanager.Manager + mgr mcmanager.Manager + locationSource locations.Source +} + +func (r *workloadWebhook) validCityCodes(ctx context.Context, c client.Client) ([]string, error) { + placementLocations, err := locations.ListPlacementLocations(ctx, c, r.locationSource) + if err != nil { + return nil, err + } + return sets.List(locations.CityCodes(placementLocations)), nil } var _ admission.Defaulter[*computev1alpha.Workload] = &workloadWebhook{} @@ -89,17 +98,9 @@ func (r *workloadWebhook) ValidateCreate(ctx context.Context, workload *computev // that means for the scheduling phase, since there would not currently be // sufficient context to know who created the workload and what locations // are valid candidates based on that. Maybe an annotation, or spec field? - var locations networkingv1alpha.LocationBindingList - if err := clusterClient.List(ctx, &locations); err != nil { - return nil, fmt.Errorf("failed to list location bindings: %w", err) - } - - validCityCodes := sets.Set[string]{} - for _, location := range locations.Items { - cityCode, ok := location.Spec.Topology[networkingv1alpha.TopologyCityCodeKey] - if ok { - validCityCodes.Insert(cityCode) - } + validCityCodes, err := r.validCityCodes(ctx, clusterClient) + if err != nil { + return nil, err } opts := validation.WorkloadValidationOptions{ @@ -107,7 +108,7 @@ func (r *workloadWebhook) ValidateCreate(ctx context.Context, workload *computev Client: clusterClient, AdmissionRequest: req, Workload: workload, - ValidCityCodes: sets.List(validCityCodes), + ValidCityCodes: validCityCodes, } if errs := validation.ValidateWorkloadCreate(workload, opts); len(errs) > 0 { @@ -134,17 +135,9 @@ func (r *workloadWebhook) ValidateUpdate(ctx context.Context, _ *computev1alpha. return nil, err } - var locations networkingv1alpha.LocationBindingList - if err := clusterClient.List(ctx, &locations); err != nil { - return nil, fmt.Errorf("failed to list location bindings: %w", err) - } - - validCityCodes := sets.Set[string]{} - for _, location := range locations.Items { - cityCode, ok := location.Spec.Topology[networkingv1alpha.TopologyCityCodeKey] - if ok { - validCityCodes.Insert(cityCode) - } + validCityCodes, err := r.validCityCodes(ctx, clusterClient) + if err != nil { + return nil, err } opts := validation.WorkloadValidationOptions{ @@ -152,7 +145,7 @@ func (r *workloadWebhook) ValidateUpdate(ctx context.Context, _ *computev1alpha. Client: clusterClient, AdmissionRequest: req, Workload: newWorkload, - ValidCityCodes: sets.List(validCityCodes), + ValidCityCodes: validCityCodes, } if errs := validation.ValidateWorkloadCreate(newWorkload, opts); len(errs) > 0 { From ed2c354fb7cb9c2e08543cb398b7acdb8bd15bf7 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Thu, 27 Aug 2026 14:36:14 -0500 Subject: [PATCH 2/4] refactor: read the locations service through typed clients The locations module is reachable after all: a replace directive bypasses the vanity path lookup that a bare `go get` fails on, which is the pattern network-services-operator already uses for the ipam module. Pin the same commit NSO pins, the one deployed to staging. Reading the locations service through its generated types replaces the unstructured client and its hand-rolled topology extraction, and lets the shared topology keys be taken from the module rather than restated. The NetworkServices path is untouched and still reads the same typed network services kinds it always has. Key changes: - Depend on go.miloapis.com/locations by pseudo-version through a replace, and register its scheme on the manager and the CLI client - Keep the absent-CRD degrade, which a typed client reaches differently: the no-match arrives wrapped in an ErrResourceDiscoveryFailed, so only errors.Is unwrapping finds it. A type missing from the scheme is a wiring mistake and still fails - Cover both states against a real API server, since compiling against the types says nothing about a client resolving the kinds - Assert the two groups agree on the city-code and serving-location keys, which is what makes switching sources safe --- cmd/main.go | 2 + cmd/main_test.go | 35 +++++ go.mod | 7 + go.sum | 2 + internal/cmd/compute/util/client.go | 7 + internal/controller/location_source_test.go | 42 +++--- internal/locations/locations.go | 130 +++++++------------ internal/locations/locations_envtest_test.go | 123 ++++++++++++++++++ internal/locations/locations_test.go | 103 ++++++++++----- 9 files changed, 317 insertions(+), 134 deletions(-) create mode 100644 internal/locations/locations_envtest_test.go diff --git a/cmd/main.go b/cmd/main.go index e79e61a3..7dbf0e6f 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -53,6 +53,7 @@ import ( computewebhook "go.datum.net/compute/internal/webhook" computev1alphawebhooks "go.datum.net/compute/internal/webhook/v1alpha" networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" + locationsv1alpha1 "go.miloapis.com/locations/api/v1alpha1" infrastructurev1alpha1 "go.miloapis.com/milo/pkg/apis/infrastructure/v1alpha1" quotav1alpha1 "go.miloapis.com/milo/pkg/apis/quota/v1alpha1" resourcemanagerv1alpha1 "go.miloapis.com/milo/pkg/apis/resourcemanager/v1alpha1" @@ -91,6 +92,7 @@ func init() { utilruntime.Must(config.RegisterDefaults(scheme)) utilruntime.Must(computev1alpha.AddToScheme(scheme)) utilruntime.Must(networkingv1alpha.AddToScheme(scheme)) + utilruntime.Must(locationsv1alpha1.AddToScheme(scheme)) utilruntime.Must(quotav1alpha1.AddToScheme(scheme)) utilruntime.Must(karmadapolicyv1alpha1.Install(scheme)) utilruntime.Must(karmadaclusterv1alpha1.Install(scheme)) diff --git a/cmd/main_test.go b/cmd/main_test.go index 466e961a..89fc4f30 100644 --- a/cmd/main_test.go +++ b/cmd/main_test.go @@ -9,6 +9,11 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" + + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" + locationsv1alpha1 "go.miloapis.com/locations/api/v1alpha1" multiclusterproviders "go.miloapis.com/milo/pkg/multicluster-runtime" "go.datum.net/compute/internal/locations" @@ -55,3 +60,33 @@ metricsServer: _, err = loadServerConfig(write(t, header+"locationSource: Nonsense\n")) require.Error(t, err) } + +// TestSchemeResolvesLocationKinds is the runtime half of the typed locations +// dependency: compiling against the types says nothing about whether a client +// can resolve them. A kind missing here fails every list at runtime with "no +// kind is registered", not at build time. +func TestSchemeResolvesLocationKinds(t *testing.T) { + for _, object := range []client.Object{ + &locationsv1alpha1.Location{}, + &locationsv1alpha1.ServingLocation{}, + &networkingv1alpha.LocationBinding{}, + &networkingv1alpha.ServingLocation{}, + } { + gvk, err := apiutil.GVKForObject(object, scheme) + require.NoErrorf(t, err, "%T must be registered on the manager scheme", object) + assert.NotEmpty(t, gvk.Kind) + } +} + +// TestServingLocationObjectIsRegistered pins the watch the deployment +// reconciler installs: whichever source is selected, the object it watches has +// to be resolvable on the manager scheme. +func TestServingLocationObjectIsRegistered(t *testing.T) { + for _, source := range []locations.Source{"", locations.SourceNetworkServices, locations.SourceLocations} { + object, err := locations.ServingLocationObject(source) + require.NoError(t, err) + + _, err = apiutil.GVKForObject(object, scheme) + require.NoErrorf(t, err, "the watch object for source %q must be registered", source) + } +} diff --git a/go.mod b/go.mod index 9247c2b6..40b8f59c 100644 --- a/go.mod +++ b/go.mod @@ -14,6 +14,11 @@ require ( // predates the Prepared condition this gate reads. Re-pin to a tagged // release once one carries it. go.datum.net/network-services-operator v0.26.1-0.20260821014231-aceb24b1b569 + // The locations module publishes no tag and its vanity path does not yet + // resolve, so it is pinned by pseudo-version through a replace. The commit + // is the one deployed to staging, and is the same one + // network-services-operator pins. + go.miloapis.com/locations v0.0.0-00010101000000-000000000000 go.miloapis.com/milo v0.32.0 golang.org/x/crypto v0.54.0 golang.org/x/sync v0.22.0 @@ -182,3 +187,5 @@ require ( go.datum.net/datumctl v0.17.1-0.20260710003126-296c2fcbbd67 mvdan.cc/sh/v3 v3.12.0 ) + +replace go.miloapis.com/locations => github.com/milo-os/locations v0.0.0-20260825185141-507ac2cbd48c diff --git a/go.sum b/go.sum index 7c7406eb..863a40e6 100644 --- a/go.sum +++ b/go.sum @@ -212,6 +212,8 @@ github.com/mattn/go-isatty v0.0.22 h1:j8l17JJ9i6VGPUFUYoTUKPSgKe/83EYU2zBC7YNKMw github.com/mattn/go-isatty v0.0.22/go.mod h1:ZXfXG4SQHsB/w3ZeOYbR0PrPwLy+n6xiMrJlRFqopa4= github.com/mfridman/tparse v0.18.0 h1:wh6dzOKaIwkUGyKgOntDW4liXSo37qg5AXbIhkMV3vE= github.com/mfridman/tparse v0.18.0/go.mod h1:gEvqZTuCgEhPbYk/2lS3Kcxg1GmTxxU7kTC8DvP0i/A= +github.com/milo-os/locations v0.0.0-20260825185141-507ac2cbd48c h1:yanFdiWe+omnmj2W1q+u5C/D62pv7AnqjGUixWYV1Co= +github.com/milo-os/locations v0.0.0-20260825185141-507ac2cbd48c/go.mod h1:gzfAfHhSMwl/N68k/uSNYXOKK3IOBJCdXYaBgCE3gdE= github.com/moby/buildkit v0.29.0 h1:wxLEFbCOJntEDjSNNN2YWd8zxltZxT5muDQ0LzpbtpU= github.com/moby/buildkit v0.29.0/go.mod h1:Dmv2FeDe34t75QuzeU87rBoZpAAkcpT5zeu4hXzmASc= github.com/moby/docker-image-spec v1.3.1 h1:jMKff3w6PgbfSa69GfNg+zN/XLhfXJGnEx3Nl2EsFP0= diff --git a/internal/cmd/compute/util/client.go b/internal/cmd/compute/util/client.go index feafc7ab..aa6fa536 100644 --- a/internal/cmd/compute/util/client.go +++ b/internal/cmd/compute/util/client.go @@ -7,6 +7,7 @@ import ( computev1alpha "go.datum.net/compute/api/v1alpha" "go.datum.net/datumctl/plugin" networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" + locationsv1alpha1 "go.miloapis.com/locations/api/v1alpha1" quotav1alpha1 "go.miloapis.com/milo/pkg/apis/quota/v1alpha1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/rest" @@ -52,6 +53,9 @@ func NewClient(project string) (client.Client, error) { if err := networkingv1alpha.AddToScheme(scheme); err != nil { return nil, fmt.Errorf("registering networking scheme: %w", err) } + if err := locationsv1alpha1.AddToScheme(scheme); err != nil { + return nil, fmt.Errorf("registering locations scheme: %w", err) + } if err := quotav1alpha1.AddToScheme(scheme); err != nil { return nil, fmt.Errorf("registering quota scheme: %w", err) } @@ -78,6 +82,9 @@ func NewPlatformClient() (client.Client, error) { } scheme := runtime.NewScheme() + if err := locationsv1alpha1.AddToScheme(scheme); err != nil { + return nil, fmt.Errorf("registering locations scheme: %w", err) + } if err := quotav1alpha1.AddToScheme(scheme); err != nil { return nil, fmt.Errorf("registering quota scheme: %w", err) } diff --git a/internal/controller/location_source_test.go b/internal/controller/location_source_test.go index 9e7ccf91..db168712 100644 --- a/internal/controller/location_source_test.go +++ b/internal/controller/location_source_test.go @@ -9,38 +9,40 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client/fake" computev1alpha "go.datum.net/compute/api/v1alpha" "go.datum.net/compute/internal/locations" + locationsv1alpha1 "go.miloapis.com/locations/api/v1alpha1" ) -var locationsServiceGroupVersion = schema.GroupVersion{Group: "locations.miloapis.com", Version: "v1alpha1"} - // newLocationsServiceScheme returns the networking scheme with the locations -// service kinds registered as unstructured, which is how compute reads them. +// service types added, mirroring what the manager registers. func newLocationsServiceScheme() *runtime.Scheme { s := newNetworkingScheme() - for _, kind := range []string{"Location", "ServingLocation"} { - s.AddKnownTypeWithName(locationsServiceGroupVersion.WithKind(kind), &unstructured.Unstructured{}) - s.AddKnownTypeWithName(locationsServiceGroupVersion.WithKind(kind+"List"), &unstructured.UnstructuredList{}) - } + _ = locationsv1alpha1.AddToScheme(s) return s } -func newLocationsServiceObject(kind, name, cityCode string) *unstructured.Unstructured { - object := &unstructured.Unstructured{Object: map[string]any{ - "metadata": map[string]any{"name": name}, - "spec": map[string]any{ - "topology": map[string]any{locations.TopologyCityCodeKey: cityCode}, +func newLocationsServiceLocation(name, cityCode string) *locationsv1alpha1.Location { + return &locationsv1alpha1.Location{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: locationsv1alpha1.LocationSpec{ + LocationClassRef: locationsv1alpha1.LocationClassReference{Name: "datum-managed"}, + Topology: map[string]string{locations.TopologyCityCodeKey: cityCode}, }, - }} - object.SetGroupVersionKind(locationsServiceGroupVersion.WithKind(kind)) - return object + } +} + +func newLocationsServiceServingLocation(name, cityCode string) *locationsv1alpha1.ServingLocation { + return &locationsv1alpha1.ServingLocation{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: locationsv1alpha1.ServingLocationSpec{ + Topology: map[string]string{locations.TopologyCityCodeKey: cityCode}, + }, + } } // TestGetDeploymentsForWorkload_LocationsSource verifies that a workload placed @@ -70,7 +72,7 @@ func TestGetDeploymentsForWorkload_LocationsSource(t *testing.T) { cl := fake.NewClientBuilder(). WithScheme(newLocationsServiceScheme()). - WithObjects(newLocationsServiceObject("Location", "dfw", locTestCityCode)). + WithObjects(newLocationsServiceLocation("dfw", locTestCityCode)). WithIndex(&computev1alpha.WorkloadDeployment{}, deploymentWorkloadUIDIndex, deploymentWorkloadUIDIndexFunc). Build() @@ -119,7 +121,7 @@ func TestResolveLocation_LocationsSource(t *testing.T) { cl := fake.NewClientBuilder(). WithScheme(newLocationsServiceScheme()). WithObjects( - newLocationsServiceObject("ServingLocation", locationName, locTestCityCode), + newLocationsServiceServingLocation(locationName, locTestCityCode), // The network services copy must be ignored by this source. newTestServingLocation("nso-"+locationName, locTestOtherCityCode), ). @@ -146,7 +148,7 @@ func TestResolveLocation_NetworkServicesSourceIgnoresLocationsService(t *testing cl := fake.NewClientBuilder(). WithScheme(newLocationsServiceScheme()). - WithObjects(newLocationsServiceObject("ServingLocation", "loc-dfw-1", locTestCityCode)). + WithObjects(newLocationsServiceServingLocation("loc-dfw-1", locTestCityCode)). Build() r := &WorkloadDeploymentReconciler{} diff --git a/internal/locations/locations.go b/internal/locations/locations.go index a7912fb3..4c6f93d6 100644 --- a/internal/locations/locations.go +++ b/internal/locations/locations.go @@ -15,21 +15,20 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/client" networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" + locationsv1alpha1 "go.miloapis.com/locations/api/v1alpha1" ) const ( // TopologyCityCodeKey is the topology key holding a location's city. - TopologyCityCodeKey = "topology.datum.net/city-code" + TopologyCityCodeKey = locationsv1alpha1.TopologyCityCodeKey // ServingLocationTopologyLabel is the cluster label a cell carries to claim // the location it serves. - ServingLocationTopologyLabel = "topology.datum.net/location" + ServingLocationTopologyLabel = locationsv1alpha1.ServingLocationTopologyLabel ) // Source names the API group locations are read from. @@ -45,20 +44,6 @@ const ( SourceLocations Source = "Locations" ) -var ( - locationGVK = schema.GroupVersionKind{ - Group: "locations.miloapis.com", - Version: "v1alpha1", - Kind: "Location", - } - - servingLocationGVK = schema.GroupVersionKind{ - Group: "locations.miloapis.com", - Version: "v1alpha1", - Kind: "ServingLocation", - } -) - // Resolve reports which source to read. An unset source reads network // services, matching the config default. func (s Source) Resolve() (Source, error) { @@ -109,33 +94,32 @@ func ListPlacementLocations(ctx context.Context, c client.Client, source Source) return nil, fmt.Errorf("failed to list location bindings: %w", err) } - locations := make([]PlacementLocation, 0, len(bindings.Items)) + found := make([]PlacementLocation, 0, len(bindings.Items)) for _, binding := range bindings.Items { - locations = append(locations, PlacementLocation{ + found = append(found, PlacementLocation{ Name: binding.Name, Topology: binding.Spec.Topology, }) } - return locations, nil + return found, nil } - items, err := listUnstructured(ctx, c, locationGVK) - if err != nil { + var list locationsv1alpha1.LocationList + if err := c.List(ctx, &list); err != nil { + if kindNotInstalled(err) { + return nil, nil + } return nil, fmt.Errorf("failed to list locations: %w", err) } - locations := make([]PlacementLocation, 0, len(items)) - for _, item := range items { - topology, err := topologyOf(item) - if err != nil { - return nil, err - } - locations = append(locations, PlacementLocation{ - Name: item.GetName(), - Topology: topology, + found := make([]PlacementLocation, 0, len(list.Items)) + for _, location := range list.Items { + found = append(found, PlacementLocation{ + Name: location.Name, + Topology: location.Spec.Topology, }) } - return locations, nil + return found, nil } // ListServingLocations returns the locations delivered to a cell. @@ -151,33 +135,32 @@ func ListServingLocations(ctx context.Context, c client.Client, source Source) ( return nil, fmt.Errorf("failed to list serving locations: %w", err) } - locations := make([]ServingLocation, 0, len(list.Items)) - for _, item := range list.Items { - locations = append(locations, ServingLocation{ - Name: item.Name, - Topology: item.Spec.Topology, + found := make([]ServingLocation, 0, len(list.Items)) + for _, servingLocation := range list.Items { + found = append(found, ServingLocation{ + Name: servingLocation.Name, + Topology: servingLocation.Spec.Topology, }) } - return locations, nil + return found, nil } - items, err := listUnstructured(ctx, c, servingLocationGVK) - if err != nil { + var list locationsv1alpha1.ServingLocationList + if err := c.List(ctx, &list); err != nil { + if kindNotInstalled(err) { + return nil, nil + } return nil, fmt.Errorf("failed to list serving locations: %w", err) } - locations := make([]ServingLocation, 0, len(items)) - for _, item := range items { - topology, err := topologyOf(item) - if err != nil { - return nil, err - } - locations = append(locations, ServingLocation{ - Name: item.GetName(), - Topology: topology, + found := make([]ServingLocation, 0, len(list.Items)) + for _, servingLocation := range list.Items { + found = append(found, ServingLocation{ + Name: servingLocation.Name, + Topology: servingLocation.Spec.Topology, }) } - return locations, nil + return found, nil } // ServingLocationObject returns the object a controller watches to learn that @@ -191,16 +174,13 @@ func ServingLocationObject(source Source) (client.Object, error) { if resolved == SourceNetworkServices { return &networkingv1alpha.ServingLocation{}, nil } - - object := &unstructured.Unstructured{} - object.SetGroupVersionKind(servingLocationGVK) - return object, nil + return &locationsv1alpha1.ServingLocation{}, nil } // CityCodes returns the cities the given locations serve. -func CityCodes(locations []PlacementLocation) sets.Set[string] { +func CityCodes(found []PlacementLocation) sets.Set[string] { codes := sets.Set[string]{} - for _, location := range locations { + for _, location := range found { if code, ok := location.CityCode(); ok { codes.Insert(code) } @@ -208,27 +188,17 @@ func CityCodes(locations []PlacementLocation) sets.Set[string] { return codes } -// listUnstructured lists a kind that may not be installed. A control plane is -// only expected to serve the kinds its consumers read, so a kind that is not -// there reads as empty rather than failing the caller. -func listUnstructured(ctx context.Context, c client.Client, gvk schema.GroupVersionKind) ([]unstructured.Unstructured, error) { - var list unstructured.UnstructuredList - list.SetGroupVersionKind(gvk.GroupVersion().WithKind(gvk.Kind + "List")) - - if err := c.List(ctx, &list); err != nil { - if apimeta.IsNoMatchError(err) || apierrors.IsNotFound(err) { - return nil, nil - } - return nil, err - } - - return list.Items, nil -} - -func topologyOf(object unstructured.Unstructured) (map[string]string, error) { - topology, _, err := unstructured.NestedStringMap(object.Object, "spec", "topology") - if err != nil { - return nil, fmt.Errorf("failed to read the topology of location %q: %w", object.GetName(), err) - } - return topology, nil +// kindNotInstalled reports whether a list failed because the control plane +// does not serve the kind. A control plane is only expected to serve the kinds +// its consumers read, so a kind that is not there reads as empty rather than +// failing the caller. +// +// The REST mapper answers first. A typed client reaches it through discovery, +// so the no-match arrives wrapped in an ErrResourceDiscoveryFailed rather than +// bare, and only errors.Is unwrapping finds it. A mapper still holding the kind +// from before the CRD went away leaves the API server to answer, with a 404. A +// type missing from the scheme is neither of these: it is a wiring mistake, and +// must keep surfacing as one. +func kindNotInstalled(err error) bool { + return apimeta.IsNoMatchError(err) || apierrors.IsNotFound(err) } diff --git a/internal/locations/locations_envtest_test.go b/internal/locations/locations_envtest_test.go new file mode 100644 index 00000000..02b7a4a2 --- /dev/null +++ b/internal/locations/locations_envtest_test.go @@ -0,0 +1,123 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package locations + +import ( + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + + locationsv1alpha1 "go.miloapis.com/locations/api/v1alpha1" +) + +// locationsCRDDir resolves the CRDs shipped by the locations module, so the +// test installs the same schemas the service serves rather than a local copy +// that can drift from the pinned commit. +func locationsCRDDir(t *testing.T) string { + t.Helper() + + out, err := exec.Command("go", "list", "-m", "-f", "{{.Dir}}", "go.miloapis.com/locations").Output() + require.NoError(t, err, "the locations module must be resolvable") + + dir := filepath.Join(strings.TrimSpace(string(out)), "config", "base", "crd", "bases") + _, err = os.Stat(dir) + require.NoError(t, err) + return dir +} + +// TestLocationsSource_AgainstAPIServer is the runtime half of the typed switch. +// Compiling against the locations types proves nothing about whether a client +// can resolve the kinds, so this runs both states against a real API server: +// the CRDs absent, which must read as no locations, and the CRDs installed, +// which must read the objects back. +func TestLocationsSource_AgainstAPIServer(t *testing.T) { + if os.Getenv("KUBEBUILDER_ASSETS") == "" { + assets := filepath.Join("..", "..", "bin", "k8s", + fmt.Sprintf("1.31.0-%s-%s", runtime.GOOS, runtime.GOARCH)) + if _, err := os.Stat(assets); err != nil { + t.Skip("no envtest assets; run via make test") + } + t.Setenv("KUBEBUILDER_ASSETS", assets) + } + + testEnv := &envtest.Environment{} + cfg, err := testEnv.Start() + require.NoError(t, err) + t.Cleanup(func() { _ = testEnv.Stop() }) + + ctx := context.Background() + scheme := testScheme(t) + + newClient := func() client.Client { + c, err := client.New(rest.CopyConfig(cfg), client.Options{Scheme: scheme}) + require.NoError(t, err) + return c + } + + t.Run("CRDs absent reads as no locations", func(t *testing.T) { + c := newClient() + + // A typed client reaches the REST mapper through discovery, which wraps + // the no-match. Assert on the raw error so a client-go or + // controller-runtime bump that changes the wrapping fails here, rather + // than quietly turning the degrade into a reconcile failure. + var absent locationsv1alpha1.LocationList + rawErr := c.List(ctx, &absent) + require.Error(t, rawErr, "the CRD really is absent, so the degrade is under test") + assert.True(t, kindNotInstalled(rawErr), + "an absent CRD must stay recognisable as such: %T / %v", rawErr, rawErr) + + found, err := ListPlacementLocations(ctx, c, SourceLocations) + require.NoError(t, err, "a control plane without the CRDs must not fail the reconcile") + assert.Empty(t, found) + + serving, err := ListServingLocations(ctx, c, SourceLocations) + require.NoError(t, err) + assert.Empty(t, serving) + + // The default source has no such degrade: its kinds are expected + // everywhere it runs, so their absence stays an error. + _, err = ListPlacementLocations(ctx, c, SourceNetworkServices) + require.Error(t, err) + }) + + _, err = envtest.InstallCRDs(cfg, envtest.CRDInstallOptions{ + Paths: []string{locationsCRDDir(t)}, + }) + require.NoError(t, err) + + t.Run("CRDs installed resolve and read back", func(t *testing.T) { + c := newClient() + + require.NoError(t, c.Create(ctx, newLocation("dfw", testCityCode))) + require.NoError(t, c.Create(ctx, &locationsv1alpha1.ServingLocation{ + ObjectMeta: metav1.ObjectMeta{Name: "dfw"}, + Spec: locationsv1alpha1.ServingLocationSpec{ + Topology: map[string]string{TopologyCityCodeKey: testCityCode}, + }, + })) + + found, err := ListPlacementLocations(ctx, c, SourceLocations) + require.NoError(t, err) + require.Len(t, found, 1) + assert.Equal(t, []string{testCityCode}, CityCodes(found).UnsortedList()) + + serving, err := ListServingLocations(ctx, c, SourceLocations) + require.NoError(t, err) + require.Len(t, serving, 1) + assert.Equal(t, "dfw", serving[0].Name) + assert.Equal(t, testCityCode, serving[0].CityCode()) + }) +} diff --git a/internal/locations/locations_test.go b/internal/locations/locations_test.go index 5c2d59bb..e3520979 100644 --- a/internal/locations/locations_test.go +++ b/internal/locations/locations_test.go @@ -11,14 +11,13 @@ import ( corev1 "k8s.io/api/core/v1" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/client/interceptor" networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" + locationsv1alpha1 "go.miloapis.com/locations/api/v1alpha1" ) const ( @@ -31,12 +30,7 @@ func testScheme(t *testing.T) *runtime.Scheme { s := runtime.NewScheme() require.NoError(t, networkingv1alpha.AddToScheme(s)) - - for _, gvk := range []schema.GroupVersionKind{locationGVK, servingLocationGVK} { - s.AddKnownTypeWithName(gvk, &unstructured.Unstructured{}) - s.AddKnownTypeWithName(gvk.GroupVersion().WithKind(gvk.Kind+"List"), &unstructured.UnstructuredList{}) - } - + require.NoError(t, locationsv1alpha1.AddToScheme(s)) return s } @@ -50,17 +44,25 @@ func newBinding(name, cityCode string) *networkingv1alpha.LocationBinding { } } -func newUnstructuredLocation(gvk schema.GroupVersionKind, name string, topology map[string]any) *unstructured.Unstructured { - object := &unstructured.Unstructured{Object: map[string]any{ - "metadata": map[string]any{"name": name}, - "spec": map[string]any{"topology": topology}, - }} - object.SetGroupVersionKind(gvk) - return object +func newLocation(name, cityCode string) *locationsv1alpha1.Location { + return &locationsv1alpha1.Location{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: locationsv1alpha1.LocationSpec{ + LocationClassRef: locationsv1alpha1.LocationClassReference{Name: "datum-managed"}, + Topology: map[string]string{TopologyCityCodeKey: cityCode}, + }, + } } -func cityTopology(cityCode string) map[string]any { - return map[string]any{TopologyCityCodeKey: cityCode} +// TestTopologyKeysAgreeAcrossSources guards the migration's central assumption: +// a city code means the same thing whichever source served it. If the two +// groups ever disagree, switching sources would silently repoint every +// placement. +func TestTopologyKeysAgreeAcrossSources(t *testing.T) { + t.Parallel() + + assert.Equal(t, networkingv1alpha.TopologyCityCodeKey, TopologyCityCodeKey) + assert.Equal(t, networkingv1alpha.ServingLocationTopologyLabel, ServingLocationTopologyLabel) } func TestSourceResolve(t *testing.T) { @@ -96,9 +98,9 @@ func TestListPlacementLocations_NetworkServices(t *testing.T) { newBinding("ord", testOtherCityCode), // A binding with no city code contributes no placement city. &networkingv1alpha.LocationBinding{ObjectMeta: metav1.ObjectMeta{Name: "nowhere"}}, - // The Locations source must not be read when network services is + // The locations service must not be read when network services is // selected. - newUnstructuredLocation(locationGVK, "lhr", cityTopology("LHR")), + newLocation("lhr", "LHR"), ). Build() @@ -114,8 +116,8 @@ func TestListPlacementLocations_Locations(t *testing.T) { cl := fake.NewClientBuilder(). WithScheme(testScheme(t)). WithObjects( - newUnstructuredLocation(locationGVK, "dfw", cityTopology(testCityCode)), - newUnstructuredLocation(locationGVK, "ord", cityTopology(testOtherCityCode)), + newLocation("dfw", testCityCode), + newLocation("ord", testOtherCityCode), // The network services source must not be read when the locations // service is selected. newBinding("lhr", "LHR"), @@ -137,24 +139,53 @@ func TestListPlacementLocations_UnknownSource(t *testing.T) { require.Error(t, err) } -// TestListPlacementLocations_KindNotInstalled covers a control plane that -// serves only the kinds its consumers read: the locations service kinds are -// absent, which must read as no locations rather than failing the caller. -func TestListPlacementLocations_KindNotInstalled(t *testing.T) { +// TestKindNotInstalled_ScopedToLocationsSource pins the degrade to the source +// that needs it. A no-match reads as no locations for the locations service, +// which may not be installed yet, and still fails for network services, whose +// kinds every control plane already serves. +func TestKindNotInstalled_ScopedToLocationsSource(t *testing.T) { t.Parallel() + noMatch := func(_ context.Context, _ client.WithWatch, list client.ObjectList, _ ...client.ListOption) error { + gvk := list.GetObjectKind().GroupVersionKind() + return &apimeta.NoKindMatchError{GroupKind: gvk.GroupKind().WithVersion("").GroupKind()} + } + cl := fake.NewClientBuilder(). WithScheme(testScheme(t)). - WithInterceptorFuncs(interceptor.Funcs{ - List: func(_ context.Context, _ client.WithWatch, _ client.ObjectList, _ ...client.ListOption) error { - return &apimeta.NoKindMatchError{GroupKind: locationGVK.GroupKind()} - }, - }). + WithInterceptorFuncs(interceptor.Funcs{List: noMatch}). Build() - found, err := ListPlacementLocations(context.Background(), cl, SourceLocations) + ctx := context.Background() + + found, err := ListPlacementLocations(ctx, cl, SourceLocations) require.NoError(t, err) assert.Empty(t, found) + + serving, err := ListServingLocations(ctx, cl, SourceLocations) + require.NoError(t, err) + assert.Empty(t, serving) + + _, err = ListPlacementLocations(ctx, cl, SourceNetworkServices) + require.Error(t, err) + + _, err = ListServingLocations(ctx, cl, SourceNetworkServices) + require.Error(t, err) +} + +// TestListLocations_SchemeMissingStillFails separates a control plane that does +// not serve the kind from a binary that forgot to register it. The first reads +// as empty; the second is a wiring mistake and must keep surfacing. +func TestListLocations_SchemeMissingStillFails(t *testing.T) { + t.Parallel() + + bare := runtime.NewScheme() + require.NoError(t, networkingv1alpha.AddToScheme(bare)) + + cl := fake.NewClientBuilder().WithScheme(bare).Build() + + _, err := ListPlacementLocations(context.Background(), cl, SourceLocations) + require.Error(t, err, "an unregistered type is a wiring mistake, not an empty control plane") } func TestListServingLocations(t *testing.T) { @@ -169,7 +200,12 @@ func TestListServingLocations(t *testing.T) { Topology: map[string]string{TopologyCityCodeKey: testCityCode}, }, }, - newUnstructuredLocation(servingLocationGVK, "locations-ord", cityTopology(testOtherCityCode)), + &locationsv1alpha1.ServingLocation{ + ObjectMeta: metav1.ObjectMeta{Name: "locations-ord"}, + Spec: locationsv1alpha1.ServingLocationSpec{ + Topology: map[string]string{TopologyCityCodeKey: testOtherCityCode}, + }, + }, ). Build() @@ -204,8 +240,7 @@ func TestServingLocationObject(t *testing.T) { object, err := ServingLocationObject(SourceLocations) require.NoError(t, err) - require.IsType(t, &unstructured.Unstructured{}, object) - assert.Equal(t, servingLocationGVK, object.GetObjectKind().GroupVersionKind()) + assert.IsType(t, &locationsv1alpha1.ServingLocation{}, object) _, err = ServingLocationObject("Nonsense") require.Error(t, err) From 5d1d24a2c574fb8ae88962515d11d5b6354ef57a Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Thu, 27 Aug 2026 14:49:50 -0500 Subject: [PATCH 3/4] feat: refuse to start without the watched location kind Reading a location the control plane does not serve degrades to no locations, but watching one cannot: registering the watch wedges the manager during cache sync, and a wedged manager says nothing about why. Setup now checks discovery first and refuses to start, naming the kind, the CustomResourceDefinition to install, and the locationSource that requires it. The check covers only the source actually selected. A deployment reading network services gains no dependency on the locations service, and it stays behind the networking integration gate, so a cell that registers no watch still needs no location kind at all. Key changes: - Add EnsureServingLocationKind, and call it before the watch is registered - Cover the guard against real discovery in envtest, across a control plane serving neither kind, one kind, and both, so each source is shown to gate on its own kind alone --- .../workloaddeployment_controller.go | 4 + ...addeployment_servinglocation_guard_test.go | 141 ++++++++++++++++++ internal/locations/locations.go | 56 +++++++ internal/locations/locations_test.go | 27 ++++ 4 files changed, 228 insertions(+) create mode 100644 internal/controller/workloaddeployment_servinglocation_guard_test.go diff --git a/internal/controller/workloaddeployment_controller.go b/internal/controller/workloaddeployment_controller.go index 329e2d9d..c959c27d 100644 --- a/internal/controller/workloaddeployment_controller.go +++ b/internal/controller/workloaddeployment_controller.go @@ -926,6 +926,10 @@ func (r *WorkloadDeploymentReconciler) SetupWithManager(mgr mcmanager.Manager, o // On cells without network-services-operator these watches would log spurious // errors for missing CRDs. if r.NetworkingEnabled { + if err := locations.EnsureServingLocationKind(mgr.GetLocalManager().GetRESTMapper(), r.LocationSource); err != nil { + return err + } + servingLocationObject, err := locations.ServingLocationObject(r.LocationSource) if err != nil { return err diff --git a/internal/controller/workloaddeployment_servinglocation_guard_test.go b/internal/controller/workloaddeployment_servinglocation_guard_test.go new file mode 100644 index 00000000..6255a2f1 --- /dev/null +++ b/internal/controller/workloaddeployment_servinglocation_guard_test.go @@ -0,0 +1,141 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package controller + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/client-go/rest" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/cluster" + "sigs.k8s.io/controller-runtime/pkg/envtest" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" + mcmanager "sigs.k8s.io/multicluster-runtime/pkg/manager" + "sigs.k8s.io/multicluster-runtime/pkg/multicluster" + mcsingle "sigs.k8s.io/multicluster-runtime/providers/single" + + "go.datum.net/compute/internal/locations" +) + +// moduleCRD resolves a single CRD manifest shipped by a dependency, so the +// test installs the schema that module actually serves. +func moduleCRD(t *testing.T, module, dir, file string) string { + t.Helper() + + out, err := exec.Command("go", "list", "-m", "-f", "{{.Dir}}", module).Output() + require.NoError(t, err) + + path := filepath.Join(strings.TrimSpace(string(out)), filepath.FromSlash(dir), file) + _, err = os.Stat(path) + require.NoError(t, err) + return path +} + +// TestSetupWithManager_ServingLocationKindGuard covers the watch guard against +// real discovery. +// +// Registering a watch on a kind the cell does not serve wedges the manager +// during cache sync, so setup refuses instead. The guard has to fire for the +// selected source and stay silent about the other one: a deployment reading +// network services must not acquire a startup dependency on the locations +// service it never opted into. +// +// The refusals run through SetupWithManager, which also settles that discovery +// answers at registration time, on a manager that has not been started. The +// permitting direction is asserted on the manager's own REST mapper, because +// controller-runtime registers controller names in a process-global set and +// TestWorkloadDeploymentSetupWithManager_CellModeNoNetworkingCRD already claims +// this reconciler's name for the package. +func TestSetupWithManager_ServingLocationKindGuard(t *testing.T) { + ctrl.SetLogger(zap.New(zap.UseDevMode(true), zap.WriteTo(os.Stderr))) + + cfg := startComputeEnvtest(t) + scheme := newLocationsServiceScheme() + + // newManager builds a manager the way cmd/main.go does for a cell. + newManager := func(t *testing.T) mcmanager.Manager { + t.Helper() + + deploymentCluster, err := cluster.New(rest.CopyConfig(cfg), func(o *cluster.Options) { o.Scheme = scheme }) + require.NoError(t, err) + + mgr, err := mcmanager.New(rest.CopyConfig(cfg), + mcsingle.New(multicluster.ClusterName("single"), deploymentCluster), + ctrl.Options{ + Scheme: scheme, + Metrics: metricsserver.Options{BindAddress: "0"}, + HealthProbeBindAddress: "0", + }) + require.NoError(t, err) + return mgr + } + + setupWithSource := func(t *testing.T, source locations.Source) error { + t.Helper() + r := &WorkloadDeploymentReconciler{NetworkingEnabled: true, LocationSource: source} + return r.SetupWithManager(newManager(t)) + } + + // ensureWithSource runs the guard the way SetupWithManager runs it, against + // the same live REST mapper. + ensureWithSource := func(t *testing.T, source locations.Source) error { + t.Helper() + return locations.EnsureServingLocationKind(newManager(t).GetLocalManager().GetRESTMapper(), source) + } + + installCRD := func(t *testing.T, path string) { + t.Helper() + _, err := envtest.InstallCRDs(cfg, envtest.CRDInstallOptions{Paths: []string{path}}) + require.NoError(t, err) + } + + networkServicesCRD := moduleCRD(t, "go.datum.net/network-services-operator", + "config/crd/bases", "networking.datumapis.com_servinglocations.yaml") + locationsCRD := moduleCRD(t, "go.miloapis.com/locations", + "config/base/crd/bases", "locations.miloapis.com_servinglocations.yaml") + + t.Run("neither kind served", func(t *testing.T) { + err := setupWithSource(t, "") + require.Error(t, err, "the default source must refuse to watch a kind the cell does not serve") + assert.Contains(t, err.Error(), "servinglocations.networking.datumapis.com", + "the error must name the CRD to install") + assert.Contains(t, err.Error(), "NetworkServices", + "the error must name the locationSource that requires it") + assert.Contains(t, err.Error(), "ServingLocation") + + err = setupWithSource(t, locations.SourceLocations) + require.Error(t, err) + assert.Contains(t, err.Error(), "servinglocations.locations.miloapis.com") + assert.Contains(t, err.Error(), "Locations") + }) + + // Install only the locations service kind. The Locations source must now be + // permitted and the default must still refuse: each source gates on the one + // kind it watches. + installCRD(t, locationsCRD) + + t.Run("only the locations service kind served", func(t *testing.T) { + require.NoError(t, ensureWithSource(t, locations.SourceLocations), + "the selected source's kind is served, so the guard must permit it") + + err := setupWithSource(t, "") + require.Error(t, err, "installing the locations service must not satisfy the default source") + assert.Contains(t, err.Error(), "servinglocations.networking.datumapis.com") + }) + + installCRD(t, networkServicesCRD) + + t.Run("both kinds served", func(t *testing.T) { + for _, source := range []locations.Source{"", locations.SourceNetworkServices, locations.SourceLocations} { + require.NoErrorf(t, ensureWithSource(t, source), + "the guard must permit source %q once its kind is served", source) + } + }) +} diff --git a/internal/locations/locations.go b/internal/locations/locations.go index 4c6f93d6..09c9026a 100644 --- a/internal/locations/locations.go +++ b/internal/locations/locations.go @@ -12,9 +12,11 @@ package locations import ( "context" "fmt" + "strings" apierrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/client" @@ -177,6 +179,60 @@ func ServingLocationObject(source Source) (client.Object, error) { return &locationsv1alpha1.ServingLocation{}, nil } +// ServingLocationGVK returns the kind a controller watches for the source. +func ServingLocationGVK(source Source) (schema.GroupVersionKind, error) { + resolved, err := source.Resolve() + if err != nil { + return schema.GroupVersionKind{}, err + } + + if resolved == SourceNetworkServices { + return networkingv1alpha.GroupVersion.WithKind("ServingLocation"), nil + } + return locationsv1alpha1.GroupVersion.WithKind("ServingLocation"), nil +} + +// EnsureServingLocationKind fails unless the control plane serves the kind the +// source watches. +// +// A watch is not a read: a list against a kind the control plane does not serve +// degrades to no locations, but a watch cannot, and registering one wedges the +// manager during cache sync. Refusing to start says which CRD is missing, where +// a wedged manager says nothing. +// +// This gates only the selected source. A deployment reading network services +// must not be made to depend on the locations service being installed. +func EnsureServingLocationKind(mapper apimeta.RESTMapper, source Source) error { + gvk, err := ServingLocationGVK(source) + if err != nil { + return err + } + + if _, err := mapper.RESTMapping(gvk.GroupKind(), gvk.Version); err != nil { + if kindNotInstalled(err) { + resolved, _ := source.Resolve() + return fmt.Errorf( + "locationSource %q watches %s, which this control plane does not serve: "+ + "install the %s CustomResourceDefinition, or set locationSource to %q", + resolved, gvk, crdName(gvk), otherSource(resolved)) + } + return fmt.Errorf("failed to determine whether %s is served: %w", gvk, err) + } + + return nil +} + +func crdName(gvk schema.GroupVersionKind) string { + return fmt.Sprintf("%ss.%s", strings.ToLower(gvk.Kind), gvk.Group) +} + +func otherSource(source Source) Source { + if source == SourceLocations { + return SourceNetworkServices + } + return SourceLocations +} + // CityCodes returns the cities the given locations serve. func CityCodes(found []PlacementLocation) sets.Set[string] { codes := sets.Set[string]{} diff --git a/internal/locations/locations_test.go b/internal/locations/locations_test.go index e3520979..e32a732b 100644 --- a/internal/locations/locations_test.go +++ b/internal/locations/locations_test.go @@ -245,3 +245,30 @@ func TestServingLocationObject(t *testing.T) { _, err = ServingLocationObject("Nonsense") require.Error(t, err) } + +// TestEnsureServingLocationKind_UnknownSource keeps an unreadable config from +// reaching the REST mapper at all. +func TestEnsureServingLocationKind_UnknownSource(t *testing.T) { + t.Parallel() + + require.Error(t, EnsureServingLocationKind(apimeta.NewDefaultRESTMapper(nil), "Nonsense")) +} + +func TestServingLocationGVK(t *testing.T) { + t.Parallel() + + for _, source := range []Source{"", SourceNetworkServices} { + gvk, err := ServingLocationGVK(source) + require.NoError(t, err) + assert.Equal(t, networkingv1alpha.GroupVersion.WithKind("ServingLocation"), gvk) + assert.Equal(t, "servinglocations.networking.datumapis.com", crdName(gvk)) + } + + gvk, err := ServingLocationGVK(SourceLocations) + require.NoError(t, err) + assert.Equal(t, locationsv1alpha1.GroupVersion.WithKind("ServingLocation"), gvk) + assert.Equal(t, "servinglocations.locations.miloapis.com", crdName(gvk)) + + _, err = ServingLocationGVK("Nonsense") + require.Error(t, err) +} From d5b3a2a3051b60fb8e5aa3eec55ab575cb832ff5 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Thu, 27 Aug 2026 17:07:00 -0500 Subject: [PATCH 4/4] chore: depend on the locations module directly The module path go.miloapis.com/locations did not resolve, so the dependency was pinned through a replace onto the GitHub path. The vanity import is now registered and the module downloads from the public proxy. Key changes: - Require go.miloapis.com/locations at its own path and drop the replace --- go.mod | 10 +++------- go.sum | 4 ++-- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 40b8f59c..006654a3 100644 --- a/go.mod +++ b/go.mod @@ -14,11 +14,9 @@ require ( // predates the Prepared condition this gate reads. Re-pin to a tagged // release once one carries it. go.datum.net/network-services-operator v0.26.1-0.20260821014231-aceb24b1b569 - // The locations module publishes no tag and its vanity path does not yet - // resolve, so it is pinned by pseudo-version through a replace. The commit - // is the one deployed to staging, and is the same one - // network-services-operator pins. - go.miloapis.com/locations v0.0.0-00010101000000-000000000000 + // Pinned by pseudo-version to the commit deployed to staging, which is the + // same one network-services-operator pins. The module publishes no tag yet. + go.miloapis.com/locations v0.0.0-20260825185141-507ac2cbd48c go.miloapis.com/milo v0.32.0 golang.org/x/crypto v0.54.0 golang.org/x/sync v0.22.0 @@ -187,5 +185,3 @@ require ( go.datum.net/datumctl v0.17.1-0.20260710003126-296c2fcbbd67 mvdan.cc/sh/v3 v3.12.0 ) - -replace go.miloapis.com/locations => github.com/milo-os/locations v0.0.0-20260825185141-507ac2cbd48c diff --git a/go.sum b/go.sum index 863a40e6..292b7aa6 100644 --- a/go.sum +++ b/go.sum @@ -212,8 +212,6 @@ github.com/mattn/go-isatty v0.0.22 h1:j8l17JJ9i6VGPUFUYoTUKPSgKe/83EYU2zBC7YNKMw github.com/mattn/go-isatty v0.0.22/go.mod h1:ZXfXG4SQHsB/w3ZeOYbR0PrPwLy+n6xiMrJlRFqopa4= github.com/mfridman/tparse v0.18.0 h1:wh6dzOKaIwkUGyKgOntDW4liXSo37qg5AXbIhkMV3vE= github.com/mfridman/tparse v0.18.0/go.mod h1:gEvqZTuCgEhPbYk/2lS3Kcxg1GmTxxU7kTC8DvP0i/A= -github.com/milo-os/locations v0.0.0-20260825185141-507ac2cbd48c h1:yanFdiWe+omnmj2W1q+u5C/D62pv7AnqjGUixWYV1Co= -github.com/milo-os/locations v0.0.0-20260825185141-507ac2cbd48c/go.mod h1:gzfAfHhSMwl/N68k/uSNYXOKK3IOBJCdXYaBgCE3gdE= github.com/moby/buildkit v0.29.0 h1:wxLEFbCOJntEDjSNNN2YWd8zxltZxT5muDQ0LzpbtpU= github.com/moby/buildkit v0.29.0/go.mod h1:Dmv2FeDe34t75QuzeU87rBoZpAAkcpT5zeu4hXzmASc= github.com/moby/docker-image-spec v1.3.1 h1:jMKff3w6PgbfSa69GfNg+zN/XLhfXJGnEx3Nl2EsFP0= @@ -340,6 +338,8 @@ go.datum.net/datumctl v0.17.1-0.20260710003126-296c2fcbbd67 h1:Mhgt688CeTh2hX7ql go.datum.net/datumctl v0.17.1-0.20260710003126-296c2fcbbd67/go.mod h1:6skEjcE7aT8VPf/HVamA/BB6Dc9IISA6c/DdYKhqWNc= go.datum.net/network-services-operator v0.26.1-0.20260821014231-aceb24b1b569 h1:14vajo15fGGEAmdystQEE261rFH4AqER9s1Vg7POUKo= go.datum.net/network-services-operator v0.26.1-0.20260821014231-aceb24b1b569/go.mod h1:A7JNOuc+e6j/KkUVCcZ7Z2odvf6JFMQlX0Zo1Awj2TY= +go.miloapis.com/locations v0.0.0-20260825185141-507ac2cbd48c h1:+BQirT3wYCgv7H2lEZtAH+dpMiWu9j/Wa+c8s4jJUIA= +go.miloapis.com/locations v0.0.0-20260825185141-507ac2cbd48c/go.mod h1:gzfAfHhSMwl/N68k/uSNYXOKK3IOBJCdXYaBgCE3gdE= go.miloapis.com/milo v0.32.0 h1:TkNIQu/37d+SEquLJ5+GmdisSl+K2RT7eEC4idg6RIs= go.miloapis.com/milo v0.32.0/go.mod h1:GKK3afjCwshfZfvhjNe1wp/H45z4m7x5oG/8xbSgU1M= go.miloapis.com/service-catalog v0.4.0 h1:LvO1WCHMCoFokpS5igWMP8kyqly9gUFQmQj5IGhwuKs=