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 22 out of 23 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- api/v1/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
pkg/uigateway/uigateway.go:201
- The Teardown doc comment says "the Gateway is rendered first, so nothing else can exist without one", but pkg/render/gateway now renders the access Role/RoleBinding before the Gateway. That makes the comment inaccurate and can mislead future maintenance around cleanup behavior.
// Teardown returns deletion components for every labeled Gateway namespace,
// plus the backend namespace, which holds the Backend and ReferenceGrant.
// No labeled Gateway means nothing to do: the Gateway is rendered first, so
// nothing else can exist without one.
func (h *Helper) Teardown(ctx context.Context) ([]render.Component, error) {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 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)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/imports/crds/operator/operator.tigera.io_whiskers.yaml:62
- The
ingressGateway.hostnamefield description is inherited from the sharedIngressGatewaySpecand currently says it "Must match the Authentication CR's managerDomain". In the Whisker CRD this is misleading because Whisker does not validate againstAuthentication.spec.managerDomain(that constraint is Manager-specific). Consider updating the shared Go doc comment onapi/v1/IngressGatewaySpec.Hostnameto be component-agnostic (or to clearly scope the managerDomain constraint to Manager) and regenerating the CRDs so the Whisker CRD docs are accurate.
hostname:
description: |-
Hostname for the Gateway listener. Must match the Authentication CR's
managerDomain when OIDC is configured (Manager only).
minLength: 1
caseydavenport
left a comment
There was a problem hiding this comment.
A few minor questions / suggestions, but otherwise LGTM.
|
|
||
| if !move { | ||
| // The Backend and ReferenceGrant live in the backend namespace, so only the | ||
| // component cleaning that namespace deletes them. A component for another |
There was a problem hiding this comment.
I am not sure about this comment - we don't do per-component RBAC here - both components are going to run with the operator RBAC, right?
| // ExtraProxyObjects are variant-specific objects rendered beside the | ||
| // proxy, only when the Gateway shares the backend namespace — elsewhere | ||
| // the GatewayAPI controller provisions per-namespace resources itself. | ||
| ExtraProxyObjects []client.Object |
There was a problem hiding this comment.
Hm, is this the right approach? I think we should be registering an Extension for this and have the extension add the extra objects?
| Protocol: &networkpolicy.TCPProtocol, | ||
| Source: v3.EntityRule{ | ||
| NamespaceSelector: fmt.Sprintf("%s == '%s'", selector.CalicoNameLabel, c.cfg.IngressGatewayNamespace), | ||
| Selector: fmt.Sprintf("gateway.envoyproxy.io/owning-gateway-name == '%s'", GatewayResourcePrefix+"-gateway"), |
There was a problem hiding this comment.
This Prefix + "-gateway" string seems brittle - I think a utils function would help ensure we don't accidentally let these drift / remove typo errors.
| // The ReferenceGrant is v1beta1: the standard has not promoted it to v1, so a | ||
| // cluster serving pre-installed Gateway API CRDs (OpenShift 4.19+) has no v1. |
There was a problem hiding this comment.
| // The ReferenceGrant is v1beta1: the standard has not promoted it to v1, so a | |
| // cluster serving pre-installed Gateway API CRDs (OpenShift 4.19+) has no v1. | |
| // ReferenceGrant is written as v1beta1, the only version served by every | |
| // Gateway API bundle we may find pre-installed (OpenShift 4.19 ships v1.2.1, | |
| // and CRDManagementPreferExisting leaves it alone). v1beta1 is still the | |
| // storage version as of Gateway API v1.6. |
| gatewayWatchPredicate := predicate.NewPredicateFuncs(func(o client.Object) bool { | ||
| return o.GetName() == resourcePrefix+"-gateway" || | ||
| o.GetName() == resourcePrefix+"-route" | ||
| }) | ||
| go utils.WaitToAddResourceWatch(c, k8sClientset, log, nil, []client.Object{ | ||
| &gapi.Gateway{ | ||
| TypeMeta: metav1.TypeMeta{Kind: "Gateway", APIVersion: "gateway.networking.k8s.io/v1"}, | ||
| ObjectMeta: metav1.ObjectMeta{Name: resourcePrefix + "-gateway"}, | ||
| }, | ||
| &gapi.HTTPRoute{ | ||
| TypeMeta: metav1.TypeMeta{Kind: "HTTPRoute", APIVersion: "gateway.networking.k8s.io/v1"}, | ||
| ObjectMeta: metav1.ObjectMeta{Name: resourcePrefix + "-route"}, | ||
| }, | ||
| }, gatewayWatchPredicate) |
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.
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.
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.
Config is now data only; Helper owns the client via NewHelper, and one entrypoint, Components, folds in the GatewayAPI fetch, class resolution, namespace creation, and the certificateManagement guard, so both controllers make a single call. Conditions come back as a typed Error; controllers set Degraded themselves and no longer requeue on configuration problems, since the CRs that fix them are watched. The package moves to pkg/uigateway with no variant knowledge: callers supply ExtraProxyObjects, built by the new pkg/enterprise/uigateway for Manager and nil for Whisker. That also stops a Whisker teardown on a variant switch from deleting the shared WAF ServiceAccount Manager's gateway depends on. A namespace the operator creates is labelled and deleted on teardown; one the user already had is never touched. MoveCleanup is renamed StaleComponents, the never-shipped pre-split Role cleanup is deleted, and a new test asserts every rendered object has a matching delete. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The typed Error carried a reason and message so callers could degrade without requeueing, but the branch it required outweighed what it bought: the specific cause still reaches TigeraStatus through the error chain. Components now returns plain errors, both controllers degrade with one generic reason and return the error, and controller-runtime's backoff replaces the watch-only wait on configuration problems. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Master moved the computed installation into status and the variant objects behind the extensions boundary, so the tests mutate Status.Computed and the gateway suites use the variant's extension; the operator-secrets RoleBinding moves to the common per-namespace path, since both variants need it for the UI gateway TLS secret. Also addresses review: the suite keeps its old package name, a doc comment kept an exported spelling, and the namespace comment predated ownership. Comment duplicates now state each rule once, at the site that enforces it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
State why the Whisker render needs the gateway namespace and where the Enterprise proxy objects apply, and trim the grants-ordering comment to the ordering rationale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The standard has not promoted ReferenceGrant to v1, so a cluster serving pre-installed Gateway API CRDs — OpenShift 4.19+ ships them — has no v1 to match. The create silently failed there, leaving cross-namespace references unresolved, and the teardown erred forever, orphaning the backend grant. v1beta1 is served everywhere; the scheme now registers it. Also point the moved suite's JUnit report back inside the repository; the old depth wrote above it and failed CI on permission. Verified on OpenShift 4.20: cross-namespace ReferenceGrant created and resolved, teardown clean, and a stock-operator Manager degrade on Enterprise OpenShift cleared by the earlier backstop removal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… namespace On OpenShift a plain namespace rejects the Envoy proxy pod: SCC admission forbids its privileged init container and fixed UIDs. Create the gateway namespace through render.CreateNamespace so it carries the same run-level and pod-security labels as the install namespace, then stamp the ownership label on top. Config now carries the Provider instead of an OpenShift bool, so the helper derives platform behavior itself. A user-supplied existing namespace is still the user's to set up; that gap applies to the whole GatewayAPI feature and is tracked separately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
f8f9336 to
fdba030
Compare
Fold the review round: thread Installation.Azure into the gateway namespace; guard the operator namespace and add GatewayName/RouteName helpers; delete the stale ReferenceGrant from the component that owns the backend namespace; point the proxy xDS egress at the controller's real namespace; delete a shared gateway namespace only when the last Gateway leaves; tear it down when the Whisker CR is deleted; warn when spec.ingressGateway is set on Enterprise; and treat unreported gateway conditions as not ready. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Components created the user-named gateway namespace with a live client call, so a render helper mutated the cluster. Return the Namespace as a rendered object and let the component handler create it, matching every other operator namespace. A read still guards it, so a namespace the user already had is left untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Trim the ReferenceGrant deletion note, make GatewayName's doc a crisp two lines, and name the components (Manager/Whisker) in namespaceDeletable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fdba030 to
dbeac9c
Compare
Description
Exposes the Whisker UI (Calico OSS) through Calico Ingress Gateway via
spec.ingressGatewayon the Whisker CR, following the flow #5032 added for Manager. When set, the controller renders a Gateway, HTTPRoute, Backend, ReferenceGrant and TLS secret, and reports Degraded until the Gateway is programmed (only on the Calico variant; on others it tears the gateway down).Main changes:
pkg/uigateway(oneHelper.Componentscall per controller); Enterprise-only objects (WAF filter SA/RoleBinding) layer on viapkg/enterprise/uigateway, so the common code carries no variant knowledge.get/list/watch; writes are self-granted per namespace at runtime. Needs the companion chart changes: [Do not merge] PMREQ-821: Grant the operator Gateway API access for Whisker CIG projectcalico/calico#13521 (OSS) and tigera/calico-private#13247 (Enterprise).gatewayNamespaceit creates (a pre-existing user namespace is never touched).v1beta1— the only version pre-installed Gateway API bundles (OpenShift 4.19+) serve.Release Note