chore: add concise error messages for ingress hostname does not supported #543
Conversation
✅ Deploy Preview for kamaji-documentation canceled.
|
There was a problem hiding this comment.
Thanks for the contribution, @reoring, I really appreciate it!
I added some comments and some remarks on the Ingress optimisation here, happy to chat here to elaborate further, or via the Kubernetes Slack workspace (#kamaji).
Makefile
Outdated
| fmt: ## Run go fmt against code. | ||
| go fmt ./... | ||
|
|
||
| vet: ## Run go vet against code. | ||
| go vet ./.. | ||
|
|
There was a problem hiding this comment.
I used to remove these targets mostly because we're relying on golangci-lint which offers fmt (as gofmt) and vet (as govet) too.
We could reuse the golint target before building the binary, rather.
There was a problem hiding this comment.
Thank you for your feedback. I've reviewed the changes made to the Makefile, and they indeed align with your suggestions. Here's a summary of the modifications:
The 'fmt' and 'vet' targets have been removed from the Makefile.
The 'build' target now depends on 'generate' and 'golint' instead of 'generate', 'fmt', and 'vet'.
The 'run' target no longer depends on 'fmt' and 'vet'.
| if hostname := lb.Hostname; len(hostname) > 0 { | ||
| // Resolve hostname to IP address | ||
| ips, err := net.LookupIP(hostname) | ||
| if err != nil { | ||
| return "", errors.Wrap(err, "cannot resolve LoadBalancer hostname to IP") | ||
| } | ||
| if len(ips) > 0 { | ||
| return ips[0].String(), nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Kubernetes and FQDNs have a tough relationship, unfortunately.
The problem here is that we don't know which resolver is responsible for the DNS resolution and it could differ from environment to environment.
The Tenant Control Plane address extracted here is going to be used as the kubernetes.default.svc endpoint address, which is then resolved by kubelet to forward traffic to the API Server from internal pods. The problem with this approach resides in the fact the DNS resolution could be changed over time, bringing the cluster Pod to the Kubernetes API Server connection broken.
At the current state, our suggestion is setting a Virtual IP for the given hostname by manually defining it, since the implications behind L7 routing with the API Server must be well known by the users: although the aim of this PR is to simplify it, we could end-up with users complaining it doesn't work despite it's more one of their problems in not understanding the complexities of the topic.
Furthermore, an Ingress exposition requires a TLS passthrough for the client-based certificates authentication, again something that could be hard to understand for newbies, without considering that routing would be based on hostname: if an Ingress Controller is listening on 1.2.3.4 and used by several TCPs (tcp-alpha.my.corp.tld, tcp-bravo.my.corp.tld, tcp-charlie.my.corp.tld) the routing would be broken, unless using a routing based on the port number, making totally useless the Ingress.
There was a problem hiding this comment.
Thank you for your detailed explanation about the challenges of using FQDNs with Kubernetes in the context of Tenant Control Plane addressing. I understand the complexities and potential issues arising from DNS resolution inconsistencies.
To address these concerns, I've implemented the following:
- A new getLoadBalancerAddress function to handle IP address extraction and hostname checks for LoadBalancer ingress.
- Comprehensive comments explaining why hostnames aren't supported, potential risks, and recommended solutions.
- A concise error message: "hostname not supported for LoadBalancer ingress: use static IP instead".
These changes aim to prevent misconfigurations and help users quickly identify and resolve issues related to hostname usage in this context. The detailed comments serve as documentation for developers, while the error message provides immediate, actionable feedback to users.
Let me know if you'd like any further modifications or have any questions about this implementation.
reoring
changed the title
- Add 'fmt' target to run go fmt against code - Add 'vet' target to run go vet against code - Include fmt and vet in the build process This change improves code quality checks in the build process.
- Enhance error reporting in GenerateCertificatePrivateKeyPair function - Add detailed error checks for CA certificate and private key parsing - Implement check for expected number of certificate files - Improve error logging in APIServerCertificate resource This commit preserves more details about certificate-related issues, aiding in debugging and troubleshooting.
Add functionality to resolve loadbalancer hostname to IP address in DeclaredControlPlaneAddress method. This enhances the existing IP address handling by allowing the use of hostnames for loadbalancers. - Add hostname check in addition to IP check - Implement hostname resolution using net.LookupIP - Return the first resolved IP address if available
- Extract LoadBalancer address logic to separate function - Remove hostname resolution for LoadBalancer ingress - Add explanatory comments on reasons for not supporting hostnames
- Remove fmt and vet targets - Update build target to use golint instead of fmt and vet - Remove fmt and vet dependencies from run target
reoring
force-pushed
the
support-ingress-hostname
branch
from
aa275ea
to
4356f5c
Compare
Thank you for reviewing my PR. I've just removed the draft status as there are no more pending changes from my side. If everything else looks good to you, I believe it's ready to be merged. Please let me know if you need any further information or if there's anything else I can do to help get this merged. Thank you again for your time and consideration. |
|
Thanks @reoring, happy to receive contributions from you! 🚀 |
This pull request adds support for ingress hostname in the TenantControlPlane, improving the flexibility of the control plane address resolution. It also includes improvements to error handling and logging in the certificate generation process, as well as some Makefile enhancements.
Key changes:
TenantControlPlane now supports resolving LoadBalancer hostnames to IP addresses when determining the control plane address.
Improved error handling and logging in the certificate generation process, providing more detailed information on failures.
Added fmt and vet targets to the Makefile for better code quality checks.
These changes enhance the robustness of the TenantControlPlane implementation and improve the developer experience by providing better tools for code maintenance.
Testing:
[x] Verify that TenantControlPlane correctly resolves LoadBalancer hostnames to IP addresses
[ ] Ensure that certificate generation provides more informative error messages on failure
Additional Notes:
As I am not deeply familiar with the full specifications of this project, I kindly request the reviewers to carefully examine these changes. If any part of this implementation does not align with the intended functionality or project goals, please let me know.
Furthermore, if there are any additional features or modifications related to this change that should be included, I would greatly appreciate your guidance and suggestions. Your expertise in this area is valuable, and I'm open to making any necessary adjustments to ensure this pull request meets all requirements and aligns with the project's vision.
Please review and provide any feedback or additional requirements you may have. I'm ready to make further modifications as needed to ensure this change fully meets the project's needs.