-
-
Notifications
You must be signed in to change notification settings - Fork 647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(server): support website custom domain ssl cert auto-gen #956
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
apiVersion: cert-manager.io/v1 | ||
kind: ClusterIssuer | ||
metadata: | ||
name: laf-issuer | ||
spec: | ||
acme: | ||
server: https://acme-v02.api.letsencrypt.org/directory | ||
email: admin@sealos.io | ||
privateKeySecretRef: | ||
name: letsencrypt-prod | ||
solvers: | ||
- http01: | ||
ingress: | ||
class: apisix | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,6 +100,10 @@ export class ServerConfig { | |
return process.env.API_SERVER_URL || 'http://localhost:3000' | ||
} | ||
|
||
static get certManagerIssuerName() { | ||
return process.env.CERT_MANAGER_ISSUER_NAME || 'laf-issuer' | ||
} | ||
|
||
/** default region conf */ | ||
static get DEFAULT_REGION_DATABASE_URL() { | ||
return process.env.DEFAULT_REGION_DATABASE_URL | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with a brief review of the code patch included: This code looks correct in terms of syntax, but there are a few points to consider when doing a code review.
It is also important to consider the context in which the code patch is being used. For example, if the code patch is being used in a production environment, it would be important to ensure that all necessary tests have been performed to ensure stability and reliability. Overall, this code patch looks to be in good condition, but it is always best practice to review code patches carefully before applying them to an existing codebase. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,172 @@ | ||
import { Injectable, Logger } from '@nestjs/common' | ||
import { Region, WebsiteHosting } from '@prisma/client' | ||
import { LABEL_KEY_APP_ID, ServerConfig } from 'src/constants' | ||
import { ClusterService } from 'src/region/cluster/cluster.service' | ||
import { GetApplicationNamespaceByAppId } from 'src/utils/getter' | ||
|
||
// This class handles the creation and deletion of website domain certificates | ||
// and ApisixTls resources using Kubernetes Custom Resource Definitions (CRDs). | ||
@Injectable() | ||
export class ApisixCrdService { | ||
private readonly logger = new Logger(ApisixCrdService.name) | ||
constructor(private readonly clusterService: ClusterService) {} | ||
|
||
// Read a certificate for a given website using cert-manager.io CRD | ||
async readWebsiteDomainCert(region: Region, website: WebsiteHosting) { | ||
try { | ||
// Get the namespace based on the application ID | ||
const namespace = GetApplicationNamespaceByAppId(website.appid) | ||
// Create a Kubernetes API client for the specified region | ||
const api = this.clusterService.makeObjectApi(region) | ||
|
||
// Make a request to read the Certificate resource | ||
const res = await api.read({ | ||
apiVersion: 'cert-manager.io/v1', | ||
kind: 'Certificate', | ||
metadata: { | ||
name: website.id, | ||
namespace, | ||
}, | ||
}) | ||
return res.body | ||
} catch (err) { | ||
if (err?.response?.body?.reason === 'NotFound') return null | ||
this.logger.error(err) | ||
this.logger.error(err?.response?.body) | ||
throw err | ||
} | ||
} | ||
|
||
// Create a certificate for a given website using cert-manager.io CRD | ||
async createWebsiteDomainCert(region: Region, website: WebsiteHosting) { | ||
// Get the namespace based on the application ID | ||
const namespace = GetApplicationNamespaceByAppId(website.appid) | ||
// Create a Kubernetes API client for the specified region | ||
const api = this.clusterService.makeObjectApi(region) | ||
|
||
// Make a request to create the Certificate resource | ||
const res = await api.create({ | ||
apiVersion: 'cert-manager.io/v1', | ||
kind: 'Certificate', | ||
// Set the metadata for the Certificate resource | ||
metadata: { | ||
name: website.id, | ||
namespace, | ||
labels: { | ||
'laf.dev/website': website.id, | ||
'laf.dev/website-domain': website.domain, | ||
[LABEL_KEY_APP_ID]: website.appid, | ||
}, | ||
}, | ||
// Define the specification for the Certificate resource | ||
spec: { | ||
secretName: website.id, | ||
dnsNames: [website.domain], | ||
issuerRef: { | ||
name: ServerConfig.certManagerIssuerName, | ||
kind: 'ClusterIssuer', | ||
}, | ||
}, | ||
}) | ||
return res.body | ||
} | ||
|
||
// Delete a certificate for a given website using cert-manager.io CRD | ||
async deleteWebsiteDomainCert(region: Region, website: WebsiteHosting) { | ||
// Get the namespace based on the application ID | ||
const namespace = GetApplicationNamespaceByAppId(website.appid) | ||
// Create a Kubernetes API client for the specified region | ||
const api = this.clusterService.makeObjectApi(region) | ||
|
||
// Make a request to delete the Certificate resource | ||
const res = await api.delete({ | ||
apiVersion: 'cert-manager.io/v1', | ||
kind: 'Certificate', | ||
metadata: { | ||
name: website.id, | ||
namespace, | ||
}, | ||
}) | ||
return res.body | ||
} | ||
|
||
// Read an ApisixTls resource for a given website using apisix.apache.org CRD | ||
async readWebsiteDomainApisixTls(region: Region, website: WebsiteHosting) { | ||
try { | ||
// Get the namespace based on the application ID | ||
const namespace = GetApplicationNamespaceByAppId(website.appid) | ||
// Create an API object for the specified region | ||
const api = this.clusterService.makeObjectApi(region) | ||
|
||
// Make a request to read the ApisixTls resource | ||
const res = await api.read({ | ||
apiVersion: 'apisix.apache.org/v2', | ||
kind: 'ApisixTls', | ||
metadata: { | ||
name: website.id, | ||
namespace, | ||
}, | ||
}) | ||
return res.body | ||
} catch (err) { | ||
if (err?.response?.body?.reason === 'NotFound') return null | ||
this.logger.error(err) | ||
this.logger.error(err?.response?.body) | ||
throw err | ||
} | ||
} | ||
|
||
// Create an ApisixTls resource for a given website using apisix.apache.org CRD | ||
async createWebsiteDomainApisixTls(region: Region, website: WebsiteHosting) { | ||
// Get the namespace based on the application ID | ||
const namespace = GetApplicationNamespaceByAppId(website.appid) | ||
// Create an API object for the specified region | ||
const api = this.clusterService.makeObjectApi(region) | ||
|
||
// Make a request to create the ApisixTls resource | ||
const res = await api.create({ | ||
apiVersion: 'apisix.apache.org/v2', | ||
kind: 'ApisixTls', | ||
// Set the metadata for the ApisixTls resource | ||
metadata: { | ||
name: website.id, | ||
namespace, | ||
labels: { | ||
'laf.dev/website': website.id, | ||
'laf.dev/website-domain': website.domain, | ||
[LABEL_KEY_APP_ID]: website.appid, | ||
}, | ||
}, | ||
// Define the specification for the ApisixTls resource | ||
spec: { | ||
hosts: [website.domain], | ||
secret: { | ||
name: website.id, | ||
namespace, | ||
}, | ||
}, | ||
}) | ||
return res.body | ||
} | ||
|
||
// Deletes the APISIX TLS configuration for a specific website domain | ||
async deleteWebsiteDomainApisixTls(region: Region, website: WebsiteHosting) { | ||
// Get the application namespace using the website's appid | ||
const namespace = GetApplicationNamespaceByAppId(website.appid) | ||
|
||
// Create an API object for the specified region | ||
const api = this.clusterService.makeObjectApi(region) | ||
|
||
// Send a delete request to remove the APISIX TLS configuration | ||
const res = await api.delete({ | ||
apiVersion: 'apisix.apache.org/v2', | ||
kind: 'ApisixTls', | ||
metadata: { | ||
name: website.id, | ||
namespace, | ||
}, | ||
}) | ||
|
||
return res.body | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -162,6 +162,20 @@ export class ApisixService { | |
} | ||
} | ||
|
||
async getRoute(region: Region, id: string) { | ||
const conf = region.gatewayConf | ||
const api_url = `${conf.apiUrl}/routes/${id}` | ||
|
||
const res = await this.httpService.axiosRef.get(api_url, { | ||
headers: { | ||
'X-API-KEY': conf.apiKey, | ||
'Content-Type': 'application/json', | ||
}, | ||
}) | ||
|
||
return res.data | ||
} | ||
|
||
async deleteRoute(region: Region, id: string) { | ||
const conf = region.gatewayConf | ||
const api_url = `${conf.apiUrl}/routes/${id}` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. by looking at the code and seeing if there are any potential bugs:
In terms of improvement suggestions, we can look at refactoring the code to make it more readable and easier to maintain. We could also add additional checks and validations to ensure the data being passed in is valid. Finally, we could add unit tests to ensure that the code is doing what it is intended to do. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ import { BucketDomainService } from './bucket-domain.service' | |
import { WebsiteTaskService } from './website-task.service' | ||
import { BucketDomainTaskService } from './bucket-domain-task.service' | ||
import { RuntimeDomainTaskService } from './runtime-domain-task.service' | ||
import { ApisixCrdService } from './apisix-crd.service'; | ||
|
||
@Module({ | ||
imports: [HttpModule], | ||
|
@@ -16,6 +17,7 @@ import { RuntimeDomainTaskService } from './runtime-domain-task.service' | |
WebsiteTaskService, | ||
BucketDomainTaskService, | ||
RuntimeDomainTaskService, | ||
ApisixCrdService, | ||
], | ||
exports: [RuntimeDomainService, BucketDomainService], | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the code review
Overall, the code patch looks good and there are no bugs or risks. However, it would be better if the code is properly documented with comments. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -310,7 +310,7 @@ export class InstanceService { | |
return res.body | ||
} catch (error) { | ||
if (error?.response?.body?.reason === 'NotFound') return null | ||
return null | ||
throw error | ||
} | ||
} | ||
|
||
|
@@ -324,7 +324,8 @@ export class InstanceService { | |
const res = await coreV1Api.readNamespacedService(serviceName, namespace) | ||
return res.body | ||
} catch (error) { | ||
return null | ||
if (error?.response?.body?.reason === 'NotFound') return null | ||
throw error | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with the code review. In both instances, the code is checking for a NotFound response, and returning null if one is found. However, in the first instance, it just returns null regardless of the response, while in the second instance, if the response isn't NotFound, it throws an error. This inconsistency should be addressed, either by having both instances throw an error or by having both instances return null no matter what the response is. It also looks like in both cases, the code is not checking for other errors that may be returned. This should be addressed as well, perhaps by adding additional checks in the catch block to handle any other errors that may be returned. Overall, the code should be made more consistent, and more checks should be added to handle any potential errors that may arise. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,8 +52,8 @@ export class ClusterService { | |
return res.body | ||
} catch (err) { | ||
this.logger.error(err) | ||
this.logger.debug(err?.response?.body) | ||
return null | ||
this.logger.error(err?.response?.body) | ||
throw err | ||
} | ||
} | ||
|
||
|
@@ -67,8 +67,8 @@ export class ClusterService { | |
} catch (err) { | ||
if (err?.response?.body?.reason === 'NotFound') return null | ||
this.logger.error(err) | ||
this.logger.debug(err?.response?.body) | ||
return null | ||
this.logger.error(err?.response?.body) | ||
throw err | ||
} | ||
} | ||
|
||
|
@@ -81,8 +81,8 @@ export class ClusterService { | |
return res | ||
} catch (err) { | ||
this.logger.error(err) | ||
this.logger.debug(err?.response?.body) | ||
return null | ||
this.logger.error(err?.response?.body) | ||
throw err | ||
} | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the code review: 1.The patch changes the way exceptions are handled in the three functions by replacing the log message and returning null with a log error and throwing an error. This is good practice as it allows any exceptions that occur to be properly logged and can be traced back to the source. 2.In the first two functions, the log message for the response body should be changed from debug to error as it provides more detailed information and is more appropriate for logging errors. 3.It's not clear why the third function is returning a response rather than throwing an exception. It might be beneficial to change it to throw an exception if there is an error. 4.It would be helpful to add comments to the code explaining what each of these functions are doing and what types of exceptions they might throw. |
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code review:
Overall, the code patch looks good and there are no obvious bugs or risks. As an improvement suggestion, it might be useful to add comments to explain the purpose of each section.