Check content-type in importer to warn against unexpected kubevirt im…

…ports (#2909) * Check content-type before importing to warn against unexpected content imports This commit adds a check in the http-datasource format reader to trigger a warning when importing unexpected content-types. Signed-off-by: Alvaro Romero <alromero@redhat.com> * Test import with unexpected content-type This commit adds a test to cover a import with unexpected content-type. It also updates the badserver to allow mocking this behavior. Signed-off-by: Alvaro Romero <alromero@redhat.com> --------- Signed-off-by: Alvaro Romero <alromero@redhat.com>
kubevirt · Nov 9, 2023 · 56fb051 · 56fb051
1 parent 1a04ba9
commit 56fb051
Show file tree

Hide file tree

Showing 4 changed files with 75 additions and 16 deletions.
diff --git a/pkg/importer/http-datasource.go b/pkg/importer/http-datasource.go
@@ -44,10 +44,12 @@ import (
 )
 
 const (
-	tempFile         = "tmpimage"
-	nbdkitPid        = "/tmp/nbdkit.pid"
-	nbdkitSocket     = "/tmp/nbdkit.sock"
-	defaultUserAgent = "cdi-golang-importer"
+	tempFile          = "tmpimage"
+	nbdkitPid         = "/tmp/nbdkit.pid"
+	nbdkitSocket      = "/tmp/nbdkit.sock"
+	defaultUserAgent  = "cdi-golang-importer"
+	httpContentType   = "Content-Type"
+	httpContentLength = "Content-Length"
 )
 
 // HTTPDataSource is the data provider for http(s) endpoints.
@@ -96,7 +98,7 @@ func NewHTTPDataSource(endpoint, accessKey, secKey, certDir string, contentType
 		return nil, errors.Wrap(err, "Error getting extra headers for HTTP client")
 	}
 
-	httpReader, contentLength, brokenForQemuImg, err := createHTTPReader(ctx, ep, accessKey, secKey, certDir, extraHeaders, secretExtraHeaders)
+	httpReader, contentLength, brokenForQemuImg, err := createHTTPReader(ctx, ep, accessKey, secKey, certDir, extraHeaders, secretExtraHeaders, contentType)
 	if err != nil {
 		cancel()
 		return nil, err
@@ -294,7 +296,7 @@ func addExtraheaders(req *http.Request, extraHeaders []string) {
 	req.Header.Add("User-Agent", defaultUserAgent)
 }
 
-func createHTTPReader(ctx context.Context, ep *url.URL, accessKey, secKey, certDir string, extraHeaders, secretExtraHeaders []string) (io.ReadCloser, uint64, bool, error) {
+func createHTTPReader(ctx context.Context, ep *url.URL, accessKey, secKey, certDir string, extraHeaders, secretExtraHeaders []string, contentType cdiv1.DataVolumeContentType) (io.ReadCloser, uint64, bool, error) {
 	var brokenForQemuImg bool
 	client, err := createHTTPClient(certDir)
 	if err != nil {
@@ -334,6 +336,16 @@ func createHTTPReader(ctx context.Context, ep *url.URL, accessKey, secKey, certD
 		return nil, uint64(0), true, errors.Errorf("expected status code 200, got %d. Status: %s", resp.StatusCode, resp.Status)
 	}
 
+	if contentType == cdiv1.DataVolumeKubeVirt {
+		// Check the content-type if we are expecting a KubeVirt img.
+		if val, ok := resp.Header[httpContentType]; ok {
+			if strings.HasPrefix(val[0], "text/") {
+				// We will continue with the import nonetheless, but content might be unexpected.
+				klog.Warningf("Unexpected content type '%s'. Content might not be a KubeVirt image.", val[0])
+			}
+		}
+	}
+
 	acceptRanges, ok := resp.Header["Accept-Ranges"]
 	if !ok || acceptRanges[0] == "none" {
 		klog.V(2).Infof("Accept-Ranges isn't bytes, avoiding qemu-img")
@@ -416,7 +428,7 @@ func getContentLength(client *http.Client, ep *url.URL, accessKey, secKey string
 func parseHTTPHeader(resp *http.Response) uint64 {
 	var err error
 	total := uint64(0)
-	if val, ok := resp.Header["Content-Length"]; ok {
+	if val, ok := resp.Header[httpContentLength]; ok {
 		total, err = strconv.ParseUint(val[0], 10, 64)
 		if err != nil {
 			klog.Errorf("could not convert content length, got %v", err)

diff --git a/pkg/importer/http-datasource_test.go b/pkg/importer/http-datasource_test.go
@@ -232,7 +232,7 @@ var _ = Describe("Http client", func() {
 
 var _ = Describe("Http reader", func() {
 	It("should fail when passed an invalid cert directory", func() {
-		_, total, _, err := createHTTPReader(context.Background(), nil, "", "", "/invalid", nil, nil)
+		_, total, _, err := createHTTPReader(context.Background(), nil, "", "", "/invalid", nil, nil, cdiv1.DataVolumeKubeVirt)
 		Expect(err).To(HaveOccurred())
 		Expect(uint64(0)).To(Equal(total))
 	})
@@ -249,7 +249,7 @@ var _ = Describe("Http reader", func() {
 		defer ts.Close()
 		ep, err := url.Parse(ts.URL)
 		Expect(err).ToNot(HaveOccurred())
-		r, total, _, err := createHTTPReader(context.Background(), ep, "user", "password", "", nil, nil)
+		r, total, _, err := createHTTPReader(context.Background(), ep, "user", "password", "", nil, nil, cdiv1.DataVolumeKubeVirt)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(uint64(25)).To(Equal(total))
 		err = r.Close()
@@ -272,7 +272,7 @@ var _ = Describe("Http reader", func() {
 		defer ts.Close()
 		ep, err := url.Parse(ts.URL)
 		Expect(err).ToNot(HaveOccurred())
-		r, total, _, err := createHTTPReader(context.Background(), ep, "user", "password", "", nil, nil)
+		r, total, _, err := createHTTPReader(context.Background(), ep, "user", "password", "", nil, nil, cdiv1.DataVolumeKubeVirt)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(uint64(25)).To(Equal(total))
 		err = r.Close()
@@ -294,7 +294,7 @@ var _ = Describe("Http reader", func() {
 		defer ts.Close()
 		ep, err := url.Parse(ts.URL)
 		Expect(err).ToNot(HaveOccurred())
-		r, total, brokenForQemuImg, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil)
+		r, total, brokenForQemuImg, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil, cdiv1.DataVolumeKubeVirt)
 		Expect(brokenForQemuImg).To(BeFalse())
 		Expect(err).ToNot(HaveOccurred())
 		Expect(uint64(25)).To(Equal(total))
@@ -315,7 +315,7 @@ var _ = Describe("Http reader", func() {
 		defer ts.Close()
 		ep, err := url.Parse(ts.URL)
 		Expect(err).ToNot(HaveOccurred())
-		r, total, _, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil)
+		r, total, _, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil, cdiv1.DataVolumeKubeVirt)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(uint64(0)).To(Equal(total))
 		err = r.Close()
@@ -339,7 +339,7 @@ var _ = Describe("Http reader", func() {
 		defer ts.Close()
 		ep, err := url.Parse(ts.URL)
 		Expect(err).ToNot(HaveOccurred())
-		r, total, brokenForQemuImg, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil)
+		r, total, brokenForQemuImg, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil, cdiv1.DataVolumeKubeVirt)
 		Expect(brokenForQemuImg).To(BeTrue())
 		Expect(err).ToNot(HaveOccurred())
 		Expect(uint64(25)).To(Equal(total))
@@ -359,7 +359,7 @@ var _ = Describe("Http reader", func() {
 		defer ts.Close()
 		ep, err := url.Parse(ts.URL)
 		Expect(err).ToNot(HaveOccurred())
-		r, total, brokenForQemuImg, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil)
+		r, total, brokenForQemuImg, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil, cdiv1.DataVolumeKubeVirt)
 		Expect(brokenForQemuImg).To(BeTrue())
 		Expect(err).ToNot(HaveOccurred())
 		Expect(uint64(25)).To(Equal(total))
@@ -374,7 +374,7 @@ var _ = Describe("Http reader", func() {
 		defer ts.Close()
 		ep, err := url.Parse(ts.URL)
 		Expect(err).ToNot(HaveOccurred())
-		_, total, _, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil)
+		_, total, _, err := createHTTPReader(context.Background(), ep, "", "", "", nil, nil, cdiv1.DataVolumeKubeVirt)
 		Expect(err).To(HaveOccurred())
 		Expect(uint64(0)).To(Equal(total))
 		Expect("expected status code 200, got 500. Status: 500 Internal Server Error").To(Equal(err.Error()))
@@ -391,7 +391,7 @@ var _ = Describe("Http reader", func() {
 		defer ts.Close()
 		ep, err := url.Parse(ts.URL)
 		Expect(err).ToNot(HaveOccurred())
-		r, total, _, err := createHTTPReader(context.Background(), ep, "", "", "", []string{"Extra-Header: 123"}, nil)
+		r, total, _, err := createHTTPReader(context.Background(), ep, "", "", "", []string{"Extra-Header: 123"}, nil, cdiv1.DataVolumeKubeVirt)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(uint64(0)).To(Equal(total))
 		err = r.Close()

diff --git a/tests/badserver_test.go b/tests/badserver_test.go
@@ -2,9 +2,14 @@ package tests
 
 import (
 	"fmt"
+	"strings"
+	"time"
+
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 
+	"kubevirt.io/containerized-data-importer/pkg/common"
+	controller "kubevirt.io/containerized-data-importer/pkg/controller/common"
 	"kubevirt.io/containerized-data-importer/tests/framework"
 	"kubevirt.io/containerized-data-importer/tests/utils"
 
@@ -31,6 +36,31 @@ var _ = Describe("Problematic server responses", func() {
 		Entry("[rfe_id:4326][test_id:5076][crit:low][vendor:cnv-qe@redhat.com][level:component] Should succeed even if Accept-Ranges doesn't exist", "/no-accept-ranges/cirros-qcow2.img"),
 	)
 
+	It("Should warn against unexpected content-types when importing a kubevirt img", func() {
+		cdiBadServer := fmt.Sprintf("http://cdi-bad-webserver.%s:9090", f.CdiInstallNs)
+		pathname := "/bad-content-type/cirros-qcow2.img"
+		dataVolume = utils.NewDataVolumeWithHTTPImport("badserver-dv", "1Gi", cdiBadServer+pathname)
+		dataVolume.Annotations[controller.AnnPodRetainAfterCompletion] = "true"
+		By("creating DataVolume")
+		dataVolume, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dataVolume)
+		Expect(err).ToNot(HaveOccurred())
+		f.ForceBindPvcIfDvIsWaitForFirstConsumer(dataVolume)
+
+		err = utils.WaitForDataVolumePhase(f, f.Namespace.Name, cdiv1.Succeeded, dataVolume.Name)
+		Expect(err).ToNot(HaveOccurred())
+
+		By("Find importer pod after completion")
+		importer, err := utils.FindPodByPrefixOnce(f.K8sClient, dataVolume.Namespace, common.ImporterPodName, common.CDILabelSelector)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(importer.DeletionTimestamp).To(BeNil())
+
+		By("Verify HTTP request error in importer log")
+		Eventually(func() bool {
+			log, _ := f.RunKubectlCommand("logs", importer.Name, "-n", importer.Namespace)
+			return strings.Contains(log, "Unexpected content type 'text/html'. Content might not be a KubeVirt image.")
+		}, time.Minute, pollingInterval).Should(BeTrue())
+	})
+
 	AfterEach(func() {
 		By("deleting DataVolume")
 		err := utils.DeleteDataVolume(f.CdiClient, f.Namespace.Name, dataVolumeName)

diff --git a/tools/cdi-func-test-bad-webserver/main.go b/tools/cdi-func-test-bad-webserver/main.go
@@ -30,6 +30,22 @@ func flaky(w http.ResponseWriter, r *http.Request) {
 	w.WriteHeader(http.StatusServiceUnavailable)
 }
 
+func badContentType(w http.ResponseWriter, r *http.Request) {
+	actualFileURL := getEquivalentFileHostURL(r.URL.String())
+
+	resp, err := http.Get(actualFileURL)
+	if err != nil {
+		panic("Couldn't fetch URL")
+	}
+
+	w.Header().Set("Content-Type", "text/html")
+
+	_, err = io.Copy(w, resp.Body)
+	if err != nil {
+		panic(fmt.Errorf("badContentType: failed to write the response; %w", err))
+	}
+}
+
 func noAcceptRanges(w http.ResponseWriter, r *http.Request) {
 	actualFileURL := getEquivalentFileHostURL(r.URL.String())
 
@@ -79,6 +95,7 @@ func main() {
 	http.HandleFunc("/forbidden-HEAD/", failHEAD)
 	http.HandleFunc("/flaky/", flaky)
 	http.HandleFunc("/no-accept-ranges/", noAcceptRanges)
+	http.HandleFunc("/bad-content-type/", badContentType)
 	err := http.ListenAndServe(":9090", nil)
 	if err != nil {
 		log.Fatal("ListenAndServe: ", err)