Address SwiftContainer review feedback

Author: eshulman2

api/v1alpha1/swiftcontainer_types.go

@@ -20,7 +20,7 @@ package v1alpha1
 // and 256 characters long and must not contain forward slashes.
 // +kubebuilder:validation:MinLength:=1
 // +kubebuilder:validation:MaxLength:=256
-// +kubebuilder:validation:Pattern:=`^[^/]+$`
+// +kubebuilder:validation:XValidation:rule="!self.contains('/')",message="name must not contain forward slashes"
 // +kubebuilder:validation:XValidation:rule="self.size() <= 256",message="name must not exceed 256 UTF-8 bytes"
 type SwiftContainerName string
 
@@ -55,13 +55,9 @@ type SwiftContainerMetadataStatus struct {
 	Value string `json:"value,omitempty"`
 }
 
-// SwiftContainerFilter defines an existing resource by its properties
+// SwiftContainerFilter defines an existing resource query.
 // +kubebuilder:validation:MinProperties:=1
 type SwiftContainerFilter struct {
-	// name of the existing resource
-	// +optional
-	Name *SwiftContainerName `json:"name,omitempty"`
-
 	// prefix filters containers by name prefix. Only containers whose names
 	// begin with this prefix will be considered.
 	// +kubebuilder:validation:MinLength:=1

api/v1alpha1/zz_generated.deepcopy.go

@@ -91,15 +91,6 @@ spec:
                       error state and will not continue to retry.
                     minProperties: 1
                     properties:
-                      name:
-                        description: name of the existing resource
-                        maxLength: 256
-                        minLength: 1
-                        pattern: ^[^/]+$
-                        type: string
-                        x-kubernetes-validations:
-                        - message: name must not exceed 256 UTF-8 bytes
-                          rule: self.size() <= 256
                       prefix:
                         description: |-
                           prefix filters containers by name prefix. Only containers whose names
@@ -115,9 +106,10 @@ spec:
                       The ORC object will enter an error state if the resource does not exist.
                     maxLength: 256
                     minLength: 1
-                    pattern: ^[^/]+$
                     type: string
                     x-kubernetes-validations:
+                    - message: name must not contain forward slashes
+                      rule: '!self.contains(''/'')'
                     - message: name must not exceed 256 UTF-8 bytes
                       rule: self.size() <= 256
                 type: object
@@ -212,11 +204,12 @@ spec:
                       the account and must not contain forward slashes.
                     maxLength: 256
                     minLength: 1
-                    pattern: ^[^/]+$
                     type: string
                     x-kubernetes-validations:
                     - message: name is immutable
                       rule: self == oldSelf
+                    - message: name must not contain forward slashes
+                      rule: '!self.contains(''/'')'
                     - message: name must not exceed 256 UTF-8 bytes
                       rule: self.size() <= 256
                   storagePolicy:

cmd/models-schema/zz_generated.openapi.go

@@ -104,66 +104,35 @@ func (actuator swiftcontainerActuator) ListOSResourcesForAdoption(ctx context.Co
 
 func (actuator swiftcontainerActuator) ListOSResourcesForImport(ctx context.Context, _ orcObjectPT, filter filterT) (iter.Seq2[*osResourceT, error], progress.ReconcileStatus) {
 	return func(yield func(*osContainerT, error) bool) {
-		if filter.Name != nil {
-			name := string(*filter.Name)
-			// If a prefix filter is also set, verify the name satisfies it.
-			// Without this check a filter {name: "x", prefix: "y-"} would
-			// silently import "x" even though it does not match the prefix.
-			if filter.Prefix != nil && !hasPrefix(name, *filter.Prefix) {
+		// List all containers and filter by prefix.
+		listOpts := containers.ListOpts{}
+		for container, err := range actuator.osClient.ListContainers(ctx, listOpts) {
+			if err != nil {
+				yield(nil, err)
 				return
 			}
-			header, err := actuator.osClient.GetContainer(ctx, name, nil)
+
+			if filter.Prefix != nil && !strings.HasPrefix(container.Name, *filter.Prefix) {
+				continue
+			}
+
+			header, err := actuator.osClient.GetContainer(ctx, container.Name, nil)
 			if err != nil {
-				if !orcerrors.IsNotFound(err) {
-					yield(nil, err)
-				}
+				yield(nil, err)
 				return
 			}
-			metadata, err := actuator.osClient.GetContainerMetadata(ctx, name)
+			metadata, err := actuator.osClient.GetContainerMetadata(ctx, container.Name)
 			if err != nil {
 				yield(nil, err)
 				return
 			}
-			yield(&osContainerT{Name: name, Metadata: metadata, GetHeader: *header}, nil)
-		} else {
-			// List all containers and filter by prefix
-			listOpts := containers.ListOpts{}
-			for container, err := range actuator.osClient.ListContainers(ctx, listOpts) {
-				if err != nil {
-					yield(nil, err)
-					return
-				}
-
-				if filter.Prefix != nil && !hasPrefix(container.Name, *filter.Prefix) {
-					continue
-				}
-
-				header, err := actuator.osClient.GetContainer(ctx, container.Name, nil)
-				if err != nil {
-					yield(nil, err)
-					return
-				}
-				metadata, err := actuator.osClient.GetContainerMetadata(ctx, container.Name)
-				if err != nil {
-					yield(nil, err)
-					return
-				}
-				if !yield(&osContainerT{Name: container.Name, Metadata: metadata, GetHeader: *header}, nil) {
-					return
-				}
+			if !yield(&osContainerT{Name: container.Name, Metadata: metadata, GetHeader: *header}, nil) {
+				return
 			}
 		}
 	}, nil
 }
 
-// hasPrefix checks if name starts with prefix.
-func hasPrefix(name, prefix string) bool {
-	if len(prefix) > len(name) {
-		return false
-	}
-	return name[:len(prefix)] == prefix
-}
-
 func (actuator swiftcontainerActuator) CreateResource(ctx context.Context, obj orcObjectPT) (*osContainerT, progress.ReconcileStatus) {
 	resource := obj.Spec.Resource
 
@@ -175,35 +144,10 @@ func (actuator swiftcontainerActuator) CreateResource(ctx context.Context, obj o
 
 	name := getResourceName(obj)
 
-	// Swift treats '/' as a path separator in container names, making the name
-	// invalid at the API level. Validate explicitly to provide a clear error
-	// message rather than a confusing HTTP error from gophercloud.
-	if strings.Contains(name, "/") {
-		return nil, progress.WrapError(
-			orcerrors.Terminal(orcv1alpha1.ConditionReasonInvalidConfiguration,
-				"container name must not contain forward slashes"))
-	}
-
-	// Swift limits container names to 256 UTF-8 bytes. The kubebuilder
-	// MaxLength:=256 marker counts Unicode code points, not bytes, so a name
-	// with multi-byte UTF-8 characters can pass API validation yet exceed the
-	// byte limit. Validate explicitly here to produce a clear error message.
-	if len(name) > 256 {
-		return nil, progress.WrapError(
-			orcerrors.Terminal(orcv1alpha1.ConditionReasonInvalidConfiguration,
-				"container name must not exceed 256 bytes"))
-	}
-
-	createOpts := containers.CreateOpts{}
-
-	if resource.ContainerRead != "" {
-		createOpts.ContainerRead = resource.ContainerRead
-	}
-	if resource.ContainerWrite != "" {
-		createOpts.ContainerWrite = resource.ContainerWrite
-	}
-	if resource.StoragePolicy != "" {
-		createOpts.StoragePolicy = resource.StoragePolicy
+	createOpts := containers.CreateOpts{
+		ContainerRead:  resource.ContainerRead,
+		ContainerWrite: resource.ContainerWrite,
+		StoragePolicy:  resource.StoragePolicy,
 	}
 
 	if len(resource.Metadata) > 0 {

config/crd/bases/openstack.k-orc.cloud_swiftcontainers.yaml

@@ -197,27 +197,6 @@ func TestListOSResourcesForImport(t *testing.T) {
 		client osclients.SwiftContainerClient
 		checks []checkFunc
 	}{
-		{
-			name:   "finds one by name",
-			filter: orcv1alpha1.SwiftContainerFilter{Name: ptr.To[orcv1alpha1.SwiftContainerName]("my-container")},
-			client: &mockSwiftContainerClient{
-				containerData: map[string]mockContainerData{
-					"my-container":    {header: containers.GetHeader{}, metadata: map[string]string{}},
-					"other-container": {header: containers.GetHeader{}, metadata: map[string]string{}},
-				},
-			},
-			checks: checks(noError, findsID("my-container"), findsN(1)),
-		},
-		{
-			name:   "finds none by name",
-			filter: orcv1alpha1.SwiftContainerFilter{Name: ptr.To[orcv1alpha1.SwiftContainerName]("missing-container")},
-			client: &mockSwiftContainerClient{
-				containerData: map[string]mockContainerData{
-					"my-container": {header: containers.GetHeader{}, metadata: map[string]string{}},
-				},
-			},
-			checks: checks(noError, findsN(0)),
-		},
 		{
 			name:   "finds multiple containers matching prefix filter",
 			filter: orcv1alpha1.SwiftContainerFilter{Prefix: ptr.To("test-")},
@@ -243,37 +222,6 @@ func TestListOSResourcesForImport(t *testing.T) {
 			},
 			checks: checks(wantError(errTest)),
 		},
-		{
-			// When both Name and Prefix are set and the named container does not
-			// match the prefix, no results should be returned. Previously the
-			// prefix predicate was silently ignored in the name-lookup branch.
-			name: "finds none when name does not match prefix",
-			filter: orcv1alpha1.SwiftContainerFilter{
-				Name:   ptr.To[orcv1alpha1.SwiftContainerName]("prod-bucket"),
-				Prefix: ptr.To("test-"),
-			},
-			client: &mockSwiftContainerClient{
-				containerData: map[string]mockContainerData{
-					"prod-bucket": {header: containers.GetHeader{}, metadata: map[string]string{}},
-				},
-			},
-			checks: checks(noError, findsN(0)),
-		},
-		{
-			// When both Name and Prefix are set and the named container matches
-			// the prefix, the container should be found normally.
-			name: "finds one when name matches prefix",
-			filter: orcv1alpha1.SwiftContainerFilter{
-				Name:   ptr.To[orcv1alpha1.SwiftContainerName]("test-bucket"),
-				Prefix: ptr.To("test-"),
-			},
-			client: &mockSwiftContainerClient{
-				containerData: map[string]mockContainerData{
-					"test-bucket": {header: containers.GetHeader{}, metadata: map[string]string{}},
-				},
-			},
-			checks: checks(noError, findsN(1), findsID("test-bucket")),
-		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			ctx := context.Background()
@@ -491,45 +439,6 @@ func TestCreateResource(t *testing.T) {
 		}
 	})
 
-	t.Run("returns terminal error for invalid container name containing slash", func(t *testing.T) {
-		ctx := context.Background()
-
-		// The actuator explicitly validates the container name before calling
-		// the Swift API. A slash in the name is caught early and returned as a
-		// terminal error with a message mentioning "forward slashes".
-		// (Kubebuilder validation normally prevents this from reaching the
-		// controller, but in unit tests API validation is not enforced.)
-		client := &mockSwiftContainerClient{}
-		actuator := swiftcontainerActuator{client}
-		// Use a name that bypasses kubebuilder validation (in unit tests, API
-		// validation is not enforced); this simulates what would happen if a
-		// slash somehow reached the actuator.
-		orcObject := &orcv1alpha1.SwiftContainer{
-			ObjectMeta: metav1.ObjectMeta{Name: "invalid"},
-			Spec: orcv1alpha1.SwiftContainerSpec{
-				Resource: &orcv1alpha1.SwiftContainerResourceSpec{
-					Name: ptr.To[orcv1alpha1.SwiftContainerName]("invalid/name"),
-				},
-			},
-		}
-
-		result, reconcileStatus := actuator.CreateResource(ctx, orcObject)
-		if result != nil {
-			t.Errorf("expected nil result, got %v", result)
-		}
-		if reconcileStatus == nil {
-			t.Fatal("expected non-nil reconcile status for terminal error")
-		}
-		_, err := reconcileStatus.NeedsReschedule()
-		if err == nil {
-			t.Error("expected error from reconcile status")
-		}
-		var termErr *orcerrors.TerminalError
-		if !errors.As(err, &termErr) {
-			t.Errorf("expected TerminalError, got %T: %v", err, err)
-		}
-	})
-
 	t.Run("returns terminal error for container name exceeding 256 bytes", func(t *testing.T) {
 		ctx := context.Background()
 
@@ -594,42 +503,6 @@ func TestCreateResource(t *testing.T) {
 }
 
 func TestContainerNameValidation(t *testing.T) {
-	t.Run("rejects names containing forward slash", func(t *testing.T) {
-		name := orcv1alpha1.SwiftContainerName("containers/bucket")
-		if !strings.Contains(string(name), "/") {
-			t.Fatal("test setup error: name should contain a slash")
-		}
-		// The actuator validates the name before calling the Swift API.
-		// A slash in the name causes an early terminal error with a message
-		// mentioning "forward slashes". (Kubebuilder pattern validation would
-		// normally catch this before it reaches the controller, but in unit
-		// tests API validation is not enforced.)
-		ctx := context.Background()
-		client := &mockSwiftContainerClient{}
-		actuator := swiftcontainerActuator{client}
-		orcObject := &orcv1alpha1.SwiftContainer{
-			ObjectMeta: metav1.ObjectMeta{Name: "invalid"},
-			Spec: orcv1alpha1.SwiftContainerSpec{
-				Resource: &orcv1alpha1.SwiftContainerResourceSpec{
-					Name: ptr.To(name),
-				},
-			},
-		}
-		_, reconcileStatus := actuator.CreateResource(ctx, orcObject)
-		if reconcileStatus == nil {
-			t.Error("expected reconcile status error for name with slash")
-			return
-		}
-		_, err := reconcileStatus.NeedsReschedule()
-		var termErr *orcerrors.TerminalError
-		if !errors.As(err, &termErr) {
-			t.Errorf("expected TerminalError for invalid name, got %T: %v", err, err)
-		}
-		if !strings.Contains(termErr.Error(), "forward slashes") {
-			t.Errorf("expected error message to mention 'forward slashes', got: %v", termErr.Error())
-		}
-	})
-
 	t.Run("rejects names exceeding 256 UTF-8 bytes", func(t *testing.T) {
 		longName := orcv1alpha1.SwiftContainerName(strings.Repeat("x", 257))
 		if len(longName) <= 256 {

internal/controllers/swiftcontainer/actuator.go

@@ -7,7 +7,7 @@ observed state corresponds to the spec.
 
 Validates that:
 - The OpenStack resource uses the name from `spec.resource.name` when it is
-  specified, rather than the ORC object name (SC-002).
+  specified, rather than the ORC object name.
 - Custom metadata key-value pairs are applied and reflected in
   `status.resource.metadata`.
 - Read ACL (`containerRead`) and write ACL (`containerWrite`) are configured

internal/controllers/swiftcontainer/actuator_test.go

@@ -8,6 +8,7 @@ status:
     name: swiftcontainer-create-minimal
     bytesUsed: 0
     objectCount: 0
+    storagePolicy: Policy-0
   conditions:
   - type: Available
     status: "True"

internal/controllers/swiftcontainer/tests/.gitkeep

@@ -6,7 +6,7 @@ Create a minimal SwiftContainer, that sets only the required fields, and verify
 that the observed state corresponds to the spec.
 
 Also validate that the OpenStack resource uses the name of the ORC object when
-`spec.resource.name` is not specified (SC-001).
+`spec.resource.name` is not specified.
 
 ## Step 01