From 8a026589be74f049dfebed090026f298002306dc Mon Sep 17 00:00:00 2001 From: Fernando Giorgetti Date: Tue, 14 Oct 2025 10:52:22 -0300 Subject: [PATCH 1/2] Allows the Controller to delegate certificate management This feature enables the Skupper Controller to delegate certificate management to an external controller, allowing the controller to be defined at: * Controller startup (through: `-certificate-controller` flag or `CERTIFICATE_CONTROLLER` env var) * Through the `certificate-controller` setting for the following resources: * Site * SecuredAccess * RouterAccess * Certificate --- .../cmd/skupper/link/kube/link_generate.go | 10 +- .../skupper/link/kube/link_generate_test.go | 20 +++ internal/kube/certificates/mgr.go | 117 +++++++++++--- internal/kube/certificates/mgr_test.go | 120 +++++++++++++- internal/kube/controller/config.go | 2 + internal/kube/controller/controller.go | 8 +- internal/kube/securedaccess/access.go | 18 ++- internal/kube/securedaccess/access_test.go | 152 +++++++++++++++--- internal/kube/site/site.go | 104 +++++++----- internal/kube/site/site_test.go | 2 +- pkg/apis/skupper/v2alpha1/types.go | 100 ++++++++++++ 11 files changed, 560 insertions(+), 93 deletions(-) diff --git a/internal/cmd/skupper/link/kube/link_generate.go b/internal/cmd/skupper/link/kube/link_generate.go index 80f0dfc8b..2371b51a5 100644 --- a/internal/cmd/skupper/link/kube/link_generate.go +++ b/internal/cmd/skupper/link/kube/link_generate.go @@ -209,8 +209,14 @@ func (cmd *CmdLinkGenerate) Run() error { Subject: getSubjectsFromEndpoints(cmd.activeSite.Status.Endpoints), }, } - - _, err := cmd.Client.Certificates(cmd.Namespace).Create(context.TODO(), &certificate, metav1.CreateOptions{}) + defaultIssuer := pkgutils.DefaultStr(cmd.activeSite.Status.DefaultIssuer, "skupper-site-ca") + defaultIssuerCert, err := cmd.Client.Certificates(cmd.Namespace).Get(context.TODO(), defaultIssuer, metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("unable to retrieve default issuer certificate %q: %w", defaultIssuer, err) + } + certificateController := defaultIssuerCert.Spec.GetCertificateController() + certificate.Spec.SetCertificateController(certificateController) + _, err = cmd.Client.Certificates(cmd.Namespace).Create(context.TODO(), &certificate, metav1.CreateOptions{}) if err != nil { return err } diff --git a/internal/cmd/skupper/link/kube/link_generate_test.go b/internal/cmd/skupper/link/kube/link_generate_test.go index 0d00485b3..c0acf65f6 100644 --- a/internal/cmd/skupper/link/kube/link_generate_test.go +++ b/internal/cmd/skupper/link/kube/link_generate_test.go @@ -9,11 +9,13 @@ import ( "github.com/skupperproject/skupper/internal/cmd/skupper/common/utils" fakeclient "github.com/skupperproject/skupper/internal/kube/client/fake" "github.com/skupperproject/skupper/pkg/apis/skupper/v2alpha1" + fakeskupperv2alpha1 "github.com/skupperproject/skupper/pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/fake" "gotest.tools/v3/assert" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + k8stesting "k8s.io/client-go/testing" ) func TestCmdLinkGenerate_ValidateInput(t *testing.T) { @@ -1004,6 +1006,24 @@ func newCmdLinkGenerateWithMocks(namespace string, k8sObjects []runtime.Object, if err != nil { return nil, err } + defaultIssuer := &v2alpha1.Certificate{ + TypeMeta: v1.TypeMeta{ + APIVersion: "skupper.io/v2alpha1", + Kind: "Certificate", + }, + ObjectMeta: v1.ObjectMeta{ + Name: "skupper-site-ca", + Namespace: namespace, + }, + } + fakeSkupperCli := client.Skupper.SkupperV2alpha1().(*fakeskupperv2alpha1.FakeSkupperV2alpha1) + fakeSkupperCli.PrependReactor("get", "certificates", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { + getAction := action.(k8stesting.GetAction) + if getAction.GetName() == "skupper-site-ca" { + return true, defaultIssuer, nil + } + return false, nil, nil + }) CmdLinkGenerate := &CmdLinkGenerate{ Client: client.GetSkupperClient().SkupperV2alpha1(), KubeClient: client.GetKubeClient(), diff --git a/internal/kube/certificates/mgr.go b/internal/kube/certificates/mgr.go index f95667c51..64c2528f0 100644 --- a/internal/kube/certificates/mgr.go +++ b/internal/kube/certificates/mgr.go @@ -11,7 +11,9 @@ import ( "strings" "time" + "github.com/skupperproject/skupper/internal/utils" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -39,26 +41,31 @@ type ControllerContext interface { // the existence of a particular Certificate resource can be // ensured. It is currently used by package internal/kube/site. type CertificateManager interface { - EnsureCA(namespace string, name string, subject string, refs []metav1.OwnerReference) error - Ensure(namespace string, name string, ca string, subject string, hosts []string, client bool, server bool, refs []metav1.OwnerReference) error + EnsureCA(namespace string, name string, subject string, controller string, refs []metav1.OwnerReference) error + Ensure(namespace string, name string, ca string, subject string, hosts []string, client bool, server bool, controller string, refs []metav1.OwnerReference) error } type CertificateManagerImpl struct { - definitions map[string]*skupperv2alpha1.Certificate - secrets map[string]*corev1.Secret - certificateWatcher *watchers.CertificateWatcher - secretWatcher *watchers.SecretWatcher - processor *watchers.EventProcessor - context ControllerContext + certificateController string + delegated map[string]*skupperv2alpha1.Certificate + definitions map[string]*skupperv2alpha1.Certificate + secrets map[string]*corev1.Secret + certificateWatcher *watchers.CertificateWatcher + secretWatcher *watchers.SecretWatcher + processor *watchers.EventProcessor + context ControllerContext } // Returns a correctly initialised CertificateManager. -func NewCertificateManager(processor *watchers.EventProcessor) *CertificateManagerImpl { - return &CertificateManagerImpl{ - definitions: map[string]*skupperv2alpha1.Certificate{}, - secrets: map[string]*corev1.Secret{}, - processor: processor, - } +func NewCertificateManager(processor *watchers.EventProcessor, certificateController string) *CertificateManagerImpl { + certMgr := &CertificateManagerImpl{ + definitions: map[string]*skupperv2alpha1.Certificate{}, + delegated: map[string]*skupperv2alpha1.Certificate{}, + secrets: map[string]*corev1.Secret{}, + processor: processor, + certificateController: certificateController, + } + return certMgr } // Allows a ControllerContext to be set for this CertificateManager. @@ -72,6 +79,10 @@ func (m *CertificateManagerImpl) Watch(watchNamespace string) { m.secretWatcher = m.processor.WatchAllSecrets(watchNamespace, watchers.FilterByNamespace(m.isControlled, m.checkSecret)) } +func (m *CertificateManagerImpl) GetCertificateController() string { + return m.certificateController +} + func (m *CertificateManagerImpl) isControlled(namespace string) bool { if m.context != nil { return m.context.IsControlled(namespace) @@ -102,11 +113,12 @@ func (m *CertificateManagerImpl) Recover() { // This method is called to ensure that a Certificate resource exists // to represent a CA (i.e. certificate issuer) with the properties // specified in the arguments. -func (m *CertificateManagerImpl) EnsureCA(namespace string, name string, subject string, refs []metav1.OwnerReference) error { +func (m *CertificateManagerImpl) EnsureCA(namespace string, name string, subject string, controller string, refs []metav1.OwnerReference) error { spec := skupperv2alpha1.CertificateSpec{ Subject: subject, Signing: true, } + spec.SetCertificateController(utils.DefaultStr(controller, m.certificateController)) return m.ensure(namespace, name, spec, refs) } @@ -118,7 +130,7 @@ func (m *CertificateManagerImpl) EnsureCA(namespace string, name string, subject // if the same owner changes the hosts then they will be changed on // the certificate. This allows the same certificate to be used for // multiple resources such as Routes. -func (m *CertificateManagerImpl) Ensure(namespace string, name string, ca string, subject string, hosts []string, client bool, server bool, refs []metav1.OwnerReference) error { +func (m *CertificateManagerImpl) Ensure(namespace string, name string, ca string, subject string, hosts []string, client bool, server bool, controller string, refs []metav1.OwnerReference) error { spec := skupperv2alpha1.CertificateSpec{ Ca: ca, Subject: subject, @@ -126,6 +138,7 @@ func (m *CertificateManagerImpl) Ensure(namespace string, name string, ca string Client: client, Server: server, } + spec.SetCertificateController(utils.DefaultStr(controller, m.certificateController)) return m.ensure(namespace, name, spec, refs) } @@ -135,6 +148,7 @@ var compareSpecUnordered []cmp.Option = []cmp.Option{ } func (m *CertificateManagerImpl) ensure(namespace string, name string, spec skupperv2alpha1.CertificateSpec, refs []metav1.OwnerReference) error { + certsCli := m.processor.GetSkupperClient().SkupperV2alpha1().Certificates(namespace) key := fmt.Sprintf("%s/%s", namespace, name) if current, ok := m.definitions[key]; ok { changed := false @@ -184,8 +198,19 @@ func (m *CertificateManagerImpl) ensure(namespace string, name string, spec skup if !changed { return nil } - updated, err := m.processor.GetSkupperClient().SkupperV2alpha1().Certificates(namespace).Update(context.Background(), current, metav1.UpdateOptions{}) + m.setLatestResourceVersion(current) + updated, err := certsCli.Update(context.Background(), current, metav1.UpdateOptions{}) if err != nil { + log.Printf("Error updating certificate %s: %s", key, err) + if apierrors.IsConflict(err) { + latest, getErr := certsCli.Get(context.Background(), current.Name, metav1.GetOptions{}) + if getErr != nil { + log.Printf("Error getting latest certificate state for %s: %s", key, err) + } else { + log.Printf("Restoring latest certificate state for %s", key) + m.definitions[key] = latest + } + } return err } log.Printf("Updated certificate %s/%s", updated.Namespace, updated.Name) @@ -214,15 +239,39 @@ func (m *CertificateManagerImpl) ensure(namespace string, name string, spec skup m.context.SetAnnotations(namespace, cert.Name, "Certificate", cert.ObjectMeta.Annotations) } - created, err := m.processor.GetSkupperClient().SkupperV2alpha1().Certificates(namespace).Create(context.Background(), cert, metav1.CreateOptions{}) + created, err := certsCli.Create(context.Background(), cert, metav1.CreateOptions{}) if err != nil { - return err + if apierrors.IsAlreadyExists(err) { + log.Printf("Certificate %s/%s already exists - loading latest", namespace, name) + created, err = certsCli.Get(context.Background(), cert.Name, metav1.GetOptions{}) + if err != nil { + return err + } + } else { + log.Printf("Error creating certificate %s: %s", key, err) + return err + } } m.definitions[key] = created return nil } } +func (m *CertificateManagerImpl) setLatestResourceVersion(current *skupperv2alpha1.Certificate) { + if !current.Spec.HasCertificateController() { + return + } + certsCli := m.processor.GetSkupperClient().SkupperV2alpha1().Certificates(current.GetNamespace()) + latest, err := certsCli.Get(context.Background(), current.Name, metav1.GetOptions{}) + if err != nil { + log.Printf("Unable to retrieve latest certificate for %q: %v", current.Key(), err) + } else if current.GetResourceVersion() != latest.GetResourceVersion() { + log.Printf("Updating certificate generation for %q", current.Key()) + current.ObjectMeta.Generation = latest.GetGeneration() + current.ObjectMeta.ResourceVersion = latest.GetResourceVersion() + } +} + // Called by EventProcessor whenever there is a change to a Certificate reasource. func (m *CertificateManagerImpl) checkCertificate(key string, certificate *skupperv2alpha1.Certificate) error { if certificate == nil { @@ -259,6 +308,9 @@ func (m *CertificateManagerImpl) checkCertificate(key string, certificate *skupp func (m *CertificateManagerImpl) reconcileSecret(key string, certificate *skupperv2alpha1.Certificate, secret *corev1.Secret) error { var err error + if m.ensureDelegated(certificate) { + return nil + } if secret != nil { err = m.updateSecret(key, certificate, secret) } else { @@ -466,6 +518,33 @@ func hasControlledAnnotation(secret *corev1.Secret) bool { return ok } +func (m *CertificateManagerImpl) ensureDelegated(certificate *skupperv2alpha1.Certificate) bool { + var controller string + var delegate bool + + if m.certificateController != "" { + controller = m.certificateController + delegate = true + } else if controller = certificate.Spec.GetCertificateController(); controller != "" { + delegate = true + } + + key := certificate.Key() + _, isDelegated := m.delegated[key] + if delegate { + if !isDelegated { + log.Printf("Certificate '%s' has been delegated to '%s'", key, controller) + m.delegated[key] = certificate + } + } else { + if isDelegated { + log.Printf("Certificate '%s' is no longer delegated", key) + delete(m.delegated, key) + } + } + return delegate +} + func hasCertificateOwner(secret *corev1.Secret) bool { for _, owner := range secret.ObjectMeta.OwnerReferences { if owner.Kind == "Certificate" && owner.APIVersion == "skupper.io/v2alpha1" { diff --git a/internal/kube/certificates/mgr_test.go b/internal/kube/certificates/mgr_test.go index 6eb8c8027..b6db86309 100644 --- a/internal/kube/certificates/mgr_test.go +++ b/internal/kube/certificates/mgr_test.go @@ -33,6 +33,7 @@ func TestCertificateManager(t *testing.T) { k8sObjects []runtime.Object skupperObjects []runtime.Object context ControllerContext + certController string calls []*Call expectedSecrets []*corev1.Secret expectedCertificates []*skupperv2alpha1.Certificate @@ -53,6 +54,22 @@ func TestCertificateManager(t *testing.T) { addCertificateStatus(certificate("foo", "test", "my-ca", "my-subject", []string{"aaa", "bbb"}, false, true, nil, nil), "", "", condition(skupperv2alpha1.CONDITION_TYPE_READY, metav1.ConditionTrue, "Ready", "OK")), }, }, + { + name: "simple recovery of all delegated certificates", + k8sObjects: []runtime.Object{ + myCaFixture, + }, + skupperObjects: []runtime.Object{ + certificate("foo", "test", "my-ca", "my-subject", []string{"aaa", "bbb"}, false, true, nil, nil), + }, + certController: "external", + expectedSecrets: []*corev1.Secret{ + secret("my-ca", "test", nil, nil, nil), + }, + expectedCertificates: []*skupperv2alpha1.Certificate{ + addCertificateStatus(certificate("foo", "test", "my-ca", "my-subject", []string{"aaa", "bbb"}, false, true, nil, nil), "", ""), + }, + }, { name: "recovery namespace not controlled", k8sObjects: []runtime.Object{ @@ -180,6 +197,80 @@ func TestCertificateManager(t *testing.T) { certificate("foo", "test", "my-ca", "my-subject", []string{"xxx", "yyy"}, false, true, map[string]string{"foo": "bar"}, map[string]string{"x": "y"}), }, }, + { + name: "ensure all delegated ca and certificate", + k8sObjects: []runtime.Object{}, + skupperObjects: []runtime.Object{}, + context: fakeContext().control("test"), + certController: "external", + calls: []*Call{ + call("my-ca", "test").ensureCa("my-cas-subject").noSecretEvent(), + call("foo", "test").ensure("my-ca", "my-subject", []string{"xxx", "yyy"}, false, true).noSecretEvent(), + }, + expectedSecrets: []*corev1.Secret{}, + expectedCertificates: []*skupperv2alpha1.Certificate{ + caCertificate("my-ca", "test", "my-cas-subject", map[string]string{"foo": "bar"}, map[string]string{"x": "y"}), + certificate("foo", "test", "my-ca", "my-subject", []string{"xxx", "yyy"}, false, true, map[string]string{"foo": "bar"}, map[string]string{"x": "y"}), + }, + }, + { + name: "ensure single delegated certificate", + k8sObjects: []runtime.Object{}, + skupperObjects: []runtime.Object{}, + context: fakeContext().control("test"), + calls: []*Call{ + call("my-ca", "test").ensureCa("my-cas-subject"), + call("foo-delegated", "test").ensure("my-ca", "my-subject", []string{"xxx", "yyy"}, false, true).setExternalController().noSecretEvent(), + call("foo-handled", "test").ensure("my-ca", "my-subject", []string{"xxx", "yyy"}, false, true), + }, + expectedSecrets: []*corev1.Secret{ + secret("my-ca", "test", nil, nil, nil), + secret("foo-handled", "test", nil, nil, nil), + }, + expectedCertificates: []*skupperv2alpha1.Certificate{ + caCertificate("my-ca", "test", "my-cas-subject", map[string]string{"foo": "bar"}, map[string]string{"x": "y"}), + certificate("foo-handled", "test", "my-ca", "my-subject", []string{"xxx", "yyy"}, false, true, map[string]string{"foo": "bar"}, map[string]string{"x": "y"}), + certificate("foo-delegated", "test", "my-ca", "my-subject", []string{"xxx", "yyy"}, false, true, map[string]string{"foo": "bar"}, map[string]string{"x": "y"}), + }, + }, + { + name: "ensure certificate no longer delegated", + k8sObjects: []runtime.Object{}, + skupperObjects: []runtime.Object{}, + context: fakeContext().control("test"), + calls: []*Call{ + call("my-ca", "test").ensureCa("my-cas-subject"), + call("foo", "test").ensure("my-ca", "my-subject", []string{"xxx", "yyy"}, false, true).setExternalController().noSecretEvent(), + call("foo", "test").ensure("my-ca", "my-subject", []string{"xxx", "yyy"}, false, true), + }, + expectedSecrets: []*corev1.Secret{ + secret("my-ca", "test", nil, nil, nil), + secret("foo", "test", nil, nil, nil), + }, + expectedCertificates: []*skupperv2alpha1.Certificate{ + caCertificate("my-ca", "test", "my-cas-subject", map[string]string{"foo": "bar"}, map[string]string{"x": "y"}), + certificate("foo", "test", "my-ca", "my-subject", []string{"xxx", "yyy"}, false, true, map[string]string{"foo": "bar"}, map[string]string{"x": "y"}), + }, + }, + { + name: "ensure existing certificate can be delegated and secret remains", + k8sObjects: []runtime.Object{}, + skupperObjects: []runtime.Object{}, + context: fakeContext().control("test"), + calls: []*Call{ + call("my-ca", "test").ensureCa("my-cas-subject"), + call("foo", "test").ensure("my-ca", "my-subject", []string{"xxx", "yyy"}, false, true), + call("foo", "test").ensure("my-ca", "my-subject", []string{"xxx", "yyy"}, false, true).setExternalController().noSecretEvent(), + }, + expectedSecrets: []*corev1.Secret{ + secret("my-ca", "test", nil, nil, nil), + secret("foo", "test", nil, nil, nil), + }, + expectedCertificates: []*skupperv2alpha1.Certificate{ + caCertificate("my-ca", "test", "my-cas-subject", map[string]string{"foo": "bar"}, map[string]string{"x": "y"}), + certificate("foo", "test", "my-ca", "my-subject", []string{"xxx", "yyy"}, false, true, map[string]string{"foo": "bar"}, map[string]string{"x": "y"}), + }, + }, { name: "update hosts for recovered certificate", k8sObjects: []runtime.Object{ @@ -251,7 +342,7 @@ func TestCertificateManager(t *testing.T) { }, context: fakeContext().control("test").label("foo", "bar").annotate("x", "y"), calls: []*Call{ - call("foo", "test").ensure("my-ca", "my-subject", []string{"xxx", "yyy"}, false, true).mustError(), + call("foo", "test").ensure("my-ca", "my-subject", []string{"xxx", "yyy"}, false, true).mustError().eventcount(0), }, expectedSecrets: []*corev1.Secret{ secret("foo", "test", nil, map[string]string{"foo": "bar"}, map[string]string{"x": "y"}), @@ -330,7 +421,7 @@ func TestCertificateManager(t *testing.T) { assert.Assert(t, err) } processor := watchers.NewEventProcessor("Controller", client) - mgr := NewCertificateManager(processor) + mgr := NewCertificateManager(processor, tt.certController) if tt.context != nil { mgr.SetControllerContext(tt.context) } @@ -350,7 +441,9 @@ func TestCertificateManager(t *testing.T) { } for i := 0; i < c.events; i++ { processor.TestProcess() - processor.TestProcess() + if c.secretEvt { + processor.TestProcess() + } } } @@ -570,6 +663,8 @@ type Call struct { signing bool refs []metav1.OwnerReference events int + controller string + secretEvt bool deleteCert bool updateCert *skupperv2alpha1.Certificate expectErr bool @@ -580,6 +675,7 @@ func call(name string, namespace string) *Call { name: name, namespace: namespace, refs: fixtureRefs, + secretEvt: true, } } @@ -591,10 +687,14 @@ func (c *Call) invoke(mgr *CertificateManagerImpl) error { _, err := mgr.processor.GetSkupperClient().SkupperV2alpha1().Certificates(c.namespace).Update(context.Background(), c.updateCert, metav1.UpdateOptions{}) return err } + controller := c.controller + if controller == "" { + controller = mgr.certificateController + } if c.signing { - return mgr.EnsureCA(c.namespace, c.name, c.subject, c.refs) + return mgr.EnsureCA(c.namespace, c.name, c.subject, controller, c.refs) } - return mgr.Ensure(c.namespace, c.name, c.ca, c.subject, c.hosts, c.client, c.server, c.refs) + return mgr.Ensure(c.namespace, c.name, c.ca, c.subject, c.hosts, c.client, c.server, controller, c.refs) } func (c *Call) ensure(ca string, subject string, hosts []string, client bool, server bool) *Call { @@ -643,6 +743,16 @@ func (c *Call) eventcount(events int) *Call { return c } +func (c *Call) noSecretEvent() *Call { + c.secretEvt = false + return c +} + +func (c *Call) setExternalController() *Call { + c.controller = "external" + return c +} + func (c *Call) mustError() *Call { c.expectErr = true return c diff --git a/internal/kube/controller/config.go b/internal/kube/controller/config.go index 334c3be60..333de01a1 100644 --- a/internal/kube/controller/config.go +++ b/internal/kube/controller/config.go @@ -17,6 +17,7 @@ type Config struct { Kubeconfig string WatchNamespace string Name string + CertificateController string RequireExplicitControl bool } @@ -47,6 +48,7 @@ func BoundConfig(flags *flag.FlagSet) (*Config, error) { iflag.StringVar(flags, &c.Kubeconfig, "kubeconfig", "KUBECONFIG", "", "A path to the kubeconfig file to use") iflag.StringVar(flags, &c.WatchNamespace, "watch-namespace", "WATCH_NAMESPACE", metav1.NamespaceAll, "The Kubernetes namespace the controller should monitor for controlled resources (will monitor all if not specified)") iflag.StringVar(flags, &c.Name, "name", "CONTROLLER_NAME", "", "A name identifying the controller. If not specified it will be deduced from the hostname.") + iflag.StringVar(flags, &c.CertificateController, "certificate-controller", "CERTIFICATE_CONTROLLER", "", "A name that identifies the controller responsible for the Certificates.") iflag.BoolVar(flags, &c.RequireExplicitControl, "require-explicit-control", "REQUIRE_EXPLICIT_CONTROL", false, "If set, this controller instance will only process resources in which there is a ConfigMap named skupper with an entry 'controller' whose value matches the controller's namespace qualified name. Controllers watching a single namespace require that ConfigMap regardless of this setting.") return c, nil } diff --git a/internal/kube/controller/controller.go b/internal/kube/controller/controller.go index fc60ab5f7..10603828c 100644 --- a/internal/kube/controller/controller.go +++ b/internal/kube/controller/controller.go @@ -133,7 +133,11 @@ func NewController(cli internalclient.Clients, config *Config) (*Controller, err controller.namespaces.watch(controller.eventProcessor, config.WatchNamespace) controller.labellingWatcher = controller.eventProcessor.WatchConfigMaps(labelling(), config.WatchNamespace, controller.labelling.Update) - controller.certMgr = certificates.NewCertificateManager(controller.eventProcessor) + controller.certMgr = certificates.NewCertificateManager(controller.eventProcessor, config.CertificateController) + if config.CertificateController != "" { + controller.log.Info("Certificate management has been delegated", + slog.String("certificate-controller", config.CertificateController)) + } controller.certMgr.SetControllerContext(controller) controller.certMgr.Watch(config.WatchNamespace) @@ -318,7 +322,7 @@ func (c *Controller) getSite(namespace string) *site.Site { if existing, ok := c.sites[namespace]; ok { return existing } - site := site.NewSite(namespace, c.eventProcessor, c.certMgr, c.accessMgr, c.siteSizing, c) + site := site.NewSite(namespace, c.eventProcessor, c.certMgr, c.accessMgr, c.siteSizing, c, c.certMgr.GetCertificateController()) c.sites[namespace] = site return site } diff --git a/internal/kube/securedaccess/access.go b/internal/kube/securedaccess/access.go index 0e40874e4..a48ddf86b 100644 --- a/internal/kube/securedaccess/access.go +++ b/internal/kube/securedaccess/access.go @@ -9,6 +9,7 @@ import ( corev1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/intstr" @@ -95,7 +96,7 @@ func (m *SecuredAccessManager) IsValidAccessType(accessType string) bool { return ok } -func (m *SecuredAccessManager) Ensure(namespace string, name string, spec skupperv2alpha1.SecuredAccessSpec, annotations map[string]string, refs []metav1.OwnerReference) error { +func (m *SecuredAccessManager) Ensure(namespace string, name string, spec skupperv2alpha1.SecuredAccessSpec, annotations map[string]string, certificateController string, refs []metav1.OwnerReference) error { key := fmt.Sprintf("%s/%s", namespace, name) if current, ok := m.definitions[key]; ok { if current.ObjectMeta.Labels == nil { @@ -150,6 +151,7 @@ func (m *SecuredAccessManager) Ensure(namespace string, name string, spec skuppe }, Spec: spec, } + sa.Spec.SetCertificateController(certificateController) if m.context != nil { if sa.ObjectMeta.Annotations == nil { sa.ObjectMeta.Annotations = map[string]string{} @@ -159,7 +161,17 @@ func (m *SecuredAccessManager) Ensure(namespace string, name string, spec skuppe } created, err := m.clients.GetSkupperClient().SkupperV2alpha1().SecuredAccesses(namespace).Create(context.Background(), sa, metav1.CreateOptions{}) if err != nil { - return err + // needed on controller restart to prevent invalid errors from being logged + if apierrors.IsAlreadyExists(err) { + log.Printf("SecuredAccess already exists %s - loading latest", key) + created, err = m.clients.GetSkupperClient().SkupperV2alpha1().SecuredAccesses(namespace).Get(context.Background(), sa.Name, metav1.GetOptions{}) + if err != nil { + return err + } + } else { + log.Printf("Error creating SecuredAccess %s: %v", key, err) + return err + } } m.definitions[key] = created return nil @@ -275,7 +287,7 @@ func (m *SecuredAccessManager) checkCertificate(sa *skupperv2alpha1.SecuredAcces if name == "" { name = sa.Name } - return m.certMgr.Ensure(sa.Namespace, name, sa.Spec.Issuer, sa.Name, getHosts(sa), false, true, ownerReferences(sa)) + return m.certMgr.Ensure(sa.Namespace, name, sa.Spec.Issuer, sa.Name, getHosts(sa), false, true, sa.Spec.GetCertificateController(), ownerReferences(sa)) } func (m *SecuredAccessManager) checkService(sa *skupperv2alpha1.SecuredAccess) (*corev1.Service, error) { diff --git a/internal/kube/securedaccess/access_test.go b/internal/kube/securedaccess/access_test.go index e9050a673..061d5076c 100644 --- a/internal/kube/securedaccess/access_test.go +++ b/internal/kube/securedaccess/access_test.go @@ -151,6 +151,109 @@ func TestSecuredAccessGeneral(t *testing.T) { }, }, }, + { + name: "loadbalancer delegated", + config: Config{ + EnabledAccessTypes: []string{ + ACCESS_TYPE_LOADBALANCER, + ACCESS_TYPE_ROUTE, + ACCESS_TYPE_LOCAL, + }, + }, + labels: map[string]string{ + "foo": "bar", + }, + annotations: map[string]string{ + "abc": "123", + }, + definition: &skupperv2alpha1.SecuredAccess{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "skupper.io/v2alpha1", + Kind: "SecuredAccess", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "mysvc", + Namespace: "test", + }, + Spec: skupperv2alpha1.SecuredAccessSpec{ + AccessType: ACCESS_TYPE_LOADBALANCER, + Selector: map[string]string{ + "app": "foo", + }, + Ports: []skupperv2alpha1.SecuredAccessPort{ + { + Name: "port1", + Port: 8080, + TargetPort: 8081, + Protocol: "TCP", + }, + }, + Certificate: "my-cert", + Issuer: "skupper-site-ca", + Settings: map[string]string{ + "certificate-controller": "external", + }, + }, + }, + expectedServices: []*corev1.Service{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "mysvc", + Namespace: "test", + Labels: map[string]string{ + "foo": "bar", + }, + Annotations: map[string]string{ + "abc": "123", + }, + }, + Spec: corev1.ServiceSpec{ + Selector: map[string]string{ + "app": "foo", + }, + Type: corev1.ServiceTypeLoadBalancer, + Ports: []corev1.ServicePort{ + { + Name: "port1", + Port: 8080, + TargetPort: intstr.IntOrString{IntVal: int32(8081)}, + Protocol: corev1.Protocol("TCP"), + }, + }, + }, + Status: corev1.ServiceStatus{ + LoadBalancer: corev1.LoadBalancerStatus{ + Ingress: []corev1.LoadBalancerIngress{ + { + Hostname: "my-ingress.my-cluster.org", + }, + }, + }, + }, + }, + }, + expectedCertificates: []MockCertificate{ + { + namespace: "test", + name: "my-cert", + ca: "skupper-site-ca", + subject: "mysvc", + hosts: []string{"mysvc", "mysvc.test", "my-ingress.my-cluster.org"}, + client: false, + server: true, + controller: "external", + refs: nil, + }, + }, + expectedStatus: "OK", + expectedEndpoints: []skupperv2alpha1.Endpoint{ + { + Name: "port1", + Port: "8080", + Host: "my-ingress.my-cluster.org", + }, + }, + }, { name: "loadbalancer with IP", config: Config{ @@ -1892,7 +1995,7 @@ func TestSecuredAccessGeneral(t *testing.T) { certs := newMockCertificateManager() m := NewSecuredAccessManager(client, certs, &tt.config, &FakeControllerContext{namespace: "test", labels: tt.labels, annotations: tt.annotations}) - err = m.Ensure(tt.definition.Namespace, tt.definition.Name, tt.definition.Spec, nil, nil) + err = m.Ensure(tt.definition.Namespace, tt.definition.Name, tt.definition.Spec, nil, tt.definition.Spec.GetCertificateController(), nil) if tt.expectedError != "" { assert.ErrorContains(t, err, tt.expectedError) } else if err != nil { @@ -2454,7 +2557,7 @@ func TestSecuredAccessManagerEnsure(t *testing.T) { } for i, args := range tt.args { - err = m.Ensure(args.namespace, args.name, args.spec, args.annotations, args.refs) + err = m.Ensure(args.namespace, args.name, args.spec, args.annotations, "", args.refs) if len(tt.expectedErrors) > i && tt.expectedErrors[i] != "" { assert.ErrorContains(t, err, tt.expectedErrors[i]) } else if err != nil { @@ -2551,7 +2654,7 @@ func TestSecuredAccessDeleted(t *testing.T) { err := client.GetSkupperClient().SkupperV2alpha1().SecuredAccesses(def.namespace).Delete(context.Background(), def.name, metav1.DeleteOptions{}) assert.Assert(t, err) controller.TestProcess() - err = m.Ensure(def.namespace, def.name, def.spec, nil, nil) + err = m.Ensure(def.namespace, def.name, def.spec, nil, "", nil) assert.Assert(t, err) controller.TestProcess() } @@ -2711,7 +2814,7 @@ func TestSecuredAccessManagerChangeDelete(t *testing.T) { m, err := newSecureAccessManagerMocks("test", tt.k8sObjects, tt.skupperObjects, tt.skupperErrorMessage) assert.Assert(t, err) - if err := m.Ensure(tt.args.namespace, tt.args.name, *tt.args.spec, *tt.args.annotations, *tt.args.refs); err != nil { + if err := m.Ensure(tt.args.namespace, tt.args.name, *tt.args.spec, *tt.args.annotations, "", *tt.args.refs); err != nil { t.Errorf("SecuredAccessManager.Ensure() error = %v", err) } // change Access Type @@ -2929,7 +3032,7 @@ func TestSecuredAccessManagerCheckService(t *testing.T) { m, err := newSecureAccessManagerMocks("test", tt.k8sObjects, tt.skupperObjects, tt.skupperErrorMessage) assert.Assert(t, err) - if err := m.Ensure(tt.args.namespace, tt.args.name, *tt.args.spec, *tt.args.annotations, *tt.args.refs); err != nil { + if err := m.Ensure(tt.args.namespace, tt.args.name, *tt.args.spec, *tt.args.annotations, "", *tt.args.refs); err != nil { t.Errorf("SecuredAccessManager.CheckServic() error = %v", err) } @@ -3073,14 +3176,15 @@ func newSecureAccessManagerMocks(namespace string, k8sObjects []runtime.Object, } type MockCertificate struct { - namespace string - name string - ca string - subject string - hosts []string - client bool - server bool - refs []metav1.OwnerReference + namespace string + name string + ca string + subject string + hosts []string + client bool + server bool + controller string + refs []metav1.OwnerReference } type MockCA struct { @@ -3110,6 +3214,7 @@ func (m *MockCertificateManager) checkCertificates(t *testing.T, expected []Mock assert.Equal(t, desired.namespace, m.certs[key].namespace) assert.Equal(t, desired.name, m.certs[key].name) assert.Equal(t, desired.subject, m.certs[key].subject) + assert.Equal(t, desired.controller, m.certs[key].controller) for _, host := range desired.hosts { assert.Assert(t, cmp.Contains(m.certs[key].hosts, host)) } @@ -3133,7 +3238,7 @@ func (m *MockCertificateManager) pushError(e string) { m.errors = append(m.errors, e) } -func (m *MockCertificateManager) EnsureCA(namespace string, name string, subject string, refs []metav1.OwnerReference) error { +func (m *MockCertificateManager) EnsureCA(namespace string, name string, subject string, controller string, refs []metav1.OwnerReference) error { m.cas[namespace+"/"+name] = MockCA{ namespace: namespace, name: name, @@ -3143,16 +3248,17 @@ func (m *MockCertificateManager) EnsureCA(namespace string, name string, subject return m.popError() } -func (m *MockCertificateManager) Ensure(namespace string, name string, ca string, subject string, hosts []string, client bool, server bool, refs []metav1.OwnerReference) error { +func (m *MockCertificateManager) Ensure(namespace string, name string, ca string, subject string, hosts []string, client bool, server bool, controller string, refs []metav1.OwnerReference) error { m.certs[namespace+"/"+name] = MockCertificate{ - namespace: namespace, - name: name, - ca: ca, - subject: subject, - hosts: hosts, - client: client, - server: server, - refs: refs, + namespace: namespace, + name: name, + ca: ca, + subject: subject, + hosts: hosts, + client: client, + server: server, + controller: controller, + refs: refs, } return m.popError() } diff --git a/internal/kube/site/site.go b/internal/kube/site/site.go index 16d6f2b00..3cc421c48 100644 --- a/internal/kube/site/site.go +++ b/internal/kube/site/site.go @@ -31,7 +31,7 @@ import ( ) type SecuredAccessFactory interface { - Ensure(namespace string, name string, spec skupperv2alpha1.SecuredAccessSpec, annotations map[string]string, refs []metav1.OwnerReference) error + Ensure(namespace string, name string, spec skupperv2alpha1.SecuredAccessSpec, annotations map[string]string, certificateController string, refs []metav1.OwnerReference) error Delete(namespace string, name string) error IsValidAccessType(accessType string) bool } @@ -42,39 +42,41 @@ type Labelling interface { } type Site struct { - initialised bool - site *skupperv2alpha1.Site - name string - namespace string - clients *watchers.EventProcessor - bindings *ExtendedBindings - links map[string]*site.Link - errors map[string]string - linkAccess site.RouterAccessMap - certs certificates.CertificateManager - access SecuredAccessFactory - accessMapping securedAccessMap - sizes *sizing.Registry - routerPods map[string]*corev1.Pod - logger *slog.Logger - currentGroups []string - labelling Labelling - profiles *secrets.ProfilesWatcher -} - -func NewSite(namespace string, eventProcessor *watchers.EventProcessor, certs certificates.CertificateManager, access SecuredAccessFactory, sizes *sizing.Registry, labelling Labelling) *Site { + initialised bool + site *skupperv2alpha1.Site + name string + namespace string + clients *watchers.EventProcessor + bindings *ExtendedBindings + links map[string]*site.Link + errors map[string]string + linkAccess site.RouterAccessMap + certs certificates.CertificateManager + certificateController string + access SecuredAccessFactory + accessMapping securedAccessMap + sizes *sizing.Registry + routerPods map[string]*corev1.Pod + logger *slog.Logger + currentGroups []string + labelling Labelling + profiles *secrets.ProfilesWatcher +} + +func NewSite(namespace string, eventProcessor *watchers.EventProcessor, certs certificates.CertificateManager, access SecuredAccessFactory, sizes *sizing.Registry, labelling Labelling, certificateController string) *Site { logger := slog.New(slog.Default().Handler()) site := &Site{ - bindings: NewExtendedBindings(eventProcessor, SSL_PROFILE_PATH), - namespace: namespace, - clients: eventProcessor, - links: map[string]*site.Link{}, - linkAccess: site.RouterAccessMap{}, - certs: certs, - access: access, - accessMapping: make(securedAccessMap), - sizes: sizes, - routerPods: map[string]*corev1.Pod{}, + bindings: NewExtendedBindings(eventProcessor, SSL_PROFILE_PATH), + namespace: namespace, + clients: eventProcessor, + links: map[string]*site.Link{}, + linkAccess: site.RouterAccessMap{}, + certs: certs, + certificateController: certificateController, + access: access, + accessMapping: make(securedAccessMap), + sizes: sizes, + routerPods: map[string]*corev1.Pod{}, logger: logger.With( slog.String("component", "kube.site.site"), ), @@ -202,13 +204,14 @@ func (s *Site) reconcile(siteDef *skupperv2alpha1.Site, inRecovery bool) error { } } // CAs for local and site access - if err := s.certs.EnsureCA(s.namespace, "skupper-site-ca", fmt.Sprintf("%s site CA", s.name), s.ownerReferences()); err != nil { + certificateController := s.GetCertificateController() + if err := s.certs.EnsureCA(s.namespace, "skupper-site-ca", fmt.Sprintf("%s site CA", s.name), certificateController, s.ownerReferences()); err != nil { return err } - if err := s.certs.EnsureCA(s.namespace, "skupper-local-ca", fmt.Sprintf("%s local CA", s.name), s.ownerReferences()); err != nil { + if err := s.certs.EnsureCA(s.namespace, "skupper-local-ca", fmt.Sprintf("%s local CA", s.name), certificateController, s.ownerReferences()); err != nil { return err } - if err := s.certs.Ensure(s.namespace, "skupper-local-server", "skupper-local-ca", "skupper-router-local", s.qualified("skupper-router-local"), false, true, s.ownerReferences()); err != nil { + if err := s.certs.Ensure(s.namespace, "skupper-local-server", "skupper-local-ca", "skupper-router-local", s.qualified("skupper-router-local"), false, true, certificateController, s.ownerReferences()); err != nil { return err } // RouterAccess for router @@ -312,6 +315,8 @@ func (s *Site) checkDefaultRouterAccess(ctxt context.Context, site *skupperv2alp }, }, } + certificateController := s.GetCertificateController() + desired.Spec.SetCertificateController(certificateController) current, ok := s.linkAccess[name] if ok { if reflect.DeepEqual(current.Spec, desired.Spec) { @@ -327,7 +332,18 @@ func (s *Site) checkDefaultRouterAccess(ctxt context.Context, site *skupperv2alp } else { created, err := s.clients.GetSkupperClient().SkupperV2alpha1().RouterAccesses(s.namespace).Create(context.Background(), desired, metav1.CreateOptions{}) if err != nil { - return err + if errors.IsAlreadyExists(err) { + s.logger.Info("Router access already exists - loading latest", slog.String("name", desired.Name)) + created, err = s.clients.GetSkupperClient().SkupperV2alpha1().RouterAccesses(s.namespace).Get(context.Background(), desired.Name, metav1.GetOptions{}) + if err != nil { + return err + } + } else { + s.logger.Error("Error creating RouterAccess", + slog.String("name", desired.Name), + slog.String("error", err.Error())) + return err + } } s.linkAccess[name] = created return nil @@ -1253,7 +1269,8 @@ func (s *Site) checkSecuredAccess() error { "internal.skupper.io/controlled": "true", "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 { + certificateController := s.GetCertificateController() + if err := s.access.Ensure(s.namespace, name, asSecuredAccessSpec(la, group, s.site.DefaultIssuer()), annotations, certificateController, routerAccessOwner(la)); err != nil { //TODO: add message to site status s.logger.Error("Error ensuring SecuredAccess for RouterAccess", slog.String("key", la.Key()), @@ -1302,7 +1319,7 @@ func (s *Site) CheckRouterAccess(name string, la *skupperv2alpha1.RouterAccess) "internal.skupper.io/controlled": "true", "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 { + if err := s.access.Ensure(s.namespace, name, asSecuredAccessSpec(la, group, s.site.DefaultIssuer()), annotations, s.GetCertificateController(), routerAccessOwner(la)); err != nil { s.logger.Error("Error ensuring SecuredAccess for RouterAccess", slog.String("key", la.Key()), slog.Any("error", err)) @@ -1380,6 +1397,17 @@ func (s *Site) TLSPriorValidRevisions() uint64 { return revisions } +func (s *Site) GetCertificateController() string { + var controller string + if s.site != nil { + controller = s.site.Spec.GetCertificateController() + } + if controller == "" { + controller = s.certificateController + } + return controller +} + func podState(pod *corev1.Pod) skupperv2alpha1.ConditionState { for _, c := range pod.Status.Conditions { if c.Status == corev1.ConditionFalse { diff --git a/internal/kube/site/site_test.go b/internal/kube/site/site_test.go index 35ec5e347..6de8dd22a 100644 --- a/internal/kube/site/site_test.go +++ b/internal/kube/site/site_test.go @@ -1131,7 +1131,7 @@ func newSiteMocks(namespace string, k8sObjects []runtime.Object, skupperObjects links: make(map[string]*site1.Link), errors: make(map[string]string), linkAccess: make(map[string]*skupperv2alpha1.RouterAccess), - certs: certificates.NewCertificateManager(controller), + certs: certificates.NewCertificateManager(controller, ""), access: securedaccess.NewSecuredAccessManager(client, nil, &securedaccess.Config{DefaultAccessType: "loadbalancer"}, nil), accessMapping: make(securedAccessMap), routerPods: make(map[string]*corev1.Pod), diff --git a/pkg/apis/skupper/v2alpha1/types.go b/pkg/apis/skupper/v2alpha1/types.go index f4aae4fe2..5522283e0 100644 --- a/pkg/apis/skupper/v2alpha1/types.go +++ b/pkg/apis/skupper/v2alpha1/types.go @@ -223,6 +223,30 @@ func (s *SiteSpec) GetRouterDataConnectionCount() string { return "" } +func (s *SiteSpec) GetCertificateController() string { + if s.Settings == nil { + return "" + } + if value, ok := s.Settings["certificate-controller"]; ok { + return value + } + return "" +} + +func (s *SiteSpec) SetCertificateController(controller string) { + if s.Settings == nil { + if controller == "" { + return + } + s.Settings = make(map[string]string) + } + if controller != "" { + s.Settings["certificate-controller"] = controller + } else { + delete(s.Settings, "certificate-controller") + } +} + func (s *Site) SetConfigured(err error) bool { if s.Status.SetCondition(CONDITION_TYPE_CONFIGURED, ErrorOrReadyCondition(err), s.ObjectMeta.Generation) { s.Status.setReady(s.requiredConditions(), s.ObjectMeta.Generation) @@ -775,6 +799,30 @@ type SecuredAccessSpec struct { Settings map[string]string `json:"settings,omitempty"` } +func (s *SecuredAccessSpec) GetCertificateController() string { + if s.Settings == nil { + return "" + } + if value, ok := s.Settings["certificate-controller"]; ok { + return value + } + return "" +} + +func (s *SecuredAccessSpec) SetCertificateController(controller string) { + if s.Settings == nil { + if controller == "" { + return + } + s.Settings = map[string]string{} + } + if controller != "" { + s.Settings["certificate-controller"] = controller + } else { + delete(s.Settings, "certificate-controller") + } +} + type SecuredAccessStatus struct { Status `json:",inline"` Endpoints []Endpoint `json:"endpoints,omitempty"` @@ -845,6 +893,34 @@ func (c *Certificate) SetReady(err error) bool { return c.Status.SetCondition(CONDITION_TYPE_READY, ErrorOrReadyCondition(err), c.ObjectMeta.Generation) } +func (c *CertificateSpec) HasCertificateController() bool { + return c.GetCertificateController() != "" +} + +func (c *CertificateSpec) GetCertificateController() string { + if c.Settings == nil { + return "" + } + if value, ok := c.Settings["certificate-controller"]; ok { + return value + } + return "" +} + +func (c *CertificateSpec) SetCertificateController(controller string) { + if c.Settings == nil { + if controller == "" { + return + } + c.Settings = make(map[string]string) + } + if controller != "" { + c.Settings["certificate-controller"] = controller + } else { + delete(c.Settings, "certificate-controller") + } +} + // +genclient // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object @@ -963,6 +1039,30 @@ type RouterAccessSpec struct { Settings map[string]string `json:"settings,omitempty"` } +func (r *RouterAccessSpec) GetCertificateController() string { + if r.Settings == nil { + return "" + } + if value, ok := r.Settings["certificate-controller"]; ok { + return value + } + return "" +} + +func (r *RouterAccessSpec) SetCertificateController(controller string) { + if r.Settings == nil { + if controller == "" { + return + } + r.Settings = make(map[string]string) + } + if controller != "" { + r.Settings["certificate-controller"] = controller + } else { + delete(r.Settings, "certificate-controller") + } +} + type RouterAccessStatus struct { Status `json:",inline"` Endpoints []Endpoint `json:"endpoints,omitempty"` From 16bb572305917a037ac0abbff059e8af05017562 Mon Sep 17 00:00:00 2001 From: Fernando Giorgetti Date: Fri, 24 Oct 2025 11:09:24 -0300 Subject: [PATCH 2/2] SecuredAccess now uses certificate-controller from provided spec --- internal/kube/securedaccess/access.go | 4 ++-- internal/kube/securedaccess/access_test.go | 10 +++++----- internal/kube/site/site.go | 7 +++---- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/internal/kube/securedaccess/access.go b/internal/kube/securedaccess/access.go index a48ddf86b..29114a316 100644 --- a/internal/kube/securedaccess/access.go +++ b/internal/kube/securedaccess/access.go @@ -96,7 +96,7 @@ func (m *SecuredAccessManager) IsValidAccessType(accessType string) bool { return ok } -func (m *SecuredAccessManager) Ensure(namespace string, name string, spec skupperv2alpha1.SecuredAccessSpec, annotations map[string]string, certificateController string, refs []metav1.OwnerReference) error { +func (m *SecuredAccessManager) Ensure(namespace string, name string, spec skupperv2alpha1.SecuredAccessSpec, annotations map[string]string, refs []metav1.OwnerReference) error { key := fmt.Sprintf("%s/%s", namespace, name) if current, ok := m.definitions[key]; ok { if current.ObjectMeta.Labels == nil { @@ -151,7 +151,7 @@ func (m *SecuredAccessManager) Ensure(namespace string, name string, spec skuppe }, Spec: spec, } - sa.Spec.SetCertificateController(certificateController) + sa.Spec.SetCertificateController(spec.GetCertificateController()) if m.context != nil { if sa.ObjectMeta.Annotations == nil { sa.ObjectMeta.Annotations = map[string]string{} diff --git a/internal/kube/securedaccess/access_test.go b/internal/kube/securedaccess/access_test.go index 061d5076c..ca3b83e88 100644 --- a/internal/kube/securedaccess/access_test.go +++ b/internal/kube/securedaccess/access_test.go @@ -1995,7 +1995,7 @@ func TestSecuredAccessGeneral(t *testing.T) { certs := newMockCertificateManager() m := NewSecuredAccessManager(client, certs, &tt.config, &FakeControllerContext{namespace: "test", labels: tt.labels, annotations: tt.annotations}) - err = m.Ensure(tt.definition.Namespace, tt.definition.Name, tt.definition.Spec, nil, tt.definition.Spec.GetCertificateController(), nil) + err = m.Ensure(tt.definition.Namespace, tt.definition.Name, tt.definition.Spec, nil, nil) if tt.expectedError != "" { assert.ErrorContains(t, err, tt.expectedError) } else if err != nil { @@ -2557,7 +2557,7 @@ func TestSecuredAccessManagerEnsure(t *testing.T) { } for i, args := range tt.args { - err = m.Ensure(args.namespace, args.name, args.spec, args.annotations, "", args.refs) + err = m.Ensure(args.namespace, args.name, args.spec, args.annotations, args.refs) if len(tt.expectedErrors) > i && tt.expectedErrors[i] != "" { assert.ErrorContains(t, err, tt.expectedErrors[i]) } else if err != nil { @@ -2654,7 +2654,7 @@ func TestSecuredAccessDeleted(t *testing.T) { err := client.GetSkupperClient().SkupperV2alpha1().SecuredAccesses(def.namespace).Delete(context.Background(), def.name, metav1.DeleteOptions{}) assert.Assert(t, err) controller.TestProcess() - err = m.Ensure(def.namespace, def.name, def.spec, nil, "", nil) + err = m.Ensure(def.namespace, def.name, def.spec, nil, nil) assert.Assert(t, err) controller.TestProcess() } @@ -2814,7 +2814,7 @@ func TestSecuredAccessManagerChangeDelete(t *testing.T) { m, err := newSecureAccessManagerMocks("test", tt.k8sObjects, tt.skupperObjects, tt.skupperErrorMessage) assert.Assert(t, err) - if err := m.Ensure(tt.args.namespace, tt.args.name, *tt.args.spec, *tt.args.annotations, "", *tt.args.refs); err != nil { + if err := m.Ensure(tt.args.namespace, tt.args.name, *tt.args.spec, *tt.args.annotations, *tt.args.refs); err != nil { t.Errorf("SecuredAccessManager.Ensure() error = %v", err) } // change Access Type @@ -3032,7 +3032,7 @@ func TestSecuredAccessManagerCheckService(t *testing.T) { m, err := newSecureAccessManagerMocks("test", tt.k8sObjects, tt.skupperObjects, tt.skupperErrorMessage) assert.Assert(t, err) - if err := m.Ensure(tt.args.namespace, tt.args.name, *tt.args.spec, *tt.args.annotations, "", *tt.args.refs); err != nil { + if err := m.Ensure(tt.args.namespace, tt.args.name, *tt.args.spec, *tt.args.annotations, *tt.args.refs); err != nil { t.Errorf("SecuredAccessManager.CheckServic() error = %v", err) } diff --git a/internal/kube/site/site.go b/internal/kube/site/site.go index 3cc421c48..93b4c8da6 100644 --- a/internal/kube/site/site.go +++ b/internal/kube/site/site.go @@ -31,7 +31,7 @@ import ( ) type SecuredAccessFactory interface { - Ensure(namespace string, name string, spec skupperv2alpha1.SecuredAccessSpec, annotations map[string]string, certificateController string, refs []metav1.OwnerReference) error + Ensure(namespace string, name string, spec skupperv2alpha1.SecuredAccessSpec, annotations map[string]string, refs []metav1.OwnerReference) error Delete(namespace string, name string) error IsValidAccessType(accessType string) bool } @@ -1269,8 +1269,7 @@ func (s *Site) checkSecuredAccess() error { "internal.skupper.io/controlled": "true", "internal.skupper.io/routeraccess": la.Name, } - certificateController := s.GetCertificateController() - if err := s.access.Ensure(s.namespace, name, asSecuredAccessSpec(la, group, s.site.DefaultIssuer()), annotations, certificateController, routerAccessOwner(la)); err != nil { + 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()), @@ -1319,7 +1318,7 @@ func (s *Site) CheckRouterAccess(name string, la *skupperv2alpha1.RouterAccess) "internal.skupper.io/controlled": "true", "internal.skupper.io/routeraccess": la.Name, } - if err := s.access.Ensure(s.namespace, name, asSecuredAccessSpec(la, group, s.site.DefaultIssuer()), annotations, s.GetCertificateController(), routerAccessOwner(la)); err != nil { + if err := s.access.Ensure(s.namespace, name, asSecuredAccessSpec(la, group, s.site.DefaultIssuer()), annotations, routerAccessOwner(la)); err != nil { s.logger.Error("Error ensuring SecuredAccess for RouterAccess", slog.String("key", la.Key()), slog.Any("error", err))