From 73fed56257cf6b60f1b3102bca54afcdc75dac68 Mon Sep 17 00:00:00 2001 From: Evan Wang Date: Tue, 30 Jun 2026 15:32:03 -0400 Subject: [PATCH 1/2] Reflect link-access SecuredAccess failures in Site status instead of only logging them --- internal/kube/site/site.go | 10 ++++- internal/kube/site/site_test.go | 68 +++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/internal/kube/site/site.go b/internal/kube/site/site.go index 37be16847..a8bf8bbff 100644 --- a/internal/kube/site/site.go +++ b/internal/kube/site/site.go @@ -228,7 +228,9 @@ func (s *Site) reconcile(siteDef *skupperv2alpha1.Site, inRecovery bool) error { s.currentGroups = s.groups() s.bindings.init(s, routerConfig) s.setBindingsConfiguredStatus(nil) - s.checkSecuredAccess() + if err := s.checkSecuredAccess(); err != nil { + s.updateConfigured(err) + } } else if len(s.currentGroups) != len(s.groups()) { s.logger.Info("EnableHA setting changed for site", slog.String("namespace", siteDef.Namespace), @@ -1537,6 +1539,7 @@ func asSecuredAccessSpec(routerAccess *skupperv2alpha1.RouterAccess, group strin func (s *Site) checkSecuredAccess() error { groups := s.groups() + var errs []string for i, group := range groups { for _, la := range s.linkAccess { name := la.Name @@ -1548,15 +1551,18 @@ func (s *Site) checkSecuredAccess() error { "internal.skupper.io/routeraccess": la.Name, } if err := s.access.Ensure(s.namespace, name, asSecuredAccessSpec(la, group, s.site.DefaultIssuer()), annotations, routerAccessOwner(la)); err != nil { - //TODO: add message to site status s.logger.Error("Error ensuring SecuredAccess for RouterAccess", slog.String("key", la.Key()), slog.Any("error", err)) + errs = append(errs, err.Error()) } else { s.accessMapping[name] = newSecuredAccessMapping(la.Name, group) } } } + if len(errs) > 0 { + return fmt.Errorf("%s", strings.Join(errs, ", ")) + } return nil } diff --git a/internal/kube/site/site_test.go b/internal/kube/site/site_test.go index 6e804b05b..8f470ba9f 100644 --- a/internal/kube/site/site_test.go +++ b/internal/kube/site/site_test.go @@ -1112,6 +1112,74 @@ func TestSite_CheckRouterAccess(t *testing.T) { } } +func TestSite_checkSecuredAccess(t *testing.T) { + tests := []struct { + name string + skupperErrorMessage string + wantErr bool + wantAccessMapping int + }{ + { + name: "secured access ensured for each router access", + wantErr: false, + wantAccessMapping: 1, + }, + { + name: "secured access ensure failure is returned, not just logged", + skupperErrorMessage: "secured access create failed", + wantErr: true, + wantAccessMapping: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s, err := newSiteMocks("test", nil, nil, tt.skupperErrorMessage, true) + assert.Assert(t, err) + + s.linkAccess["my-ra"] = &skupperv2alpha1.RouterAccess{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-ra", + Namespace: "test", + }, + Spec: skupperv2alpha1.RouterAccessSpec{ + AccessType: "loadbalancer", + }, + } + + err = s.checkSecuredAccess() + if (err != nil) != tt.wantErr { + t.Errorf("Site.checkSecuredAccess() error = %v, wantErr %v", err, tt.wantErr) + } + if tt.skupperErrorMessage != "" && err != nil { + assert.ErrorContains(t, err, tt.skupperErrorMessage) + } + assert.Equal(t, tt.wantAccessMapping, len(s.accessMapping)) + }) + } +} + +func TestSite_checkSecuredAccess_reportsErrorOnInitialization(t *testing.T) { + s, err := newSiteMocks("test", nil, nil, "secured access create failed", true) + assert.Assert(t, err) + + s.linkAccess["my-ra"] = &skupperv2alpha1.RouterAccess{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-ra", + Namespace: "test", + }, + Spec: skupperv2alpha1.RouterAccessSpec{ + AccessType: "loadbalancer", + }, + } + + if err := s.checkSecuredAccess(); err != nil { + s.updateConfigured(err) + } + + assert.Assert(t, !s.site.IsConfigured(), "site should not be reported as configured when SecuredAccess creation fails") +} + func Test_NetworkStatusUpdate(t *testing.T) { type args struct { siteRecord []skupperv2alpha1.SiteRecord From 2b044ebb73fe627f83a0fe51cb2959f9ad5378fa Mon Sep 17 00:00:00 2001 From: Evan Wang Date: Wed, 1 Jul 2026 10:43:42 -0400 Subject: [PATCH 2/2] Addressed copilot's comments --- internal/kube/site/site.go | 8 ++++---- internal/kube/site/site_test.go | 6 ++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/internal/kube/site/site.go b/internal/kube/site/site.go index a8bf8bbff..3a9afa9b9 100644 --- a/internal/kube/site/site.go +++ b/internal/kube/site/site.go @@ -229,7 +229,7 @@ func (s *Site) reconcile(siteDef *skupperv2alpha1.Site, inRecovery bool) error { s.bindings.init(s, routerConfig) s.setBindingsConfiguredStatus(nil) if err := s.checkSecuredAccess(); err != nil { - s.updateConfigured(err) + return err } } else if len(s.currentGroups) != len(s.groups()) { s.logger.Info("EnableHA setting changed for site", @@ -1539,7 +1539,7 @@ func asSecuredAccessSpec(routerAccess *skupperv2alpha1.RouterAccess, group strin func (s *Site) checkSecuredAccess() error { groups := s.groups() - var errs []string + var errs []error for i, group := range groups { for _, la := range s.linkAccess { name := la.Name @@ -1554,14 +1554,14 @@ func (s *Site) checkSecuredAccess() error { s.logger.Error("Error ensuring SecuredAccess for RouterAccess", slog.String("key", la.Key()), slog.Any("error", err)) - errs = append(errs, err.Error()) + errs = append(errs, fmt.Errorf("%s: %w", la.Key(), err)) } else { s.accessMapping[name] = newSecuredAccessMapping(la.Name, group) } } } if len(errs) > 0 { - return fmt.Errorf("%s", strings.Join(errs, ", ")) + return stderrors.Join(errs...) } return nil } diff --git a/internal/kube/site/site_test.go b/internal/kube/site/site_test.go index 8f470ba9f..24a2b9d87 100644 --- a/internal/kube/site/site_test.go +++ b/internal/kube/site/site_test.go @@ -1173,10 +1173,8 @@ func TestSite_checkSecuredAccess_reportsErrorOnInitialization(t *testing.T) { }, } - if err := s.checkSecuredAccess(); err != nil { - s.updateConfigured(err) - } - + err = s.Reconcile(s.site) + assert.Assert(t, err != nil) assert.Assert(t, !s.site.IsConfigured(), "site should not be reported as configured when SecuredAccess creation fails") }