Skip to content

Commit

Permalink
errcheck, virt-controller, watchers: always handle errors
Browse files Browse the repository at this point in the history 
Signed-off-by: Miguel Duarte Barroso <[email protected]>
  • Loading branch information
@maiqueb
maiqueb committed Aug 24, 2022
1 parent 8a7e7a9 commit 526c400
Show file tree
Hide file tree
Showing 15 changed files with 239 additions and 171 deletions.
1 change: 0 additions & 1 deletion nogo_config.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,6 @@
"pkg/unsafepath/": "KubeVirt unsafepath pkg does not pass errcheck yet",
"pkg/util/": "KubeVirt util pkg does not pass errcheck yet",
"pkg/virt-api/": "KubeVirt virt-api pkg does not pass errcheck yet",
"pkg/virt-controller/watch/": "KubeVirt virt-controller/watch pkg does not pass errcheck yet",
"pkg/virt-exportserver/": "KubeVirt virt-exportserver pkg does not pass errcheck yet",
"pkg/virt-handler/": "KubeVirt virt-handler pkg does not pass errcheck yet",
"pkg/virt-launcher/": "KubeVirt virt-launcher pkg does not pass errcheck yet",
Expand Down
13 changes: 7 additions & 6 deletions pkg/storage/export/export/export.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1118,7 +1118,6 @@ func (ctrl *VMExportController) getLinks(pvcs []*corev1.PersistentVolumeClaim, e

func (ctrl *VMExportController) internalExportCa() (string, error) {
key := controller.NamespacedKey(ctrl.KubevirtNamespace, components.KubeVirtExportCASecretName)
ctrl.ConfigMapInformer.GetStore().GetByKey(key)
obj, exists, err := ctrl.ConfigMapInformer.GetStore().GetByKey(key)
if err != nil || !exists {
return "", err
Expand Down Expand Up @@ -1310,7 +1309,7 @@ func (ctrl *VMExportController) getRouteCert(hostName string) (string, error) {
func (ctrl *VMExportController) findCertByHostName(hostName string, certs []*x509.Certificate) (string, error) {
for _, cert := range certs {
if ctrl.matchesOrWildCard(hostName, cert.Subject.CommonName) {
return buildPemFromCert(cert), nil
return buildPemFromCert(cert)
}
for _, extension := range cert.Extensions {
if extension.Id.String() == subjectAltNameId {
Expand All @@ -1323,7 +1322,7 @@ func (ctrl *VMExportController) findCertByHostName(hostName string, certs []*x50
names := strings.Split(value, " ")
for _, name := range names {
if ctrl.matchesOrWildCard(hostName, name) {
return buildPemFromCert(cert), nil
return buildPemFromCert(cert)
}
}
}
Expand All @@ -1332,10 +1331,12 @@ func (ctrl *VMExportController) findCertByHostName(hostName string, certs []*x50
return "", nil
}

func buildPemFromCert(cert *x509.Certificate) string {
func buildPemFromCert(cert *x509.Certificate) (string, error) {
pemOut := strings.Builder{}
pem.Encode(&pemOut, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw})
return strings.TrimSpace(pemOut.String())
if err := pem.Encode(&pemOut, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw}); err != nil {
return "", err
}
return strings.TrimSpace(pemOut.String()), nil
}

func (ctrl *VMExportController) matchesOrWildCard(hostName, compare string) bool {
Expand Down
148 changes: 84 additions & 64 deletions pkg/storage/export/export/export_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,20 +200,22 @@ var _ = Describe("Export controlleer", func() {
mockVMExportQueue = testutils.NewMockWorkQueue(controller.vmExportQueue)
controller.vmExportQueue = mockVMExportQueue

cmInformer.GetStore().Add(&k8sv1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: controller.KubevirtNamespace,
Name: components.KubeVirtExportCASecretName,
},
Data: map[string]string{
"ca-bundle": "replace me with ca cert",
},
})
Expect(
cmInformer.GetStore().Add(&k8sv1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: controller.KubevirtNamespace,
Name: components.KubeVirtExportCASecretName,
},
Data: map[string]string{
"ca-bundle": "replace me with ca cert",
},
}),
).To(Succeed())
})

AfterEach(func() {
controller.caCertManager.Stop()
os.RemoveAll(certDir)
Expect(os.RemoveAll(certDir)).To(Succeed())
})

generateExpectedCert := func() string {
Expand All @@ -227,7 +229,7 @@ var _ = Describe("Export controlleer", func() {
cert, err := certutil.NewSelfSignedCACertWithAltNames(config, key, time.Hour, "hahaha.wwoo", "*.apps-crc.testing", "fgdgd.dfsgdf")
Expect(err).ToNot(HaveOccurred())
pemOut := strings.Builder{}
pem.Encode(&pemOut, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw})
Expect(pem.Encode(&pemOut, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw})).To(Succeed())
return strings.TrimSpace(pemOut.String())
}

Expand All @@ -244,7 +246,7 @@ var _ = Describe("Export controlleer", func() {
cert, err := certutil.NewSelfSignedCACert(config, key, time.Hour)
Expect(err).ToNot(HaveOccurred())
pemOut := strings.Builder{}
pem.Encode(&pemOut, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw})
Expect(pem.Encode(&pemOut, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw})).To(Succeed())
return strings.TrimSpace(pemOut.String()) + "\n" + expectedPem
}

Expand Down Expand Up @@ -504,20 +506,22 @@ var _ = Describe("Export controlleer", func() {
Expect(service).ToNot(BeNil())
Expect(service.Status.Conditions[0].Type).To(Equal("test"))

serviceInformer.GetStore().Add(&k8sv1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: controller.getExportServiceName(testVMExport),
Namespace: testVMExport.Namespace,
},
Spec: k8sv1.ServiceSpec{},
Status: k8sv1.ServiceStatus{
Conditions: []metav1.Condition{
{
Type: "test2",
Expect(
serviceInformer.GetStore().Add(&k8sv1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: controller.getExportServiceName(testVMExport),
Namespace: testVMExport.Namespace,
},
Spec: k8sv1.ServiceSpec{},
Status: k8sv1.ServiceStatus{
Conditions: []metav1.Condition{
{
Type: "test2",
},
},
},
},
})
}),
).To(Succeed())
service, err = controller.getOrCreateExportService(testVMExport)
Expect(err).ToNot(HaveOccurred())
Expect(service).ToNot(BeNil())
Expand Down Expand Up @@ -686,15 +690,17 @@ var _ = Describe("Export controlleer", func() {

It("Should properly update VMExport status with a valid token and archive pvc no route", func() {
testVMExport := createPVCVMExport()
pvcInformer.GetStore().Add(&k8sv1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: testPVCName,
Namespace: testNamespace,
Annotations: map[string]string{
annContentType: "archive",
Expect(
pvcInformer.GetStore().Add(&k8sv1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: testPVCName,
Namespace: testNamespace,
Annotations: map[string]string{
annContentType: "archive",
},
},
},
})
}),
).To(Succeed())
expectExporterCreate(k8sClient, k8sv1.PodRunning)
vmExportClient.Fake.PrependReactor("update", "virtualmachineexports", func(action testing.Action) (handled bool, obj runtime.Object, err error) {
update, ok := action.(testing.UpdateAction)
Expand All @@ -717,17 +723,20 @@ var _ = Describe("Export controlleer", func() {

It("Should properly update VMExport status with a valid token and kubevirt pvc with route", func() {
testVMExport := createPVCVMExport()
pvcInformer.GetStore().Add(&k8sv1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: testPVCName,
Namespace: testNamespace,
Annotations: map[string]string{
annContentType: "kubevirt",
Expect(
pvcInformer.GetStore().Add(&k8sv1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: testPVCName,
Namespace: testNamespace,
Annotations: map[string]string{
annContentType: "kubevirt",
},
},
},
})
}),
).To(Succeed())
expectExporterCreate(k8sClient, k8sv1.PodRunning)
controller.RouteCache.Add(routeToHostAndService(components.VirtExportProxyServiceName))
Expect(
controller.RouteCache.Add(routeToHostAndService(components.VirtExportProxyServiceName))).To(Succeed())

vmExportClient.Fake.PrependReactor("update", "virtualmachineexports", func(action testing.Action) (handled bool, obj runtime.Object, err error) {
update, ok := action.(testing.UpdateAction)
Expand Down Expand Up @@ -801,8 +810,8 @@ var _ = Describe("Export controlleer", func() {
verifyLinksEmpty(vmExport)
return true, vmExport, nil
})
controller.PodInformer.GetStore().Add(pod)
controller.PVCInformer.GetStore().Add(pvc)
Expect(controller.PodInformer.GetStore().Add(pod)).To(Succeed())
Expect(controller.PVCInformer.GetStore().Add(pvc)).To(Succeed())
retry, err := controller.updateVMExport(testVMExport)
Expect(err).ToNot(HaveOccurred())
Expect(retry).To(BeEquivalentTo(requeueTime))
Expand Down Expand Up @@ -838,7 +847,7 @@ var _ = Describe("Export controlleer", func() {
ContentType: contentType,
},
}
controller.DataVolumeInformer.GetStore().Add(dv)
Expect(controller.DataVolumeInformer.GetStore().Add(dv)).To(Succeed())
pvc := &k8sv1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: "test-dv",
Expand Down Expand Up @@ -1066,8 +1075,10 @@ var _ = Describe("Export controlleer", func() {
Expect(vmExport.Status.Phase).To(Equal(exportv1.Skipped))
return true, vmExport, nil
})
vmSnapshotInformer.GetStore().Add(createTestVMSnapshot(true))
vmSnapshotContentInformer.GetStore().Add(createTestVMSnapshotContentNoVolumes("snapshot-content"))
Expect(vmSnapshotInformer.GetStore().Add(createTestVMSnapshot(true))).To(Succeed())
Expect(
vmSnapshotContentInformer.GetStore().Add(createTestVMSnapshotContentNoVolumes("snapshot-content")),
).To(Succeed())
retry, err := controller.updateVMExport(testVMExport)
Expect(err).ToNot(HaveOccurred())
Expect(retry).To(BeEquivalentTo(0))
Expand Down Expand Up @@ -1129,8 +1140,10 @@ var _ = Describe("Export controlleer", func() {
})
expectExporterCreate(k8sClient, k8sv1.PodPending)

vmSnapshotInformer.GetStore().Add(createTestVMSnapshot(true))
vmSnapshotContentInformer.GetStore().Add(createTestVMSnapshotContent("snapshot-content"))
Expect(vmSnapshotInformer.GetStore().Add(createTestVMSnapshot(true))).To(Succeed())
Expect(
vmSnapshotContentInformer.GetStore().Add(createTestVMSnapshotContent("snapshot-content")),
).To(Succeed())
fakeVolumeSnapshotProvider.Add(createTestVolumeSnapshot(testVolumesnapshotName))
retry, err := controller.updateVMExport(testVMExport)
Expect(err).ToNot(HaveOccurred())
Expand Down Expand Up @@ -1169,9 +1182,11 @@ var _ = Describe("Export controlleer", func() {
return true, nil, nil
})
expectExporterCreate(k8sClient, k8sv1.PodPending)
pvcInformer.GetStore().Add(createRestoredPVC("test-test-snapshot"))
vmSnapshotInformer.GetStore().Add(createTestVMSnapshot(true))
vmSnapshotContentInformer.GetStore().Add(createTestVMSnapshotContent("snapshot-content"))
Expect(pvcInformer.GetStore().Add(createRestoredPVC("test-test-snapshot"))).To(Succeed())
Expect(vmSnapshotInformer.GetStore().Add(createTestVMSnapshot(true))).To(Succeed())
Expect(
vmSnapshotContentInformer.GetStore().Add(createTestVMSnapshotContent("snapshot-content")),
).To(Succeed())
fakeVolumeSnapshotProvider.Add(createTestVolumeSnapshot(testVolumesnapshotName))
retry, err := controller.updateVMExport(testVMExport)
Expect(err).ToNot(HaveOccurred())
Expand Down Expand Up @@ -1220,9 +1235,11 @@ var _ = Describe("Export controlleer", func() {
return true, pvc, nil
})
expectExporterCreate(k8sClient, k8sv1.PodRunning)
controller.RouteCache.Add(routeToHostAndService(components.VirtExportProxyServiceName))
vmSnapshotInformer.GetStore().Add(createTestVMSnapshot(true))
vmSnapshotContentInformer.GetStore().Add(createTestVMSnapshotContent("snapshot-content"))
Expect(controller.RouteCache.Add(routeToHostAndService(components.VirtExportProxyServiceName))).To(Succeed())
Expect(vmSnapshotInformer.GetStore().Add(createTestVMSnapshot(true))).To(Succeed())
Expect(
vmSnapshotContentInformer.GetStore().Add(createTestVMSnapshotContent("snapshot-content")),
).To(Succeed())
fakeVolumeSnapshotProvider.Add(createTestVolumeSnapshot(testVolumesnapshotName))
retry, err := controller.updateVMExport(testVMExport)
Expect(err).ToNot(HaveOccurred())
Expand Down Expand Up @@ -1270,12 +1287,13 @@ var _ = Describe("Export controlleer", func() {
return true, pvc, nil
})
expectExporterCreate(k8sClient, k8sv1.PodRunning)
controller.RouteCache.Add(routeToHostAndService(components.VirtExportProxyServiceName))
vmSnapshotInformer.GetStore().Add(createTestVMSnapshot(true))
Expect(
controller.RouteCache.Add(routeToHostAndService(components.VirtExportProxyServiceName))).To(Succeed())
Expect(vmSnapshotInformer.GetStore().Add(createTestVMSnapshot(true))).To(Succeed())
content := createTestVMSnapshotContent("snapshot-content")
content.Spec.Source.VirtualMachine.Spec.Template.Spec.Volumes[0].DataVolume = nil
content.Spec.Source.VirtualMachine.Spec.Template.Spec.Volumes[0].MemoryDump = &virtv1.MemoryDumpVolumeSource{}
vmSnapshotContentInformer.GetStore().Add(content)
Expect(vmSnapshotContentInformer.GetStore().Add(content)).To(Succeed())
fakeVolumeSnapshotProvider.Add(createTestVolumeSnapshot(testVolumesnapshotName))
retry, err := controller.updateVMExport(testVMExport)
Expect(err).ToNot(HaveOccurred())
Expand Down Expand Up @@ -1313,9 +1331,9 @@ var _ = Describe("Export controlleer", func() {
Fail("Should not create PVCs")
return true, nil, nil
})
vmSnapshotInformer.GetStore().Add(createTestVMSnapshot(false))
Expect(vmSnapshotInformer.GetStore().Add(createTestVMSnapshot(false))).To(Succeed())
content := createTestVMSnapshotContent("snapshot-content")
vmSnapshotContentInformer.GetStore().Add(content)
Expect(vmSnapshotContentInformer.GetStore().Add(content)).To(Succeed())
fakeVolumeSnapshotProvider.Add(createTestVolumeSnapshot(testVolumesnapshotName))
retry, err := controller.updateVMExport(testVMExport)
Expect(err).ToNot(HaveOccurred())
Expand All @@ -1324,7 +1342,7 @@ var _ = Describe("Export controlleer", func() {
})

DescribeTable("should find host when Ingress is defined", func(ingress *networkingv1.Ingress, hostname string) {
controller.IngressCache.Add(ingress)
Expect(controller.IngressCache.Add(ingress)).To(Succeed())
host, _ := controller.getExternalLinkHostAndCert()
Expect(hostname).To(Equal(host))
},
Expand All @@ -1337,8 +1355,8 @@ var _ = Describe("Export controlleer", func() {
)

DescribeTable("should find host when route is defined", func(route *routev1.Route, hostname, expectedCert string) {
controller.RouteCache.Add(route)
controller.RouteConfigMapInformer.GetStore().Add(createRouteConfigMap())
Expect(controller.RouteCache.Add(route)).To(Succeed())
Expect(controller.RouteConfigMapInformer.GetStore().Add(createRouteConfigMap())).To(Succeed())
host, cert := controller.getExternalLinkHostAndCert()
Expect(hostname).To(Equal(host))
Expect(expectedCert).To(Equal(cert))
Expand All @@ -1349,8 +1367,10 @@ var _ = Describe("Export controlleer", func() {
)

It("should pick ingress over route if both exist", func() {
controller.IngressCache.Add(validIngressDefaultBackend(components.VirtExportProxyServiceName))
controller.RouteCache.Add(routeToHostAndService(components.VirtExportProxyServiceName))
Expect(
controller.IngressCache.Add(validIngressDefaultBackend(components.VirtExportProxyServiceName)),
).To(Succeed())
Expect(controller.RouteCache.Add(routeToHostAndService(components.VirtExportProxyServiceName))).To(Succeed())
host, _ := controller.getExternalLinkHostAndCert()
Expect("backend-host").To(Equal(host))
})
Expand Down
4 changes: 3 additions & 1 deletion pkg/storage/snapshot/snapshot_base.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,9 @@ func (ctrl *VMSnapshotController) Run(threadiness int, stopCh <-chan struct{}) e
<-stopCh

for crd := range ctrl.dynamicInformerMap {
ctrl.deleteDynamicInformer(crd)
if _, err := ctrl.deleteDynamicInformer(crd); err != nil {
log.Log.Warningf("failed to delete %s informer: %v", crd, err)
}
}

return nil
Expand Down
1 change: 1 addition & 0 deletions pkg/virt-controller/watch/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ go_library(
"//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/runtime:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library",
"//vendor/k8s.io/client-go/kubernetes/scheme:go_default_library",
Expand Down
Loading

0 comments on commit 526c400

Please sign in to comment.