feat: implement lets encrypt cert issuer #294
Conversation
|
""" WalkthroughThis update introduces a new certificate issuer subsystem leveraging the lego ACME client, enabling automated issuance and management of public TLS certificates (e.g., Let's Encrypt) for the provider gateway. It adds configuration structures, storage mechanisms, DNS challenge support, and command-line flags to control certificate issuance. Existing TLS handling is improved for robustness and flexibility. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CobraCmd
participant CertIssuerConfig
participant CertIssuer
participant ACME_CA
participant DNSProvider
User->>CobraCmd: Run provider service with cert-issuer flags
CobraCmd->>CertIssuerConfig: Parse and validate config
CobraCmd->>CertIssuer: Initialize CertIssuer (lego)
CertIssuer->>DNSProvider: Setup DNS challenge provider(s)
CertIssuer->>ACME_CA: Register account (with optional external account binding)
loop Certificate issuance/renewal
CertIssuer->>DNSProvider: Present DNS-01 challenge
CertIssuer->>ACME_CA: Request certificate
ACME_CA-->>CertIssuer: Issue certificate
CertIssuer->>CertIssuer: Save certificate to storage
end
CertIssuer-->>CobraCmd: Ready with issued certificate
Assessment against linked issues
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (6)
tools/certissuer/lego.go (1)
129-129: Use wrapped static errors instead of dynamic errors.Replace the dynamic error with a wrapped static error for better error handling practices.
Define a static error and wrap it:
+var ErrUnsupportedDNSProvider = errors.New("unsupported DNS provider") + func initProviders(providers []string) ([]challenge.Provider, error) { // ... existing code ... default: - return res, fmt.Errorf("lego: unsupported dns provider %s", provider) + return res, fmt.Errorf("%w: %s", ErrUnsupportedDNSProvider, provider) }🧰 Tools
🪛 golangci-lint (1.64.8)
129-129: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("lego: unsupported dns provider %s", provider)"
(err113)
cmd/provider-services/cmd/flags.go (2)
301-310: Add descriptions for KID and HMAC flags.The
cert-issuer-kidandcert-issuer-hmacflags lack descriptions, making their purpose unclear to users.Add descriptions:
- cmd.Flags().String(FlagCertIssuerKID, "", "") + cmd.Flags().String(FlagCertIssuerKID, "", "Key ID for external account binding (required by some ACME providers)") if err := viper.BindPFlag(FlagCertIssuerKID, cmd.Flags().Lookup(FlagCertIssuerKID)); err != nil { return err } - cmd.Flags().String(FlagCertIssuerHMAC, "", "") + cmd.Flags().String(FlagCertIssuerHMAC, "", "HMAC key for external account binding (required by some ACME providers)")
291-294: Consider setting default storage directory.The flag description mentions a default of
$AP_HOME/.certissuerbut the default value is empty. Consider setting this default programmatically.You could set a default based on the home directory:
- cmd.Flags().String(FlagCertIssuerStorageDir, "", "path to store acme information. defaults to $AP_HOME/.certissuer") + defaultCertIssuerDir := filepath.Join(cfg.Home, ".certissuer") + cmd.Flags().String(FlagCertIssuerStorageDir, defaultCertIssuerDir, "path to store acme information")cmd/provider-services/cmd/certs.go (1)
383-390: Consider enhancing error handling for certificate reloading.While the logging is helpful, consider propagating the error when certificate loading fails to allow for proper error handling upstream.
} else if c.cOpts.tlsCertFile != "" && evt.Name == c.cOpts.tlsCertFile { c.log.Info(fmt.Sprintf("detected certificate change, reloading")) cert, err := tls.LoadX509KeyPair(c.cOpts.tlsCertFile, c.cOpts.tlsKeyFile) if err != nil { c.log.Error("unable to load tls certificate", "err", err) + // Consider sending error to a channel or triggering a retry mechanism } else { c.caCerts = []tls.Certificate{cert} + c.log.Info("successfully reloaded tls certificate") } }tools/certissuer/storage_test.go (2)
14-52: Consider extracting test domain as a constant.The test implementation is thorough and covers the happy path well. However, the domain string "example.com" is repeated across tests.
Add a constant at the package level:
+const testDomain = "example.com" + func TestCertificatesStorage_MoveToArchive(t *testing.T) { - domain := "example.com" + domain := testDomain🧰 Tools
🪛 golangci-lint (1.64.8)
15-15: string
example.comhas 3 occurrences, make it a constant(goconst)
136-150: Consider pre-allocating the filenames slice.Since the number of extensions is known, pre-allocating the slice would be a minor optimization.
func generateTestFiles(t *testing.T, dir, domain string) []string { t.Helper() - var filenames []string + extensions := []string{extIssuer, extCert, extKey, extPem, extPfx, extResource} + filenames := make([]string, 0, len(extensions)) - for _, ext := range []string{extIssuer, extCert, extKey, extPem, extPfx, extResource} { + for _, ext := range extensions {🧰 Tools
🪛 golangci-lint (1.64.8)
[medium] 143-143: G306: Expect WriteFile permissions to be 0600 or less
(gosec)
139-139: Consider pre-allocating
filenames(prealloc)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cmd/provider-services/cmd/certs.go(5 hunks)cmd/provider-services/cmd/flags.go(1 hunks)cmd/provider-services/cmd/run.go(4 hunks)go.mod(10 hunks)tools/certissuer/account.go(1 hunks)tools/certissuer/config.go(1 hunks)tools/certissuer/errors.go(1 hunks)tools/certissuer/lego.go(1 hunks)tools/certissuer/storage.go(1 hunks)tools/certissuer/storage_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
tools/certissuer/config.go (1)
tools/certissuer/errors.go (1)
ErrInvalidConfig(8-8)
tools/certissuer/lego.go (2)
tools/certissuer/config.go (1)
Config(7-14)tools/certissuer/storage.go (2)
NewStorage(108-130)StorageConfig(44-48)
cmd/provider-services/cmd/run.go (2)
tools/certissuer/config.go (1)
Config(7-14)config.go (1)
Config(16-30)
tools/certissuer/storage.go (1)
tools/certissuer/account.go (1)
Account(10-14)
🪛 golangci-lint (1.64.8)
tools/certissuer/lego.go
[warning] 24-24: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _
(revive)
20-20: field ctx is unused
(unused)
21-21: field cl is unused
(unused)
96-96: func (*certIssuer).run is unused
(unused)
129-129: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("lego: unsupported dns provider %s", provider)"
(err113)
tools/certissuer/storage_test.go
15-15: string example.com has 3 occurrences, make it a constant
(goconst)
[medium] 143-143: G306: Expect WriteFile permissions to be 0600 or less
(gosec)
139-139: Consider pre-allocating filenames
(prealloc)
tools/certissuer/storage.go
284-284: do not define dynamic errors, use wrapped static errors instead: "errors.New("unknown private key type")"
(err113)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: integration-tests / crd-e2e
- GitHub Check: lint
- GitHub Check: release-dry-run
- GitHub Check: build-bins
🔇 Additional comments (12)
go.mod (3)
14-14: LGTM! Dependencies support the certificate issuer feature.The addition of
go-acme/lego/v4for ACME client functionality andhashicorp/go-retryablehttpfor resilient HTTP operations are appropriate for implementing Let's Encrypt certificate issuance.Also applies to: 25-25
85-87: Indirect dependency changes support new functionality.The addition of OpenTelemetry, Google Cloud libraries, and other indirect dependencies aligns with the certificate issuer functionality requirements. Version adjustments like
golang.org/x/toolsdowngrade appear to be compatibility-driven.Also applies to: 268-272, 283-284
45-45:✅ Verification successful
Verify gRPC version compatibility and security implications.
The gRPC version is upgraded to v1.71.0 but immediately replaced with v1.63.2 due to compatibility issues. This creates a significant version gap that may have security or functionality implications.
Also applies to: 80-82
🏁 Script executed:
#!/bin/bash # Description: Check for security advisories between gRPC v1.63.2 and v1.71.0 echo "Checking gRPC security advisories..." gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: GO, package: "google.golang.org/grpc") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 1985
No security vulnerabilities in gRPC v1.63.2 or v1.71.0
A GitHub security advisory scan shows that none of the known vulnerabilities affect either the replaced v1.63.2 or the upgraded v1.71.0 release. The replace directive does not introduce new security risks; it merely pins the version for compatibility reasons. If you resolve the compatibility issues with newer gRPC, you can safely upgrade to v1.71.0.
tools/certissuer/errors.go (1)
1-9: LGTM! Clean error definition following Go conventions.The sentinel error
ErrInvalidConfigis properly defined usingerrors.Newwith a clear, descriptive message. This follows Go best practices for package-level error definitions.tools/certissuer/config.go (1)
7-14: LGTM! Config struct covers essential certificate issuer parameters.The struct appropriately captures all necessary configuration for ACME certificate issuance including authentication (KID, HMAC), storage (StorageDir), CA endpoint (CADirURL), DNS providers, and user email.
tools/certissuer/account.go (2)
9-14: LGTM! Good encapsulation design for sensitive data.The Account struct properly encapsulates the private key field while exposing necessary public fields with JSON tags. This follows security best practices by preventing accidental serialization of the private key.
16-31: LGTM! Clean interface implementation with compile-time verification.The Account correctly implements the
registration.Userinterface with:
- Compile-time interface compliance check
- Simple, focused getter methods
- Proper encapsulation of the private key through the GetPrivateKey method
cmd/provider-services/cmd/run.go (3)
53-53: LGTM!The import is correctly placed and follows the project's import grouping conventions.
112-118: LGTM!The certificate issuer flag constants follow the established naming convention and provide comprehensive configuration options.
259-261: LGTM!Good refactoring to centralize flag registration. The error handling with panic is appropriate for initialization failures.
cmd/provider-services/cmd/certs.go (2)
93-93: LGTM! Simplified function signature.The removal of the explicit error return is appropriate since none of the option functions in this file return errors.
151-163: Good improvement: Graceful handling of missing certificates.The check for certificate file existence before loading prevents startup failures when certificates haven't been created yet, which is particularly useful when integrating with the new Let's Encrypt certificate issuer.
troian
force-pushed
the
issue-302
branch
3 times, most recently
from
7dace03 to
418ffb4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
tools/certissuer/lego.go (2)
84-88:⚠️ Potential issueFix undefined error variable.
Line 87 references an undefined
errvariable. The error should be properly defined.Apply this fix:
if client.GetExternalAccountRequired() { if cfg.KID == "" || cfg.HMAC == "" { log.Error(fmt.Sprintf("server requires External Account Binding. Config options KID and HMAC must be set")) - return nil, err + return nil, fmt.Errorf("server requires External Account Binding but KID and HMAC are not set") }
164-175: 🛠️ Refactor suggestionImplement certificate renewal monitoring.
The
runmethod currently contains only placeholder logic. Since it's actively started as a goroutine, consider implementing the certificate renewal monitoring functionality.Consider implementing the renewal logic:
func (cl *certIssuer) run() error { - - // resp, err := cl.cl.Certificate.GetRenewalInfo(certificate.RenewalInfoRequest{}) - // - // resp.ShouldRenewAt() + ticker := time.NewTicker(24 * time.Hour) // Check daily + defer ticker.Stop() + for { select { case <-cl.ctx.Done(): return cl.ctx.Err() + case <-ticker.C: + // TODO: Implement certificate renewal check + // This would involve: + // 1. Loading existing certificates from storage + // 2. Checking if they need renewal + // 3. Renewing certificates that are close to expiry + // Example structure: + // if err := cl.checkAndRenewCertificates(); err != nil { + // cl.log.Error("Certificate renewal check failed", "error", err) + // } } } }tools/certissuer/storage.go (2)
448-464: Use wrapped static errors instead of dynamic errors.Follow Go best practices by defining static errors and wrapping them with context.
Define a static error at the package level:
var ErrUnknownPrivateKeyType = errors.New("unknown private key type")Then update the function:
func loadPrivateKey(file string) (crypto.PrivateKey, error) { keyBytes, err := os.ReadFile(file) if err != nil { return nil, err } keyBlock, _ := pem.Decode(keyBytes) switch keyBlock.Type { case "RSA PRIVATE KEY": return x509.ParsePKCS1PrivateKey(keyBlock.Bytes) case "EC PRIVATE KEY": return x509.ParseECPrivateKey(keyBlock.Bytes) } - return nil, errors.New("unknown private key type") + return nil, fmt.Errorf("%w: %s", ErrUnknownPrivateKeyType, keyBlock.Type) }
205-227: 🛠️ Refactor suggestionConsider modifying GetPrivateKey to return errors.
The method logs errors but doesn't return them, which could lead to nil private keys being returned without the caller knowing why.
Consider updating the Storage interface and implementation:
type Storage interface { AccountSetup() (*Account, certcrypto.KeyType) AccountSave(*Account) error AccountLoad(crypto.PrivateKey) *Account - GetPrivateKey(certcrypto.KeyType) crypto.PrivateKey + GetPrivateKey(certcrypto.KeyType) (crypto.PrivateKey, error) ArchiveDomain(domain string) error }Then update the implementation to return errors properly. This will require updating callers to handle the error.
🧹 Nitpick comments (1)
tools/certissuer/storage.go (1)
299-311: Fix error logging format.The error logging uses printf-style format strings, but the logger may not support them. Use structured logging instead.
Update the error logging:
func (s *storage) ReadResource(domain string) certificate.Resource { raw, err := s.ReadFile(domain, extResource) if err != nil { - s.log.Error("error while loading the meta data for domain %s\n\t%v", domain, err) + s.log.Error("error while loading the meta data for domain", "domain", domain, "error", err) } var resource certificate.Resource if err = json.Unmarshal(raw, &resource); err != nil { - s.log.Error("error while marshaling the meta data for domain %s\n\t%v", domain, err) + s.log.Error("error while unmarshaling the meta data for domain", "domain", domain, "error", err) } return resource }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cmd/provider-services/cmd/certs.go(5 hunks)cmd/provider-services/cmd/flags.go(1 hunks)cmd/provider-services/cmd/run.go(9 hunks)go.mod(10 hunks)tools/certissuer/account.go(1 hunks)tools/certissuer/config.go(1 hunks)tools/certissuer/errors.go(1 hunks)tools/certissuer/lego.go(1 hunks)tools/certissuer/storage.go(1 hunks)tools/certissuer/storage_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- tools/certissuer/errors.go
- tools/certissuer/account.go
- tools/certissuer/storage_test.go
- cmd/provider-services/cmd/flags.go
- go.mod
- cmd/provider-services/cmd/certs.go
- cmd/provider-services/cmd/run.go
- tools/certissuer/config.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
tools/certissuer/storage.go (1)
tools/certissuer/account.go (1)
Account(10-14)
🪛 golangci-lint (1.64.8)
tools/certissuer/storage.go
529-529: undefined: pkcs12
(typecheck)
530-530: undefined: pkcs12
(typecheck)
533-533: undefined: pkcs12
(typecheck)
🪛 GitHub Check: build-bins
tools/certissuer/storage.go
[failure] 23-23:
missing go.sum entry for module providing package software.sslmate.com/src/go-pkcs12 (imported by github.com/akash-network/provider/tools/certissuer); to add:
🪛 GitHub Check: lint
tools/certissuer/storage.go
[failure] 533-533:
undefined: pkcs12 (typecheck)
[failure] 530-530:
undefined: pkcs12 (typecheck)
[failure] 529-529:
undefined: pkcs12 (typecheck)
🪛 GitHub Check: tests
tools/certissuer/storage.go
[failure] 23-23:
missing go.sum entry for module providing package software.sslmate.com/src/go-pkcs12 (imported by github.com/akash-network/provider/tools/certissuer); to add:
[failure] 23-23:
missing go.sum entry for module providing package software.sslmate.com/src/go-pkcs12 (imported by github.com/akash-network/provider/tools/certissuer); to add:
[failure] 23-23:
missing go.sum entry for module providing package software.sslmate.com/src/go-pkcs12 (imported by github.com/akash-network/provider/tools/certissuer); to add:
[failure] 23-23:
missing go.sum entry for module providing package software.sslmate.com/src/go-pkcs12 (imported by github.com/akash-network/provider/tools/certissuer); to add:
[failure] 23-23:
missing go.sum entry for module providing package software.sslmate.com/src/go-pkcs12 (imported by github.com/akash-network/provider/tools/certissuer); to add:
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests / crd-e2e
troian
force-pushed
the
issue-302
branch
4 times, most recently
from
a2c9ab0 to
264abbd
Compare
fixes akash-network/support#302 Signed-off-by: Artur Troian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tools/certissuer/lego.go (2)
166-177: 🛠️ Refactor suggestionImplement certificate renewal logic in the run method.
The
runmethod currently only blocks until context cancellation but doesn't implement any certificate management functionality. Consider implementing certificate renewal monitoring:func (cl *certIssuer) run() error { - - // resp, err := cl.cl.Certificate.GetRenewalInfo(certificate.RenewalInfoRequest{}) - // - // resp.ShouldRenewAt() + ticker := time.NewTicker(24 * time.Hour) // Check daily for renewals + defer ticker.Stop() + for { select { case <-cl.ctx.Done(): return cl.ctx.Err() + case <-ticker.C: + // TODO: Implement certificate renewal monitoring + // Check certificates that need renewal and renew them } } }
18-20: 🛠️ Refactor suggestionConsider extending the CertIssuer interface for actual certificate operations.
The current interface only provides a
Close()method but lacks essential certificate issuance functionality. Consider adding methods for certificate management:type CertIssuer interface { + // IssueCertificate issues a new certificate for the given domain + IssueCertificate(ctx context.Context, domain string) (*certificate.Resource, error) + // RenewCertificate renews an existing certificate + RenewCertificate(ctx context.Context, domain string) (*certificate.Resource, error) Close() error }
🧹 Nitpick comments (4)
tools/certissuer/lego.go (1)
89-89: Use wrapped static errors instead of dynamic errors.Following Go best practices, replace dynamic error creation with predefined static errors.
For line 89:
+var ErrExternalAccountBindingRequired = errors.New("server requires External Account Binding but KID and HMAC are not set") + return nil, fmt.Errorf("server requires External Account Binding. Config options KID and HMAC must be set") + return nil, ErrExternalAccountBindingRequiredFor line 199:
+var ErrUnsupportedDNSProvider = errors.New("unsupported DNS provider") + - return res, fmt.Errorf("lego: unsupported dns provider %s", provider) + return res, fmt.Errorf("%w: %s", ErrUnsupportedDNSProvider, provider)Also applies to: 199-199
🧰 Tools
🪛 golangci-lint (1.64.8)
89-89: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("server requires External Account Binding. Config options KID and HMAC must be set")"
(err113)
tools/certissuer/storage_test.go (2)
15-15: Extract repeated string literal to a constant.The string "example.com" appears multiple times. Extract it to a constant for better maintainability.
+const testDomain = "example.com" + func TestCertificatesStorage_MoveToArchive(t *testing.T) { - domain := "example.com" + domain := testDomainApply the same change to the other test functions.
Also applies to: 55-55, 93-93
🧰 Tools
🪛 golangci-lint (1.64.8)
15-15: string
example.comhas 3 occurrences, make it a constant(goconst)
🪛 GitHub Check: lint
[failure] 15-15:
stringexample.comhas 3 occurrences, make it a constant (goconst)
139-139: Pre-allocate slice capacity for better performance.Pre-allocating the slice with known capacity improves performance and follows Go best practices.
- var filenames []string + filenames := make([]string, 0, len([]string{extIssuer, extCert, extKey, extPem, extPfx, extResource}))🧰 Tools
🪛 golangci-lint (1.64.8)
139-139: Consider pre-allocating
filenames(prealloc)
🪛 GitHub Check: lint
[failure] 139-139:
Consider pre-allocatingfilenames(prealloc)tools/certissuer/storage.go (1)
388-388: Use wrapped static errors instead of dynamic errors.Following Go best practices, replace dynamic error creation with predefined static errors for better error handling and testing.
+var ( + ErrUnableToParsePrivateKey = errors.New("unable to parse private key") + ErrUnableToParsePrivateKey = errors.New("unable to parse certificate") + ErrUnsupportedPrivateKeyType = errors.New("unsupported private key type") +) + func (s *storage) WritePFXFile(domain string, certRes *certificate.Resource) error { certPemBlock, _ := pem.Decode(certRes.Certificate) if certPemBlock == nil { - return fmt.Errorf("unable to parse Certificate for domain %s", domain) + return fmt.Errorf("%w for domain %s", ErrUnableToParsePrivateKey, domain) } // ... keyPemBlock, _ := pem.Decode(certRes.PrivateKey) if keyPemBlock == nil { - return fmt.Errorf("unable to parse PrivateKey for domain %s", domain) + return fmt.Errorf("%w for domain %s", ErrUnableToParsePrivateKey, domain) } // ... default: - return fmt.Errorf("unsupported PrivateKey type '%s' for domain %s", keyPemBlock.Type, domain) + return fmt.Errorf("%w '%s' for domain %s", ErrUnsupportedPrivateKeyType, keyPemBlock.Type, domain)Also applies to: 403-403, 421-421
🧰 Tools
🪛 golangci-lint (1.64.8)
388-388: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("unable to parse Certificate for domain %s", domain)"
(err113)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cmd/provider-services/cmd/certs.go(5 hunks)cmd/provider-services/cmd/flags.go(1 hunks)cmd/provider-services/cmd/run.go(9 hunks)go.mod(11 hunks)tools/certissuer/account.go(1 hunks)tools/certissuer/config.go(1 hunks)tools/certissuer/errors.go(1 hunks)tools/certissuer/lego.go(1 hunks)tools/certissuer/storage.go(1 hunks)tools/certissuer/storage_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tools/certissuer/account.go
🚧 Files skipped from review as they are similar to previous changes (5)
- tools/certissuer/errors.go
- tools/certissuer/config.go
- cmd/provider-services/cmd/certs.go
- cmd/provider-services/cmd/flags.go
- go.mod
🧰 Additional context used
🧬 Code Graph Analysis (1)
tools/certissuer/storage_test.go (1)
tools/certissuer/storage.go (2)
StorageConfig(47-57)NewStorage(124-146)
🪛 golangci-lint (1.64.8)
tools/certissuer/lego.go
89-89: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("server requires External Account Binding. Config options KID and HMAC must be set")"
(err113)
199-199: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("lego: unsupported dns provider %s", provider)"
(err113)
tools/certissuer/storage.go
388-388: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("unable to parse Certificate for domain %s", domain)"
(err113)
403-403: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("unable to parse PrivateKey for domain %s", domain)"
(err113)
421-421: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf("unsupported PrivateKey type '%s' for domain %s", keyPemBlock.Type, domain)"
(err113)
cmd/provider-services/cmd/run.go
221-221: ineffectual assignment to err
(ineffassign)
tools/certissuer/storage_test.go
15-15: string example.com has 3 occurrences, make it a constant
(goconst)
139-139: Consider pre-allocating filenames
(prealloc)
🪛 GitHub Check: lint
tools/certissuer/storage_test.go
[failure] 139-139:
Consider pre-allocating filenames (prealloc)
[failure] 15-15:
string example.com has 3 occurrences, make it a constant (goconst)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-bins
🔇 Additional comments (5)
tools/certissuer/storage_test.go (1)
14-52: LGTM! Well-structured test for archive functionality.The test comprehensively verifies that domain-specific certificate files are correctly moved to the archive directory with proper timestamped naming.
🧰 Tools
🪛 golangci-lint (1.64.8)
15-15: string
example.comhas 3 occurrences, make it a constant(goconst)
🪛 GitHub Check: lint
[failure] 15-15:
stringexample.comhas 3 occurrences, make it a constant (goconst)cmd/provider-services/cmd/run.go (2)
205-222: LGTM! Certificate issuer integration is well implemented.The conditional initialization of the certificate issuer with proper configuration validation and error handling addresses the previous concern about the issuer not being instantiated.
🧰 Tools
🪛 golangci-lint (1.64.8)
221-221: ineffectual assignment to err
(ineffassign)
253-255: LGTM! Proper cleanup handling for certificate issuer.The cleanup logic properly handles the certificate issuer shutdown in the context cancellation goroutine.
tools/certissuer/storage.go (2)
47-72: LGTM! Well-designed storage interface.The Storage interface provides comprehensive methods for ACME account and certificate management with clear separation of concerns.
124-146: LGTM! Robust storage initialization.The NewStorage function properly handles URL parsing, path construction, and directory structure creation with appropriate error handling.
fixes akash-network/support#302
Summary by CodeRabbit