Skip to content

add insecureSkipVerify to http source type #4167

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
xphyr wants to merge 11 commits into
base: main
Choose a base branch
from
Open

add insecureSkipVerify to http source type #4167

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9fea512
Adding new "insecureSkipVerify" option for http datasources
xphyr Jun 5, 2026
3bdcb7f
updated tests to check for working insecureSkipVerify
xphyr Jun 8, 2026
8e9ba34
adding in functional test
xphyr Jun 8, 2026
4649eeb
updating docs to reflect the new insecureSkipVerify option
xphyr Jun 9, 2026
02e1bee
simplifying the datavolume functional test
xphyr Jun 9, 2026
8b34ebd
adding in unit test for validating proper handling of AnnInsecureSkip…
xphyr Jun 10, 2026
2aea8ad
added unit test to import-controller around isSecureTLS function
xphyr Jun 10, 2026
32285f2
working on new tests, still not quite there yet
xphyr Jun 12, 2026
d7620a4
fixed the issue with my first unit test
xphyr Jun 12, 2026
e73d9a4
working on new tests, still not quite there yet
xphyr Jun 12, 2026
ab32873
adding in additional tests from @akalenyu
xphyr Jul 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/openapi-spec/swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4794,6 +4794,10 @@
"default": ""
}
},
"insecureSkipVerify": {
"description": "InsecureSkipVerify is a flag to skip certificate verification for the HTTP endpoint",
"type": "boolean"
},
"secretExtraHeaders": {
"description": "SecretExtraHeaders is a list of Secret references, each containing an extra HTTP header that may include sensitive information",
"type": "array",
Expand Down
2 changes: 1 addition & 1 deletion cmd/cdi-importer/importer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ func newDataSource(source string, contentType string, volumeMode v1.PersistentVo

switch source {
case cc.SourceHTTP:
ds, err := importer.NewHTTPDataSource(getHTTPEp(ep), acc, sec, certDir, cdiv1.DataVolumeContentType(contentType), checksum)
ds, err := importer.NewHTTPDataSource(getHTTPEp(ep), acc, sec, certDir, cdiv1.DataVolumeContentType(contentType), checksum, insecureTLS)
if err != nil {
errorCannotConnectDataSource(err, "http")
}
Expand Down
3 changes: 2 additions & 1 deletion doc/datavolumes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ spec:
## Source

### HTTP/S3/GCS/Registry source
DataVolumes are an abstraction on top of the annotations one can put on PVCs to trigger CDI. As such DVs have the notion of a 'source' that allows one to specify the source of the data. To import data from an external source, the source has to be either 'http' ,'S3', 'GCS' or 'registry'. If your source requires authentication, you can also pass in a `secretRef` to a Kubernetes [Secret](../manifests/example/endpoint-secret.yaml) containing the authentication information. TLS certificates for https/registry sources may be specified in a [ConfigMap](../manifests/example/cert-configmap.yaml) and referenced by `certConfigMap`. `secretRef` and `certConfigMap` must be in the same namespace as the DataVolume.
DataVolumes are an abstraction on top of the annotations one can put on PVCs to trigger CDI. As such DVs have the notion of a 'source' that allows one to specify the source of the data. To import data from an external source, the source has to be either 'http' ,'S3', 'GCS' or 'registry'. If your source requires authentication, you can also pass in a `secretRef` to a Kubernetes [Secret](../manifests/example/endpoint-secret.yaml) containing the authentication information. TLS certificates for https/registry sources may be specified in a [ConfigMap](../manifests/example/cert-configmap.yaml) and referenced by `certConfigMap`. `secretRef` and `certConfigMap` must be in the same namespace as the DataVolume. TLS verification can also be bypassed by applying `insecureSkipVerify: true` to the http source.

```yaml
apiVersion: cdi.kubevirt.io/v1beta1
Expand All @@ -143,6 +143,7 @@ spec:
[Get GCS example](../manifests/example/import-kubevirt-datavolume-gcs.yaml)
[Get secret example](../manifests/example/endpoint-secret.yaml)
[Get certificate example](../manifests/example/cert-configmap.yaml)
[Get insecureSkipVerify example](../manifests/example/import-kubevirt-datavolume-skip-tls.yaml)

#### Checksum Validation
For HTTP/HTTPS sources, you can specify a checksum to verify the integrity of the downloaded data. This helps ensure that the image has not been tampered with during transmission. The checksum field is optional but recommended for production environments.
Expand Down
14 changes: 14 additions & 0 deletions manifests/example/import-kubevirt-datavolume-skip-tls.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# This example assumes you are using a default storage class
apiVersion: cdi.kubevirt.io/v1beta1
kind: DataVolume
metadata:
name: my-data-volume
spec:
source:
http:
url: "https://172.16.15.6/iso/cirros-0.4.0-x86_64-disk.img"
insecureSkipVerify: true
storage:
resources:
requests:
storage: 500Mi
7 changes: 7 additions & 0 deletions pkg/apis/core/v1beta1/openapi_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions pkg/controller/common/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1753,6 +1753,9 @@ func UpdateHTTPAnnotations(annotations map[string]string, http *cdiv1.DataVolume
if http.Checksum != "" {
annotations[AnnChecksum] = http.Checksum
}
if http.InsecureSkipVerify != nil && *http.InsecureSkipVerify {
annotations[AnnInsecureSkipVerify] = "true"
}
Comment on lines +1756 to +1758

@akalenyu akalenyu Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we get some unit tests that validate AnnInsecureSkipVerify is correctly set when InsecureSkipVerify is true, or correctly absent when nil/false?

@xphyr xphyr Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think I now have this covered here: 8b34ebd Please be sure to take a close look, writing unit tests is NOT a strength of mine, so I relied on the help of AI to build this.

@akalenyu akalenyu Jun 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also a solid unit test, but I think we could take a similar approach to my suggestion #4167 (comment) in pkg/controller/populators/import-populator_test.go

@xphyr xphyr Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this unit test, I feel like we are testing at the proper level... reviewing the rest of the tests for this package none of them appear to be going through the process of checking the environment variables assigned to a pod. While I agree testing for this in the import-controller makes sense, since this is a helper package/function should we not just be testing the results of calling the helper function? Perhaps I am misunderstanding how you would like me to test this. Please advise and thanks for the assistance.

for index, header := range http.ExtraHeaders {
annotations[fmt.Sprintf("%s.%d", AnnExtraHeaders, index)] = header
}
Expand Down
32 changes: 32 additions & 0 deletions pkg/controller/common/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,38 @@ var _ = Describe("GetRequestedImageSize", func() {
})
})

var _ = Describe("UpdateHTTPAnnotations", func() {
It("Should set AnnInsecureSkipVerify to true when DataVolumeSourceHTTP.InsecureSkipVerify is true", func() {
insecureSkipVerify := true
annotations := map[string]string{}
UpdateHTTPAnnotations(annotations, &cdiv1.DataVolumeSourceHTTP{
URL: "http://example.com",
InsecureSkipVerify: &insecureSkipVerify,
})
Expect(annotations[AnnInsecureSkipVerify]).To(Equal("true"))
})

It("Should not set AnnInsecureSkipVerify when DataVolumeSourceHTTP.InsecureSkipVerify is false", func() {
insecureSkipVerify := false
annotations := map[string]string{}
UpdateHTTPAnnotations(annotations, &cdiv1.DataVolumeSourceHTTP{
URL: "http://example.com",
InsecureSkipVerify: &insecureSkipVerify,
})
_, exists := annotations[AnnInsecureSkipVerify]
Expect(exists).To(BeFalse())
})

It("Should not set AnnInsecureSkipVerify when DataVolumeSourceHTTP.InsecureSkipVerify is absent", func() {
annotations := map[string]string{}
UpdateHTTPAnnotations(annotations, &cdiv1.DataVolumeSourceHTTP{
URL: "http://example.com",
})
_, exists := annotations[AnnInsecureSkipVerify]
Expect(exists).To(BeFalse())
})
})

var _ = Describe("GetStorageClassByName", func() {
It("Should return the default storage class name", func() {
client := CreateClient(
Expand Down
4 changes: 2 additions & 2 deletions pkg/controller/import-controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,9 +713,9 @@ func (r *ImportReconciler) createImportEnvVar(pvc *corev1.PersistentVolumeClaim)
}

func (r *ImportReconciler) isInsecureTLS(pvc *corev1.PersistentVolumeClaim, cdiConfig *cdiv1.CDIConfig) (bool, error) {
// Check if insecureSkipVerify annotation is set (only applicable for ImageIO sources)
// Check if insecureSkipVerify annotation is set (only applicable for ImageIO sources and HTTP sources)
source, sourceOk := pvc.Annotations[cc.AnnSource]
if sourceOk && source == cc.SourceImageio {
if sourceOk && (source == cc.SourceImageio || source == cc.SourceHTTP) {
if insecureSkipVerify, ok := pvc.Annotations[cc.AnnInsecureSkipVerify]; ok && insecureSkipVerify == "true" {

@akalenyu akalenyu Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some unit tests for this would also be great, specifically that no AnnInsecureSkipVerify results in no env var for TLS bypass. Just trying to cover everything that guards the runtime importer app receiving insecure=true

@xphyr xphyr Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have also written a test here: 2aea8ad with the help of AI. I think that does what you are suggesting.

@akalenyu akalenyu Jun 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good! I was hoping to assert on the resulting pod env vars though to cover more ground. We should have existing tests that do something along the lines of

_, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "testPvc1", Namespace: "default"}})
Expect(err).ToNot(HaveOccurred())
pod := &corev1.Pod{}
err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "importer-testPvc1", Namespace: "default"}, pod)
<ASSERTION>

@xphyr xphyr Jun 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am attempting to re-write the test as you are looking for. Before I move forward with the other unit test, can you review this to see if it is what you were looking for. fixed the issue with my first unit test If this is the proper direction, I will review the unit test for util.go. Please note I have not yet removed the other test. I wanted to ensure that this was the way you wanted to go, before deleting that code. If this is good, I will delete the first test that I wrote.

@akalenyu akalenyu Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this was the direction. I was looking for something along those lines:
akalenyu@d6c22d0

@xphyr xphyr Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I have integrated that into the most recent change. I think we are all set. Thank you for your continued assistance.

return true, nil
}
Expand Down
88 changes: 88 additions & 0 deletions pkg/controller/import-controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,68 @@ var _ = Describe("ImportConfig Controller reconcile loop", func() {
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("Pod is not owned by PVC"))
})

It("Should not set the InsecureTLS environment variable if the AnnInsecureSkipVerify annotation is set to false", func() {
pvc := cc.CreatePvc("testPvc1", "default", map[string]string{cc.AnnEndpoint: testEndPoint, cc.AnnImportPod: "importer-testPvc1", cc.AnnInsecureSkipVerify: "false"}, nil)
pvc.Status.Phase = v1.ClaimBound
reconciler = createImportReconciler(pvc)
_, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "testPvc1", Namespace: "default"}})
Expect(err).ToNot(HaveOccurred())
pod := &corev1.Pod{}
err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "importer-testPvc1", Namespace: "default"}, pod)
Expect(err).ToNot(HaveOccurred())
foundAnnInsecureSkipVerify := false
for _, envVar := range pod.Spec.Containers[0].Env {
if envVar.Name == common.InsecureTLSVar {
foundAnnInsecureSkipVerify = true
Expect(envVar.Value).To(Equal("false"))
}
}
Expect(foundAnnInsecureSkipVerify).To(BeTrue())
})

It("Should set the InsecureTLS environment variable to false if the AnnInsecureSkipVerify annotation is absent", func() {
pvc := cc.CreatePvc("testPvc1", "default", map[string]string{cc.AnnEndpoint: testEndPoint, cc.AnnImportPod: "importer-testPvc1"}, nil)
pvc.Status.Phase = v1.ClaimBound
reconciler = createImportReconciler(pvc)
_, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "testPvc1", Namespace: "default"}})
Expect(err).ToNot(HaveOccurred())
pod := &corev1.Pod{}
err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "importer-testPvc1", Namespace: "default"}, pod)
Expect(err).ToNot(HaveOccurred())
foundAnnInsecureSkipVerify := false
for _, envVar := range pod.Spec.Containers[0].Env {
if envVar.Name == common.InsecureTLSVar {
foundAnnInsecureSkipVerify = true
Expect(envVar.Value).To(Equal("false"))
}
}
Expect(foundAnnInsecureSkipVerify).To(BeTrue())
})

It("Should set the InsecureTLS environment variable to true if the AnnInsecureSkipVerify annotation is set to true", func() {
pvc := cc.CreatePvc("testPvc1", "default", map[string]string{
cc.AnnEndpoint: testEndPoint,
cc.AnnImportPod: "importer-testPvc1",
cc.AnnInsecureSkipVerify: "true",
cc.AnnSource: cc.SourceHTTP,
}, nil)
pvc.Status.Phase = v1.ClaimBound
reconciler = createImportReconciler(pvc)
_, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "testPvc1", Namespace: "default"}})
Expect(err).ToNot(HaveOccurred())
pod := &corev1.Pod{}
err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "importer-testPvc1", Namespace: "default"}, pod)
Expect(err).ToNot(HaveOccurred())
foundAnnInsecureSkipVerify := false
for _, envVar := range pod.Spec.Containers[0].Env {
if envVar.Name == common.InsecureTLSVar {
foundAnnInsecureSkipVerify = true
Expect(envVar.Value).To(Equal("true"))
}
}
Expect(foundAnnInsecureSkipVerify).To(BeTrue())
})
})

var _ = Describe("Update PVC from POD", func() {
Expand Down Expand Up @@ -1256,6 +1318,32 @@ var _ = Describe("getInsecureTLS", func() {
)
})

var _ = Describe("isInsecureTLS", func() {
DescribeTable("should", func(source, insecureSkipVerify string, expected bool) {
annotations := map[string]string{
cc.AnnSource: source,
}
if insecureSkipVerify != "" {
annotations[cc.AnnInsecureSkipVerify] = insecureSkipVerify
}
pvc := cc.CreatePvc("testPVC", "default", annotations, nil)
reconciler := createImportReconciler(pvc)

cdiConfig := &cdiv1.CDIConfig{}
err := reconciler.client.Get(context.TODO(), types.NamespacedName{Name: common.ConfigName}, cdiConfig)
Expect(err).ToNot(HaveOccurred())

result, err := reconciler.isInsecureTLS(pvc, cdiConfig)
Expect(err).ToNot(HaveOccurred())
Expect(result).To(Equal(expected))
},
Entry("return true when AnnInsecureSkipVerify is set to true for http source", cc.SourceHTTP, "true", true),
Entry("return true when AnnInsecureSkipVerify is set to true for imageio source", cc.SourceImageio, "true", true),
Entry("return false when AnnInsecureSkipVerify is not present for http source", cc.SourceHTTP, "", false),
Entry("return false when AnnInsecureSkipVerify is not present for imageio source", cc.SourceImageio, "", false),
)
})

var _ = Describe("GetContentType", func() {
pvcNoAnno := cc.CreatePvc("testPVCNoAnno", "default", nil, nil)
pvcArchiveAnno := cc.CreatePvc("testPVCArchiveAnno", "default", map[string]string{cc.AnnContentType: string(cdiv1.DataVolumeArchive)}, nil)
Expand Down
11 changes: 5 additions & 6 deletions pkg/importer/http-datasource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,14 +84,13 @@
contentLength uint64
// checksumValidator validates the checksum of downloaded data
checksumValidator *ChecksumValidator

n image.NbdkitOperation
n image.NbdkitOperation

@akalenyu akalenyu Jun 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my editor corrects this back, how come you had to pad the spaces?

@xphyr xphyr Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its funny, my editor puts all the spaces in. Even when I remove them, it puts them back. (vscode). If you want I will edit the file outside vscode and commit to remove this change.

@xphyr xphyr Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dug into this, its go fmt that is making this formatting change. I am using go1.25 on my dev machine ...

}

var createNbdkitCurl = image.NewNbdkitCurl

// NewHTTPDataSource creates a new instance of the http data provider.
func NewHTTPDataSource(endpoint, accessKey, secKey, certDir string, contentType cdiv1.DataVolumeContentType, checksum string) (*HTTPDataSource, error) {
func NewHTTPDataSource(endpoint, accessKey, secKey, certDir string, contentType cdiv1.DataVolumeContentType, checksum string, insecureSkipVerify bool) (*HTTPDataSource, error) {
ep, err := ParseEndpoint(endpoint)
if err != nil {
return nil, errors.Wrapf(err, "unable to parse endpoint %q", endpoint)
Expand All @@ -104,7 +103,7 @@
return nil, errors.Wrap(err, "Error getting extra headers for HTTP client")
}

httpReader, contentLength, brokenForQemuImg, err := createHTTPReader(ctx, ep, accessKey, secKey, certDir, extraHeaders, secretExtraHeaders, contentType)
httpReader, contentLength, brokenForQemuImg, err := createHTTPReader(ctx, ep, accessKey, secKey, certDir, extraHeaders, secretExtraHeaders, contentType, insecureSkipVerify)
if err != nil {
cancel()
return nil, err
Expand Down Expand Up @@ -365,9 +364,9 @@
req.Header.Add("User-Agent", defaultUserAgent)
}

func createHTTPReader(ctx context.Context, ep *url.URL, accessKey, secKey, certDir string, extraHeaders, secretExtraHeaders []string, contentType cdiv1.DataVolumeContentType) (io.ReadCloser, uint64, bool, error) {
func createHTTPReader(ctx context.Context, ep *url.URL, accessKey, secKey, certDir string, extraHeaders, secretExtraHeaders []string, contentType cdiv1.DataVolumeContentType, insecureSkipVerify bool) (io.ReadCloser, uint64, bool, error) {

Check warning on line 367 in pkg/importer/http-datasource.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

This function has 9 parameters, which is greater than the 7 authorized. 


See more on https://sonarcloud.io/project/issues?id=kubevirt_containerized-data-importer&issues=AZ6tH7XzixdoOq-N26ny&open=AZ6tH7XzixdoOq-N26ny&pullRequest=4167

Check failure on line 367 in pkg/importer/http-datasource.go

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this method to reduce its Cognitive Complexity from 18 to the 15 allowed. 


See more on https://sonarcloud.io/project/issues?id=kubevirt_containerized-data-importer&issues=AZ6tH7XzixdoOq-N26nx&open=AZ6tH7XzixdoOq-N26nx&pullRequest=4167
var brokenForQemuImg bool
client, err := createHTTPClient(certDir, false)
client, err := createHTTPClient(certDir, insecureSkipVerify)
if err != nil {
return nil, uint64(0), false, errors.Wrap(err, "Error creating http client")
}
Expand Down
Loading