diff --git a/install.sh b/install.sh index ed65e6a..319ff4e 100755 --- a/install.sh +++ b/install.sh @@ -16,7 +16,7 @@ set -eu # tag that disagrees with it. Pinned rather than resolved from the GitHub # API at runtime: the unauthenticated API allows 60 requests per hour per IP, # which one NATed customer site can exhaust between two engineers. -VERSION="${BUDCTL_VERSION:-0.3.1}" +VERSION="${BUDCTL_VERSION:-0.3.2}" REPO="BudEcosystem/budctl" die() { echo "install.sh: $*" >&2; exit 1; } diff --git a/internal/checks/components.go b/internal/checks/components.go index 532a32b..1f59948 100644 --- a/internal/checks/components.go +++ b/internal/checks/components.go @@ -112,16 +112,22 @@ func init() { engine.Register(&engine.Check{ ID: "components.ingress", Group: "components", Severity: engine.Block, - // DependsOn platform: OpenShift has no IngressClass to find — the - // Ingress Operator owns the path — and reporting a vanilla-only - // sub-check as PASS there (or the reverse) is the exact wrongness - // FRD-020 §5.0 exists to prevent. - DependsOn: []string{"platform"}, + // DependsOn platform: on OpenShift the Ingress Operator owns the path and + // its router serves the openshift-default IngressClass, so the health + // question is asked of the operator rather than of a controller Service — + // reporting a vanilla-only sub-check as PASS there (or the reverse) is the + // exact wrongness FRD-020 §5.0 exists to prevent. DependsOn config: the + // class the chart's Ingresses request comes from the render, and + // `--only components` must still produce one. + DependsOn: []string{"platform", "config"}, Run: func(ctx context.Context, c *engine.Ctx) engine.Result { ch := engine.Lookup("components.ingress") if c.Kube == nil { return ch.Skip("cluster unreachable: the ingress path was not inspected") } + if unserved := componentUnservedIngressObjects(ctx, c); len(unserved) > 0 { + return componentUnservedIngressResult(ctx, c, ch, unserved) + } if c.Platform.IsOpenShift() { return openShiftIngress(ctx, c, ch) } @@ -369,10 +375,125 @@ func openShiftIngress(ctx context.Context, c *engine.Ctx, ch *engine.Check) engi } } + // A healthy router serves only the Ingresses that name its class. The Bud + // charts default to className traefik, and an Ingress naming a class this + // cluster does not have is turned into a Route by nothing — the install + // completes and not one hostname answers. + controllers := map[string]string{} + routerClasses, inventory := []string{}, []string{} + for _, cl := range c.Kube.List(ctx, "ingressclasses.networking.k8s.io", "") { + ctrl := cl.DigString("spec", "controller") + controllers[cl.Name()] = ctrl + inventory = append(inventory, fmt.Sprintf("%s (%s)", cl.Name(), ctrl)) + if ctrl == openShiftIngressToRoute { + routerClasses = append(routerClasses, cl.Name()) + } + } + routerClasses = Sorted(routerClasses) + ev.Output += "\ningressclasses: " + firstNonEmpty(strings.Join(Sorted(inventory), ", "), "none") + useClass := "openshift-default" + if len(routerClasses) > 0 { + useClass = routerClasses[0] + } + + detail := []string{} + requested := componentRequestedIngressClasses(c) + switch { + case len(requested) > 0: + var missing, foreign, served []string + for _, want := range requested { + ctrl, ok := controllers[want] + switch { + case !ok: + missing = append(missing, want) + case ctrl != openShiftIngressToRoute: + foreign = append(foreign, fmt.Sprintf("%s (%s)", want, ctrl)) + default: + served = append(served, want) + } + } + if len(missing) > 0 { + return ch.Fail( + fmt.Sprintf("the chart's Ingresses request IngressClass %s, which does not exist on this cluster: the OpenShift router turns an Ingress into a Route only for its own class, so those hostnames are never served", + stQuoteList(missing)), + fmt.Sprintf("set ingress.className: %s in the values of every chart that publishes an Ingress (bud, keycloak, seaweedfs, argocd), and ingress.isTraefik: false so no Traefik objects are rendered", useClass), + "IngressClasses on this cluster: "+firstNonEmpty(strings.Join(Sorted(inventory), ", "), "none"), + ).WithEvidence(ev) + } + if len(foreign) > 0 { + return ch.FailAs(engine.Risk, + fmt.Sprintf("the chart's Ingresses request %s, served by a controller other than the OpenShift router: this check verified the router, not that controller, so those hostnames are unverified", + strings.Join(foreign, ", ")), + fmt.Sprintf("set ingress.className: %s so the router serves them, or confirm that controller is running and exposed on 80 and 443", useClass), + ).WithEvidence(ev) + } + detail = append(detail, fmt.Sprintf("the chart's Ingresses request %s, which the OpenShift router serves (%s)", strings.Join(served, ", "), openShiftIngressToRoute)) + case len(Rendered(c)) == 0: + why := "no --values" + if c.Opts.ChartDir != "" || len(c.Opts.ValuesFiles) > 0 { + why = "the chart did not render with the supplied values (config.render carries the error)" + } + detail = append(detail, fmt.Sprintf("%s: the class the chart's Ingresses request was not compared with this cluster's IngressClasses. The Bud charts default to ingress.className: traefik and ingress.isTraefik: true; on OpenShift set ingress.className: %s and ingress.isTraefik: false", why, useClass)) + } + return ch.Pass( fmt.Sprintf("the OpenShift Ingress Operator is Available and the default IngressController serves %d %s on *.%s", replicas, Plural(replicas, "replica", "replicas"), domain), + detail..., ).WithEvidence(ev). - Bounds("Route admission is not evaluated: the IngressController's domain and any routeSelector decide whether a given hostname is admitted, and `domains.*` checks DNS and the inbound path rather than admission policy.") + Bounds("Route admission is not evaluated: the IngressController's domain and any routeSelector decide whether a given hostname is admitted, and `domains.*` checks DNS and the inbound path rather than admission policy. Only the chart passed with --chart is rendered, so the class keycloak, seaweedfs and argocd request is not compared.") +} + +// openShiftIngressToRoute is the controller behind OpenShift's own IngressClass: +// the component that turns an Ingress into a Route the router serves. +const openShiftIngressToRoute = "openshift.io/ingress-to-route" + +// componentUnservedIngressObjects lists the rendered objects that belong to +// Traefik's own API when this cluster does not serve it. The Bud charts render +// Traefik Middlewares unless ingress.isTraefik is false, and on a cluster without +// Traefik's CRDs — OpenShift, or any cluster on ingress-nginx — the sync stops at +// the first one with "no matches for kind Middleware". +func componentUnservedIngressObjects(ctx context.Context, c *engine.Ctx) []string { + out := []string{} + for _, o := range Rendered(c) { + av := o.APIVersion() + group, _, _ := strings.Cut(av, "/") + if group != "traefik.io" && group != "traefik.containo.us" { + continue + } + if c.Kube.HasAPIVersion(ctx, av) { + continue + } + out = append(out, fmt.Sprintf("%s/%s (%s)", o.Kind(), o.Name(), av)) + } + return Sorted(out) +} + +func componentUnservedIngressResult(ctx context.Context, c *engine.Ctx, ch *engine.Check, unserved []string) engine.Result { + className := "a class this cluster has" + if c.Platform.IsOpenShift() { + className = "openshift-default" + } else if names := componentClassNames(componentClassStatesByName(ctx, c)); len(names) > 0 { + className = "one of " + strings.Join(names, ", ") + } + return ch.Fail( + fmt.Sprintf("the chart renders %d Traefik %s, but this cluster serves no Traefik API: the sync stops at the first one with \"no matches for kind\", so the Application never finishes applying", + len(unserved), Plural(len(unserved), "object", "objects")), + fmt.Sprintf("set ingress.isTraefik: false in the values of every chart that publishes an Ingress (bud, keycloak, seaweedfs, argocd), and ingress.className to %s", className), + unserved..., + ).WithEvidence(engine.Evidence{ + What: "rendered objects in the traefik.io / traefik.containo.us API groups, against the API versions this cluster serves", + Output: strings.Join(unserved, "\n"), + }) +} + +// componentClassStatesByName is the IngressClass inventory without the endpoint +// matching, for remedies that only need the names. +func componentClassStatesByName(ctx context.Context, c *engine.Ctx) []componentClassState { + out := []componentClassState{} + for _, cl := range c.Kube.List(ctx, "ingressclasses.networking.k8s.io", "") { + out = append(out, componentClassState{name: cl.Name(), controller: cl.DigString("spec", "controller")}) + } + return out } func vanillaIngress(ctx context.Context, c *engine.Ctx, ch *engine.Check) engine.Result { @@ -382,7 +503,7 @@ func vanillaIngress(ctx context.Context, c *engine.Ctx, ch *engine.Check) engine "no IngressClass exists: every Ingress the install creates stays unclaimed, so not one hostname the chart publishes is ever reachable", "install an ingress controller before applying the ApplicationSets — neither of them provides one. Any controller is acceptable: "+ "`helm install ingress-nginx ingress-nginx/ingress-nginx -n ingress-nginx --create-namespace`, k3s's bundled Traefik, or the cloud provider's. "+ - "Then set global.ingress.className to its class, or mark the class default with the `ingressclass.kubernetes.io/is-default-class: \"true\"` annotation.", + "Then set ingress.className to its class, or mark the class default with the `ingressclass.kubernetes.io/is-default-class: \"true\"` annotation.", ).WithEvidence(engine.Evidence{What: "kubectl get ingressclass", Output: "No resources found"}) } @@ -434,7 +555,7 @@ func vanillaIngress(ctx context.Context, c *engine.Ctx, ch *engine.Check) engine if !ok { return ch.Fail( fmt.Sprintf("the chart's Ingresses request IngressClass %q, which does not exist in this cluster: those Ingresses are claimed by no controller and none of their hostnames answers", want), - fmt.Sprintf("either set global.ingress.className in your values to a class that exists (%s), or install the controller that provides %q", strings.Join(componentClassNames(states), ", "), want), + fmt.Sprintf("either set ingress.className in your values to a class that exists (%s), or install the controller that provides %q", strings.Join(componentClassNames(states), ", "), want), ).WithEvidence(ev) } scope = append(scope, st) @@ -483,7 +604,7 @@ func vanillaIngress(ctx context.Context, c *engine.Ctx, ch *engine.Check) engine detail = append(detail, "other classes with no ready controller: "+strings.Join(controllerDown, "; ")) } if !componentAnyDefault(states) && len(requested) == 0 { - detail = append(detail, "no IngressClass is marked default; set global.ingress.className in values so the chart's Ingresses name one explicitly") + detail = append(detail, "no IngressClass is marked default; set ingress.className in values so the chart's Ingresses name one explicitly") } return ch.Pass( fmt.Sprintf("%s %s a controller with ready endpoints: %s", diff --git a/internal/checks/components_test.go b/internal/checks/components_test.go index 60163a5..299a810 100644 --- a/internal/checks/components_test.go +++ b/internal/checks/components_test.go @@ -768,8 +768,9 @@ func TestComponentsIngressOpenShiftPublishesRouterAddress(t *testing.T) { } } -// An OpenShift cluster must never be judged by IngressClass: seeding a dead one -// alongside a healthy operator must not change the verdict. +// An OpenShift cluster is not judged by an IngressClass the chart does not +// request: seeding a dead one alongside a healthy operator must not change the +// verdict. func TestComponentsIngressOpenShiftIgnoresIngressClasses(t *testing.T) { f := openShift(). with("clusteroperators.config.openshift.io", "", componentsClusterOperator("ingress", true, false)). @@ -780,6 +781,133 @@ func TestComponentsIngressOpenShiftIgnoresIngressClasses(t *testing.T) { assertStatus(t, run(t, f, "components.ingress"), "PASS") } +// componentsOpenShiftRouter is a healthy OpenShift ingress path with the +// IngressClass the Ingress Operator creates for its router. +func componentsOpenShiftRouter() *fakeCluster { + return openShift(). + with("clusteroperators.config.openshift.io", "", componentsClusterOperator("ingress", true, false)). + with("ingresscontrollers.operator.openshift.io", "openshift-ingress-operator", + ingressController("default", "apps.ocp.example.com", 1)). + with("ingressclasses.networking.k8s.io", "", ingressClass("openshift-default", openShiftIngressToRoute)) +} + +func componentsTraefikMiddleware(name string) adapters.Object { + return adapters.Object{ + "apiVersion": "traefik.io/v1alpha1", "kind": "Middleware", + "metadata": map[string]any{"name": name, "namespace": "bud"}, + "spec": map[string]any{"redirectScheme": map[string]any{"scheme": "https"}}, + } +} + +// The Bud charts default to className traefik. On OpenShift that Ingress is +// turned into a Route by nothing, so a perfectly healthy router serves none of +// the chart's hostnames — and the operator check alone called this PASS. +func TestComponentsIngressOpenShiftBlocksWhenChartRequestsAClassTheRouterDoesNotServe(t *testing.T) { + r, _ := componentsRun(t, componentsOpenShiftRouter(), "components.ingress", func(c *engine.Ctx) { + c.Set(engine.KeyRenderedObjects, []adapters.Object{componentsIngressObject("bud-bud", "traefik", false)}) + }) + assertStatus(t, r, "BLOCK") + componentsAssertSummaryHas(t, r, `"traefik"`) + for _, want := range []string{"ingress.className: openshift-default", "ingress.isTraefik: false"} { + if !strings.Contains(r.Remedy, want) { + t.Fatalf("remedy does not name %q: %s", want, r.Remedy) + } + } +} + +// The tcs openshift values: className openshift-default, served by the router. +func TestComponentsIngressOpenShiftPassesWhenChartRequestsTheRouterClass(t *testing.T) { + r, _ := componentsRun(t, componentsOpenShiftRouter(), "components.ingress", func(c *engine.Ctx) { + c.Set(engine.KeyRenderedObjects, []adapters.Object{ + componentsIngressObject("bud-bud", "openshift-default", false), + componentsIngressObject("bud-novu", "openshift-default", false), + }) + }) + assertStatus(t, r, "PASS") + componentsAssertDetailHas(t, r, "request openshift-default, which the OpenShift router serves") +} + +// A class served by a controller installed beside the router exists, but the +// router's health says nothing about it: unverified, not passed. +func TestComponentsIngressOpenShiftRisksOnAClassServedByAnotherController(t *testing.T) { + f := componentsOpenShiftRouter(). + with("ingressclasses.networking.k8s.io", "", + ingressClass("openshift-default", openShiftIngressToRoute), + ingressClass("nginx", "k8s.io/ingress-nginx")) + r, _ := componentsRun(t, f, "components.ingress", func(c *engine.Ctx) { + c.Set(engine.KeyRenderedObjects, []adapters.Object{componentsIngressObject("bud-bud", "nginx", false)}) + }) + assertStatus(t, r, "RISK") + componentsAssertSummaryHas(t, r, "k8s.io/ingress-nginx") +} + +// Without --values the class cannot be compared, and the pass has to say what +// the values must set rather than imply the chart defaults will work. +func TestComponentsIngressOpenShiftPassWithoutValuesNamesTheClassToSet(t *testing.T) { + r := run(t, componentsOpenShiftRouter(), "components.ingress") + assertStatus(t, r, "PASS") + componentsAssertDetailHas(t, r, "ingress.className: openshift-default and ingress.isTraefik: false") +} + +// --values given but the chart refused to render: "no --values" would send the +// operator looking for a flag they already passed. +func TestComponentsIngressOpenShiftSaysTheRenderFailedWhenValuesWereGiven(t *testing.T) { + f := componentsOpenShiftRouter().withOpts(func(o *engine.Options) { + o.ChartDir = "infra/charts/bud" + o.ValuesFiles = []string{"values.openshift.yaml"} + }) + r := run(t, f, "components.ingress") + assertStatus(t, r, "PASS") + componentsAssertDetailHas(t, r, "the chart did not render with the supplied values") +} + +// ingress.isTraefik defaults to true, so the charts render Traefik Middlewares. +// Wherever Traefik's CRDs are not installed — OpenShift, or ingress-nginx — the +// sync stops at the first one, before a single hostname is published. +func TestComponentsIngressBlocksOnTraefikObjectsTheClusterDoesNotServe(t *testing.T) { + cases := []struct { + name string + cluster *fakeCluster + class string + className string + }{ + {"openshift", componentsOpenShiftRouter(), "openshift-default", "openshift-default"}, + {"vanilla on ingress-nginx", componentsHealthyBase(), "nginx", "nginx"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + r, _ := componentsRun(t, tc.cluster, "components.ingress", func(c *engine.Ctx) { + c.Set(engine.KeyRenderedObjects, []adapters.Object{ + componentsTraefikMiddleware("bud-https-redirect"), + componentsIngressObject("bud-bud", tc.class, false), + }) + }) + assertStatus(t, r, "BLOCK") + componentsAssertSummaryHas(t, r, "Traefik") + componentsAssertDetailHas(t, r, "Middleware/bud-https-redirect (traefik.io/v1alpha1)") + if !strings.Contains(r.Remedy, "ingress.isTraefik: false") || !strings.Contains(r.Remedy, tc.className) { + t.Fatalf("remedy must turn Traefik objects off and name a class this cluster has (%s): %s", tc.className, r.Remedy) + } + }) + } +} + +// Where Traefik's API is served, its Middlewares are exactly what the chart +// needs — they must not be reported. +func TestComponentsIngressAcceptsTraefikObjectsWhereTraefikIsServed(t *testing.T) { + f := componentsHealthyBase(). + with("ingressclasses.networking.k8s.io", "", componentsDefaultIngressClass("traefik", "traefik.io/ingress-controller")). + with("endpointslices.discovery.k8s.io", "", componentsEndpointSlice("kube-system", "traefik", 2, 0)) + f.apiGroups["traefik.io/v1alpha1"] = true + r, _ := componentsRun(t, f, "components.ingress", func(c *engine.Ctx) { + c.Set(engine.KeyRenderedObjects, []adapters.Object{ + componentsTraefikMiddleware("bud-https-redirect"), + componentsIngressObject("bud-bud", "traefik", false), + }) + }) + assertStatus(t, r, "PASS") +} + // --------------------------------------------------------------------------- // components.metrics-server // ---------------------------------------------------------------------------