diff --git a/README.md b/README.md index eee45b0..a6b0737 100644 --- a/README.md +++ b/README.md @@ -131,9 +131,9 @@ go build ./... go test ./... ``` -The test suite covers 108 unit tests across 4 packages using only the standard -`testing` package and `controller-runtime`'s fake client — no external cluster -required. +The test suite covers 168 unit tests across 4 packages (93% statement coverage +in `internal/`) using only the standard `testing` package and +`controller-runtime`'s fake client — no external cluster required. ### Lint and format @@ -163,7 +163,7 @@ CI uses `nix-build` for reproducible builds. The `tests` derivation runs external toolchain needed: ```sh -# CI check: go fmt + go vet + 108 unit tests +# CI check: go fmt + go vet + 168 unit tests nix-build nix -A tests # Build the manager binary only diff --git a/ROADMAP.md b/ROADMAP.md index 92625fd..5b03849 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -104,6 +104,11 @@ Feature: OpenStack Authentication via Application Credential Given a clouds.yaml with auth_type "password" When the operator attempts to create a Cloud client Then an UnsupportedAuthenticationError is raised + + Scenario: Token response is not valid JSON + Given Keystone returns HTTP 201 with a malformed JSON body + When the operator requests a token + Then an error is returned ``` #### US1.2 — Service Catalog Filtering by Interface and Region @@ -127,6 +132,21 @@ Feature: OpenStack Service Catalog And no region is configured When the catalog is loaded Then the first endpoint matching the interface is retained for each service + + Scenario: No interface configured + Given a clouds.yaml entry without an "interface" value + When the catalog is loaded + Then the "public" interface is used by default + + Scenario: Catalog request fails with a non-404 error + Given the catalog endpoint returns HTTP 500 + When the operator loads the catalog + Then an error is returned + + Scenario: Catalog response is not valid JSON + Given the catalog endpoint returns a malformed JSON body + When the operator loads the catalog + Then an error is returned ``` #### US1.3 — Revoked or Invalid Credential Handling @@ -162,6 +182,61 @@ Feature: Custom CA Certificate Given a Kubernetes secret without a "cacert" entry When the operator initialises the TLS transport Then the system CA is used for TLS verification + + Scenario: CA certificate content is not valid PEM + Given a Kubernetes secret with a "cacert" entry that is not valid PEM data + When the operator initialises the TLS transport + Then an error is returned +``` + +#### US1.5 — Authentication via Username/Password (v3password) + +```gherkin +Feature: OpenStack Authentication via Username/Password + In order to support clouds that do not use application credentials + As an operator + I want to authenticate using a v3password auth block with the correct Keystone scope + + Scenario: Project-scoped request with a project ID + Given a clouds.yaml auth block with "project_id" set + When the token request body is built + Then the request is scoped to that project by ID + + Scenario: Project-scoped request with a project name and explicit project domain + Given a clouds.yaml auth block with "project_name" and "project_domain_id" (or "project_domain_name") set + When the token request body is built + Then the request is scoped to that project by name, with the given domain + + Scenario: Project-scoped request falls back to the user domain + Given a clouds.yaml auth block with "project_name" set but no project domain + And "user_domain_id" or "user_domain_name" is set + When the token request body is built + Then the project scope's domain falls back to the user domain + + Scenario: Project name with no domain information at all + Given a clouds.yaml auth block with only "project_name" set + When the token request body is built + Then the project scope omits the domain key entirely + + Scenario: Domain-scoped request + Given a clouds.yaml auth block with "domain_id" or "domain_name" set and no project + When the token request body is built + Then the request is scoped to that domain + + Scenario: No scoping information at all + Given a clouds.yaml auth block with no project or domain fields + When the token request body is built + Then no "scope" key is included in the request + + Scenario: User identified by user_id + Given a clouds.yaml auth block with "user_id" set + When the token request body is built + Then the user is identified by ID, and the "name"/domain fields are omitted + + Scenario: Username without any user domain + Given a clouds.yaml auth block with "username" set and no user domain fields + When the token request body is built + Then the user block omits the domain key entirely ``` --- @@ -221,6 +296,11 @@ Feature: Floating IP Deletion Given a FIP still appears in every verification listing When the purge exhausts its polling attempts Then an error is returned mentioning the cluster name + + Scenario: Floating IP already deleted + Given a FIP deletion returns HTTP 404 + When the purge attempts to delete the FIP + Then the deletion is treated as successful, not as an error ``` > Deletion verification for FIPs, LBs, security groups, volumes and snapshots @@ -230,6 +310,39 @@ Feature: Floating IP Deletion > OpenStack's eventual consistency (`PENDING_DELETE` states) without > incurring a wait when nothing needs one. +#### US2.3 — Defensive Pagination Parsing + +```gherkin +Feature: Paginated Listing Robustness + In order to avoid crashing on unexpected OpenStack API responses + As an operator + I want paginated list requests to degrade gracefully on malformed data + + Scenario: Malformed top-level JSON in a list page + Given an OpenStack list endpoint returns a body that is not valid JSON + When the page is parsed + Then an error is returned + + Scenario: Pagination links field absent + Given a list response without a "_links" field + When the next page URL is resolved + Then pagination stops after the current page (no error) + + Scenario: Pagination links field malformed + Given a "_links" field that is not an array of link objects + When the next page URL is resolved + Then pagination stops after the current page (no error) + + Scenario: No "next" relation present + Given a "_links" array without a "next" entry + When the next page URL is resolved + Then pagination stops after the current page (no error) +``` + +> `nextPageURL` and `listPages` are shared by FIPs, load balancers and +> security groups; the scenarios above were validated against the FIP +> listing endpoint but apply identically to the other two. + --- ### Epic 3 — Octavia Load Balancer Cleanup @@ -387,6 +500,21 @@ Feature: Volume Deletion Policy Then the cluster's volumes are kept ``` +#### US5.3 — Defensive Handling of Malformed Cinder Responses + +```gherkin +Feature: Cinder Response Robustness + Scenario: Top-level volume/snapshot list response is not valid JSON + Given the Cinder API returns a malformed JSON body for a list request + When the volumes or snapshots of a cluster are listed + Then an error is returned + + Scenario: "volumes"/"snapshots" key is not an array + Given the Cinder API returns a list response where the items key is not an array + When the volumes or snapshots of a cluster are listed + Then an error is returned +``` + --- ### Epic 6 — Cinder Snapshot Management @@ -431,14 +559,61 @@ Feature: Application Credential Deletion Given the annotation "credential-policy" = "delete" on the secret And other finalizers are still present on the OpenStackCluster When the purge is complete - Then the Application Credential is not deleted - And a FinalizerStillPresentError is raised to trigger a retry + Then the credential secret is not deleted + And the janitor finalizer is not removed + And a retry annotation is set to trigger a later reconcile Scenario: Application Credential cannot be deleted (403) Given the Application Credential is restricted (no unrestricted flag) When the appcred deletion is attempted Then a warning is emitted And the Kubernetes secret deletion proceeds anyway + + Scenario: clouds.yaml cannot be parsed while resolving the credential ID + Given a malformed clouds.yaml + When the application credential ID is extracted for deletion + Then an error is returned +``` + +#### US7.2 — Purge Orchestration Across Resource Types + +```gherkin +Feature: OpenStack Resource Purge Orchestration + In order to guarantee a consistent, predictable cleanup sequence + As an operator + I want resource types to be purged in a fixed order, stopping on the first failure + + Scenario: Authentication fails + Given an invalid clouds.yaml + When the purge is triggered + Then the authentication error is returned immediately and no resources are touched + + Scenario: Floating IP deletion fails + Given the Floating IP listing or deletion fails + When the purge is triggered + Then the error is returned immediately + And load balancers, security groups, volumes and the application credential are not touched + + Scenario: Volume deletion fails + Given include_volumes is true and volume listing or deletion fails + When the purge is triggered + Then the error is returned immediately + And the application credential is not deleted, even if include_appcred is true + + Scenario: Volumes policy disabled + Given include_volumes is false + When the purge is triggered + Then snapshots and volumes are not listed or deleted + + Scenario: Application credential deletion requested + Given include_appcred is true and all prior steps succeed + When the purge is triggered + Then the application credential is deleted via the Identity API + + Scenario: Full successful purge + Given include_volumes and include_appcred set as configured, and no step fails + When the purge is triggered + Then all resource types are processed in order and no error is returned ``` --- @@ -522,6 +697,60 @@ Feature: Retry via Random Annotation Then the 404 ApiError is ignored ``` +#### US8.5 — Robust Reconcile Error Handling + +```gherkin +Feature: Reconcile Resilience to Kubernetes API Errors + In order to avoid silently losing track of clusters + As an operator + I want Kubernetes API errors during reconciliation to be surfaced or handled explicitly + + Scenario: Fetching the OpenStackCluster fails with a non-NotFound error + Given the Kubernetes API returns an error other than NotFound when fetching the cluster + When Reconcile runs + Then the error is propagated + + Scenario: Adding the finalizer fails + Given the update to the OpenStackCluster fails + When Reconcile attempts to add the janitor finalizer + Then the error is propagated, wrapped as "adding finalizer" + + Scenario: Fetching the identity secret fails with a non-NotFound error + Given the Kubernetes API returns an error other than NotFound when fetching the credential secret + When Reconcile runs during deletion + Then the error is propagated, wrapped as "fetching identity secret" + + Scenario: Identity secret does not exist + Given the secret referenced by spec.identityRef does not exist + When Reconcile runs during deletion + Then Reconcile returns without error and does not attempt a purge + + Scenario: CloudName not specified + Given spec.identityRef.cloudName is empty + When Reconcile runs during deletion + Then the cloud name "openstack" is used to authenticate + + Scenario: Retry annotation patch fails with a non-NotFound error + Given a purge failure followed by a failure to patch the retry annotation + When Reconcile handles the purge error + Then the error is propagated + + Scenario: Deleting the credential secret fails with a non-NotFound error + Given credential-policy is "delete", this is the last finalizer, and the secret deletion fails + When Reconcile completes a successful purge + Then the error is propagated, wrapped as "deleting credential secret" + + Scenario: Removing the finalizer fails + Given a successful purge and no pending credential-policy deletion + When Reconcile attempts to remove the janitor finalizer + Then the error is propagated, wrapped as "removing finalizer" + + Scenario: No PurgeFunc configured + Given the reconciler has no injected PurgeFunc + When Reconcile triggers a purge + Then the real openstack.PurgeResources implementation is used +``` + --- ### Epic 9 — Operator Configuration @@ -697,6 +926,27 @@ Feature: Cinder Service Detection with Aliases Then a CatalogError is raised with the appropriate message ``` +#### US12.3 — Transient Error Classification + +```gherkin +Feature: Distinguishing Transient from Fatal Deletion Errors + Scenario: Non-HTTP error is never treated as transient + Given a deletion attempt fails with an error that is not an HTTP status error (e.g. a network-level failure) + When the operator classifies the error + Then it is not treated as transient (only HTTP 400/409 are) +``` + +#### US12.4 — HTTP Response Body Read Failures + +```gherkin +Feature: Robustness to Interrupted HTTP Responses + Scenario: Response body cannot be fully read + Given an OpenStack API call returns a successful status code + And the response body fails while being read (e.g. connection interrupted mid-transfer) + When the operator processes the response + Then an error is returned +``` + --- ## Actions @@ -709,6 +959,7 @@ Feature: Cinder Service Detection with Aliases 6. [x] Migrate the Helm chart for the Go image 7. [x] Implement epics 11 (observability) and 12 (robustness) 8. [x] OCI build via Nix (without Flake) + CycloneDX SBOM (US10.3 — outside initial plan) +9. [x] Raise `internal/` test coverage from 77.5% to 93.0% (53 new tests — see Test Coverage below) ## Final Result @@ -717,11 +968,45 @@ Feature: Cinder Service Detection with Aliases | OpenStack client | `internal/openstack/cloud.go`, `resources.go`, `purge.go` | | Controller | `internal/controller/openstackcluster_controller.go`, `metrics.go` | | Config | `internal/controller/config.go` (env vars) | -| Tests | 108 tests (4 packages) | +| Tests | 168 tests (4 packages) | | Packaging | `nix/default.nix`, `nix/nixpkgs.nix` | | Helm | `chart/` — Deployment, ClusterRole, RBAC, health probes | | CI | `.github/workflows/build-push-artifacts.yaml` (Nix + skopeo + SBOM) | +### Test Coverage (`internal/`) + +| Metric | Before | After | +|---|---|---| +| Coverage (statements) | 77.5% | 93.0% | +| Tests | 115 | 168 | + +New tests added (53), targeting the functions that were at 0% or had untested +error branches: + +| Function(s) | Coverage before → after | File | +|---|---|---| +| `PurgeResources` | 0% → 82.6% | `internal/openstack/purge_test.go` (new) — 8 tests via a combined self-referential mock server (network/load-balancer/volumev3/identity) | +| `Reconcile` | 69% → 98.3% | `internal/controller/openstackcluster_controller_test.go` — 11 tests: Get/Update/Patch/Delete error injection via `interceptor.Funcs`, credential-policy branches (last finalizer vs. others still present), cloud name default, nil-`PurgeFunc` fallback | +| `otherFinalizer` | 0% → 100% | `internal/controller/internal_test.go` (new, white-box) | +| `deleteSecret`, `getSecret` | 0% / 50% → 100% | covered indirectly through the `Reconcile` tests above | +| `httpStatusError.Error/StatusCode`, `isTransient`, `AppCredentialID`, `passwordScope`, `passwordTokenBody`, `loadCACert` | various → 100% | `internal/openstack/cloud_test.go` | +| `doDelete` (404 path), `nextPageURL` (pagination edge cases), `listVolumeItems` (malformed JSON) | various → 100% or near | `internal/openstack/resources_test.go` | +| `isTransient` (non-HTTP error), `doGet` (body-read error) | → 100% / 81.8% | `internal/openstack/internal_test.go` (new, white-box) | + +Residual gaps accepted as out of scope (negligible risk vs. setup/runtime cost): +`SetupWithManager` (pure `ctrl.Manager` wiring), the real `time.Sleep` +fallbacks in `Session.sleep` / `OpenStackClusterReconciler.sleep`, the +`x509.SystemCertPool()` OS-level error path in `loadCACert`, and the +malformed-URL branch of `newDeleteRequest`. + +The corresponding use cases were written up as Gherkin scenarios and folded +into the relevant epics above: US1.2 (catalog defaults/errors), US1.4 (CA +cert), new US1.5 (password auth & scope), US2.2/new US2.3 (FIP 404 handling, +pagination robustness), new US5.3 (Cinder response robustness), US7.1/new +US7.2 (appcred edge cases, purge orchestration), new US8.5 (Reconcile error +handling), new US12.3/US12.4 (transient error classification, body-read +failures). + ## Implementation Order ``` diff --git a/internal/controller/internal_test.go b/internal/controller/internal_test.go new file mode 100644 index 0000000..f0a8df0 --- /dev/null +++ b/internal/controller/internal_test.go @@ -0,0 +1,41 @@ +/* +Copyright 2026. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import "testing" + +func TestOtherFinalizer(t *testing.T) { + cases := []struct { + name string + finalizers []string + skip string + want string + }{ + {"multiple, one to skip", []string{"a.finalizer", Finalizer, "b.finalizer"}, Finalizer, "a.finalizer"}, + {"empty list", nil, Finalizer, ""}, + {"single matching (skip target only)", []string{Finalizer}, Finalizer, ""}, + {"single non-matching", []string{"other.finalizer"}, Finalizer, "other.finalizer"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got := otherFinalizer(c.finalizers, c.skip) + if got != c.want { + t.Errorf("otherFinalizer(%v, %q) = %q, want %q", c.finalizers, c.skip, got, c.want) + } + }) + } +} diff --git a/internal/controller/openstackcluster_controller_test.go b/internal/controller/openstackcluster_controller_test.go index 13a3de6..ede0dc1 100644 --- a/internal/controller/openstackcluster_controller_test.go +++ b/internal/controller/openstackcluster_controller_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "strings" "testing" "time" @@ -17,6 +18,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/azimuth-cloud/cluster-api-janitor-openstack/internal/controller" @@ -48,6 +50,25 @@ func newReconciler(purgeFunc func(context.Context, openstack.PurgeOptions) error return r, c } +func newReconcilerWithInterceptors( + purgeFunc func(context.Context, openstack.PurgeOptions) error, + interceptors interceptor.Funcs, + objs ...client.Object, +) (*controller.OpenStackClusterReconciler, client.Client) { + c := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(objs...). + WithInterceptorFuncs(interceptors). + Build() + r := &controller.OpenStackClusterReconciler{ + Client: c, + Scheme: testScheme, + PurgeFunc: purgeFunc, + SleepFunc: func(time.Duration) {}, + } + return r, c +} + func newCluster(name, namespace string, opts ...func(*infrav1.OpenStackCluster)) *infrav1.OpenStackCluster { c := &infrav1.OpenStackCluster{ ObjectMeta: metav1.ObjectMeta{ @@ -283,3 +304,246 @@ func TestReconcile_IgnoresNotFound_WhenClusterDeletedDuringRetry(t *testing.T) { t.Fatalf("expected no error when cluster deleted during retry annotation, got: %v", err) } } + +// ── Additional error-path and branch coverage ──────────────────────────────── + +// Scenario: fetching the cluster fails with a non-NotFound error → propagated +func TestReconcile_GetError_NonNotFound_Propagates(t *testing.T) { + r, _ := newReconcilerWithInterceptors(nil, interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { + return errors.New("boom") + }, + }) + + if _, err := r.Reconcile(context.Background(), reconcileRequest("mycluster", "default")); err == nil { + t.Fatal("expected error to be propagated, got nil") + } +} + +// Scenario: adding the finalizer fails on Update → propagated +func TestReconcile_AddFinalizer_UpdateError_Propagates(t *testing.T) { + cluster := newCluster("mycluster", "default") + r, _ := newReconcilerWithInterceptors(nil, interceptor.Funcs{ + Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + return errors.New("update failed") + }, + }, cluster) + + _, err := r.Reconcile(context.Background(), reconcileRequest("mycluster", "default")) + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "adding finalizer:") { + t.Errorf("expected error to wrap %q, got: %v", "adding finalizer:", err) + } +} + +// Scenario: fetching the identity secret fails with a non-NotFound error → propagated +func TestReconcile_GetSecret_NonNotFoundError_Propagates(t *testing.T) { + cluster := newCluster("mycluster", "default", withFinalizer, withDeletionTimestamp) + r, _ := newReconcilerWithInterceptors(nil, interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key types.NamespacedName, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*corev1.Secret); ok { + return errors.New("secret get failed") + } + return c.Get(ctx, key, obj, opts...) + }, + }, cluster) + + _, err := r.Reconcile(context.Background(), reconcileRequest("mycluster", "default")) + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "fetching identity secret:") { + t.Errorf("expected error to wrap %q, got: %v", "fetching identity secret:", err) + } +} + +// Scenario: identity secret does not exist → Reconcile returns early without error +func TestReconcile_SecretNotFound_ReturnsEarlyWithoutError(t *testing.T) { + cluster := newCluster("mycluster", "default", withFinalizer, withDeletionTimestamp) + purgeCalled := false + r, _ := newReconciler(func(context.Context, openstack.PurgeOptions) error { + purgeCalled = true + return nil + }, cluster) // no secret created + + res, err := r.Reconcile(context.Background(), reconcileRequest("mycluster", "default")) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if res != (ctrl.Result{}) { + t.Errorf("expected empty result, got: %v", res) + } + if purgeCalled { + t.Error("expected purge NOT to be called when identity secret is absent") + } +} + +// Scenario: IdentityRef.CloudName empty → defaults to "openstack" +func TestReconcile_CloudName_DefaultsToOpenstack_WhenEmpty(t *testing.T) { + cluster := newCluster("mycluster", "default", withFinalizer, withDeletionTimestamp, + func(c *infrav1.OpenStackCluster) { c.Spec.IdentityRef.CloudName = "" }) + secret := newSecret("cloud-credentials", "default") + + var capturedCloudName string + r, _ := newReconciler(func(_ context.Context, opts openstack.PurgeOptions) error { + capturedCloudName = opts.CloudName + return nil + }, cluster, secret) + + if _, err := r.Reconcile(context.Background(), reconcileRequest("mycluster", "default")); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if capturedCloudName != "openstack" { + t.Errorf("expected CloudName to default to %q, got %q", "openstack", capturedCloudName) + } +} + +// Scenario: purge fails and the subsequent retry-annotation Patch also fails +// with a non-NotFound error → propagated +func TestReconcile_AnnotateRetry_NonNotFoundError_Propagates(t *testing.T) { + cluster := newCluster("mycluster", "default", withFinalizer, withDeletionTimestamp) + secret := newSecret("cloud-credentials", "default") + + r, _ := newReconcilerWithInterceptors( + func(context.Context, openstack.PurgeOptions) error { return errors.New("purge failed") }, + interceptor.Funcs{ + Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + return errors.New("patch failed") + }, + }, + cluster, secret, + ) + + _, err := r.Reconcile(context.Background(), reconcileRequest("mycluster", "default")) + if err == nil { + t.Fatal("expected error to be propagated, got nil") + } +} + +// Scenario: credential policy "delete" and this is the last finalizer → +// secret deleted and janitor finalizer removed +func TestReconcile_CredentialPolicyDelete_LastFinalizer_DeletesSecretAndRemovesFinalizer(t *testing.T) { + cluster := newCluster("mycluster", "default", withFinalizer, withDeletionTimestamp) + secret := newSecret("cloud-credentials", "default") + secret.Annotations = map[string]string{controller.CredentialPolicyAnnotation: controller.PolicyDelete} + + r, c := newReconciler(func(context.Context, openstack.PurgeOptions) error { return nil }, cluster, secret) + + if _, err := r.Reconcile(context.Background(), reconcileRequest("mycluster", "default")); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + var gotSecret corev1.Secret + err := c.Get(context.Background(), types.NamespacedName{Name: "cloud-credentials", Namespace: "default"}, &gotSecret) + if !apierrors.IsNotFound(err) { + t.Errorf("expected credential secret to be deleted, got err: %v", err) + } + + got := getClusterOrNil(t, c, "mycluster", "default") + if got != nil && controllerutil.ContainsFinalizer(got, controller.Finalizer) { + t.Error("expected janitor finalizer to be removed") + } +} + +// Scenario: credential policy "delete" but other finalizers remain → secret +// kept, retry annotation set, janitor finalizer NOT removed +func TestReconcile_CredentialPolicyDelete_OtherFinalizersPresent_SecretKeptRetryAnnotated(t *testing.T) { + cluster := newCluster("mycluster", "default", withFinalizer, withDeletionTimestamp) + controllerutil.AddFinalizer(cluster, "other.finalizer.example.com") + secret := newSecret("cloud-credentials", "default") + secret.Annotations = map[string]string{controller.CredentialPolicyAnnotation: controller.PolicyDelete} + + r, c := newReconciler(func(context.Context, openstack.PurgeOptions) error { return nil }, cluster, secret) + + if _, err := r.Reconcile(context.Background(), reconcileRequest("mycluster", "default")); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + var gotSecret corev1.Secret + if err := c.Get(context.Background(), types.NamespacedName{Name: "cloud-credentials", Namespace: "default"}, &gotSecret); err != nil { + t.Errorf("expected credential secret to still exist, got err: %v", err) + } + + got := getClusterOrNil(t, c, "mycluster", "default") + if got == nil { + t.Fatal("expected cluster to still exist") + } + if got.Annotations[controller.RetryAnnotation] == "" { + t.Error("expected retry annotation to be set") + } + if !controllerutil.ContainsFinalizer(got, controller.Finalizer) { + t.Error("expected janitor finalizer to still be present") + } +} + +// Scenario: removing the janitor finalizer fails on Update → propagated +func TestReconcile_RemoveFinalizer_UpdateError_Propagates(t *testing.T) { + cluster := newCluster("mycluster", "default", withFinalizer, withDeletionTimestamp) + secret := newSecret("cloud-credentials", "default") // no credential-policy-delete annotation + + r, _ := newReconcilerWithInterceptors( + func(context.Context, openstack.PurgeOptions) error { return nil }, + interceptor.Funcs{ + Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + return errors.New("update failed") + }, + }, + cluster, secret, + ) + + _, err := r.Reconcile(context.Background(), reconcileRequest("mycluster", "default")) + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "removing finalizer:") { + t.Errorf("expected error to wrap %q, got: %v", "removing finalizer:", err) + } +} + +// Scenario: credential policy "delete", last finalizer, but deleting the +// secret fails with a non-NotFound error → propagated +func TestDeleteSecret_ErrorPath_ViaReconcile(t *testing.T) { + cluster := newCluster("mycluster", "default", withFinalizer, withDeletionTimestamp) + secret := newSecret("cloud-credentials", "default") + secret.Annotations = map[string]string{controller.CredentialPolicyAnnotation: controller.PolicyDelete} + + r, _ := newReconcilerWithInterceptors( + func(context.Context, openstack.PurgeOptions) error { return nil }, + interceptor.Funcs{ + Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + return errors.New("delete failed") + }, + }, + cluster, secret, + ) + + _, err := r.Reconcile(context.Background(), reconcileRequest("mycluster", "default")) + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "deleting credential secret:") { + t.Errorf("expected error to wrap %q, got: %v", "deleting credential secret:", err) + } +} + +// Scenario: PurgeFunc is nil → falls back to the real openstack.PurgeResources, +// which fails fast (no matching cloud in clouds.yaml) and triggers a retry. +func TestPurge_NilPurgeFunc_FallsBackToPurgeResources(t *testing.T) { + cluster := newCluster("mycluster", "default", withFinalizer, withDeletionTimestamp) + secret := newSecret("cloud-credentials", "default") // clouds.yaml: "clouds: {}" — no "openstack" entry + r, c := newReconciler(nil, cluster, secret) // PurgeFunc left nil + + if _, err := r.Reconcile(context.Background(), reconcileRequest("mycluster", "default")); err != nil { + t.Fatalf("expected nil (retry handled internally), got: %v", err) + } + + got := getClusterOrNil(t, c, "mycluster", "default") + if got == nil { + t.Fatal("cluster not found after reconcile") + } + if got.Annotations[controller.RetryAnnotation] == "" { + t.Error("expected retry annotation to be set after fallback PurgeResources failure") + } +} diff --git a/internal/openstack/cloud_test.go b/internal/openstack/cloud_test.go index 9992b41..3f543f9 100644 --- a/internal/openstack/cloud_test.go +++ b/internal/openstack/cloud_test.go @@ -6,9 +6,11 @@ import ( "crypto/x509" "encoding/json" "encoding/pem" + "errors" "fmt" "net/http" "net/http/httptest" + "strings" "testing" "github.com/azimuth-cloud/cluster-api-janitor-openstack/internal/openstack" @@ -279,6 +281,154 @@ func TestAuthenticate_Password_Successful(t *testing.T) { } } +// Scenario: passwordScope variants (project id, project name with each domain +// fallback, project name with no domain info, domain-only scopes, no scope). +func TestAuthenticate_PasswordScope_Variants(t *testing.T) { + cases := []struct { + name string + authExtra string + wantScope func(t *testing.T, scope map[string]any) + }{ + {"project_id set", " project_id: proj-999\n", func(t *testing.T, s map[string]any) { + proj, _ := s["project"].(map[string]any) + if proj["id"] != "proj-999" { + t.Errorf("expected project id proj-999, got %v", proj) + } + }}, + {"project_name + project_domain_id", " project_name: myproj\n project_domain_id: dom-1\n", func(t *testing.T, s map[string]any) { + proj, _ := s["project"].(map[string]any) + domain, _ := proj["domain"].(map[string]any) + if proj["name"] != "myproj" || domain["id"] != "dom-1" { + t.Errorf("unexpected: %v", s) + } + }}, + {"project_name + project_domain_name", " project_name: myproj\n project_domain_name: MyDomain\n", func(t *testing.T, s map[string]any) { + proj, _ := s["project"].(map[string]any) + domain, _ := proj["domain"].(map[string]any) + if domain["name"] != "MyDomain" { + t.Errorf("unexpected: %v", s) + } + }}, + {"project_name + user_domain_id fallback", " project_name: myproj\n user_domain_id: udom-1\n", func(t *testing.T, s map[string]any) { + proj, _ := s["project"].(map[string]any) + domain, _ := proj["domain"].(map[string]any) + if domain["id"] != "udom-1" { + t.Errorf("expected fallback to user_domain_id, got: %v", s) + } + }}, + {"project_name + user_domain_name fallback", " project_name: myproj\n user_domain_name: UserDom\n", func(t *testing.T, s map[string]any) { + proj, _ := s["project"].(map[string]any) + domain, _ := proj["domain"].(map[string]any) + if domain["name"] != "UserDom" { + t.Errorf("expected fallback to user_domain_name, got: %v", s) + } + }}, + {"project_name, no domain info", " project_name: myproj\n", func(t *testing.T, s map[string]any) { + proj, _ := s["project"].(map[string]any) + if _, ok := proj["domain"]; ok { + t.Errorf("expected no domain key, got: %v", proj) + } + }}, + {"domain_id only, no project", " domain_id: dom-only\n", func(t *testing.T, s map[string]any) { + domain, _ := s["domain"].(map[string]any) + if domain["id"] != "dom-only" { + t.Errorf("unexpected: %v", s) + } + }}, + {"domain_name only, no project", " domain_name: DomOnly\n", func(t *testing.T, s map[string]any) { + domain, _ := s["domain"].(map[string]any) + if domain["name"] != "DomOnly" { + t.Errorf("unexpected: %v", s) + } + }}, + {"nothing set, no scope", "", func(t *testing.T, s map[string]any) { + if s != nil { + t.Errorf("expected nil scope, got: %v", s) + } + }}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + ks := newKeystoneServer(t) + ks.catalog = catalogWith(serviceEntry("compute", endpoint("public", "RegionOne", "http://x"))) + clouds := fmt.Sprintf(` +clouds: + openstack: + auth_type: v3password + auth: + auth_url: %s + username: alice + password: s3cret +%s`, ks.URL, c.authExtra) + _, err := openstack.Authenticate(context.Background(), clouds, "openstack", "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + auth, _ := ks.lastTokenBody["auth"].(map[string]any) + var scopeMap map[string]any + if scope, ok := auth["scope"]; ok && scope != nil { + scopeMap, _ = scope.(map[string]any) + } + c.wantScope(t, scopeMap) + }) + } +} + +// Scenario: user_id set → skips username and domain +func TestAuthenticate_Password_UserIDSet_SkipsUsernameAndDomain(t *testing.T) { + ks := newKeystoneServer(t) + ks.catalog = catalogWith(serviceEntry("compute", endpoint("public", "RegionOne", "http://x"))) + clouds := fmt.Sprintf(` +clouds: + openstack: + auth_type: v3password + auth: + auth_url: %s + user_id: uid-123 + password: s3cret +`, ks.URL) + _, err := openstack.Authenticate(context.Background(), clouds, "openstack", "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + auth, _ := ks.lastTokenBody["auth"].(map[string]any) + identity, _ := auth["identity"].(map[string]any) + pw, _ := identity["password"].(map[string]any) + user, _ := pw["user"].(map[string]any) + if user["id"] != "uid-123" { + t.Errorf("expected user id uid-123, got %v", user) + } + if _, hasName := user["name"]; hasName { + t.Errorf("expected no name key when user_id set, got %v", user) + } +} + +// Scenario: username set without any user domain → domain key omitted +func TestAuthenticate_Password_NoUserDomain_OmitsDomainKey(t *testing.T) { + ks := newKeystoneServer(t) + ks.catalog = catalogWith(serviceEntry("compute", endpoint("public", "RegionOne", "http://x"))) + clouds := fmt.Sprintf(` +clouds: + openstack: + auth_type: v3password + auth: + auth_url: %s + username: alice + password: s3cret +`, ks.URL) + _, err := openstack.Authenticate(context.Background(), clouds, "openstack", "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + auth, _ := ks.lastTokenBody["auth"].(map[string]any) + identity, _ := auth["identity"].(map[string]any) + pw, _ := identity["password"].(map[string]any) + user, _ := pw["user"].(map[string]any) + if _, hasDomain := user["domain"]; hasDomain { + t.Errorf("expected no domain key, got %v", user) + } +} + // Scenario: auth_type omitted but username present is inferred as v3password func TestAuthenticate_Password_InferredFromUsername(t *testing.T) { ks := newKeystoneServer(t) @@ -368,6 +518,116 @@ func TestAuthenticate_TokenRequestFails_NonFatal(t *testing.T) { } } +// Scenario: httpStatusError exposes the HTTP status code and message +func TestHTTPStatusError_ExposesStatusCode(t *testing.T) { + ks := newKeystoneServer(t) + ks.tokenStatus = http.StatusInternalServerError + + clouds := buildCloudsYAML(ks.URL, "v3applicationcredential") + _, err := openstack.Authenticate(context.Background(), clouds, "openstack", "") + + if err == nil { + t.Fatal("expected error, got nil") + } + var target interface{ StatusCode() int } + if !errors.As(err, &target) { + t.Fatalf("expected error implementing StatusCode() int, got %T: %v", err, err) + } + if target.StatusCode() != http.StatusInternalServerError { + t.Errorf("expected StatusCode() 500, got %d", target.StatusCode()) + } + if err.Error() != "HTTP 500" { + t.Errorf("expected message %q, got %q", "HTTP 500", err.Error()) + } +} + +// Scenario: Token response body is not valid JSON +func TestAuthenticate_TokenResponse_InvalidJSON_ReturnsError(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/v3/auth/tokens", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("X-Subject-Token", "tok") + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte("{not valid json")) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + clouds := buildCloudsYAML(srv.URL, "v3applicationcredential") + _, err := openstack.Authenticate(context.Background(), clouds, "openstack", "") + + if err == nil { + t.Fatal("expected JSON decode error, got nil") + } +} + +// Scenario: Catalog request fails with a non-404 error +func TestAuthenticate_CatalogReturnsServerError_PropagatesError(t *testing.T) { + ks := newKeystoneServer(t) + ks.catalogStatus = http.StatusInternalServerError + + clouds := buildCloudsYAML(ks.URL, "v3applicationcredential") + _, err := openstack.Authenticate(context.Background(), clouds, "openstack", "") + + if err == nil { + t.Fatal("expected error for catalog HTTP 500, got nil") + } +} + +// Scenario: Catalog response body is not valid JSON +func TestAuthenticate_CatalogResponse_InvalidJSON_ReturnsError(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/v3/auth/tokens", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("X-Subject-Token", "tok") + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(map[string]any{ + "token": map[string]any{"user": map[string]any{"id": "u"}}, + }) + }) + mux.HandleFunc("/v3/auth/catalog", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("not json")) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + clouds := buildCloudsYAML(srv.URL, "v3applicationcredential") + _, err := openstack.Authenticate(context.Background(), clouds, "openstack", "") + + if err == nil { + t.Fatal("expected catalog JSON decode error, got nil") + } +} + +// Scenario: No interface configured → defaults to "public" +func TestAuthenticate_NoInterfaceSpecified_DefaultsToPublic(t *testing.T) { + ks := newKeystoneServer(t) + ks.catalog = catalogWith( + serviceEntry("compute", endpoint("public", "RegionOne", "http://compute-public.example.com")), + serviceEntry("network", endpoint("internal", "RegionOne", "http://network-internal.example.com")), + ) + + clouds := fmt.Sprintf(` +clouds: + openstack: + auth_type: v3applicationcredential + auth: + auth_url: %s + application_credential_id: appcred-abc123 + application_credential_secret: super-secret +`, ks.URL) // no "interface:" line + session, err := openstack.Authenticate(context.Background(), clouds, "openstack", "") + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !session.HasEndpoint("compute") { + t.Error("expected compute endpoint (public, default interface)") + } + if session.HasEndpoint("network") { + t.Error("expected network endpoint absent (only internal, default is public)") + } +} + // Scenario: Catalog returns 404 // Given a valid Keystone URL but the catalog returns 404 // When the operator loads the catalog @@ -627,6 +887,21 @@ func TestAuthenticate_InvalidCACert_TLSFails(t *testing.T) { } } +// Scenario: CA cert content is not valid PEM → AppendCertsFromPEM fails +func TestAuthenticate_MalformedPEM_ReturnsError(t *testing.T) { + ks := newTLSKeystoneServer(t) + + clouds := buildCloudsYAML(ks.URL, "v3applicationcredential") + _, err := openstack.Authenticate(context.Background(), clouds, "openstack", "not a valid pem certificate at all") + + if err == nil { + t.Fatal("expected error for malformed CA cert PEM, got nil") + } + if !strings.Contains(err.Error(), "append CA certificate") { + t.Errorf("expected AppendCertsFromPEM failure message, got: %v", err) + } +} + // ── AppCredentialID ───────────────────────────────────────────────────────── // Scenario: Extracting application credential ID from clouds.yaml @@ -649,6 +924,14 @@ clouds: } } +// Scenario: Malformed clouds.yaml → parse error +func TestAppCredentialID_InvalidYAML_ReturnsError(t *testing.T) { + _, err := openstack.AppCredentialID("not: valid: yaml: :", "openstack") + if err == nil { + t.Fatal("expected YAML parse error, got nil") + } +} + // Scenario: Cloud absent → error func TestAppCredentialID_CloudNotFound(t *testing.T) { clouds := ` diff --git a/internal/openstack/internal_test.go b/internal/openstack/internal_test.go new file mode 100644 index 0000000..d72e5b7 --- /dev/null +++ b/internal/openstack/internal_test.go @@ -0,0 +1,57 @@ +/* +Copyright 2026. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package openstack + +import ( + "context" + "errors" + "net/http" + "testing" +) + +func TestIsTransient_NonHTTPStatusError_ReturnsFalse(t *testing.T) { + if isTransient(errors.New("boom")) { + t.Error("expected isTransient to return false for a non-httpStatusError") + } +} + +// erroringBody is an io.ReadCloser whose Read always fails, used to simulate +// a network error while streaming a response body. +type erroringBody struct{} + +func (erroringBody) Read([]byte) (int, error) { return 0, errors.New("simulated read error") } +func (erroringBody) Close() error { return nil } + +// brokenTransport returns a 200 response with a body that fails to read, +// exercising doGet's io.ReadAll error path. +type brokenTransport struct{} + +func (brokenTransport) RoundTrip(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Body: erroringBody{}, + Header: make(http.Header), + }, nil +} + +func TestDoGet_ReadBodyError_ReturnsError(t *testing.T) { + s := &Session{httpClient: &http.Client{Transport: brokenTransport{}}} + _, err := s.doGet(context.Background(), "http://example.invalid/resource") + if err == nil { + t.Fatal("expected error reading response body, got nil") + } +} diff --git a/internal/openstack/purge_test.go b/internal/openstack/purge_test.go new file mode 100644 index 0000000..84fae8d --- /dev/null +++ b/internal/openstack/purge_test.go @@ -0,0 +1,325 @@ +package openstack_test + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/go-logr/logr" + + "github.com/azimuth-cloud/cluster-api-janitor-openstack/internal/openstack" +) + +// purgeTestServer is a single mock OpenStack server that advertises every +// service PurgeResources touches (network, load-balancer, volumev3, identity) +// from one self-referential catalog, so PurgeResources can run end-to-end +// against a single httptest.Server. +type purgeTestServer struct { + *httptest.Server + mu sync.Mutex + + tokenStatus int // default http.StatusCreated; set 404 to simulate a deleted appcred + + fipList []fipRecord + lbList []lbRecord + sgList []sgRecord + volList []cinderVolumeRecord + snapList []cinderVolumeRecord + + fipListStatus int // non-zero overrides the GET /v2.0/floatingips response status + volListStatus int // non-zero overrides the GET /volumes/detail response status + + fipListCalls, lbListCalls, sgListCalls, volListCalls, snapListCalls, appcredDeleteCalls int + deletedAppcredID string +} + +func newPurgeTestServer(t *testing.T) *purgeTestServer { + t.Helper() + srv := &purgeTestServer{tokenStatus: http.StatusCreated} + mux := http.NewServeMux() + + mux.HandleFunc("/v3/auth/tokens", func(w http.ResponseWriter, r *http.Request) { + srv.mu.Lock() + status := srv.tokenStatus + srv.mu.Unlock() + if status >= 400 { + w.WriteHeader(status) + return + } + w.Header().Set("X-Subject-Token", "purge-token") + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _ = json.NewEncoder(w).Encode(map[string]any{ + "token": map[string]any{"user": map[string]any{"id": "purge-user"}}, + }) + }) + + mux.HandleFunc("/v3/auth/catalog", func(w http.ResponseWriter, r *http.Request) { + selfURL := "http://" + r.Host + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "catalog": []any{ + map[string]any{"type": "network", "endpoints": []any{ + map[string]any{"interface": "public", "region_id": "RegionOne", "url": selfURL}, + }}, + map[string]any{"type": "load-balancer", "endpoints": []any{ + map[string]any{"interface": "public", "region_id": "RegionOne", "url": selfURL}, + }}, + map[string]any{"type": "volumev3", "endpoints": []any{ + map[string]any{"interface": "public", "region_id": "RegionOne", "url": selfURL}, + }}, + map[string]any{"type": "identity", "endpoints": []any{ + map[string]any{"interface": "public", "region_id": "RegionOne", "url": selfURL}, + }}, + }, + }) + }) + + mux.HandleFunc("/v2.0/floatingips", func(w http.ResponseWriter, r *http.Request) { + srv.mu.Lock() + srv.fipListCalls++ + status := srv.fipListStatus + list := srv.fipList + srv.mu.Unlock() + if status != 0 { + w.WriteHeader(status) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"floatingips": list}) + }) + mux.HandleFunc("/v2.0/floatingips/", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + + mux.HandleFunc("/v2/lbaas/loadbalancers", func(w http.ResponseWriter, r *http.Request) { + srv.mu.Lock() + srv.lbListCalls++ + list := srv.lbList + srv.mu.Unlock() + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"loadbalancers": list}) + }) + mux.HandleFunc("/v2/lbaas/loadbalancers/", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + + mux.HandleFunc("/v2.0/security-groups", func(w http.ResponseWriter, r *http.Request) { + srv.mu.Lock() + srv.sgListCalls++ + list := srv.sgList + srv.mu.Unlock() + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"security_groups": list}) + }) + mux.HandleFunc("/v2.0/security-groups/", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + + mux.HandleFunc("/snapshots/detail", func(w http.ResponseWriter, r *http.Request) { + srv.mu.Lock() + srv.snapListCalls++ + list := srv.snapList + srv.mu.Unlock() + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"snapshots": list}) + }) + mux.HandleFunc("/snapshots/", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + + mux.HandleFunc("/volumes/detail", func(w http.ResponseWriter, r *http.Request) { + srv.mu.Lock() + srv.volListCalls++ + status := srv.volListStatus + list := srv.volList + srv.mu.Unlock() + if status != 0 { + w.WriteHeader(status) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"volumes": list}) + }) + mux.HandleFunc("/volumes/", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + + mux.HandleFunc("/v3/users/", func(w http.ResponseWriter, r *http.Request) { + parts := strings.Split(r.URL.Path, "/application_credentials/") + srv.mu.Lock() + srv.appcredDeleteCalls++ + if len(parts) == 2 { + srv.deletedAppcredID = parts[1] + } + srv.mu.Unlock() + w.WriteHeader(http.StatusNoContent) + }) + + srv.Server = httptest.NewServer(mux) + t.Cleanup(srv.Server.Close) + return srv +} + +func buildPurgeCloudsYAML(authURL string) string { + return fmt.Sprintf(` +clouds: + openstack: + auth_type: v3applicationcredential + auth: + auth_url: %s + application_credential_id: purge-appcred-id + application_credential_secret: purge-secret + interface: public + region_name: RegionOne +`, authURL) +} + +func TestPurgeResources_AuthenticateError_Propagates(t *testing.T) { + err := openstack.PurgeResources(context.Background(), openstack.PurgeOptions{ + CloudsYAML: "not: valid: yaml: :", + CloudName: "openstack", + Logger: logr.Discard(), + }) + if err == nil { + t.Fatal("expected error from Authenticate, got nil") + } +} + +func TestPurgeResources_Unauthenticated_IncludeAppcredTrue_ReturnsNilAndSkips(t *testing.T) { + srv := newPurgeTestServer(t) + srv.tokenStatus = http.StatusNotFound + + err := openstack.PurgeResources(context.Background(), openstack.PurgeOptions{ + CloudsYAML: buildPurgeCloudsYAML(srv.URL), + CloudName: "openstack", + IncludeAppcred: true, + Logger: logr.Discard(), + }) + if err != nil { + t.Fatalf("expected nil error when appcred already deleted, got: %v", err) + } +} + +func TestPurgeResources_Unauthenticated_IncludeAppcredFalse_ReturnsAuthenticationError(t *testing.T) { + srv := newPurgeTestServer(t) + srv.tokenStatus = http.StatusNotFound + + err := openstack.PurgeResources(context.Background(), openstack.PurgeOptions{ + CloudsYAML: buildPurgeCloudsYAML(srv.URL), + CloudName: "openstack", + IncludeAppcred: false, + Logger: logr.Discard(), + }) + var target *openstack.AuthenticationError + if !errorAs(err, &target) { + t.Fatalf("expected *AuthenticationError, got %T: %v", err, err) + } +} + +func TestPurgeResources_DeleteFloatingIPsError_ShortCircuits(t *testing.T) { + srv := newPurgeTestServer(t) + srv.fipListStatus = http.StatusInternalServerError + + err := openstack.PurgeResources(context.Background(), openstack.PurgeOptions{ + CloudsYAML: buildPurgeCloudsYAML(srv.URL), + CloudName: "openstack", + IncludeVolumes: true, + IncludeAppcred: true, + Logger: logr.Discard(), + }) + if err == nil { + t.Fatal("expected error from DeleteFloatingIPs, got nil") + } + + srv.mu.Lock() + defer srv.mu.Unlock() + if srv.lbListCalls != 0 || srv.sgListCalls != 0 || srv.volListCalls != 0 || srv.snapListCalls != 0 || srv.appcredDeleteCalls != 0 { + t.Errorf("expected no further resource calls after FIP error, got lb=%d sg=%d vol=%d snap=%d appcred=%d", + srv.lbListCalls, srv.sgListCalls, srv.volListCalls, srv.snapListCalls, srv.appcredDeleteCalls) + } +} + +func TestPurgeResources_DeleteVolumesError_ShortCircuits(t *testing.T) { + srv := newPurgeTestServer(t) + srv.volListStatus = http.StatusInternalServerError + + err := openstack.PurgeResources(context.Background(), openstack.PurgeOptions{ + CloudsYAML: buildPurgeCloudsYAML(srv.URL), + CloudName: "openstack", + IncludeVolumes: true, + IncludeAppcred: true, + Logger: logr.Discard(), + }) + if err == nil { + t.Fatal("expected error from DeleteVolumes, got nil") + } + + srv.mu.Lock() + defer srv.mu.Unlock() + if srv.appcredDeleteCalls != 0 { + t.Errorf("expected DeleteAppCredential not to be called after DeleteVolumes error, got %d calls", srv.appcredDeleteCalls) + } +} + +func TestPurgeResources_IncludeVolumesFalse_SkipsSnapshotsAndVolumes(t *testing.T) { + srv := newPurgeTestServer(t) + + err := openstack.PurgeResources(context.Background(), openstack.PurgeOptions{ + CloudsYAML: buildPurgeCloudsYAML(srv.URL), + CloudName: "openstack", + IncludeVolumes: false, + IncludeAppcred: false, + Logger: logr.Discard(), + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + srv.mu.Lock() + defer srv.mu.Unlock() + if srv.volListCalls != 0 || srv.snapListCalls != 0 { + t.Errorf("expected no volume/snapshot list calls when IncludeVolumes is false, got vol=%d snap=%d", srv.volListCalls, srv.snapListCalls) + } +} + +func TestPurgeResources_IncludeAppcredTrue_CallsDeleteAppCredential(t *testing.T) { + srv := newPurgeTestServer(t) + + err := openstack.PurgeResources(context.Background(), openstack.PurgeOptions{ + CloudsYAML: buildPurgeCloudsYAML(srv.URL), + CloudName: "openstack", + IncludeVolumes: false, + IncludeAppcred: true, + Logger: logr.Discard(), + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + srv.mu.Lock() + defer srv.mu.Unlock() + if srv.deletedAppcredID != "purge-appcred-id" { + t.Errorf("expected appcred %q to be deleted, got %q", "purge-appcred-id", srv.deletedAppcredID) + } +} + +func TestPurgeResources_FullSuccess_ReturnsNil(t *testing.T) { + srv := newPurgeTestServer(t) + + err := openstack.PurgeResources(context.Background(), openstack.PurgeOptions{ + CloudsYAML: buildPurgeCloudsYAML(srv.URL), + CloudName: "openstack", + IncludeVolumes: true, + IncludeAppcred: false, + Logger: logr.Discard(), + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} diff --git a/internal/openstack/resources_test.go b/internal/openstack/resources_test.go index 3d507aa..4a33bb7 100644 --- a/internal/openstack/resources_test.go +++ b/internal/openstack/resources_test.go @@ -612,6 +612,101 @@ func TestDeleteFloatingIPs_NothingToDelete_NoVerification(t *testing.T) { } } +// Scenario: DELETE returns 404 (already gone) → treated as success, not an error +func TestDeleteFloatingIPs_DeleteReturns404_TreatedAsSuccess(t *testing.T) { + srv := newNetworkTestServer(t) + fip := fipRecord{ID: "fip-404", Description: fipDesc("mycluster")} + srv.fipLists = [][]fipRecord{{fip}, {}} + srv.deleteStatus["fip-404"] = http.StatusNotFound + + session := srv.authenticate(t) + if err := session.DeleteFloatingIPs(context.Background(), logr.Discard(), "mycluster"); err != nil { + t.Fatalf("expected 404 on delete to be treated as success, got: %v", err) + } +} + +// Scenario: pagination edge cases in nextPageURL — malformed top-level JSON, +// missing "*_links" field, malformed links array, and no "next" rel present. +func TestListPages_PaginationEdgeCases(t *testing.T) { + cases := []struct { + name string + body string + wantErr bool + }{ + { + name: "malformed top-level JSON", + body: `{not valid`, + wantErr: true, + }, + { + name: "missing floatingips_links field", + body: `{"floatingips": [{"id":"a","description":""}]}`, + wantErr: false, + }, + { + name: "malformed links array", + body: `{"floatingips": [], "floatingips_links": "not an array"}`, + wantErr: false, + }, + { + name: "no next rel present", + body: `{"floatingips": [], "floatingips_links": [{"rel":"previous","href":"http://x/prev"}]}`, + wantErr: false, + }, + } + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/v3/auth/tokens", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("X-Subject-Token", "tok") + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(map[string]any{ + "token": map[string]any{"user": map[string]any{"id": "u"}}, + }) + }) + mux.HandleFunc("/v3/auth/catalog", func(w http.ResponseWriter, r *http.Request) { + selfURL := "http://" + r.Host + _ = json.NewEncoder(w).Encode(map[string]any{ + "catalog": []any{ + map[string]any{"type": "network", "endpoints": []any{ + map[string]any{"interface": "public", "region_id": "RegionOne", "url": selfURL}, + }}, + }, + }) + }) + mux.HandleFunc("/v2.0/floatingips", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(tt.body)) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + cloudsYAML := fmt.Sprintf(` +clouds: + openstack: + auth_type: v3applicationcredential + auth: + auth_url: %s + application_credential_id: id + application_credential_secret: secret + interface: public +`, srv.URL) + session, err := openstack.Authenticate(context.Background(), cloudsYAML, "openstack", "") + if err != nil { + t.Fatalf("authenticate: %v", err) + } + session.SleepFunc = func(d time.Duration) {} + + err = session.DeleteFloatingIPs(context.Background(), logr.Discard(), "mycluster") + if tt.wantErr && err == nil { + t.Fatal("expected error, got nil") + } + if !tt.wantErr && err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + } +} + // Scenario: Pas d'endpoint "network" dans le catalogue → CatalogError func TestDeleteFloatingIPs_NoNetworkEndpoint_ReturnsCatalogError(t *testing.T) { // Use a Keystone server that only advertises a "compute" endpoint (no "network") @@ -1353,6 +1448,69 @@ clouds: return session } +// newRawVolumesServer builds a self-referential "volumev3" Keystone+Cinder mock +// whose /volumes/detail handler returns a caller-supplied raw response body, +// for exercising listVolumeItems' JSON-decode error paths. +func newRawVolumesServer(t *testing.T, body string) *openstack.Session { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("/v3/auth/tokens", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("X-Subject-Token", "tok") + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(map[string]any{ + "token": map[string]any{"user": map[string]any{"id": "u"}}, + }) + }) + mux.HandleFunc("/v3/auth/catalog", func(w http.ResponseWriter, r *http.Request) { + selfURL := "http://" + r.Host + _ = json.NewEncoder(w).Encode(map[string]any{ + "catalog": []any{ + map[string]any{"type": "volumev3", "endpoints": []any{ + map[string]any{"interface": "public", "region_id": "RegionOne", "url": selfURL}, + }}, + }, + }) + }) + mux.HandleFunc("/volumes/detail", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(body)) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + cloudsYAML := fmt.Sprintf(` +clouds: + openstack: + auth_type: v3applicationcredential + auth: + auth_url: %s + application_credential_id: id + application_credential_secret: secret + interface: public +`, srv.URL) + session, err := openstack.Authenticate(context.Background(), cloudsYAML, "openstack", "") + if err != nil { + t.Fatalf("authenticate: %v", err) + } + session.SleepFunc = func(d time.Duration) {} + return session +} + +// Scenario: top-level list response is not valid JSON +func TestDeleteVolumes_MalformedListResponse_ReturnsError(t *testing.T) { + session := newRawVolumesServer(t, `{not valid json`) + if err := session.DeleteVolumes(context.Background(), logr.Discard(), "mycluster"); err == nil { + t.Fatal("expected error for malformed top-level JSON") + } +} + +// Scenario: "volumes" key is present but not an array +func TestDeleteVolumes_MalformedVolumesKey_ReturnsError(t *testing.T) { + session := newRawVolumesServer(t, `{"volumes": "not an array"}`) + if err := session.DeleteVolumes(context.Background(), logr.Discard(), "mycluster"); err == nil { + t.Fatal("expected error for malformed volumes key") + } +} + // ── Epic 5: Cinder Volume Management ───────────────────────────────────────── // ── US5.1: Identify volumes of a cluster ─────────────────────────────────────