From 6abe75c30c58a00de09b0cdca921202c0a9bd01a Mon Sep 17 00:00:00 2001 From: zerospiel Date: Thu, 19 Dec 2024 16:41:59 +0100 Subject: [PATCH] Fix numerous issues * fix comment/naming (closes #754) * comment isNonMajor, provide examples (closes #753) * simplifie VerifyAPI (closes #758) * enhance helmSpec String symbol (closes #756) * simplified makeClientFromSecret (closes #762) * renamed ArtifactReady and commented out (closes #764) * added const for SA NS path in CurrentNamespace symbol (closes #772) * isTemplateChainValid symbol: advice maps with the len (closes #774) * validateReleaseWithValues symbol: simplify (closes #776) * fix typo in test node name (closes #783) * call linter from a single make target (closes #796) --- Makefile | 10 +++++----- api/v1alpha1/common.go | 5 +++-- api/v1alpha1/compatibility_contract.go | 2 ++ api/v1alpha1/compatibility_contract_test.go | 20 ++++++++++++++++++- api/v1alpha1/management_types.go | 2 +- api/v1alpha1/templates_common.go | 12 +++++++++-- internal/certmanager/verifier.go | 7 ++----- .../accessmanagement_controller_test.go | 2 +- .../controller/managedcluster_controller.go | 6 ++---- internal/controller/template_controller.go | 2 +- internal/credspropagation/common.go | 6 +----- internal/helm/chart.go | 4 +++- internal/utils/kube.go | 8 +++++++- internal/webhook/management_webhook_test.go | 2 +- internal/webhook/templatechain_webhook.go | 4 ++-- 15 files changed, 60 insertions(+), 32 deletions(-) diff --git a/Makefile b/Makefile index 561d85d38..4b13c28d0 100644 --- a/Makefile +++ b/Makefile @@ -105,7 +105,7 @@ tidy: go mod tidy .PHONY: test -test: generate-all fmt vet envtest tidy external-crd ## Run tests. +test: generate-all envtest tidy external-crd ## Run tests. KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test $$(go list ./... | grep -v /e2e) -coverprofile cover.out # Utilize Kind or modify the e2e tests to load the image locally, enabling @@ -118,11 +118,11 @@ test-e2e: cli-install KIND_CLUSTER_NAME="hmc-test" KIND_VERSION=$(KIND_VERSION) go test ./test/e2e/ -v -ginkgo.v -ginkgo.timeout=3h -timeout=3h $$ginkgo_label_flag .PHONY: lint -lint: golangci-lint ## Run golangci-lint linter & yamllint +lint: golangci-lint fmt vet ## Run golangci-lint linter & yamllint @$(GOLANGCI_LINT) run --timeout=$(GOLANGCI_LINT_TIMEOUT) .PHONY: lint-fix -lint-fix: golangci-lint ## Run golangci-lint linter and perform fixes +lint-fix: golangci-lint fmt vet ## Run golangci-lint linter and perform fixes @$(GOLANGCI_LINT) run --fix .PHONY: add-license @@ -180,11 +180,11 @@ LD_FLAGS += -X github.com/Mirantis/hmc/internal/build.Version=$(VERSION) LD_FLAGS += -X github.com/Mirantis/hmc/internal/telemetry.segmentToken=$(SEGMENT_TOKEN) .PHONY: build -build: generate-all fmt vet ## Build manager binary. +build: generate-all ## Build manager binary. go build -ldflags="${LD_FLAGS}" -o bin/manager cmd/main.go .PHONY: run -run: generate-all fmt vet ## Run a controller from your host. +run: generate-all ## Run a controller from your host. go run ./cmd/main.go # If you wish to build the manager image targeting other platforms you can use the --platform flag. diff --git a/api/v1alpha1/common.go b/api/v1alpha1/common.go index 16f3ef3e4..fdcfef653 100644 --- a/api/v1alpha1/common.go +++ b/api/v1alpha1/common.go @@ -43,9 +43,10 @@ type ( const ( // Provider CAPA - ProviderCAPAName = "cluster-api-provider-aws" + ProviderAWSName = "cluster-api-provider-aws" // Provider Azure - ProviderAzureName = "cluster-api-provider-azure" + ProviderAzureName = "cluster-api-provider-azure" + // Provider vSphere ProviderVSphereName = "cluster-api-provider-vsphere" // Provider K0smotron ProviderK0smotronName = "k0smotron" diff --git a/api/v1alpha1/compatibility_contract.go b/api/v1alpha1/compatibility_contract.go index 632ebf913..33eb35b5f 100644 --- a/api/v1alpha1/compatibility_contract.go +++ b/api/v1alpha1/compatibility_contract.go @@ -61,6 +61,8 @@ func isCAPIContractSingleVersion(version string) bool { return err == nil } +// isNonMajor checks is a given version with "alpha" or "beta" version prefix +// is a CAPI non-major version. Expects only ASCII chars, otherwise will return false (expected). func isNonMajor(version, prefix string, prefixIdx int) bool { majorVer := version[:prefixIdx] prefixedVer := version[prefixIdx+len(prefix):] diff --git a/api/v1alpha1/compatibility_contract_test.go b/api/v1alpha1/compatibility_contract_test.go index 3bd889e6d..27b210c17 100644 --- a/api/v1alpha1/compatibility_contract_test.go +++ b/api/v1alpha1/compatibility_contract_test.go @@ -14,7 +14,10 @@ package v1alpha1 -import "testing" +import ( + "fmt" + "testing" +) func Test_isCAPIContractVersion(t *testing.T) { tests := []struct { @@ -34,6 +37,8 @@ func Test_isCAPIContractVersion(t *testing.T) { {"v1alpha", false}, {"v1beta", false}, {"v1alpha1beta1", false}, + {"vNONSENSEalpha1beta1", false}, + {"v©", false}, } for _, test := range tests { @@ -44,6 +49,19 @@ func Test_isCAPIContractVersion(t *testing.T) { } } +func Example_isNonMajor() { + _, _ = fmt.Printf("isNonMajor(\"1alpha1\", \"alpha\", 1): %v\n", isNonMajor("1alpha1", "alpha", 1)) + _, _ = fmt.Printf("isNonMajor(\"1beta1\", \"beta\", 1): %v\n", isNonMajor("1beta1", "beta", 1)) + _, _ = fmt.Printf("isNonMajor(\"NONSENSEbeta1\", \"beta\", 8): %v\n", isNonMajor("NONSENSEbeta1", "beta", 8)) + _, _ = fmt.Printf("isNonMajor(\"beta1\", \"beta\", 1): %v\n", isNonMajor("beta1", "beta", 1)) + + // Output: + // isNonMajor("1alpha1", "alpha", 1): true + // isNonMajor("1beta1", "beta", 1): true + // isNonMajor("NONSENSEbeta1", "beta", 8): false + // isNonMajor("beta1", "beta", 1): false +} + func Test_isCAPIContractSingleVersion(t *testing.T) { tests := []struct { version string diff --git a/api/v1alpha1/management_types.go b/api/v1alpha1/management_types.go index 850450c35..542a0def5 100644 --- a/api/v1alpha1/management_types.go +++ b/api/v1alpha1/management_types.go @@ -100,7 +100,7 @@ func (in *Component) HelmValues() (values map[string]any, err error) { func GetDefaultProviders() []Provider { return []Provider{ {Name: ProviderK0smotronName}, - {Name: ProviderCAPAName}, + {Name: ProviderAWSName}, {Name: ProviderAzureName}, {Name: ProviderVSphereName}, {Name: ProviderSveltosName}, diff --git a/api/v1alpha1/templates_common.go b/api/v1alpha1/templates_common.go index 8e457ee17..8cead596a 100644 --- a/api/v1alpha1/templates_common.go +++ b/api/v1alpha1/templates_common.go @@ -54,10 +54,18 @@ type HelmSpec struct { func (s *HelmSpec) String() string { if s.ChartRef != nil { - return s.ChartRef.Namespace + "/" + s.ChartRef.Name + ", Kind=" + s.ChartRef.Kind + if s.ChartRef.Namespace != "" { + return s.ChartRef.Namespace + "/" + s.ChartRef.Name + ", Kind=" + s.ChartRef.Kind + } + + return s.ChartRef.Name + ", Kind=" + s.ChartRef.Kind + } + + if s.ChartSpec.Version != "" { + return s.ChartSpec.Chart + ": " + s.ChartSpec.Version } - return s.ChartSpec.Chart + ": " + s.ChartSpec.Version + return s.ChartSpec.Chart } // TemplateStatusCommon defines the observed state of Template common for all Template types diff --git a/internal/certmanager/verifier.go b/internal/certmanager/verifier.go index 30e58b644..85290dd9d 100644 --- a/internal/certmanager/verifier.go +++ b/internal/certmanager/verifier.go @@ -26,9 +26,6 @@ func VerifyAPI(ctx context.Context, restcfg *rest.Config, namespace string) erro if err != nil { return err } - err = checker.Check(ctx) - if err != nil { - return err - } - return nil + + return checker.Check(ctx) } diff --git a/internal/controller/accessmanagement_controller_test.go b/internal/controller/accessmanagement_controller_test.go index 377a8d5a8..d53328e35 100644 --- a/internal/controller/accessmanagement_controller_test.go +++ b/internal/controller/accessmanagement_controller_test.go @@ -163,7 +163,7 @@ var _ = Describe("Template Management Controller", func() { Expect(k8sClient.Create(ctx, am)).To(Succeed()) } - By("creating custom resources for the Kind ClusterTemplateChain, ServiceTemplateChain amd Credentials") + By("creating custom resources for the Kind ClusterTemplateChain, ServiceTemplateChain and Credentials") for _, obj := range []crclient.Object{ ctChain, ctChainToDelete, ctChainUnmanaged, stChain, stChainToDelete, stChainUnmanaged, diff --git a/internal/controller/managedcluster_controller.go b/internal/controller/managedcluster_controller.go index ffb983dc0..c89fdb1cc 100644 --- a/internal/controller/managedcluster_controller.go +++ b/internal/controller/managedcluster_controller.go @@ -495,11 +495,9 @@ func validateReleaseWithValues(ctx context.Context, actionConfig *action.Configu if err != nil { return err } + _, err = install.RunWithContext(ctx, hcChart, vals) - if err != nil { - return err - } - return nil + return err } // updateStatus updates the status for the ManagedCluster object. diff --git a/internal/controller/template_controller.go b/internal/controller/template_controller.go index 0dc3fd252..a4738b42d 100644 --- a/internal/controller/template_controller.go +++ b/internal/controller/template_controller.go @@ -216,7 +216,7 @@ func (r *TemplateReconciler) ReconcileTemplate(ctx context.Context, template tem } status.ChartVersion = hcChart.Spec.Version - if reportStatus, err := helm.ArtifactReady(hcChart); err != nil { + if reportStatus, err := helm.ShouldReportStatusOnArtifactReadiness(hcChart); err != nil { l.Info("HelmChart Artifact is not ready") if reportStatus { _ = r.updateStatus(ctx, template, err.Error()) diff --git a/internal/credspropagation/common.go b/internal/credspropagation/common.go index 9d72e5759..01529f480 100644 --- a/internal/credspropagation/common.go +++ b/internal/credspropagation/common.go @@ -86,11 +86,7 @@ func makeClientFromSecret(kubeconfSecret *corev1.Secret) (client.Client, error) if err != nil { return nil, err } - cl, err := client.New(restConfig, client.Options{ + return client.New(restConfig, client.Options{ Scheme: scheme, }) - if err != nil { - return nil, err - } - return cl, nil } diff --git a/internal/helm/chart.go b/internal/helm/chart.go index 5a9543b82..d3be58f95 100644 --- a/internal/helm/chart.go +++ b/internal/helm/chart.go @@ -22,7 +22,9 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func ArtifactReady(chart *sourcev1.HelmChart) (reportStatus bool, _ error) { +// ShouldReportStatusOnArtifactReadiness checks whether an artifact for the given chart is ready, +// returns error and the flags, signaling if the caller should report the status. +func ShouldReportStatusOnArtifactReadiness(chart *sourcev1.HelmChart) (bool, error) { for _, c := range chart.Status.Conditions { if c.Type == "Ready" { if chart.Generation != c.ObservedGeneration { diff --git a/internal/utils/kube.go b/internal/utils/kube.go index 54ad37a15..052d5d482 100644 --- a/internal/utils/kube.go +++ b/internal/utils/kube.go @@ -49,14 +49,20 @@ func EnsureDeleteAllOf(ctx context.Context, cl client.Client, gvk schema.GroupVe } func CurrentNamespace() string { + // Referencing https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/clientcmd/client_config.go#L631-L646 + // for simplicity + ns, found := os.LookupEnv("POD_NAMESPACE") if found { return ns } - nsb, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace") + + const serviceAccountNs = "/var/run/secrets/kubernetes.io/serviceaccount/namespace" + nsb, err := os.ReadFile(serviceAccountNs) if err == nil && len(nsb) > 0 { return string(nsb) } + return DefaultSystemNamespace } diff --git a/internal/webhook/management_webhook_test.go b/internal/webhook/management_webhook_test.go index 054bed6ec..d79c8d6a2 100644 --- a/internal/webhook/management_webhook_test.go +++ b/internal/webhook/management_webhook_test.go @@ -106,7 +106,7 @@ func TestManagementValidateUpdate(t *testing.T) { validStatus := v1alpha1.TemplateValidationStatus{Valid: true} componentAwsDefaultTpl := v1alpha1.Provider{ - Name: v1alpha1.ProviderCAPAName, + Name: v1alpha1.ProviderAWSName, Component: v1alpha1.Component{ Template: template.DefaultName, }, diff --git a/internal/webhook/templatechain_webhook.go b/internal/webhook/templatechain_webhook.go index 3611ca32e..f202780dc 100644 --- a/internal/webhook/templatechain_webhook.go +++ b/internal/webhook/templatechain_webhook.go @@ -125,8 +125,8 @@ func (*ServiceTemplateChainValidator) Default(_ context.Context, _ runtime.Objec } func isTemplateChainValid(spec v1alpha1.TemplateChainSpec) admission.Warnings { - supportedTemplates := make(map[string]bool) - availableForUpgrade := make(map[string]bool) + supportedTemplates := make(map[string]bool, len(spec.SupportedTemplates)) + availableForUpgrade := make(map[string]bool, len(spec.SupportedTemplates)) for _, supportedTemplate := range spec.SupportedTemplates { supportedTemplates[supportedTemplate.Name] = true for _, template := range supportedTemplate.AvailableUpgrades {