fix: add full push when higress-https configmap updated and fix certmagic storage (#1105)

pkg/bootstrap/server.go

@@ -391,7 +391,7 @@ func (s *Server) initAutomaticHttps() error {
 		ServerAddress: s.CertHttpAddress,
 		Email:         s.AutomaticHttpsEmail,
 	}
-	certServer, err := cert.NewServer(s.kubeClient.Kube(), certOption)
+	certServer, err := cert.NewServer(s.kubeClient.Kube(), s.xdsServer, certOption)
 	if err != nil {
 		return err
 	}

pkg/cert/certmgr.go

@@ -17,10 +17,15 @@ package cert
 import (
 	"context"
 	"fmt"
+	"os"
+	"reflect"
 	"sync"
 
 	"github.com/caddyserver/certmagic"
 	"github.com/mholt/acmez"
+	"go.uber.org/zap"
+	"go.uber.org/zap/zapcore"
+	"istio.io/istio/pilot/pkg/model"
 	"k8s.io/client-go/kubernetes"
 )
 
@@ -28,6 +33,10 @@ const (
 	EventCertObtained = "cert_obtained"
 )
 
+var (
+	cfg *certmagic.Config
+)
+
 type CertMgr struct {
 	cfg           *certmagic.Config
 	client        kubernetes.Interface
@@ -39,9 +48,10 @@ type CertMgr struct {
 	ingressSolver acmez.Solver
 	configMgr     *ConfigMgr
 	secretMgr     *SecretMgr
+	XDSUpdater    model.XDSUpdater
 }
 
-func InitCertMgr(opts *Option, clientSet kubernetes.Interface, config *Config) (*CertMgr, error) {
+func InitCertMgr(opts *Option, clientSet kubernetes.Interface, config *Config, XDSUpdater model.XDSUpdater, configMgr *ConfigMgr) (*CertMgr, error) {
 	CertLog.Infof("certmgr init config: %+v", config)
 	// Init certmagic config
 	// First make a pointer to a Cache as we need to reference the same Cache in
@@ -49,21 +59,29 @@ func InitCertMgr(opts *Option, clientSet kubernetes.Interface, config *Config) (
 	var cache *certmagic.Cache
 	var storage certmagic.Storage
 	storage, _ = NewConfigmapStorage(opts.Namespace, clientSet)
-	renewalWindowRatio := float64(config.RenewBeforeDays / RenewMaxDays)
+	renewalWindowRatio := float64(config.RenewBeforeDays) / float64(RenewMaxDays)
+	logger := zap.New(zapcore.NewCore(
+		zapcore.NewConsoleEncoder(zap.NewProductionEncoderConfig()),
+		os.Stderr,
+		zap.DebugLevel,
+	))
 	magicConfig := certmagic.Config{
 		RenewalWindowRatio: renewalWindowRatio,
 		Storage:            storage,
+		Logger:             logger,
 	}
 	cache = certmagic.NewCache(certmagic.CacheOptions{
 		GetConfigForCert: func(cert certmagic.Certificate) (*certmagic.Config, error) {
 			// Here we use New to get a valid Config associated with the same cache.
 			// The provided Config is used as a template and will be completed with
 			// any defaults that are set in the Default config.
-			return certmagic.New(cache, magicConfig), nil
+			return cfg, nil
 		},
+		Logger: logger,
 	})
 	// init certmagic
-	cfg := certmagic.New(cache, magicConfig)
+	cfg = certmagic.New(cache, magicConfig)
+
 	// Init certmagic acme
 	issuer := config.GetIssuer(IssuerTypeLetsencrypt)
 	if issuer == nil {
@@ -85,7 +103,6 @@ func InitCertMgr(opts *Option, clientSet kubernetes.Interface, config *Config) (
 	// init issuers
 	cfg.Issuers = []certmagic.Issuer{myACME}
 
-	configMgr, _ := NewConfigMgr(opts.Namespace, clientSet)
 	secretMgr, _ := NewSecretMgr(opts.Namespace, clientSet)
 
 	certMgr := &CertMgr{
@@ -97,6 +114,7 @@ func InitCertMgr(opts *Option, clientSet kubernetes.Interface, config *Config) (
 		configMgr:     configMgr,
 		secretMgr:     secretMgr,
 		cache:         cache,
+		XDSUpdater:    XDSUpdater,
 	}
 	certMgr.cfg.OnEvent = certMgr.OnEvent
 	return certMgr, nil
@@ -149,18 +167,31 @@ func (s *CertMgr) Reconcile(ctx context.Context, oldConfig *Config, newConfig *C
 		// sync email
 		s.myACME.Email = newIssuer.Email
 		// sync RenewalWindowRatio
-		s.cfg.RenewalWindowRatio = float64(newConfig.RenewBeforeDays / RenewMaxDays)
+		renewalWindowRatio := float64(newConfig.RenewBeforeDays) / float64(RenewMaxDays)
+		s.cfg.RenewalWindowRatio = renewalWindowRatio
 		// start cache
 		s.cache.Start()
 		// sync domains
-		s.manageSync(context.Background(), newDomains)
 		s.configMgr.SetConfig(newConfig)
+		CertLog.Infof("certMgr start to manageSync domains:+v%", newDomains)
+		s.manageSync(context.Background(), newDomains)
+		CertLog.Infof("certMgr manageSync domains done")
 	} else {
 		// stop cache  maintainAssets
 		s.cache.Stop()
 		s.configMgr.SetConfig(newConfig)
 	}
 
+	if oldConfig != nil && newConfig != nil {
+		if oldConfig.FallbackForInvalidSecret != newConfig.FallbackForInvalidSecret || !reflect.DeepEqual(oldConfig.CredentialConfig, newConfig.CredentialConfig) {
+			CertLog.Infof("ingress need to full push")
+			s.XDSUpdater.ConfigUpdate(&model.PushRequest{
+				Full:   true,
+				Reason: []model.TriggerReason{"higress-https-updated"},
+			})
+		}
+	}
+
 	return nil
 }
 

pkg/cert/config.go

@@ -99,20 +99,22 @@ func ParseTLSSecret(tlsSecret string) (string, string) {
 
 func (c *Config) Validate() error {
 	// check acmeIssuer
-	if len(c.ACMEIssuer) == 0 {
-		return fmt.Errorf("acmeIssuer is empty")
-	}
-	for _, issuer := range c.ACMEIssuer {
-		switch issuer.Name {
-		case IssuerTypeLetsencrypt:
-			if issuer.Email == "" {
-				return fmt.Errorf("acmeIssuer %s email is empty", issuer.Name)
+	if c.AutomaticHttps {
+		if len(c.ACMEIssuer) == 0 {
+			return fmt.Errorf("no acmeIssuer configuration found when automaticHttps is enable")
+		}
+		for _, issuer := range c.ACMEIssuer {
+			switch issuer.Name {
+			case IssuerTypeLetsencrypt:
+				if issuer.Email == "" {
+					return fmt.Errorf("acmeIssuer %s email is empty", issuer.Name)
+				}
+				if !ValidateEmail(issuer.Email) {
+					return fmt.Errorf("acmeIssuer %s email %s is invalid", issuer.Name, issuer.Email)
+				}
+			default:
+				return fmt.Errorf("acmeIssuer name %s is not supported", issuer.Name)
 			}
-			if !ValidateEmail(issuer.Email) {
-				return fmt.Errorf("acmeIssuer %s email %s is invalid", issuer.Name, issuer.Email)
-			}
-		default:
-			return fmt.Errorf("acmeIssuer name %s is not supported", issuer.Name)
 		}
 	}
 	// check credentialConfig

pkg/cert/secret.go

@@ -41,6 +41,7 @@ func NewSecretMgr(namespace string, client kubernetes.Interface) (*SecretMgr, er
 }
 
 func (s *SecretMgr) Update(domain string, secretName string, privateKey []byte, certificate []byte, notBefore time.Time, notAfter time.Time, isRenew bool) error {
+	CertLog.Infof("update secret, domain:%s, secretName:%s, notBefore:%v, notAfter:%v, isRenew:%t", domain, secretName, notBefore, notAfter, isRenew)
 	name := secretName
 	namespace := s.namespace
 	namespaceP, secretP := ParseTLSSecret(secretName)
@@ -77,6 +78,7 @@ func (s *SecretMgr) constructSecret(domain string, name string, namespace string
 	annotationMap["higress.io/cert-notAfter"] = notAfter.Format("2006-01-02 15:04:05")
 	annotationMap["higress.io/cert-notBefore"] = notBefore.Format("2006-01-02 15:04:05")
 	annotationMap["higress.io/cert-renew"] = strconv.FormatBool(isRenew)
+	annotationMap["higress.io/cert-source"] = string(IssuerTypeLetsencrypt)
 	if isRenew {
 		annotationMap["higress.io/cert-renew-time"] = time.Now().Format("2006-01-02 15:04:05")
 	}

pkg/cert/server.go

@@ -22,6 +22,7 @@ import (
 	"time"
 
 	"github.com/caddyserver/certmagic"
+	"istio.io/istio/pilot/pkg/model"
 	"k8s.io/client-go/kubernetes"
 )
 
@@ -37,12 +38,14 @@ type Server struct {
 	clientSet  kubernetes.Interface
 	controller *Controller
 	certMgr    *CertMgr
+	XDSUpdater model.XDSUpdater
 }
 
-func NewServer(clientSet kubernetes.Interface, opts *Option) (*Server, error) {
+func NewServer(clientSet kubernetes.Interface, XDSUpdater model.XDSUpdater, opts *Option) (*Server, error) {
 	server := &Server{
-		clientSet: clientSet,
-		opts:      opts,
+		clientSet:  clientSet,
+		opts:       opts,
+		XDSUpdater: XDSUpdater,
 	}
 	return server, nil
 }
@@ -65,7 +68,7 @@ func (s *Server) InitServer() error {
 		return err
 	}
 	// init certmgr
-	certMgr, err := InitCertMgr(s.opts, s.clientSet, defaultConfig) // config and start
+	certMgr, err := InitCertMgr(s.opts, s.clientSet, defaultConfig, s.XDSUpdater, configMgr) // config and start
 	s.certMgr = certMgr
 	// init controller
 	controller, err := NewController(s.clientSet, s.opts.Namespace, certMgr, configMgr)

pkg/cert/storage.go

@@ -32,7 +32,7 @@ import (
 )
 
 const (
-	CertificatesPrefix              = "/certificates"
+	CertificatesPrefix              = "certificates"
 	ConfigmapStoreCertficatesPrefix = "higress-cert-store-certificates-"
 	ConfigmapStoreDefaultName       = "higress-cert-store-default"
 )
@@ -155,7 +155,7 @@ func (s *ConfigmapStorage) List(ctx context.Context, prefix string, recursive bo
 	// Check if the prefix corresponds to a specific key
 	hashPrefix := fastHash([]byte(prefix))
 	if strings.HasPrefix(prefix, CertificatesPrefix) {
-		// If the prefix is "/certificates", get all ConfigMaps and traverse each one
+		// If the prefix is "certificates/", get all ConfigMaps and traverse each one
 		// List all ConfigMaps in the namespace with label higress.io/cert-https=true
 		configmaps, err := s.client.CoreV1().ConfigMaps(s.namespace).List(ctx, metav1.ListOptions{FieldSelector: "metadata.annotations['higress.io/cert-https'] == 'true'"})
 		if err != nil {
@@ -289,14 +289,29 @@ func (s *ConfigmapStorage) String() string {
 	return "ConfigmapStorage"
 }
 
+// getConfigmapStoreNameByKey determines the storage name for a given key.
+// It checks if the key starts with 'certificates/' and if so, the key pattern should match one of the following:
+// 'certificates/<issuerKey>/<domain>/<domain>.json',
+// 'certificates/<issuerKey>/<domain>/<domain>.crt',
+// or 'certificates/<issuerKey>/<domain>/<domain>.key'.
+// It then returns the corresponding ConfigMap name.
+// If the key does not start with 'certificates/', it returns the default store name.
+//
+// Parameters:
+//
+// key - The configuration map key that needs to be mapped to a storage name.
+//
+// Returns:
+//
+// string - The calculated or default storage name based on the key.
 func (s *ConfigmapStorage) getConfigmapStoreNameByKey(key string) string {
-	parts := strings.SplitN(key, "/", 10)
-	if len(parts) >= 4 && parts[1] == "certificates" {
-		domain := strings.TrimSuffix(parts[3], ".crt")
-		domain = strings.TrimSuffix(domain, ".key")
-		domain = strings.TrimSuffix(domain, ".json")
-		issuerKey := parts[2]
-		return ConfigmapStoreCertficatesPrefix + fastHash([]byte(issuerKey+domain))
+	if strings.HasPrefix(key, "certificates/") {
+		parts := strings.Split(key, "/")
+		if len(parts) >= 4 && parts[0] == "certificates" {
+			domain := parts[2]
+			issuerKey := parts[1]
+			return ConfigmapStoreCertficatesPrefix + fastHash([]byte(issuerKey+domain))
+		}
 	}
 	return ConfigmapStoreDefaultName
 }

pkg/cert/storage_test.go

@@ -39,22 +39,29 @@ func TestGetConfigmapStoreNameByKey(t *testing.T) {
 	}{
 		{
 			name:     "certificate crt",
-			key:      "/certificates/issuerKey/domain.crt",
+			key:      "certificates/issuerKey/domain/domain.crt",
 			expected: "higress-cert-store-certificates-" + fastHash([]byte("issuerKey"+"domain")),
 		},
+
+		{
+			name:     "47.237.14.136.sslip.io crt",
+			key:      "certificates/acme-v02.api.letsencrypt.org-directory/47.237.14.136.sslip.io/47.237.14.136.sslip.io.crt",
+			expected: "higress-cert-store-certificates-" + fastHash([]byte("acme-v02.api.letsencrypt.org-directory"+"47.237.14.136.sslip.io")),
+		},
+
 		{
 			name:     "certificate meta",
-			key:      "/certificates/issuerKey/domain.json",
+			key:      "certificates/issuerKey/domain/domain.json",
 			expected: "higress-cert-store-certificates-" + fastHash([]byte("issuerKey"+"domain")),
 		},
 		{
 			name:     "certificate key",
-			key:      "/certificates/issuerKey/domain.key",
+			key:      "certificates/issuerKey/domain/domain.key",
 			expected: "higress-cert-store-certificates-" + fastHash([]byte("issuerKey"+"domain")),
 		},
 		{
 			name:     "user key",
-			key:      "/users/hello/2",
+			key:      "users/hello/2",
 			expected: "higress-cert-store-default",
 		},
 		{
@@ -82,7 +89,7 @@ func TestExists(t *testing.T) {
 	assert.NoError(t, err)
 
 	// Store a test key
-	testKey := "/certificates/issuer1/domain1.crt"
+	testKey := "certificates/issuer1/domain1/domain1.crt"
 	err = storage.Store(context.Background(), testKey, []byte("test-data"))
 	assert.NoError(t, err)
 
@@ -94,17 +101,17 @@ func TestExists(t *testing.T) {
 	}{
 		{
 			name:        "Existing Key",
-			key:         "/certificates/issuer1/domain1.crt",
+			key:         "certificates/issuer1/domain1/domain1.crt",
 			shouldExist: true,
 		},
 		{
 			name:        "Non-Existent Key1",
-			key:         "/certificates/issuer2/domain2.crt",
+			key:         "certificates/issuer2/domain2/domain2.crt",
 			shouldExist: false,
 		},
 		{
 			name:        "Non-Existent Key2",
-			key:         "/users/hello/a",
+			key:         "users/hello/a",
 			shouldExist: false,
 		},
 		// Add more test cases as needed
@@ -129,7 +136,7 @@ func TestLoad(t *testing.T) {
 	assert.NoError(t, err)
 
 	// Store a test key
-	testKey := "/certificates/issuer1/domain1.crt"
+	testKey := "certificates/issuer1/domain1/domain1.crt"
 	testValue := []byte("test-data")
 	err = storage.Store(context.Background(), testKey, testValue)
 	assert.NoError(t, err)
@@ -143,13 +150,13 @@ func TestLoad(t *testing.T) {
 	}{
 		{
 			name:        "Existing Key",
-			key:         "/certificates/issuer1/domain1.crt",
+			key:         "certificates/issuer1/domain1/domain1.crt",
 			expected:    testValue,
 			shouldError: false,
 		},
 		{
 			name:        "Non-Existent Key",
-			key:         "/certificates/issuer2/domain2.crt",
+			key:         "certificates/issuer2/domain2/domain2.crt",
 			expected:    nil,
 			shouldError: true,
 		},
@@ -192,28 +199,28 @@ func TestStore(t *testing.T) {
 		shouldError           bool
 	}{
 		{
-			name:                  "Store Key with /certificates prefix",
-			key:                   "/certificates/issuer1/domain1.crt",
+			name:                  "Store Key with certificates prefix",
+			key:                   "certificates/issuer1/domain1/domain1.crt",
 			value:                 []byte("test-data1"),
-			expected:              map[string]string{fastHash([]byte("/certificates/issuer1/domain1.crt")): `{"k":"/certificates/issuer1/domain1.crt","v":"dGVzdC1kYXRhMQ=="}`},
+			expected:              map[string]string{fastHash([]byte("certificates/issuer1/domain1/domain1.crt")): `{"k":"certificates/issuer1/domain1/domain1.crt","v":"dGVzdC1kYXRhMQ=="}`},
 			expectedConfigmapName: "higress-cert-store-certificates-" + fastHash([]byte("issuer1"+"domain1")),
 			shouldError:           false,
 		},
 		{
-			name:  "Store Key with /certificates prefix (additional data)",
-			key:   "/certificates/issuer2/domain2.crt",
+			name:  "Store Key with certificates prefix (additional data)",
+			key:   "certificates/issuer2/domain2/domain2.crt",
 			value: []byte("test-data2"),
 			expected: map[string]string{
-				fastHash([]byte("/certificates/issuer2/domain2.crt")): `{"k":"/certificates/issuer2/domain2.crt","v":"dGVzdC1kYXRhMg=="}`,
+				fastHash([]byte("certificates/issuer2/domain2/domain2.crt")): `{"k":"certificates/issuer2/domain2/domain2.crt","v":"dGVzdC1kYXRhMg=="}`,
 			},
 			expectedConfigmapName: "higress-cert-store-certificates-" + fastHash([]byte("issuer2"+"domain2")),
 			shouldError:           false,
 		},
 		{
-			name:                  "Store Key without /certificates prefix",
-			key:                   "/other/path/data.txt",
+			name:                  "Store Key without certificates prefix",
+			key:                   "other/path/data.txt",
 			value:                 []byte("test-data3"),
-			expected:              map[string]string{fastHash([]byte("/other/path/data.txt")): `{"k":"/other/path/data.txt","v":"dGVzdC1kYXRhMw=="}`},
+			expected:              map[string]string{fastHash([]byte("other/path/data.txt")): `{"k":"other/path/data.txt","v":"dGVzdC1kYXRhMw=="}`},
 			expectedConfigmapName: "higress-cert-store-default",
 			shouldError:           false,
 		},
@@ -256,17 +263,17 @@ func TestList(t *testing.T) {
 	// Store some test data
 	// Store some test data
 	testKeys := []string{
-		"/certificates/issuer1/domain1.crt",
-		"/certificates/issuer1/domain2.crt",
-		"/certificates/issuer1/domain3.crt", // Added another domain for issuer1
-		"/certificates/issuer2/domain4.crt",
-		"/certificates/issuer2/domain5.crt",
-		"/certificates/issuer3/subdomain1/domain6.crt",            // Two-level subdirectory under issuer3
-		"/certificates/issuer3/subdomain1/subdomain2/domain7.crt", // Two more levels under issuer3
-		"/other-prefix/key1/file1",
-		"/other-prefix/key1/file2",
-		"/other-prefix/key2/file3",
-		"/other-prefix/key2/file4",
+		"certificates/issuer1/domain1/domain1.crt",
+		"certificates/issuer1/domain2/domain2.crt",
+		"certificates/issuer1/domain3/domain3.crt", // Added another domain for issuer1
+		"certificates/issuer2/domain4/domain4.crt",
+		"certificates/issuer2/domain5/domain5.crt",
+		"certificates/issuer3/domain6/domain6.crt",               // Two-level subdirectory under issuer3
+		"certificates/issuer3/subdomain1/subdomain2/domain7.crt", // Two more levels under issuer3
+		"other-prefix/key1/file1",
+		"other-prefix/key1/file2",
+		"other-prefix/key2/file3",
+		"other-prefix/key2/file4",
 	}
 
 	for _, key := range testKeys {
@@ -283,34 +290,34 @@ func TestList(t *testing.T) {
 	}{
 		{
 			name:      "List Certificates (Non-Recursive)",
-			prefix:    "/certificates",
+			prefix:    "certificates",
 			recursive: false,
-			expected:  []string{"/certificates/issuer1", "/certificates/issuer2", "/certificates/issuer3"},
+			expected:  []string{"certificates/issuer1", "certificates/issuer2", "certificates/issuer3"},
 		},
 		{
 			name:      "List Certificates (Recursive)",
-			prefix:    "/certificates",
+			prefix:    "certificates",
 			recursive: true,
-			expected:  []string{"/certificates/issuer1/domain1.crt", "/certificates/issuer1/domain2.crt", "/certificates/issuer1/domain3.crt", "/certificates/issuer2/domain4.crt", "/certificates/issuer2/domain5.crt", "/certificates/issuer3/subdomain1/domain6.crt", "/certificates/issuer3/subdomain1/subdomain2/domain7.crt"},
+			expected:  []string{"certificates/issuer1/domain1/domain1.crt", "certificates/issuer1/domain2/domain2.crt", "certificates/issuer1/domain3/domain3.crt", "certificates/issuer2/domain4/domain4.crt", "certificates/issuer2/domain5/domain5.crt", "certificates/issuer3/domain6/domain6.crt", "certificates/issuer3/subdomain1/subdomain2/domain7.crt"},
 		},
 		{
 			name:      "List Other Prefix (Non-Recursive)",
-			prefix:    "/other-prefix",
+			prefix:    "other-prefix",
 			recursive: false,
-			expected:  []string{"/other-prefix/key1", "/other-prefix/key2"},
+			expected:  []string{"other-prefix/key1", "other-prefix/key2"},
 		},
 
 		{
 			name:      "List Other Prefix (Non-Recursive)",
-			prefix:    "/other-prefix/key1",
+			prefix:    "other-prefix/key1",
 			recursive: false,
-			expected:  []string{"/other-prefix/key1/file1", "/other-prefix/key1/file2"},
+			expected:  []string{"other-prefix/key1/file1", "other-prefix/key1/file2"},
 		},
 		{
 			name:      "List Other Prefix (Recursive)",
-			prefix:    "/other-prefix",
+			prefix:    "other-prefix",
 			recursive: true,
-			expected:  []string{"/other-prefix/key1/file1", "/other-prefix/key1/file2", "/other-prefix/key2/file3", "/other-prefix/key2/file4"},
+			expected:  []string{"other-prefix/key1/file1", "other-prefix/key1/file2", "other-prefix/key2/file3", "other-prefix/key2/file4"},
 		},
 	}