Skip to content

Conversation

@nfebe
Copy link
Contributor

@nfebe nfebe commented Feb 7, 2026

Certificate requests were skipped because the orchestrator pre-disabled SSL.Enabled for domains without existing certs, and RequestCertificatesForDomains required both Enabled and AutoCert. Now only AutoCert drives certificate requests.

Also default new domain container_port from legacy networking config when omitted, preventing wrong port in nginx upstream.

Closes #75

@sourceant
Copy link

sourceant bot commented Feb 7, 2026

Code Review Summary

This pull request introduces several important improvements to domain and SSL certificate management, alongside comprehensive test coverage. Key changes involve refining the logic for adding domains to deployments, especially concerning the defaulting of container ports, and correcting the behavior of SSL certificate requests to properly consider auto-certification independently of the SSL enablement status. The added test cases significantly boost confidence in the robustness and correctness of these features.

🚀 Key Improvements

  • The addDomain API endpoint now correctly defaults ContainerPort for new domains from existing legacy networking configurations if no port is explicitly provided. This improves user experience and ensures backward compatibility.
  • The SSL manager's RequestCertificatesForDomains and GetDomainsNeedingCertificates functions have been updated to evaluate AutoCert status independently of SSL.Enabled. This resolves a potential issue where the orchestrator might temporarily disable SSL during certificate issuance, preventing auto-renewal or initial requests.
  • Extensive new test cases have been added across domains_test.go, nginx/manager_test.go, and ssl/manager_test.go. These tests validate the updated domain logic, Nginx configuration generation for SSL toggling and container ports, and the revised SSL certificate request logic, ensuring the changes work as intended.

Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete. See the overview comment for a summary.

@nfebe nfebe changed the title fix(ssl): Resolve missing SSL config when adding domain to fix(ssl): Resolve missing SSL blocks in vhosts Feb 7, 2026
@nfebe nfebe force-pushed the fix/ssl-config-missing-on-new-domain branch from 2a9faff to 96c23f3 Compare February 7, 2026 10:58
Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete. See the overview comment for a summary.

Certificate requests were skipped because the orchestrator
pre-disabled SSL.Enabled for domains without existing certs,
and RequestCertificatesForDomains required both Enabled and
AutoCert. Now only AutoCert drives certificate requests.

Also default new domain container_port from legacy networking
config when omitted, preventing wrong port in nginx upstream.

Closes #75

Signed-off-by: nfebe <fenn25.fn@gmail.com>
@nfebe nfebe force-pushed the fix/ssl-config-missing-on-new-domain branch from 96c23f3 to 4f3478c Compare February 7, 2026 11:10
Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete. No specific code suggestions were generated. See the overview comment for a summary.

@nfebe nfebe merged commit 2260072 into main Feb 7, 2026
5 checks passed
@nfebe nfebe deleted the fix/ssl-config-missing-on-new-domain branch February 7, 2026 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SSL not configured when adding new domain to project with existing legacy domain

1 participant