From 6c5c300def1846c7e9402d0c38a0488eb463a41a Mon Sep 17 00:00:00 2001 From: Terry Howe Date: Mon, 21 Sep 2026 07:38:48 -0600 Subject: [PATCH] test: address second round of review feedback on OCI e2e tests - Verify pulled chart contents against the source fixture instead of only checking that a path of the expected name exists, so a registry serving a different artifact under the same tag fails the test. - Skip the invalid-credential test against a registry that serves anonymous access, where there is no authentication to exercise. - Delete the namespace the Kubernetes test creates, and wait for it to go, so runs do not accumulate namespaces. A caller-supplied HELM_E2E_NAMESPACE is left in place. Signed-off-by: Terry Howe --- test/e2e/README.md | 9 +++ test/e2e/oci_test.go | 161 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 169 insertions(+), 1 deletion(-) diff --git a/test/e2e/README.md b/test/e2e/README.md index 799fb00d4..678cec038 100644 --- a/test/e2e/README.md +++ b/test/e2e/README.md @@ -24,6 +24,11 @@ $ export HELM_E2E_REGISTRY=localhost:5000/charts $ export HELM_E2E_PLAIN_HTTP=true ``` +A default `registry:2` deployment serves anonymous access and accepts any +credential. Against such a registry there is no authentication to exercise, so +the invalid-credential test detects this and skips itself. Run against an +auth-enabled registry to cover that path. + ## Configuration | Variable | Required | Description | @@ -41,3 +46,7 @@ variable fails the test rather than silently skipping it. Each run pushes to repositories suffixed with a random run ID, so concurrent runs and repeated runs in a shared namespace do not collide. + +The Kubernetes test deletes the namespace it creates. If you supply +`HELM_E2E_NAMESPACE`, that namespace is left in place and only the releases are +uninstalled. diff --git a/test/e2e/oci_test.go b/test/e2e/oci_test.go index 03a382d84..92699fcbe 100644 --- a/test/e2e/oci_test.go +++ b/test/e2e/oci_test.go @@ -19,12 +19,24 @@ limitations under the License. package e2e import ( + "archive/tar" + "bytes" + "compress/gzip" + "context" "encoding/base64" "encoding/json" + "io" "os" "path/filepath" "strings" "testing" + "time" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/tools/clientcmd" ) // TestOCIRegistryPushPull pushes chart archives to the configured OCI registry @@ -98,6 +110,10 @@ func TestOCIRegistryPushPull(t *testing.T) { if fi.IsDir() != tt.expectPullDir { t.Errorf("expected directory=%t, got directory=%t", tt.expectPullDir, fi.IsDir()) } + + // Verify the round-trip actually returned the chart we pushed, + // not merely some artifact under the same tag. + assertChartMatches(t, chart(t, tt.chart), pullDir, tt.expectPullDir) }) } } @@ -112,7 +128,10 @@ func TestOCIRegistryInvalidCredentials(t *testing.T) { // credential itself. out, err := h.login(t, "invalid-"+h.runID) if err == nil { - t.Fatal("expected registry login to fail with an invalid credential, but it succeeded") + // An anonymous registry, such as a default registry:2 deployment, + // accepts any credential. There is no authentication to exercise, so + // skip rather than report a failure the registry cannot produce. + t.Skipf("Skipping invalid credential test: %s accepts unauthenticated access", h.registryHost()) } if !isAuthError(out) { t.Fatalf("expected an authentication failure, got a different error:\n%s", out) @@ -189,9 +208,13 @@ func TestOCIRegistryInstallToKubernetes(t *testing.T) { h := newHarness(t) h.mustLogin(t) + // A caller-supplied namespace is left alone; one we create for the run is + // removed again so repeated runs do not accumulate namespaces in the + // cluster. namespace := os.Getenv(envNamespace) if namespace == "" { namespace = "helm-e2e-" + h.runID + t.Cleanup(func() { deleteNamespace(t, namespace) }) } repo := h.repo("install") @@ -245,3 +268,139 @@ func TestOCIRegistryInstallToKubernetes(t *testing.T) { }) } } + +// assertChartMatches verifies that what was pulled into pullDir is the same +// chart as the fixture at src. Existence and file-vs-directory checks alone +// would pass if the registry served a different artifact under the same tag. +// +// A chart pulled without --untar is the pushed archive verbatim, so it is +// compared byte for byte. With --untar, helm expands the archive, so every +// regular file in the source archive is compared against its extracted +// counterpart. +func assertChartMatches(t *testing.T, src, pullDir string, untarred bool) { + t.Helper() + + if !untarred { + assertFilesEqual(t, src, filepath.Join(pullDir, filepath.Base(src))) + return + } + + want := archiveFiles(t, src) + if len(want) == 0 { + t.Fatalf("fixture %s contains no files", src) + } + for name, content := range want { + extracted := filepath.Join(pullDir, filepath.FromSlash(name)) + got, err := os.ReadFile(extracted) + if err != nil { + t.Errorf("expected extracted file %s: %v", extracted, err) + continue + } + if !bytes.Equal(got, content) { + t.Errorf("extracted file %s does not match the pushed chart", name) + } + } +} + +// assertFilesEqual compares two files byte for byte. +func assertFilesEqual(t *testing.T, want, got string) { + t.Helper() + wantBytes, err := os.ReadFile(want) + if err != nil { + t.Fatalf("reading %s: %v", want, err) + } + gotBytes, err := os.ReadFile(got) + if err != nil { + t.Fatalf("reading %s: %v", got, err) + } + if !bytes.Equal(wantBytes, gotBytes) { + t.Errorf("pulled chart %s does not match the pushed chart %s (%d vs %d bytes)", + got, want, len(gotBytes), len(wantBytes)) + } +} + +// archiveFiles returns the regular files in a gzipped tar archive, keyed by +// their slash-separated path within the archive. +func archiveFiles(t *testing.T, path string) map[string][]byte { + t.Helper() + + f, err := os.Open(path) + if err != nil { + t.Fatalf("opening %s: %v", path, err) + } + defer f.Close() + + gz, err := gzip.NewReader(f) + if err != nil { + t.Fatalf("reading %s as gzip: %v", path, err) + } + defer gz.Close() + + files := map[string][]byte{} + tr := tar.NewReader(gz) + for { + hdr, err := tr.Next() + if err == io.EOF { + break + } + if err != nil { + t.Fatalf("reading %s as tar: %v", path, err) + } + if hdr.Typeflag != tar.TypeReg { + continue + } + content, err := io.ReadAll(tr) + if err != nil { + t.Fatalf("reading %s from %s: %v", hdr.Name, path, err) + } + files[hdr.Name] = content + } + return files +} + +// deleteNamespace removes a namespace created by the test run. It is best +// effort in the sense that it reports a failure rather than aborting the test, +// but it does not silently leave the namespace behind. +func deleteNamespace(t *testing.T, namespace string) { + t.Helper() + + cfg, err := clientcmd.NewNonInteractiveDeferredLoadingClientConfig( + clientcmd.NewDefaultClientConfigLoadingRules(), + &clientcmd.ConfigOverrides{}, + ).ClientConfig() + if err != nil { + t.Errorf("building kube client config to delete namespace %s: %v", namespace, err) + return + } + client, err := kubernetes.NewForConfig(cfg) + if err != nil { + t.Errorf("building kube client to delete namespace %s: %v", namespace, err) + return + } + + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + + t.Logf("deleting namespace %s", namespace) + if err := client.CoreV1().Namespaces().Delete(ctx, namespace, metav1.DeleteOptions{}); err != nil { + if apierrors.IsNotFound(err) { + return + } + t.Errorf("deleting namespace %s: %v", namespace, err) + return + } + + // Deletion is asynchronous. Wait for the namespace to actually go away so + // that a namespace wedged in Terminating is reported rather than quietly + // accumulating in the cluster. + err = wait.PollUntilContextCancel(ctx, time.Second, true, func(ctx context.Context) (bool, error) { + _, err := client.CoreV1().Namespaces().Get(ctx, namespace, metav1.GetOptions{}) + if apierrors.IsNotFound(err) { + return true, nil + } + return false, err + }) + if err != nil { + t.Errorf("waiting for namespace %s to be deleted: %v", namespace, err) + } +}