PMREQ-821: Whisker access via Calico Ingress Gateway - #5146
Conversation
dbb9669 to
16a3367
Compare
16a3367 to
eddfec4
Compare
There was a problem hiding this comment.
Pull request overview
Adds Calico Ingress Gateway (CIG) exposure support for the Whisker UI by introducing a spec.ingressGateway configuration on the Whisker CR and reusing/refactoring the existing Manager gateway implementation into shared controller logic (pkg/controller/uigateway) and enhanced gateway rendering (pkg/render/gateway).
Changes:
- Add
spec.ingressGatewayto the Whisker API/CRD and reconcile/render Gateway API resources (Gateway/HTTPRoute/Backend/ReferenceGrant/TLS Secret) when configured. - Extract shared UI-gateway controller behaviors (watches, cleanup, namespace provisioning, class resolution, health read-back) into
pkg/controller/uigatewayand wire Manager/Whisker controllers to use it. - Extend gateway rendering to support configurable HTTPRoute request timeouts and introduce namespace-scoped “writer” RBAC to confine Gateway API write verbs to configured namespaces.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/render/whisker/component.go | Adds gateway-aware ingress rule to Whisker NetworkPolicy when CIG is configured. |
| pkg/render/whisker/component_test.go | Tests NetworkPolicy behavior with/without ingress gateway namespace. |
| pkg/render/gateway/component.go | Adds route request timeout support, writer RBAC objects, and adjusts render/delete ordering and variant behavior. |
| pkg/render/gateway/component_test.go | Updates/extends gateway render & deletion tests for writer RBAC, NP behavior, and timeouts. |
| pkg/imports/crds/operator/operator.tigera.io_whiskers.yaml | Adds spec.ingressGateway schema to Whisker CRD. |
| pkg/imports/crds/operator/operator.tigera.io_managers.yaml | Aligns IngressGatewaySpec docs to “degrades until specified” behavior. |
| pkg/controller/whisker/controller.go | Implements Whisker ingress gateway reconciliation, cleanup, TLS minting, and health gating. |
| pkg/controller/whisker/controller_test.go | Adds reconciliation tests for gateway resources, TLS persistence, unhealthy requeue, and variant gating. |
| pkg/controller/uigateway/uigateway.go | New shared helper for gateway cleanup discovery, class resolution, namespace creation, watch setup, and health read-back. |
| pkg/controller/uigateway/uigateway_test.go | Unit tests for shared gateway health and cleanup helper behaviors. |
| pkg/controller/uigateway/uigateway_suite_test.go | New Ginkgo suite wiring for uigateway tests. |
| pkg/controller/manager/manager_controller.go | Refactors Manager gateway watch/cleanup/health logic to use uigateway. |
| pkg/controller/manager/manager_controller_test.go | Adds/updates test coverage for gateway missing-GatewayAPI degrade behavior. |
| pkg/controller/manager/gateway_status_test.go | Removes Manager-specific gateway status tests now covered by shared uigateway tests. |
| pkg/controller/gatewayapi/gatewayapi_controller.go | Ensures operator-secrets RoleBinding is written in gateway namespaces on both variants. |
| pkg/controller/gatewayapi/gatewayapi_controller_test.go | Adds Calico-variant test for per-namespace bundle + operator-secrets RoleBinding (no WAF resources). |
| api/v1/whisker_types.go | Adds IngressGateway *IngressGatewaySpec to WhiskerSpec with kubebuilder optional semantics. |
| api/v1/ingress_gateway_types.go | Updates IngressGatewaySpec docs to match “component degrades” behavior. |
| api/v1/zz_generated.deepcopy.go | Regenerates deepcopy for new WhiskerSpec field. |
Files not reviewed (1)
- api/v1/zz_generated.deepcopy.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // The writer Role comes first: it is what carries the write verbs for the | ||
| // kinds below, so it has to exist before them. The first reconcile's writes | ||
| // may still be denied while the authorizer catches up; the requeue succeeds, | ||
| // the same way the TLS secret does below. | ||
| objs = append(objs, writerObjects(c.cfg.ResourcePrefix, c.cfg.GatewayNamespace)...) | ||
| if c.cfg.GatewayNamespace != c.cfg.BackendNamespace { | ||
| // The Backend and ReferenceGrant are written in the backend namespace. | ||
| objs = append(objs, writerObjects(c.cfg.ResourcePrefix, c.cfg.BackendNamespace)...) | ||
| } |
eddfec4 to
c6e0456
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- api/v1/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
pkg/imports/crds/operator/operator.tigera.io_whiskers.yaml:62
- The Whisker CRD's
spec.ingressGateway.hostnamedescription references AuthenticationmanagerDomain(and notes “Manager only”), which is confusing/unrelated for Whisker users. Since this YAML is generated, the underlying Go type comment should be updated so the generated Whisker CRD docs are component-appropriate (e.g., describe Whisker behavior only, or clearly separate Manager vs Whisker semantics), then re-run code generation to refresh this file.
hostname:
description: |-
Hostname for the Gateway listener. Must match the Authentication CR's
managerDomain when OIDC is configured (Manager only).
minLength: 1
c6e0456 to
55fdf02
Compare
55fdf02 to
9627a4a
Compare
| // Teardown returns deletion components for every labeled Gateway namespace, | ||
| // plus the backend namespace, which contains the Backend and ReferenceGrant. | ||
| // | ||
| // If no labeled Gateway exists, nothing is returned. The Gateway is rendered | ||
| // before any other gateway resources, so those resources cannot exist without | ||
| // a corresponding Gateway. This also avoids touching kinds the cluster may not | ||
| // serve: Backend is an Envoy Gateway resource, which may be unavailable when | ||
| // the Gateway API CRDs were installed independently. Attempting to delete an | ||
| // unserved kind would fail the reconcile. | ||
| func (c *Config) Teardown(ctx context.Context) ([]render.Component, error) { | ||
| namespaces, gatewayCRDsPresent, err := c.Namespaces(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if !gatewayCRDsPresent || len(namespaces) == 0 { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
This version creates the access Role and RoleBinding first. The behavior is essentially the same: RBAC needs to exist before the Gateway can be created.
I'm keeping the current behavior for this PR. Hitting this requires a failed reconcile and the user clearing spec.ingressGateway before the retry. Otherwise, the Gateway is created, cleanup finds everything by label, and re-setting the field re-adopts the resources.
Bringing back the backstop would require another cluster-wide Role/RoleBinding discovery path and additional permissions, which doesn't seem worth the complexity right now.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- api/v1/zz_generated.deepcopy.go: Generated file
Suppressed comments (2)
pkg/render/gateway/component.go:532
- In the move-cleanup path, this deletion component will also drop the writer Role/RoleBinding when the old gateway namespace equals the backend namespace (gwNS == bkNS). That contradicts the comment (“backend namespace keeps its grant”) and can leave a window where subsequent teardown (spec removal) loses the namespace-scoped write RBAC needed to delete backend/route resources.
drop := writerObjects(prefix, gwNS)
if gwNS != bkNS && !move {
drop = append(drop, writerObjects(prefix, bkNS)...)
}
pkg/render/gateway/component.go:187
- The gateway writer Role currently grants update/delete on all Gateways/HTTPRoutes/ReferenceGrants/Backends in the namespace. This is broader than necessary (it could affect user-managed Gateway API resources in that namespace) even though the operator only intends to manage its own named resources.
{
APIGroups: []string{gapi.GroupName},
Resources: []string{"gateways", "httproutes", "referencegrants"},
Verbs: []string{"create", "update", "delete"},
},
9627a4a to
d339d2a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- api/v1/zz_generated.deepcopy.go: Generated file
Suppressed comments (3)
pkg/controller/uigateway/uigateway.go:130
Teardown()assumes no component-owned resources can exist unless a labeled Gateway exists, butpkg/render/gateway.Component.Objects()now renders the writer Role/RoleBinding before the Gateway. If Gateway creation fails (e.g., Gateway CRDs missing, webhook rejection), those RBAC objects can be left behind andTeardown()will return early (len(namespaces)==0) and never clean them up.
if !gatewayCRDsPresent || len(namespaces) == 0 {
pkg/render/gateway/component.go:195
- The writer RoleBinding is rendered without the component's gateway cleanup label. Adding the same label used on the Gateway makes it possible to discover/clean up these grants even if the Gateway never gets created.
ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace},
pkg/render/gateway/component.go:173
- The writer Role is rendered without the component's gateway cleanup label. If a reconcile creates this Role but fails before creating the labeled Gateway, label-driven cleanup (via listing labeled Gateways) cannot discover the namespace later, leaving the RBAC grant behind.
This issue also appears on line 195 of the same file.
ObjectMeta: metav1.ObjectMeta{Name: WriterRoleName(resourcePrefix), Namespace: namespace},
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- api/v1/zz_generated.deepcopy.go: Generated file
Suppressed comments (2)
pkg/render/gateway/component.go:181
- The access Role grants write verbs for Gateway API resources that are not written in this namespace. In split-namespace mode (GatewayNamespace != BackendNamespace), the operator only writes Gateways/HTTPRoutes in the gateway namespace and only writes Backends/ReferenceGrants in the backend namespace, but this Role currently grants all of them in both namespaces. Narrowing the rules per-namespace would reduce blast radius while keeping the same functionality.
Rules: []rbacv1.PolicyRule{
{
APIGroups: []string{gapi.GroupName},
Resources: []string{"gateways", "httproutes", "referencegrants"},
Verbs: []string{"create", "update", "delete"},
},
{
APIGroups: []string{EnvoyGatewayGroup},
Resources: []string{"backends"},
Verbs: []string{"create", "update", "delete"},
},
pkg/controller/uigateway/uigateway.go:200
- unhealthyCondition treats a missing Accepted/Programmed/ResolvedRefs condition as healthy. That means the controller can clear Degraded once the Gateway/HTTPRoute objects exist even if the Gateway controller hasn't written conditions yet, which can contradict the documented behavior of staying Degraded until the Gateway is programmed. Consider treating missing conditions as not-ready (or updating the stated behavior/tests to match the current semantics).
// unhealthyCondition returns a message when the named condition exists and is
// not True. A missing condition is healthy: the controller has not written
// its verdict yet, and Accepted/Programmed gate readiness once it does.
func unhealthyCondition(conditions []metav1.Condition, condType, msgPrefix string) string {
for _, cond := range conditions {
if cond.Type == condType && cond.Status != metav1.ConditionTrue {
return fmt.Sprintf("%s: %s", msgPrefix, cond.Message)
}
}
return ""
Move the gateway helper logic out of the manager controller into a shared package so the Whisker controller can reuse it: label-driven namespace listing and cleanup, gateway/route health read-back, namespace provisioning, class resolution, and watch setup. The manager controller now delegates to uigateway.Config; manager-only logic (multi-tenant guard, managerDomain host check) stays in place. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds spec.ingressGateway to the Whisker CR. When set, the controller renders a Gateway, HTTPRoute, Envoy Gateway Backend and ReferenceGrant, mints the listener certificate, and reports Degraded until the Gateway is programmed. The HTTPRoute disables the request timeout so SSE flow-log streams stay open, the whisker NetworkPolicy admits only this gateway's proxy pods on 8443, and the gateway re-originates TLS to Whisker's HTTPS port against the trusted bundle. The proxy NetworkPolicy and the operator-secrets RoleBinding render on both variants: calico-system carries a default-deny on Calico too, and the operator needs secret access in a custom gateway namespace either way. Nothing is rendered on a non-Calico variant, where Whisker itself is deleted. Cleanup keys off the labelled Gateway alone, which is rendered before every other resource and deleted after them. Write access comes from a Role the operator self-grants per namespace rather than from the cluster-wide ClusterRole. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rename the per-namespace Role and RoleBinding from <prefix>-gateway-writer to <prefix>-ingressgateway-access, matching how the operator names the grants it gives its own ServiceAccount (tigera-operator-secrets, tigera-waypoint-l7-envoyfilters) rather than naming them after verbs. Label both with operator.tigera.io/gateway so they are discoverable the same way as the Gateway they exist for. Drop the unused bool from Namespaces(): both callers discarded it, since an unserved Gateway kind already yields no namespaces.
801ad81 to
c611714
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- api/v1/zz_generated.deepcopy.go: Generated file
Suppressed comments (2)
pkg/controller/uigateway/uigateway.go:123
- Teardown’s comment/behavior assumes that if there is no labeled Gateway then “those resources cannot exist without a corresponding Gateway”, so cleanup returns nothing. However, the gateway component does not label HTTPRoute/Backend/Secret/ReferenceGrant objects, so they can be left behind if a user deletes the Gateway manually and clears spec.ingressGateway before the next reconcile. In that case, Teardown will skip cleanup and orphan those resources (not just the access Role/RoleBinding). Consider adding a fallback namespace discovery mechanism (e.g., list labeled Roles/RoleBindings) and/or labeling the other rendered resources so they remain discoverable for cleanup even if the Gateway object is gone.
// If no labeled Gateway exists, nothing is returned. The Gateway is rendered
// before any other gateway resources, so those resources cannot exist without
// a corresponding Gateway. This also avoids touching kinds the cluster may not
// serve: Backend is an Envoy Gateway resource, which may be unavailable when
// the Gateway API CRDs were installed independently. Attempting to delete an
// unserved kind would fail the reconcile.
func (c *Config) Teardown(ctx context.Context) ([]render.Component, error) {
pkg/render/gateway/component.go:184
- The generated -ingressgateway-access Role grants write access to Gateway/HTTPRoute/ReferenceGrant and EnvoyGateway Backends in every namespace where it is created. When GatewayNamespace != BackendNamespace, this over-grants: the gateway namespace does not need Backend writes, and the backend namespace does not need Gateway/HTTPRoute writes. Since this PR’s goal is to narrow permissions, it would be better to scope Role rules to only the resources actually written in that specific namespace.
Rules: []rbacv1.PolicyRule{
{
APIGroups: []string{gapi.GroupName},
Resources: []string{"gateways", "httproutes", "referencegrants"},
Verbs: []string{"create", "update", "delete"},
},
{
APIGroups: []string{EnvoyGatewayGroup},
Resources: []string{"backends"},
Verbs: []string{"create", "update", "delete"},
},
rene-dekker
left a comment
rene-dekker
left a comment
There was a problem hiding this comment.
Review notes, mostly around the gateway/backend namespace split. The two NetworkPolicy ones look like they'd stop a Manager gateway from working at all; the rest are teardown edge cases.
One finding that wouldn't attach inline (the line isn't in this PR's diff) — pkg/render/gateway/component.go:380, the proxy NetworkPolicy's xDS egress rule:
It targets calico-gateway-api-controller in c.cfg.BackendNamespace, but that controller only ever runs in calico-system (pkg/render/gatewayapi/gateway_api.go:68). For Whisker this is correct by coincidence — the backend namespace is calico-system. For Manager it's wrong: with gatewayNamespace: tigera-manager (the only configuration in which this policy renders for Manager today) the proxy is denied egress to the xDS controller on 18000/18001, so the Gateway never reaches Programmed. Should be common.CalicoNamespace. Pre-existing rather than introduced here, but it becomes reachable once Whisker shares this render path.
Separately (not inline, since it isn't in the diff): the delete list in gatewayDeletionComponent.Objects() is a hand-maintained mirror of the create path — it re-derives every name and every move / gwNS != bkNS / Enterprise conditional independently. The move finding below is a drift between the two. Discovering the delete set by Listing on operator.tigera.io/gateway (labelling all the rendered objects, not just Gateway/Role/RoleBinding) and feeding the found objects into objsToDelete would remove the class of bug rather than this instance of it.
| rgatewayapi.GatewayNamespaceRoleBinding(c.cfg.GatewayNamespace), | ||
| c.proxyNetworkPolicy(), | ||
| ) | ||
| if c.cfg.GatewayNamespace == c.cfg.BackendNamespace { |
There was a problem hiding this comment.
This gate and its rationale don't line up for Manager.
GatewayNamespace == BackendNamespace only holds for Whisker. For Manager, BackendNamespace is helper.InstallNamespace() (tigera-manager) while IngressGatewaySpec.NamespaceOrDefault() returns calico-system — so with the default spec.ingressGateway (no gatewayNamespace set), the Envoy proxy pods land in calico-system, this branch is skipped, and no proxy NetworkPolicy is rendered.
calico-system does carry the operator-managed default-deny (calicoSystemDefaultDenyForCalicoSystem(), pkg/controller/installation/core_controller.go:2253), and reconcileGatewayNamespaceResources explicitly skips gw.Namespace == common.CalicoNamespace — so neither path covers it.
Should the condition be gwNS == bkNS || gwNS == common.CalicoNamespace?
(Pre-existing from #5032, but this PR rewrites the block and the comment justifying it.)
There was a problem hiding this comment.
render.ManagerNamespace is now common.CalicoNamespace, with tigera-manager retained only as LegacyManagerNamespace. In single-tenant mode, helper.InstallNamespace() returns calico-system, which is also what NamespaceOrDefault() returns. Therefore, gwNS == bkNS holds with the default spec, and the proxy NetworkPolicy and WAF ServiceAccount are rendered. Multi-tenant mode cannot reach this path either, since spec.ingressGateway is rejected a few lines earlier.
| // The Role and RoleBinding go last, after the resources they permit deleting, | ||
| // and in the reverse of the render order. On a move the backend namespace | ||
| // keeps its resources, so it keeps its grant. | ||
| objs = append(objs, c.roleBinding(gwNS), c.role(gwNS)) |
There was a problem hiding this comment.
The comment says "on a move the backend namespace keeps its resources, so it keeps its grant", but the !move guard only covers line 534. Line 533 unconditionally deletes roleBinding(gwNS)/role(gwNS).
When gwNS == bkNS and the gateway moves out of the backend namespace — e.g. Whisker moving from calico-system to a custom namespace — this deletes the backend namespace's write grant even though the Backend and ReferenceGrant stay there.
The render component normally re-creates it later in the same reconcile, so it's self-healing in the common case. But if that Role create fails, the operator is left with no write access to a Backend it still owns, and nothing in the teardown path restores it.
There was a problem hiding this comment.
Fixed by splitting the grant by purpose into two separately named Roles: the Gateway namespace gets Gateways/HTTPRoutes, while the backend namespace gets ReferenceGrants/Backends. This avoids overlap when namespaces coincide, scopes each Role to the resources it needs, and removes the backend Role only during teardown. Upgrades also clean up the legacy combined Role.
| namespaces = append(namespaces, c.BackendNamespace) | ||
| } | ||
| var components []render.Component | ||
| for _, ns := range namespaces { |
There was a problem hiding this comment.
Ordering hazard across the emitted components.
Each non-backend component's tail drops the backend-namespace Role/RoleBinding (component.go:534), and namespaces here are sorted with BackendNamespace appended last. If a labeled Gateway exists in both bkNS and another namespace — a partially completed move, or a hand-labelled Gateway — the first component revokes the operator's write grant in bkNS, and then a later component still has to delete the ReferenceGrant there. That Delete now 403s, and Teardown never re-creates the grant, so the reconcile is stuck degraded with no retry that can make progress.
Deleting the bkNS grant only from the component whose gwNS == bkNS (or ordering bkNS first) avoids it.
More generally: the self-authorization ordering constraint is currently maintained by slice position across a variable-length component list. Since CreateOrUpdateOrDelete runs all creates then all deletes for a single component (pkg/controller/utils/component.go:487 → :598), collapsing this into one component with both lists populated would make the invariant structural instead.
There was a problem hiding this comment.
Fixed. The cleanup component now deletes only objects in its own namespace. Backend and ReferenceGrant are removed by the component for the backend namespace, rather than by every component. Teardown always includes that namespace, ensuring both are cleaned up.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if len(namespaces) == 0 { |
There was a problem hiding this comment.
Returning nothing when no labeled Gateway exists drops the old manager backstop, which was gatewayCRDsPresent && !slices.Contains(namespaces, installNS) → always include the install namespace.
The previous render order emitted the ReferenceGrant before the Gateway, so a partial render from the pre-upgrade operator can leave a ReferenceGrant/Backend in the install namespace with no labeled Gateway pointing at them. After upgrade, clearing spec.ingressGateway will now never clean those up.
Related to the known limitation in the description, but this is a second, separate path into it — worth a line in that section at minimum.
There was a problem hiding this comment.
This version creates the access Role and RoleBinding first. The behavior is essentially the same: RBAC needs to exist before the Gateway can be created.
I'm keeping the current behavior for this PR. Hitting this requires a failed reconcile and the user clearing spec.ingressGateway before the retry. Otherwise, the Gateway is created, cleanup finds everything by label, and re-setting the field re-adopts the resources.
Bringing back the backstop would require another cluster-wide Role/RoleBinding discovery path and additional permissions, which doesn't seem worth the complexity right now.
| return reconcile.Result{}, err | ||
| } | ||
|
|
||
| gatewayClassName, err := uigateway.ResolveClassName(gw, gatewayAPI) |
There was a problem hiding this comment.
A gateway configuration error returns before ch.CreateOrUpdateOrDelete runs for certComponent and whisker.Whisker(cfg), so Whisker itself never gets deployed.
On a fresh cluster where the user sets spec.ingressGateway up front with, say, two GatewayClasses and no gatewayClassName, the result is no Whisker at all rather than Whisker without a gateway. Same for a missing GatewayAPI CR or a namespace-create failure.
This mirrors Manager, but the blast radius differs: for Manager the gateway is an add-on to an already-rendered component; here it takes down the whole thing. Worth degrading and rendering Whisker without the gateway instead.
There was a problem hiding this comment.
Per the design doc discussion, we decided to keep the halt but make the behavior explicit. The previous message incorrectly implied that only Gateway resources would not be rendered, when the reconcile actually stops and nothing is rendered. The message now reads, “GatewayAPI CR not found; GatewayAPI is a prerequisite for spec.ingressGateway,”
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- api/v1/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
pkg/controller/whisker/controller.go:291
uigateway.Config.Enterpriseis inverted for Whisker (!installationSpec.Variant.IsEnterprise()), which makes cleanup treat Calico (OSS) installs as Enterprise. That can cause the deletion path to try to manage Enterprise-only gateway namespace resources (e.g., WAF SA/RoleBinding) that Whisker never renders. Whisker’s gateway resources should always be treated as non-Enterprise.
Enterprise: !installationSpec.Variant.IsEnterprise(),
Split the namespaced grant per purpose: the gateway namespace gets gateways and httproutes, the backend namespace referencegrants and backends. Neither namespace holds verbs it never uses, and no single object serves two purposes when the namespaces coincide — which is what made cleanup drop a grant the Backend still depended on. Upgrades remove the combined Role left behind. Delete only what a cleanup run's own namespace holds, so a run cannot revoke a grant a later one still needs and then leave it unable to finish. Degrade instead of writing a TLS secret with no private key when certificateManagement is enabled, and say what a missing GatewayAPI CR costs rather than implying only gateway resources are skipped. Take both Enterprise flags from the installation variant, rename the deletion component's namespace field to StaleNamespace, and trim comments to what the code does not already say.
df77b5f to
6ff02a3
Compare
| // false on Calico, where the gateway is rendered; the WAF ServiceAccount | ||
| // is Enterprise-only and Whisker's gateway never creates it. | ||
| Enterprise: installationSpec.Variant.IsEnterprise(), |
Two of the four were hardcoded, so the same question was answered two ways in one file. resolveGateway takes the installation spec to do it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- api/v1/zz_generated.deepcopy.go: Generated file
Suppressed comments (2)
pkg/render/gateway/component_test.go:417
- This test claims the access grant is dropped last, but it only checks the types of the last two objects and that the access Role exists somewhere in the list. In the same-namespace case there are two grants, so the last Role/RoleBinding could be the backend grant and the test would still pass. Tighten the assertion to verify the names of the last Role/RoleBinding are the access grant.
last := toDelete[len(toDelete)-1]
secondLast := toDelete[len(toDelete)-2]
Expect(last).To(BeAssignableToTypeOf(&rbacv1.Role{}))
Expect(secondLast).To(BeAssignableToTypeOf(&rbacv1.RoleBinding{}))
Expect(findObject[*rbacv1.Role](toDelete, prefix+"-ingressgateway-access", gwNS)).NotTo(BeNil())
pkg/render/gateway/component.go:571
- The deletion grant order doesn’t match the comment about deleting grants in the reverse of render order. When gateway and backend share a namespace, this currently deletes the gateway access grant before the backend access grant (same as render order), which makes the comment misleading and can cause future ordering assertions to be incorrect. Consider swapping the append order so backend grant is deleted first and gateway grant last (reverse of render).
objs = append(objs, c.roleBinding(staleNS, gatewayAccessSuffix), c.role(staleNS, gatewayAccessSuffix))
if staleNS == bkNS && !move {
objs = append(objs, c.roleBinding(bkNS, backendAccessSuffix), c.role(bkNS, backendAccessSuffix))
}
| TLSSecretName: ManagerGatewayTLSSecretName, | ||
| BackendNamespace: helper.InstallNamespace(), | ||
| Enterprise: installationSpec.Variant.IsEnterprise(), | ||
| } |
There was a problem hiding this comment.
This is called a "helper" but the struct is just config, which is a bit unusual.
If we want this to fit the normal pattern, we should do something like this:
gwHelper := uigateway.NewHelper(...)func NewHelper(...) Helper { }type Helper struct // Or interface{}| TLSSecretName: ManagerGatewayTLSSecretName, | ||
| BackendNamespace: helper.InstallNamespace(), | ||
| Enterprise: installationSpec.Variant.IsEnterprise(), | ||
| } |
There was a problem hiding this comment.
This is called a "helper" but the struct is just config, which is a bit unusual.
If we want this to fit the normal pattern, we should do something like this:
gwHelper := uigateway.NewHelper(...)func NewHelper(...) Helper { }type Helper struct // Or interface{}, for testing purposes.| } | ||
| } else if instance.Spec.IngressGateway != nil { | ||
| gwComp, gwKeyPair, result, err := r.resolveGateway(ctx, instance, authenticationCR, certificateManager, helper, logc) | ||
| gwComp, gwKeyPair, result, err := r.resolveGateway(ctx, instance, installationSpec, authenticationCR, certificateManager, helper, logc) |
There was a problem hiding this comment.
Many of the other funcs moved to the helper - should this one as well?
|
|
||
| // Config identifies one UI component's gateway resources. | ||
| type Config struct { | ||
| Client client.Client |
There was a problem hiding this comment.
This is a bit of a smell - a Config struct shouldn't contain a client, at least how we tend to structure the rest of the operator code.
| // Enterprise controls whether the proxy SA and RoleBinding are part of | ||
| // the component's rendered set; the proxy NetworkPolicy is rendered on | ||
| // both variants. | ||
| Enterprise bool |
There was a problem hiding this comment.
Probably better to pass the Variant here.
| ResourcePrefix string | ||
| GatewayNamespace string | ||
| ResourcePrefix string | ||
| // StaleNamespace is the namespace being cleaned up. |
There was a problem hiding this comment.
| // StaleNamespace is the namespace being cleaned up. |
Claude loves to throw obvious comments in above new struct fields even when no other field on the struct has them...
| @@ -383,19 +495,20 @@ func (c *gatewayDeletionComponent) Objects() (objsToCreate, objsToDelete []clien | |||
| objs := []client.Object{ | |||
| &corev1.Secret{ | |||
There was a problem hiding this comment.
Mostly just curious... do you know why we have a gateway deletion component instead of just using the gateway component and inverting create / delete?
| @@ -383,19 +495,20 @@ func (c *gatewayDeletionComponent) Objects() (objsToCreate, objsToDelete []clien | |||
| objs := []client.Object{ | |||
| &corev1.Secret{ | |||
There was a problem hiding this comment.
Mostly just curious... do you know why we have a gateway deletion component instead of just using the gateway component and inverting create / delete? Seems like we need to be careful to keep these in sync?
| // The Gateway goes after the resources found through it, mirroring the render. | ||
| // If an earlier delete fails, it stays and the next reconcile still finds the | ||
| // leftovers by its label. |
There was a problem hiding this comment.
I think a finalizer is the correct way to do this? Relying on ordering can be finicky / error-prone, and finalizers are the k8s native way to say "keep this around until I am done with it".
Let's not do it in this PR, but perhaps as a follow-on to keep things tidy?
| Action: v3.Allow, | ||
| Protocol: &networkpolicy.TCPProtocol, | ||
| Source: v3.EntityRule{ | ||
| NamespaceSelector: fmt.Sprintf("%s == '%s'", selector.CalicoNameLabel, c.cfg.IngressGatewayNamespace), |
There was a problem hiding this comment.
note to self: we should rename the CalicoNameLabel
Description
New feature: expose the Whisker UI (Calico OSS) through Calico Ingress Gateway, following the same flow #5032 added for Manager.
spec.ingressGatewayto the Whisker CR. When set, the Whisker controller renders a Gateway, HTTPRoute, Backend, and ReferenceGrant, mints a gateway TLS secret, and reports Degraded until the Gateway is programmed. The gateway is rendered only on the Calico variant: Whisker itself is deleted on other variants, so a gateway there would point at a Service the same reconcile is removing. On those variants the controller tears the gateway down instead.pkg/controller/uigateway(label-driven cleanup, health read-back, namespace provisioning, class resolution, watch setup). No Manager behavior change.get/list/watchongateways,httproutes,referencegrantsandbackends— the controller-runtime cache needs reads in every namespace. For writes, the operator grants itself a namespaced Role and RoleBinding,<prefix>-ingressgateway-accessbound to its own ServiceAccount, in each namespace it writes to (the gateway namespace, and the install namespace when they differ). This follows the existing waypoint EnvoyFilter Role pattern and relies on the operator'sbind/escalateverbs. Manager gets the same treatment, since both components sharepkg/render/gateway.Requires the companion chart changes that drop the cluster-wide write verbs: PMREQ-821: Grant the operator Gateway API access for Whisker CIG projectcalico/calico#13521 (OSS) and tigera/calico-private#13247 (Enterprise).
0srequest timeout so SSE flow-log streams stay open; the Whisker NetworkPolicy admits only this Gateway's Envoy proxy pods on 8443, selecting their namespace bykubernetes.io/metadata.name; the gateway re-originates TLS to Whisker's HTTPS port, validated against the trusted CA bundle.calico-systemhas an operator-managed default-deny on Calico too) and creates the operator-secrets RoleBinding in gateway namespaces on both variants.Testing
calico-systemand in a custom namespace, a namespace move, and teardown on spec removal. UI reachable over the load balancer with HTTPS 200; flow-log SSE stream held open and delivering events. Run with the narrowed ClusterRole applied: SubjectAccessReview confirms the operator's cluster-widecreateon all four kinds is denied, whilecreatein the configured namespace is allowed, and the self-granted Role/RoleBinding appear on the first reconcile./and/login200), Enterprise-only WAF ServiceAccount and proxy NetworkPolicy rendered — confirms the shared render change does not regress Manager.Authentication.spec.managerDomainpointed at the gateway hostname and the two callback URLs registered, the whole Dex redirect chain completes headlessly. Dex advertises the gateway hostname as its issuer, and the id_token it mints is accepted by Manager's API —/api/v1/namespacesreturns 200, and/api/v1/versionmoves from 401 to 403, so the identity is validated rather than ignored.Known limitation
Cleanup finds this feature's resources by the
operator.tigera.io/gatewaylabel on the Gateway, so anything created before the Gateway is invisible to it if the Gateway itself never appears. Two paths lead there, both needing a failure plus a user action inside the window it opens:spec.ingressGatewayis cleared before the next reconcile converges, they are left behind.Neither is an escalation: the leftovers grant write access on Gateway API kinds in a single namespace to the operator's own ServiceAccount and to nothing else, and re-setting
spec.ingressGatewayre-adopts them. Every ordinary sequence self-heals, since the reconcile retries and the Gateway then exists for cleanup to find.Accepted for this PR rather than fixed, to keep cleanup single-sourced on the Gateway rather than adding a second discovery path with its own cluster-wide read grant.
Release Note