From 1cfce8d144093528874f2265d739e8eab3d2001c Mon Sep 17 00:00:00 2001 From: Pavel Tishkov Date: Tue, 23 Jun 2026 17:06:27 +0300 Subject: [PATCH 1/3] fix(uploader): sync ingress with publicDomainTemplate via server-side apply Uploader Ingress was created once with client.Create and never updated on AlreadyExists, so changing publicDomainTemplate left existing upload Ingresses with a stale host in spec.rules[].host, spec.tls[].hosts and the AnnUploadURL annotation shown to users. Replace Create with server-side apply (client.Patch + client.Apply) so the Ingress is reconciled on every upload-flow reconcile: created when absent, patched when spec/annotations drift. Field ownership is stable across controller restarts. Signed-off-by: Pavel Tishkov --- .../pkg/controller/service/uploader_service.go | 4 ++-- .../pkg/controller/uploader/uploader_ingress.go | 15 +++++++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/service/uploader_service.go b/images/virtualization-artifact/pkg/controller/service/uploader_service.go index f556262f62..4ff54fd52f 100644 --- a/images/virtualization-artifact/pkg/controller/service/uploader_service.go +++ b/images/virtualization-artifact/pkg/controller/service/uploader_service.go @@ -106,8 +106,8 @@ func (s UploaderService) Start( return err } - ing, err := uploader.NewIngress(s.getIngressSettings(ownerRef, sup)).Create(ctx, s.client) - if err != nil && !k8serrors.IsAlreadyExists(err) { + ing, err := uploader.NewIngress(s.getIngressSettings(ownerRef, sup)).Apply(ctx, s.client) + if err != nil { return err } diff --git a/images/virtualization-artifact/pkg/controller/uploader/uploader_ingress.go b/images/virtualization-artifact/pkg/controller/uploader/uploader_ingress.go index b4d3deab28..5aa701709f 100644 --- a/images/virtualization-artifact/pkg/controller/uploader/uploader_ingress.go +++ b/images/virtualization-artifact/pkg/controller/uploader/uploader_ingress.go @@ -52,14 +52,21 @@ func NewIngress(settings *IngressSettings) *Ingress { return &Ingress{settings} } -func (i *Ingress) Create(ctx context.Context, client client.Client) (*netv1.Ingress, error) { - ing := i.makeSpec() +// Apply reconciles the Ingress using server-side apply: it creates the Ingress +// if absent and updates spec/annotations when they drift (e.g. when +// publicDomainTemplate changes UPLOADER_INGRESS_HOST). Field ownership is +// stable across controller restarts. +const fieldOwner = "virtualization-controller" - if err := client.Create(ctx, ing); err != nil { +func (i *Ingress) Apply(ctx context.Context, c client.Client) (*netv1.Ingress, error) { + desired := i.makeSpec() + desired.SetGroupVersionKind(netv1.SchemeGroupVersion.WithKind("Ingress")) + + if err := c.Patch(ctx, desired, client.Apply, client.FieldOwner(fieldOwner)); err != nil { return nil, err } - return ing, nil + return desired, nil } // makeSpec fills Ingress structure with uploader settings. From 7102b75e390aee1fca81040aa8171b81716e5a57 Mon Sep 17 00:00:00 2001 From: Pavel Tishkov Date: Thu, 25 Jun 2026 10:19:25 +0300 Subject: [PATCH 2/3] fix(uploader): sync ingress on every upload-flow reconcile Apply (SSA) was only called in UploaderService.Start, which runs when uploader supplements are missing. In the steady WaitForUserUpload state the pod/svc/ingress already exist, so Start was never called and the Ingress stayed stale when publicDomainTemplate changed. Add EnsureIngress/IngressHostDrifted/ExpectedIngressHost to UploaderService and call EnsureIngress in the isUploaderReady branch of the VI/CVI/VD upload-flows only when the cached Ingress host drifted from the configured UPLOADER_INGRESS_HOST. Steady-state cost is one string compare per reconcile; a Patch runs only on actual drift. Signed-off-by: Pavel Tishkov --- .../cvi/internal/source/interfaces.go | 3 + .../controller/cvi/internal/source/mock.go | 155 ++++++++++++++++- .../controller/cvi/internal/source/upload.go | 11 ++ .../controller/service/uploader_service.go | 35 ++++ .../controller/vd/internal/source/upload.go | 12 ++ .../vi/internal/source/interfaces.go | 3 + .../pkg/controller/vi/internal/source/mock.go | 157 ++++++++++++++++-- .../controller/vi/internal/source/upload.go | 22 +++ 8 files changed, 379 insertions(+), 19 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/cvi/internal/source/interfaces.go b/images/virtualization-artifact/pkg/controller/cvi/internal/source/interfaces.go index d08ad8ee06..319c54e031 100644 --- a/images/virtualization-artifact/pkg/controller/cvi/internal/source/interfaces.go +++ b/images/virtualization-artifact/pkg/controller/cvi/internal/source/interfaces.go @@ -57,6 +57,9 @@ type Uploader interface { Unprotect(ctx context.Context, sup supplements.Generator, pod *corev1.Pod, svc *corev1.Service, ing *netv1.Ingress) error GetExternalURL(ctx context.Context, ing *netv1.Ingress) string GetInClusterURL(ctx context.Context, svc *corev1.Service) string + EnsureIngress(ctx context.Context, obj client.Object, sup supplements.Generator) (*netv1.Ingress, error) + IngressHostDrifted(ing *netv1.Ingress) bool + ExpectedIngressHost() string } type Stat interface { diff --git a/images/virtualization-artifact/pkg/controller/cvi/internal/source/mock.go b/images/virtualization-artifact/pkg/controller/cvi/internal/source/mock.go index 9173f8edc3..9aacfa0a69 100644 --- a/images/virtualization-artifact/pkg/controller/cvi/internal/source/mock.go +++ b/images/virtualization-artifact/pkg/controller/cvi/internal/source/mock.go @@ -588,6 +588,12 @@ var _ Uploader = &UploaderMock{} // CleanUpFunc: func(ctx context.Context, sup supplements.Generator) (bool, error) { // panic("mock out the CleanUp method") // }, +// EnsureIngressFunc: func(ctx context.Context, obj client.Object, sup supplements.Generator) (*netv1.Ingress, error) { +// panic("mock out the EnsureIngress method") +// }, +// ExpectedIngressHostFunc: func() string { +// panic("mock out the ExpectedIngressHost method") +// }, // GetExternalURLFunc: func(ctx context.Context, ing *netv1.Ingress) string { // panic("mock out the GetExternalURL method") // }, @@ -603,6 +609,9 @@ var _ Uploader = &UploaderMock{} // GetServiceFunc: func(ctx context.Context, sup supplements.Generator) (*corev1.Service, error) { // panic("mock out the GetService method") // }, +// IngressHostDriftedFunc: func(ing *netv1.Ingress) bool { +// panic("mock out the IngressHostDrifted method") +// }, // ProtectFunc: func(ctx context.Context, sup supplements.Generator, pod *corev1.Pod, svc *corev1.Service, ing *netv1.Ingress) error { // panic("mock out the Protect method") // }, @@ -622,6 +631,12 @@ type UploaderMock struct { // CleanUpFunc mocks the CleanUp method. CleanUpFunc func(ctx context.Context, sup supplements.Generator) (bool, error) + // EnsureIngressFunc mocks the EnsureIngress method. + EnsureIngressFunc func(ctx context.Context, obj client.Object, sup supplements.Generator) (*netv1.Ingress, error) + + // ExpectedIngressHostFunc mocks the ExpectedIngressHost method. + ExpectedIngressHostFunc func() string + // GetExternalURLFunc mocks the GetExternalURL method. GetExternalURLFunc func(ctx context.Context, ing *netv1.Ingress) string @@ -637,6 +652,9 @@ type UploaderMock struct { // GetServiceFunc mocks the GetService method. GetServiceFunc func(ctx context.Context, sup supplements.Generator) (*corev1.Service, error) + // IngressHostDriftedFunc mocks the IngressHostDrifted method. + IngressHostDriftedFunc func(ing *netv1.Ingress) bool + // ProtectFunc mocks the Protect method. ProtectFunc func(ctx context.Context, sup supplements.Generator, pod *corev1.Pod, svc *corev1.Service, ing *netv1.Ingress) error @@ -655,6 +673,18 @@ type UploaderMock struct { // Sup is the sup argument value. Sup supplements.Generator } + // EnsureIngress holds details about calls to the EnsureIngress method. + EnsureIngress []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // Obj is the obj argument value. + Obj client.Object + // Sup is the sup argument value. + Sup supplements.Generator + } + // ExpectedIngressHost holds details about calls to the ExpectedIngressHost method. + ExpectedIngressHost []struct { + } // GetExternalURL holds details about calls to the GetExternalURL method. GetExternalURL []struct { // Ctx is the ctx argument value. @@ -690,6 +720,11 @@ type UploaderMock struct { // Sup is the sup argument value. Sup supplements.Generator } + // IngressHostDrifted holds details about calls to the IngressHostDrifted method. + IngressHostDrifted []struct { + // Ing is the ing argument value. + Ing *netv1.Ingress + } // Protect holds details about calls to the Protect method. Protect []struct { // Ctx is the ctx argument value. @@ -732,15 +767,18 @@ type UploaderMock struct { Ing *netv1.Ingress } } - lockCleanUp sync.RWMutex - lockGetExternalURL sync.RWMutex - lockGetInClusterURL sync.RWMutex - lockGetIngress sync.RWMutex - lockGetPod sync.RWMutex - lockGetService sync.RWMutex - lockProtect sync.RWMutex - lockStart sync.RWMutex - lockUnprotect sync.RWMutex + lockCleanUp sync.RWMutex + lockEnsureIngress sync.RWMutex + lockExpectedIngressHost sync.RWMutex + lockGetExternalURL sync.RWMutex + lockGetInClusterURL sync.RWMutex + lockGetIngress sync.RWMutex + lockGetPod sync.RWMutex + lockGetService sync.RWMutex + lockIngressHostDrifted sync.RWMutex + lockProtect sync.RWMutex + lockStart sync.RWMutex + lockUnprotect sync.RWMutex } // CleanUp calls CleanUpFunc. @@ -779,6 +817,73 @@ func (mock *UploaderMock) CleanUpCalls() []struct { return calls } +// EnsureIngress calls EnsureIngressFunc. +func (mock *UploaderMock) EnsureIngress(ctx context.Context, obj client.Object, sup supplements.Generator) (*netv1.Ingress, error) { + if mock.EnsureIngressFunc == nil { + panic("UploaderMock.EnsureIngressFunc: method is nil but Uploader.EnsureIngress was just called") + } + callInfo := struct { + Ctx context.Context + Obj client.Object + Sup supplements.Generator + }{ + Ctx: ctx, + Obj: obj, + Sup: sup, + } + mock.lockEnsureIngress.Lock() + mock.calls.EnsureIngress = append(mock.calls.EnsureIngress, callInfo) + mock.lockEnsureIngress.Unlock() + return mock.EnsureIngressFunc(ctx, obj, sup) +} + +// EnsureIngressCalls gets all the calls that were made to EnsureIngress. +// Check the length with: +// +// len(mockedUploader.EnsureIngressCalls()) +func (mock *UploaderMock) EnsureIngressCalls() []struct { + Ctx context.Context + Obj client.Object + Sup supplements.Generator +} { + var calls []struct { + Ctx context.Context + Obj client.Object + Sup supplements.Generator + } + mock.lockEnsureIngress.RLock() + calls = mock.calls.EnsureIngress + mock.lockEnsureIngress.RUnlock() + return calls +} + +// ExpectedIngressHost calls ExpectedIngressHostFunc. +func (mock *UploaderMock) ExpectedIngressHost() string { + if mock.ExpectedIngressHostFunc == nil { + panic("UploaderMock.ExpectedIngressHostFunc: method is nil but Uploader.ExpectedIngressHost was just called") + } + callInfo := struct { + }{} + mock.lockExpectedIngressHost.Lock() + mock.calls.ExpectedIngressHost = append(mock.calls.ExpectedIngressHost, callInfo) + mock.lockExpectedIngressHost.Unlock() + return mock.ExpectedIngressHostFunc() +} + +// ExpectedIngressHostCalls gets all the calls that were made to ExpectedIngressHost. +// Check the length with: +// +// len(mockedUploader.ExpectedIngressHostCalls()) +func (mock *UploaderMock) ExpectedIngressHostCalls() []struct { +} { + var calls []struct { + } + mock.lockExpectedIngressHost.RLock() + calls = mock.calls.ExpectedIngressHost + mock.lockExpectedIngressHost.RUnlock() + return calls +} + // GetExternalURL calls GetExternalURLFunc. func (mock *UploaderMock) GetExternalURL(ctx context.Context, ing *netv1.Ingress) string { if mock.GetExternalURLFunc == nil { @@ -959,6 +1064,38 @@ func (mock *UploaderMock) GetServiceCalls() []struct { return calls } +// IngressHostDrifted calls IngressHostDriftedFunc. +func (mock *UploaderMock) IngressHostDrifted(ing *netv1.Ingress) bool { + if mock.IngressHostDriftedFunc == nil { + panic("UploaderMock.IngressHostDriftedFunc: method is nil but Uploader.IngressHostDrifted was just called") + } + callInfo := struct { + Ing *netv1.Ingress + }{ + Ing: ing, + } + mock.lockIngressHostDrifted.Lock() + mock.calls.IngressHostDrifted = append(mock.calls.IngressHostDrifted, callInfo) + mock.lockIngressHostDrifted.Unlock() + return mock.IngressHostDriftedFunc(ing) +} + +// IngressHostDriftedCalls gets all the calls that were made to IngressHostDrifted. +// Check the length with: +// +// len(mockedUploader.IngressHostDriftedCalls()) +func (mock *UploaderMock) IngressHostDriftedCalls() []struct { + Ing *netv1.Ingress +} { + var calls []struct { + Ing *netv1.Ingress + } + mock.lockIngressHostDrifted.RLock() + calls = mock.calls.IngressHostDrifted + mock.lockIngressHostDrifted.RUnlock() + return calls +} + // Protect calls ProtectFunc. func (mock *UploaderMock) Protect(ctx context.Context, sup supplements.Generator, pod *corev1.Pod, svc *corev1.Service, ing *netv1.Ingress) error { if mock.ProtectFunc == nil { diff --git a/images/virtualization-artifact/pkg/controller/cvi/internal/source/upload.go b/images/virtualization-artifact/pkg/controller/cvi/internal/source/upload.go index 5f3d5c6d0b..c05003087f 100644 --- a/images/virtualization-artifact/pkg/controller/cvi/internal/source/upload.go +++ b/images/virtualization-artifact/pkg/controller/cvi/internal/source/upload.go @@ -243,6 +243,17 @@ func (ds UploadDataSource) Sync(ctx context.Context, cvi *v1alpha2.ClusterVirtua cvi.Status.Phase = v1alpha2.ImageWaitForUserUpload cvi.Status.Target.RegistryURL = ds.statService.GetDVCRImageName(pod) + + // Re-sync the uploader Ingress when publicDomainTemplate changed after + // the supplements were created. Apply is a no-op when fields already match. + if ds.uploaderService.IngressHostDrifted(ing) { + log.Info("Syncing uploader Ingress: host drifted", "old", ing.Spec.Rules[0].Host, "new", ds.uploaderService.ExpectedIngressHost()) + ing, err = ds.uploaderService.EnsureIngress(ctx, cvi, supgen) + if err != nil { + return reconcile.Result{}, err + } + } + cvi.Status.ImageUploadURLs = &v1alpha2.ImageUploadURLs{ External: ds.uploaderService.GetExternalURL(ctx, ing), InCluster: ds.uploaderService.GetInClusterURL(ctx, svc), diff --git a/images/virtualization-artifact/pkg/controller/service/uploader_service.go b/images/virtualization-artifact/pkg/controller/service/uploader_service.go index 4ff54fd52f..461f37e513 100644 --- a/images/virtualization-artifact/pkg/controller/service/uploader_service.go +++ b/images/virtualization-artifact/pkg/controller/service/uploader_service.go @@ -275,6 +275,41 @@ func (s UploaderService) getServiceSettings(ownerRef *metav1.OwnerReference, sup } } +// EnsureIngress reconciles the uploader Ingress with server-side apply and +// returns the fresh object. It is cheap: callers should invoke it only when the +// already-fetched Ingress host drifted from the configured UPLOADER_INGRESS_HOST +// (e.g. after publicDomainTemplate change). When there is no drift, skip the call: +// Apply is a no-op only when managed fields already match, but avoiding the +// round-trip entirely keeps steady-state reconcile free of extra writes. +func (s UploaderService) EnsureIngress(ctx context.Context, obj client.Object, sup supplements.Generator) (*netv1.Ingress, error) { + ownerRef := metav1.NewControllerRef(obj, obj.GetObjectKind().GroupVersionKind()) + ing, err := uploader.NewIngress(s.getIngressSettings(ownerRef, sup)).Apply(ctx, s.client) + if err != nil { + return nil, err + } + if err := supplements.EnsureForIngress(ctx, s.client, sup, ing, s.dvcrSettings); err != nil { + return nil, err + } + return ing, nil +} + +// ExpectedIngressHost returns the host the uploader Ingress should expose, +// derived from UPLOADER_INGRESS_HOST (i.e. the rendered publicDomainTemplate). +func (s UploaderService) ExpectedIngressHost() string { + return s.dvcrSettings.UploaderIngressSettings.Host +} + +// IngressHostDrifted reports whether the given Ingress host differs from the +// configured one. An absent Ingress (nil) or an Ingress without rules is +// treated as drift so callers fall back to EnsureIngress / Start. +func (s UploaderService) IngressHostDrifted(ing *netv1.Ingress) bool { + expected := s.ExpectedIngressHost() + if ing == nil || len(ing.Spec.Rules) == 0 { + return true + } + return ing.Spec.Rules[0].Host != expected +} + func (s UploaderService) getIngressSettings(ownerRef *metav1.OwnerReference, sup supplements.Generator) *uploader.IngressSettings { uploaderIng := sup.UploaderIngress() uploaderSvc := sup.UploaderService() diff --git a/images/virtualization-artifact/pkg/controller/vd/internal/source/upload.go b/images/virtualization-artifact/pkg/controller/vd/internal/source/upload.go index a13fbbb404..235ea09aa3 100644 --- a/images/virtualization-artifact/pkg/controller/vd/internal/source/upload.go +++ b/images/virtualization-artifact/pkg/controller/vd/internal/source/upload.go @@ -202,6 +202,18 @@ func (ds UploadDataSource) Sync(ctx context.Context, vd *v1alpha2.VirtualDisk) ( vd.Status.Phase = v1alpha2.DiskWaitForUserUpload setReadyConditionWithWFFCAccounting(vd, cb, metav1.ConditionFalse, vdcondition.WaitForUserUpload, "Waiting for the user upload.") + + // Re-sync the uploader Ingress when publicDomainTemplate changed + // after the supplements were created. Apply is a no-op when fields + // already match. + if ds.uploaderService.IngressHostDrifted(ing) { + log.Info("Syncing uploader Ingress: host drifted", "old", ing.Spec.Rules[0].Host, "new", ds.uploaderService.ExpectedIngressHost()) + ing, err = ds.uploaderService.EnsureIngress(ctx, vd, supgen) + if err != nil { + return reconcile.Result{}, err + } + } + vd.Status.ImageUploadURLs = &v1alpha2.ImageUploadURLs{ External: ds.uploaderService.GetExternalURL(ctx, ing), InCluster: ds.uploaderService.GetInClusterURL(ctx, svc), diff --git a/images/virtualization-artifact/pkg/controller/vi/internal/source/interfaces.go b/images/virtualization-artifact/pkg/controller/vi/internal/source/interfaces.go index 15c88d7335..7ef5263749 100644 --- a/images/virtualization-artifact/pkg/controller/vi/internal/source/interfaces.go +++ b/images/virtualization-artifact/pkg/controller/vi/internal/source/interfaces.go @@ -57,6 +57,9 @@ type Uploader interface { Unprotect(ctx context.Context, sup supplements.Generator, pod *corev1.Pod, svc *corev1.Service, ing *netv1.Ingress) error GetExternalURL(ctx context.Context, ing *netv1.Ingress) string GetInClusterURL(ctx context.Context, svc *corev1.Service) string + EnsureIngress(ctx context.Context, obj client.Object, sup supplements.Generator) (*netv1.Ingress, error) + IngressHostDrifted(ing *netv1.Ingress) bool + ExpectedIngressHost() string } type Stat interface { diff --git a/images/virtualization-artifact/pkg/controller/vi/internal/source/mock.go b/images/virtualization-artifact/pkg/controller/vi/internal/source/mock.go index a21a10c351..171212b6c6 100644 --- a/images/virtualization-artifact/pkg/controller/vi/internal/source/mock.go +++ b/images/virtualization-artifact/pkg/controller/vi/internal/source/mock.go @@ -530,6 +530,12 @@ var _ Uploader = &UploaderMock{} // CleanUpSupplementsFunc: func(ctx context.Context, sup supplements.Generator) (bool, error) { // panic("mock out the CleanUpSupplements method") // }, +// EnsureIngressFunc: func(ctx context.Context, obj client.Object, sup supplements.Generator) (*netv1.Ingress, error) { +// panic("mock out the EnsureIngress method") +// }, +// ExpectedIngressHostFunc: func() string { +// panic("mock out the ExpectedIngressHost method") +// }, // GetExternalURLFunc: func(ctx context.Context, ing *netv1.Ingress) string { // panic("mock out the GetExternalURL method") // }, @@ -545,6 +551,9 @@ var _ Uploader = &UploaderMock{} // GetServiceFunc: func(ctx context.Context, sup supplements.Generator) (*corev1.Service, error) { // panic("mock out the GetService method") // }, +// IngressHostDriftedFunc: func(ing *netv1.Ingress) bool { +// panic("mock out the IngressHostDrifted method") +// }, // ProtectFunc: func(ctx context.Context, sup supplements.Generator, pod *corev1.Pod, svc *corev1.Service, ing *netv1.Ingress) error { // panic("mock out the Protect method") // }, @@ -567,6 +576,12 @@ type UploaderMock struct { // CleanUpSupplementsFunc mocks the CleanUpSupplements method. CleanUpSupplementsFunc func(ctx context.Context, sup supplements.Generator) (bool, error) + // EnsureIngressFunc mocks the EnsureIngress method. + EnsureIngressFunc func(ctx context.Context, obj client.Object, sup supplements.Generator) (*netv1.Ingress, error) + + // ExpectedIngressHostFunc mocks the ExpectedIngressHost method. + ExpectedIngressHostFunc func() string + // GetExternalURLFunc mocks the GetExternalURL method. GetExternalURLFunc func(ctx context.Context, ing *netv1.Ingress) string @@ -582,6 +597,9 @@ type UploaderMock struct { // GetServiceFunc mocks the GetService method. GetServiceFunc func(ctx context.Context, sup supplements.Generator) (*corev1.Service, error) + // IngressHostDriftedFunc mocks the IngressHostDrifted method. + IngressHostDriftedFunc func(ing *netv1.Ingress) bool + // ProtectFunc mocks the Protect method. ProtectFunc func(ctx context.Context, sup supplements.Generator, pod *corev1.Pod, svc *corev1.Service, ing *netv1.Ingress) error @@ -607,6 +625,18 @@ type UploaderMock struct { // Sup is the sup argument value. Sup supplements.Generator } + // EnsureIngress holds details about calls to the EnsureIngress method. + EnsureIngress []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // Obj is the obj argument value. + Obj client.Object + // Sup is the sup argument value. + Sup supplements.Generator + } + // ExpectedIngressHost holds details about calls to the ExpectedIngressHost method. + ExpectedIngressHost []struct { + } // GetExternalURL holds details about calls to the GetExternalURL method. GetExternalURL []struct { // Ctx is the ctx argument value. @@ -642,6 +672,11 @@ type UploaderMock struct { // Sup is the sup argument value. Sup supplements.Generator } + // IngressHostDrifted holds details about calls to the IngressHostDrifted method. + IngressHostDrifted []struct { + // Ing is the ing argument value. + Ing *netv1.Ingress + } // Protect holds details about calls to the Protect method. Protect []struct { // Ctx is the ctx argument value. @@ -684,16 +719,19 @@ type UploaderMock struct { Ing *netv1.Ingress } } - lockCleanUp sync.RWMutex - lockCleanUpSupplements sync.RWMutex - lockGetExternalURL sync.RWMutex - lockGetInClusterURL sync.RWMutex - lockGetIngress sync.RWMutex - lockGetPod sync.RWMutex - lockGetService sync.RWMutex - lockProtect sync.RWMutex - lockStart sync.RWMutex - lockUnprotect sync.RWMutex + lockCleanUp sync.RWMutex + lockCleanUpSupplements sync.RWMutex + lockEnsureIngress sync.RWMutex + lockExpectedIngressHost sync.RWMutex + lockGetExternalURL sync.RWMutex + lockGetInClusterURL sync.RWMutex + lockGetIngress sync.RWMutex + lockGetPod sync.RWMutex + lockGetService sync.RWMutex + lockIngressHostDrifted sync.RWMutex + lockProtect sync.RWMutex + lockStart sync.RWMutex + lockUnprotect sync.RWMutex } // CleanUp calls CleanUpFunc. @@ -768,6 +806,73 @@ func (mock *UploaderMock) CleanUpSupplementsCalls() []struct { return calls } +// EnsureIngress calls EnsureIngressFunc. +func (mock *UploaderMock) EnsureIngress(ctx context.Context, obj client.Object, sup supplements.Generator) (*netv1.Ingress, error) { + if mock.EnsureIngressFunc == nil { + panic("UploaderMock.EnsureIngressFunc: method is nil but Uploader.EnsureIngress was just called") + } + callInfo := struct { + Ctx context.Context + Obj client.Object + Sup supplements.Generator + }{ + Ctx: ctx, + Obj: obj, + Sup: sup, + } + mock.lockEnsureIngress.Lock() + mock.calls.EnsureIngress = append(mock.calls.EnsureIngress, callInfo) + mock.lockEnsureIngress.Unlock() + return mock.EnsureIngressFunc(ctx, obj, sup) +} + +// EnsureIngressCalls gets all the calls that were made to EnsureIngress. +// Check the length with: +// +// len(mockedUploader.EnsureIngressCalls()) +func (mock *UploaderMock) EnsureIngressCalls() []struct { + Ctx context.Context + Obj client.Object + Sup supplements.Generator +} { + var calls []struct { + Ctx context.Context + Obj client.Object + Sup supplements.Generator + } + mock.lockEnsureIngress.RLock() + calls = mock.calls.EnsureIngress + mock.lockEnsureIngress.RUnlock() + return calls +} + +// ExpectedIngressHost calls ExpectedIngressHostFunc. +func (mock *UploaderMock) ExpectedIngressHost() string { + if mock.ExpectedIngressHostFunc == nil { + panic("UploaderMock.ExpectedIngressHostFunc: method is nil but Uploader.ExpectedIngressHost was just called") + } + callInfo := struct { + }{} + mock.lockExpectedIngressHost.Lock() + mock.calls.ExpectedIngressHost = append(mock.calls.ExpectedIngressHost, callInfo) + mock.lockExpectedIngressHost.Unlock() + return mock.ExpectedIngressHostFunc() +} + +// ExpectedIngressHostCalls gets all the calls that were made to ExpectedIngressHost. +// Check the length with: +// +// len(mockedUploader.ExpectedIngressHostCalls()) +func (mock *UploaderMock) ExpectedIngressHostCalls() []struct { +} { + var calls []struct { + } + mock.lockExpectedIngressHost.RLock() + calls = mock.calls.ExpectedIngressHost + mock.lockExpectedIngressHost.RUnlock() + return calls +} + // GetExternalURL calls GetExternalURLFunc. func (mock *UploaderMock) GetExternalURL(ctx context.Context, ing *netv1.Ingress) string { if mock.GetExternalURLFunc == nil { @@ -948,6 +1053,38 @@ func (mock *UploaderMock) GetServiceCalls() []struct { return calls } +// IngressHostDrifted calls IngressHostDriftedFunc. +func (mock *UploaderMock) IngressHostDrifted(ing *netv1.Ingress) bool { + if mock.IngressHostDriftedFunc == nil { + panic("UploaderMock.IngressHostDriftedFunc: method is nil but Uploader.IngressHostDrifted was just called") + } + callInfo := struct { + Ing *netv1.Ingress + }{ + Ing: ing, + } + mock.lockIngressHostDrifted.Lock() + mock.calls.IngressHostDrifted = append(mock.calls.IngressHostDrifted, callInfo) + mock.lockIngressHostDrifted.Unlock() + return mock.IngressHostDriftedFunc(ing) +} + +// IngressHostDriftedCalls gets all the calls that were made to IngressHostDrifted. +// Check the length with: +// +// len(mockedUploader.IngressHostDriftedCalls()) +func (mock *UploaderMock) IngressHostDriftedCalls() []struct { + Ing *netv1.Ingress +} { + var calls []struct { + Ing *netv1.Ingress + } + mock.lockIngressHostDrifted.RLock() + calls = mock.calls.IngressHostDrifted + mock.lockIngressHostDrifted.RUnlock() + return calls +} + // Protect calls ProtectFunc. func (mock *UploaderMock) Protect(ctx context.Context, sup supplements.Generator, pod *corev1.Pod, svc *corev1.Service, ing *netv1.Ingress) error { if mock.ProtectFunc == nil { diff --git a/images/virtualization-artifact/pkg/controller/vi/internal/source/upload.go b/images/virtualization-artifact/pkg/controller/vi/internal/source/upload.go index 2404cccea4..00b2b148ea 100644 --- a/images/virtualization-artifact/pkg/controller/vi/internal/source/upload.go +++ b/images/virtualization-artifact/pkg/controller/vi/internal/source/upload.go @@ -195,6 +195,17 @@ func (ds UploadDataSource) StoreToPVC(ctx context.Context, vi *v1alpha2.VirtualI Reason(vicondition.WaitForUserUpload). Message("Waiting for the user upload.") + // Re-sync the uploader Ingress when publicDomainTemplate changed + // after the supplements were created. Apply is a no-op when fields + // already match. + if ds.uploaderService.IngressHostDrifted(ing) { + log.Info("Syncing uploader Ingress: host drifted", "old", ing.Spec.Rules[0].Host, "new", ds.uploaderService.ExpectedIngressHost()) + ing, err = ds.uploaderService.EnsureIngress(ctx, vi, supgen) + if err != nil { + return reconcile.Result{}, err + } + } + vi.Status.ImageUploadURLs = &v1alpha2.ImageUploadURLs{ External: ds.uploaderService.GetExternalURL(ctx, ing), InCluster: ds.uploaderService.GetInClusterURL(ctx, svc), @@ -486,6 +497,17 @@ func (ds UploadDataSource) StoreToDVCR(ctx context.Context, vi *v1alpha2.Virtual vi.Status.Phase = v1alpha2.ImageWaitForUserUpload vi.Status.Target.RegistryURL = ds.statService.GetDVCRImageName(pod) + + // Re-sync the uploader Ingress when publicDomainTemplate changed after + // the supplements were created. Apply is a no-op when fields already match. + if ds.uploaderService.IngressHostDrifted(ing) { + log.Info("Syncing uploader Ingress: host drifted", "old", ing.Spec.Rules[0].Host, "new", ds.uploaderService.ExpectedIngressHost()) + ing, err = ds.uploaderService.EnsureIngress(ctx, vi, supgen) + if err != nil { + return reconcile.Result{}, err + } + } + vi.Status.ImageUploadURLs = &v1alpha2.ImageUploadURLs{ External: ds.uploaderService.GetExternalURL(ctx, ing), InCluster: ds.uploaderService.GetInClusterURL(ctx, svc), From 03d5a22e62a4507ac0254a93cd014e0a3f0c6eae Mon Sep 17 00:00:00 2001 From: Pavel Tishkov Date: Thu, 25 Jun 2026 10:57:37 +0300 Subject: [PATCH 3/3] fix(uploader): sync ingress before readiness probe IsUploaderReady HTTPS-probes the Ingress host, so the Ingress must be synced before the probe runs. Moving the drift check ahead of IsUploaderReady ensures a stale host (after publicDomainTemplate change) is corrected first; otherwise readiness fails with a TLS error and the isUploaderReady branch that updates status.ImageUploadURLs is never reached. The drift check is skipped when the uploader pod is absent, so initial supplement creation still flows through Start. Signed-off-by: Pavel Tishkov --- .../controller/cvi/internal/source/upload.go | 27 ++++++---- .../controller/vd/internal/source/upload.go | 28 +++++----- .../controller/vi/internal/source/upload.go | 54 +++++++++++-------- 3 files changed, 64 insertions(+), 45 deletions(-) diff --git a/images/virtualization-artifact/pkg/controller/cvi/internal/source/upload.go b/images/virtualization-artifact/pkg/controller/cvi/internal/source/upload.go index c05003087f..785b60469c 100644 --- a/images/virtualization-artifact/pkg/controller/cvi/internal/source/upload.go +++ b/images/virtualization-artifact/pkg/controller/cvi/internal/source/upload.go @@ -101,6 +101,22 @@ func (ds UploadDataSource) Sync(ctx context.Context, cvi *v1alpha2.ClusterVirtua return reconcile.Result{}, err } + // Sync the uploader Ingress host before the readiness probe: + // IsUploaderReady HTTPS-probes the Ingress host, so a stale host (e.g. after + // publicDomainTemplate changed) makes readiness fail with a TLS error. + // Initial creation is handled by Start, so skip when the pod is absent. + if pod != nil && ds.uploaderService.IngressHostDrifted(ing) { + var oldHost string + if ing != nil && len(ing.Spec.Rules) > 0 { + oldHost = ing.Spec.Rules[0].Host + } + log.Info("Syncing uploader Ingress: host drifted", "old", oldHost, "new", ds.uploaderService.ExpectedIngressHost()) + ing, err = ds.uploaderService.EnsureIngress(ctx, cvi, supgen) + if err != nil { + return reconcile.Result{}, err + } + } + isUploaderReady, err := ds.statService.IsUploaderReady(pod, svc, ing, tlsSecret) if err != nil { return reconcile.Result{}, err @@ -243,17 +259,6 @@ func (ds UploadDataSource) Sync(ctx context.Context, cvi *v1alpha2.ClusterVirtua cvi.Status.Phase = v1alpha2.ImageWaitForUserUpload cvi.Status.Target.RegistryURL = ds.statService.GetDVCRImageName(pod) - - // Re-sync the uploader Ingress when publicDomainTemplate changed after - // the supplements were created. Apply is a no-op when fields already match. - if ds.uploaderService.IngressHostDrifted(ing) { - log.Info("Syncing uploader Ingress: host drifted", "old", ing.Spec.Rules[0].Host, "new", ds.uploaderService.ExpectedIngressHost()) - ing, err = ds.uploaderService.EnsureIngress(ctx, cvi, supgen) - if err != nil { - return reconcile.Result{}, err - } - } - cvi.Status.ImageUploadURLs = &v1alpha2.ImageUploadURLs{ External: ds.uploaderService.GetExternalURL(ctx, ing), InCluster: ds.uploaderService.GetInClusterURL(ctx, svc), diff --git a/images/virtualization-artifact/pkg/controller/vd/internal/source/upload.go b/images/virtualization-artifact/pkg/controller/vd/internal/source/upload.go index 235ea09aa3..204d2b959c 100644 --- a/images/virtualization-artifact/pkg/controller/vd/internal/source/upload.go +++ b/images/virtualization-artifact/pkg/controller/vd/internal/source/upload.go @@ -127,6 +127,22 @@ func (ds UploadDataSource) Sync(ctx context.Context, vd *v1alpha2.VirtualDisk) ( return reconcile.Result{}, err } + // Sync the uploader Ingress host before the readiness probe: + // IsUploaderReady HTTPS-probes the Ingress host, so a stale host (e.g. after + // publicDomainTemplate changed) makes readiness fail with a TLS error. + // Initial creation is handled by Start, so skip when the pod is absent. + if pod != nil && ds.uploaderService.IngressHostDrifted(ing) { + var oldHost string + if ing != nil && len(ing.Spec.Rules) > 0 { + oldHost = ing.Spec.Rules[0].Host + } + log.Info("Syncing uploader Ingress: host drifted", "old", oldHost, "new", ds.uploaderService.ExpectedIngressHost()) + ing, err = ds.uploaderService.EnsureIngress(ctx, vd, supgen) + if err != nil { + return reconcile.Result{}, err + } + } + isUploaderReady, err := ds.statService.IsUploaderReady(pod, svc, ing, tlsSecret) if err != nil { return reconcile.Result{}, err @@ -202,18 +218,6 @@ func (ds UploadDataSource) Sync(ctx context.Context, vd *v1alpha2.VirtualDisk) ( vd.Status.Phase = v1alpha2.DiskWaitForUserUpload setReadyConditionWithWFFCAccounting(vd, cb, metav1.ConditionFalse, vdcondition.WaitForUserUpload, "Waiting for the user upload.") - - // Re-sync the uploader Ingress when publicDomainTemplate changed - // after the supplements were created. Apply is a no-op when fields - // already match. - if ds.uploaderService.IngressHostDrifted(ing) { - log.Info("Syncing uploader Ingress: host drifted", "old", ing.Spec.Rules[0].Host, "new", ds.uploaderService.ExpectedIngressHost()) - ing, err = ds.uploaderService.EnsureIngress(ctx, vd, supgen) - if err != nil { - return reconcile.Result{}, err - } - } - vd.Status.ImageUploadURLs = &v1alpha2.ImageUploadURLs{ External: ds.uploaderService.GetExternalURL(ctx, ing), InCluster: ds.uploaderService.GetInClusterURL(ctx, svc), diff --git a/images/virtualization-artifact/pkg/controller/vi/internal/source/upload.go b/images/virtualization-artifact/pkg/controller/vi/internal/source/upload.go index 00b2b148ea..fd4bcf6098 100644 --- a/images/virtualization-artifact/pkg/controller/vi/internal/source/upload.go +++ b/images/virtualization-artifact/pkg/controller/vi/internal/source/upload.go @@ -109,6 +109,22 @@ func (ds UploadDataSource) StoreToPVC(ctx context.Context, vi *v1alpha2.VirtualI return reconcile.Result{}, err } + // Sync the uploader Ingress host before the readiness probe: + // IsUploaderReady HTTPS-probes the Ingress host, so a stale host (e.g. after + // publicDomainTemplate changed) makes readiness fail with a TLS error. + // Initial creation is handled by Start, so skip when the pod is absent. + if pod != nil && ds.uploaderService.IngressHostDrifted(ing) { + var oldHost string + if ing != nil && len(ing.Spec.Rules) > 0 { + oldHost = ing.Spec.Rules[0].Host + } + log.Info("Syncing uploader Ingress: host drifted", "old", oldHost, "new", ds.uploaderService.ExpectedIngressHost()) + ing, err = ds.uploaderService.EnsureIngress(ctx, vi, supgen) + if err != nil { + return reconcile.Result{}, err + } + } + isUploaderReady, err := ds.statService.IsUploaderReady(pod, svc, ing, tlsSecret) if err != nil { return reconcile.Result{}, err @@ -195,17 +211,6 @@ func (ds UploadDataSource) StoreToPVC(ctx context.Context, vi *v1alpha2.VirtualI Reason(vicondition.WaitForUserUpload). Message("Waiting for the user upload.") - // Re-sync the uploader Ingress when publicDomainTemplate changed - // after the supplements were created. Apply is a no-op when fields - // already match. - if ds.uploaderService.IngressHostDrifted(ing) { - log.Info("Syncing uploader Ingress: host drifted", "old", ing.Spec.Rules[0].Host, "new", ds.uploaderService.ExpectedIngressHost()) - ing, err = ds.uploaderService.EnsureIngress(ctx, vi, supgen) - if err != nil { - return reconcile.Result{}, err - } - } - vi.Status.ImageUploadURLs = &v1alpha2.ImageUploadURLs{ External: ds.uploaderService.GetExternalURL(ctx, ing), InCluster: ds.uploaderService.GetInClusterURL(ctx, svc), @@ -387,6 +392,22 @@ func (ds UploadDataSource) StoreToDVCR(ctx context.Context, vi *v1alpha2.Virtual return reconcile.Result{}, err } + // Sync the uploader Ingress host before the readiness probe: + // IsUploaderReady HTTPS-probes the Ingress host, so a stale host (e.g. after + // publicDomainTemplate changed) makes readiness fail with a TLS error. + // Initial creation is handled by Start, so skip when the pod is absent. + if pod != nil && ds.uploaderService.IngressHostDrifted(ing) { + var oldHost string + if ing != nil && len(ing.Spec.Rules) > 0 { + oldHost = ing.Spec.Rules[0].Host + } + log.Info("Syncing uploader Ingress: host drifted", "old", oldHost, "new", ds.uploaderService.ExpectedIngressHost()) + ing, err = ds.uploaderService.EnsureIngress(ctx, vi, supgen) + if err != nil { + return reconcile.Result{}, err + } + } + isUploaderReady, err := ds.statService.IsUploaderReady(pod, svc, ing, tlsSecret) if err != nil { return reconcile.Result{}, err @@ -497,17 +518,6 @@ func (ds UploadDataSource) StoreToDVCR(ctx context.Context, vi *v1alpha2.Virtual vi.Status.Phase = v1alpha2.ImageWaitForUserUpload vi.Status.Target.RegistryURL = ds.statService.GetDVCRImageName(pod) - - // Re-sync the uploader Ingress when publicDomainTemplate changed after - // the supplements were created. Apply is a no-op when fields already match. - if ds.uploaderService.IngressHostDrifted(ing) { - log.Info("Syncing uploader Ingress: host drifted", "old", ing.Spec.Rules[0].Host, "new", ds.uploaderService.ExpectedIngressHost()) - ing, err = ds.uploaderService.EnsureIngress(ctx, vi, supgen) - if err != nil { - return reconcile.Result{}, err - } - } - vi.Status.ImageUploadURLs = &v1alpha2.ImageUploadURLs{ External: ds.uploaderService.GetExternalURL(ctx, ing), InCluster: ds.uploaderService.GetInClusterURL(ctx, svc),