From 70911731e39f2dbfdf95bea19b92f1df43c0e0e2 Mon Sep 17 00:00:00 2001 From: reoring Date: Tue, 13 Aug 2024 16:05:43 +0900 Subject: [PATCH 1/6] chore: add fmt and vet targets - 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. --- Makefile | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Makefile b/Makefile index 126bb609..7ce0adfc 100644 --- a/Makefile +++ b/Makefile @@ -170,6 +170,12 @@ BUILD_DATE ?= $$(git log -1 --format="%at" | xargs -I{} date -d @{} +%Y-%m- get_version: @echo -n v$(VERSION) +fmt: ## Run go fmt against code. + go fmt ./... + +vet: ## Run go vet against code. + go vet ./.. + build: generate fmt vet ## Build manager binary. go build -o bin/manager main.go From 35ac5872c5b4653e75edb1769f58ecb2258fa6ad Mon Sep 17 00:00:00 2001 From: reoring Date: Tue, 13 Aug 2024 16:08:24 +0900 Subject: [PATCH 2/6] chore: improve error handling and logging for certificate operations - 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. --- internal/kubeadm/certificates.go | 23 +++++++++++++++----- internal/resources/api_server_certificate.go | 5 ++--- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/internal/kubeadm/certificates.go b/internal/kubeadm/certificates.go index fbb362ec..74bca7f6 100644 --- a/internal/kubeadm/certificates.go +++ b/internal/kubeadm/certificates.go @@ -44,21 +44,32 @@ func GenerateCACertificatePrivateKeyPair(baseName string, config *Configuration) func GenerateCertificatePrivateKeyPair(baseName string, config *Configuration, ca CertificatePrivateKeyPair) (*CertificatePrivateKeyPair, error) { defer deleteCertificateDirectory(config.InitConfiguration.CertificatesDir) - certificate, _ := cryptoKamaji.ParseCertificateBytes(ca.Certificate) - signer, _ := cryptoKamaji.ParsePrivateKeyBytes(ca.PrivateKey) + certificate, err := cryptoKamaji.ParseCertificateBytes(ca.Certificate) + if err != nil { + return nil, fmt.Errorf("failed to parse CA certificate: %v", err) + } + + signer, err := cryptoKamaji.ParsePrivateKeyBytes(ca.PrivateKey) + if err != nil { + return nil, fmt.Errorf("failed to parse CA private key: %v", err) + } kubeadmCert, err := getKubeadmCert(baseName) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get kubeadm cert: %v", err) } if err = initPhaseFromCA(kubeadmCert, config, certificate, signer); err != nil { - return nil, err + return nil, fmt.Errorf("failed to initialize phase from CA: %v", err) } contents, err := readCertificateFiles(baseName, config.InitConfiguration.CertificatesDir, "crt", "key") if err != nil { - return nil, err + return nil, fmt.Errorf("failed to read certificate files: %v", err) + } + + if len(contents) != 2 { + return nil, fmt.Errorf("unexpected number of certificate files: expected 2, got %d", len(contents)) } certificatePrivateKeyPair := &CertificatePrivateKeyPair{ @@ -66,7 +77,7 @@ func GenerateCertificatePrivateKeyPair(baseName string, config *Configuration, c PrivateKey: contents[1], } - return certificatePrivateKeyPair, err + return certificatePrivateKeyPair, nil } func getKubeadmCert(baseName string) (*certs.KubeadmCert, error) { diff --git a/internal/resources/api_server_certificate.go b/internal/resources/api_server_certificate.go index a056dad7..c6b18fad 100644 --- a/internal/resources/api_server_certificate.go +++ b/internal/resources/api_server_certificate.go @@ -128,9 +128,8 @@ func (r *APIServerCertificate) mutate(ctx context.Context, tenantControlPlane *k config, err := getStoredKubeadmConfiguration(ctx, r.Client, r.TmpDirectory, tenantControlPlane) if err != nil { - logger.Error(err, "cannot retrieve kubeadm configuration") - - return err + logger.Error(err, "cannot generate certificate and private key in api server certificate", "details", err.Error()) + return fmt.Errorf("failed to generate certificate and private key: %w", err) } ca := kubeadm.CertificatePrivateKeyPair{ From ef062cbc23c3a660e1253c6b6d01e76d7b059488 Mon Sep 17 00:00:00 2001 From: reoring Date: Tue, 13 Aug 2024 16:10:00 +0900 Subject: [PATCH 3/6] feat: support loadbalancer hostname resolution 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 --- api/v1alpha1/tenantcontrolplane_funcs.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/api/v1alpha1/tenantcontrolplane_funcs.go b/api/v1alpha1/tenantcontrolplane_funcs.go index b2276f90..399c702b 100644 --- a/api/v1alpha1/tenantcontrolplane_funcs.go +++ b/api/v1alpha1/tenantcontrolplane_funcs.go @@ -65,6 +65,16 @@ func (in *TenantControlPlane) DeclaredControlPlaneAddress(ctx context.Context, c if ip := lb.IP; len(ip) > 0 { return ip, nil } + 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 + } + } } } From df1a7d42880f6495beabdd8ad402d1b8192eab03 Mon Sep 17 00:00:00 2001 From: reoring Date: Wed, 14 Aug 2024 07:29:10 +0900 Subject: [PATCH 4/6] fix: Remove hostname support for LoadBalancer ingress - Extract LoadBalancer address logic to separate function - Remove hostname resolution for LoadBalancer ingress - Add explanatory comments on reasons for not supporting hostnames --- api/v1alpha1/tenantcontrolplane_funcs.go | 44 ++++++++++++++++-------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/api/v1alpha1/tenantcontrolplane_funcs.go b/api/v1alpha1/tenantcontrolplane_funcs.go index 399c702b..4b81ddd1 100644 --- a/api/v1alpha1/tenantcontrolplane_funcs.go +++ b/api/v1alpha1/tenantcontrolplane_funcs.go @@ -61,22 +61,36 @@ func (in *TenantControlPlane) DeclaredControlPlaneAddress(ctx context.Context, c return "", kamajierrors.NonExposedLoadBalancerError{} } - for _, lb := range loadBalancerStatus.Ingress { - if ip := lb.IP; len(ip) > 0 { - return ip, nil - } - 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 - } - } - } + return getLoadBalancerAddress(loadBalancerStatus.Ingress) } return "", kamajierrors.MissingValidIPError{} } + +// getLoadBalancerAddress extracts the IP address from LoadBalancer ingress. +// It also checks and rejects hostname usage for LoadBalancer ingress. +// +// Reasons for not supporting hostnames: +// - DNS resolution can differ across environments, leading to inconsistent behavior. +// - It may cause connectivity problems between Kubernetes components. +// - The DNS resolution could change over time, potentially breaking cluster-to-API-server connections. +// +// Recommended solutions: +// - Use a static IP address to ensure stable and predictable communication within the cluster. +// - If a hostname is necessary, consider setting up a Virtual IP (VIP) for the given hostname. +// - Alternatively, use an external load balancer that can provide a stable IP address. +// +// Note: Implementing L7 routing with the API Server requires a deep understanding of the implications. +// Users should be aware of the complexities involved, including potential issues with TLS passthrough +// for client-based certificate authentication in Ingress expositions. +func getLoadBalancerAddress(ingress []corev1.LoadBalancerIngress) (string, error) { + for _, lb := range ingress { + if ip := lb.IP; len(ip) > 0 { + return ip, nil + } + if hostname := lb.Hostname; len(hostname) > 0 { + return "", fmt.Errorf("hostname not supported for LoadBalancer ingress: use static IP instead") + } + } + return "", kamajierrors.MissingValidIPError{} +} From d770a2360b9ee3e9b5b031021a9210bc49612938 Mon Sep 17 00:00:00 2001 From: reoring Date: Wed, 14 Aug 2024 07:58:16 +0900 Subject: [PATCH 5/6] fix: replace fmt and vet with golint - Remove fmt and vet targets - Update build target to use golint instead of fmt and vet - Remove fmt and vet dependencies from run target --- Makefile | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/Makefile b/Makefile index 7ce0adfc..b1ccb3fd 100644 --- a/Makefile +++ b/Makefile @@ -170,16 +170,10 @@ BUILD_DATE ?= $$(git log -1 --format="%at" | xargs -I{} date -d @{} +%Y-%m- get_version: @echo -n v$(VERSION) -fmt: ## Run go fmt against code. - go fmt ./... - -vet: ## Run go vet against code. - go vet ./.. - -build: generate fmt vet ## Build manager binary. +build: generate golint ## Build manager binary. go build -o bin/manager main.go -run: manifests generate fmt vet ## Run a controller from your host. +run: manifests generate ## Run a controller from your host. go run ./main.go docker-build: ## Build docker image with the manager. From 4356f5ca33646ae4d2b3d68bb83606c862b7c887 Mon Sep 17 00:00:00 2001 From: reoring Date: Wed, 14 Aug 2024 07:58:42 +0900 Subject: [PATCH 6/6] fix: lint errors --- api/v1alpha1/tenantcontrolplane_funcs.go | 1 + internal/kubeadm/certificates.go | 10 +++++----- internal/resources/api_server_certificate.go | 1 + 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/api/v1alpha1/tenantcontrolplane_funcs.go b/api/v1alpha1/tenantcontrolplane_funcs.go index 4b81ddd1..bd30a962 100644 --- a/api/v1alpha1/tenantcontrolplane_funcs.go +++ b/api/v1alpha1/tenantcontrolplane_funcs.go @@ -92,5 +92,6 @@ func getLoadBalancerAddress(ingress []corev1.LoadBalancerIngress) (string, error return "", fmt.Errorf("hostname not supported for LoadBalancer ingress: use static IP instead") } } + return "", kamajierrors.MissingValidIPError{} } diff --git a/internal/kubeadm/certificates.go b/internal/kubeadm/certificates.go index 74bca7f6..bdb151b5 100644 --- a/internal/kubeadm/certificates.go +++ b/internal/kubeadm/certificates.go @@ -46,26 +46,26 @@ func GenerateCertificatePrivateKeyPair(baseName string, config *Configuration, c certificate, err := cryptoKamaji.ParseCertificateBytes(ca.Certificate) if err != nil { - return nil, fmt.Errorf("failed to parse CA certificate: %v", err) + return nil, fmt.Errorf("failed to parse CA certificate: %w", err) } signer, err := cryptoKamaji.ParsePrivateKeyBytes(ca.PrivateKey) if err != nil { - return nil, fmt.Errorf("failed to parse CA private key: %v", err) + return nil, fmt.Errorf("failed to parse CA private key: %w", err) } kubeadmCert, err := getKubeadmCert(baseName) if err != nil { - return nil, fmt.Errorf("failed to get kubeadm cert: %v", err) + return nil, fmt.Errorf("failed to get kubeadm cert: %w", err) } if err = initPhaseFromCA(kubeadmCert, config, certificate, signer); err != nil { - return nil, fmt.Errorf("failed to initialize phase from CA: %v", err) + return nil, fmt.Errorf("failed to initialize phase from CA: %w", err) } contents, err := readCertificateFiles(baseName, config.InitConfiguration.CertificatesDir, "crt", "key") if err != nil { - return nil, fmt.Errorf("failed to read certificate files: %v", err) + return nil, fmt.Errorf("failed to read certificate files: %w", err) } if len(contents) != 2 { diff --git a/internal/resources/api_server_certificate.go b/internal/resources/api_server_certificate.go index c6b18fad..076f50f0 100644 --- a/internal/resources/api_server_certificate.go +++ b/internal/resources/api_server_certificate.go @@ -129,6 +129,7 @@ func (r *APIServerCertificate) mutate(ctx context.Context, tenantControlPlane *k config, err := getStoredKubeadmConfiguration(ctx, r.Client, r.TmpDirectory, tenantControlPlane) if err != nil { logger.Error(err, "cannot generate certificate and private key in api server certificate", "details", err.Error()) + return fmt.Errorf("failed to generate certificate and private key: %w", err) }