Skip to content

Commit f7ada0a

Browse files
MikelAlejoBRMikelAlejoBR
committed
docs: design of the "disabled integrations" feature
These are the design documents used for the "disabled integrations" feature, which aims at showing or hiding the available integrations in Sandbox depending on the configuration specified in the ToolchainConfig resource. SANDBOX-1769
1 parent 4ff1ff9 commit f7ada0a

File tree

2 files changed

+349
-0
lines changed

2 files changed

+349
-0
lines changed

‎docs/proposals/enabled-integrations-design.md‎

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
# Integration Visibility Configuration
2+
3+
**Status:** Final
4+
5+
## Overview
6+
7+
Developer Sandbox integrates with several downstream services (e.g. OpenShift Lightspeed, DevSpaces, Workato, etc.). When one of these downstream integrations experiences issues, there is currently no way to hide it from customers without a code change and redeployment.
8+
9+
This design adds a `disabledIntegrations` field to `ToolchainConfig` so that operators can dynamically hide broken integrations from users. The registration service exposes this list via the existing `/api/v1/uiconfig` endpoint, allowing the UI to conditionally hide integration entry points at runtime.
10+
11+
The field uses a **denylist** approach: the normal state is an empty field (all integrations enabled), and admins only add the 1-2 broken integrations when something goes wrong. This avoids the operational burden of maintaining a full allowlist of every integration.
12+
13+
## Design Principles
14+
15+
- **Minimal API surface change**: add a single new field to the existing `ToolchainConfig` CRD rather than introducing a new CRD.
16+
- **Safe defaults**: when the field is not set, all integrations are enabled (backward compatible with existing deployments).
17+
- **Operational simplicity**: the common case (everything healthy) requires zero configuration; only broken integrations are listed.
18+
- **Consistency with existing patterns**: follows the same conventions used by `UICanaryDeploymentWeight` and `WorkatoWebHookURL` for CRD placement and REST exposure.
19+
- **Decoupled from feature toggles**: feature toggles control per-Space probabilistic template rendering; this mechanism controls UI-level visibility of integrations for all users.
20+
21+
## Architecture / How It Works
22+
23+
```
24+
┌──────────────┐ ┌──────────────────┐ ┌─────────────────────┐
25+
│ Cluster │ │ Registration │ │ Sandbox UI │
26+
│ Admin │──────▶│ Service │──────▶│ (Browser) │
27+
│ │ │ │ │ │
28+
│ applies │ │ GET /api/v1/ │ │ fetches uiconfig, │
29+
│ ToolchainCfg │ │ uiconfig │ │ reads disabled- │
30+
└──────────────┘ │ (JWT-secured) │ │ Integrations, │
31+
│ └──────────────────┘ │ hides those │
32+
│ ▲ └─────────────────────┘
33+
▼ │
34+
┌──────────────────────────┐ │
35+
│ToolchainConfig CR │ │
36+
│ spec.host │ │
37+
│ .registrationService │ │
38+
│ .disabledIntegrations:│ │
39+
│ - "openshift" │────┘
40+
└──────────────────────────┘
41+
(loaded via commonconfig.LoadLatest)
42+
```
43+
44+
1. **Cluster admin** updates the `ToolchainConfig` CR, adding broken integrations to `disabledIntegrations`.
45+
2. **Registration service** picks up the change via its cached client (same mechanism used for all other config — `commonconfig.LoadLatest`).
46+
3. **`GET /api/v1/uiconfig`** (JWT-secured) serves the disabled list alongside existing fields (`uiCanaryDeploymentWeight`, `workatoWebHookURL`).
47+
4. **UI** fetches the endpoint and hides any integration present in the `disabledIntegrations` array.
48+
49+
## Core Concepts
50+
51+
### Integration identifier
52+
53+
Each integration is represented by a value of the `IntegrationName` type, a named string with kubebuilder enum validation. The valid identifiers are defined as Go constants in the API module:
54+
55+
| Constant | Value |
56+
|----------|-------|
57+
| `IntegrationOpenShift` | `"openshift"` |
58+
| `IntegrationOpenShiftAI` | `"openshift-ai"` |
59+
| `IntegrationDevSpaces` | `"devspaces"` |
60+
| `IntegrationAnsibleAutomationPlatform` | `"ansible-automation-platform"` |
61+
| `IntegrationOpenShiftVirtualization` | `"openshift-virtualization"` |
62+
63+
Kubernetes rejects any value not in this enum at admission time, preventing typos and misconfiguration. Adding a new integration requires an API module change and CRD regeneration.
64+
65+
### Default behavior
66+
67+
When `disabledIntegrations` is nil or empty, **all integrations are enabled**. This preserves backward compatibility — existing deployments without the field continue to work with everything visible. Admins only populate the field when they need to hide something.
68+
69+
### Field location
70+
71+
The `disabledIntegrations` field lives under `spec.host.registrationService` in the `ToolchainConfig` CRD, co-located with other UI-facing configuration (`uiCanaryDeploymentWeight`, `workatoWebHookURL`).
72+
73+
### CRD field definition
74+
75+
```go
76+
// In api/v1alpha1/toolchainconfig_types.go:
77+
78+
// +kubebuilder:validation:Enum=openshift;openshift-ai;devspaces;ansible-automation-platform;openshift-virtualization
79+
type IntegrationName string
80+
81+
const (
82+
IntegrationOpenShift IntegrationName = "openshift"
83+
IntegrationOpenShiftAI IntegrationName = "openshift-ai"
84+
IntegrationDevSpaces IntegrationName = "devspaces"
85+
IntegrationAnsibleAutomationPlatform IntegrationName = "ansible-automation-platform"
86+
IntegrationOpenShiftVirtualization IntegrationName = "openshift-virtualization"
87+
)
88+
89+
// In RegistrationServiceConfig:
90+
91+
// DisabledIntegrations specifies the list of integrations that should be
92+
// hidden/disabled in the UI. When nil or empty, all integrations are
93+
// considered enabled. Only listed integrations are hidden.
94+
// +optional
95+
// +listType=set
96+
DisabledIntegrations []IntegrationName `json:"disabledIntegrations,omitempty"`
97+
```
98+
99+
### REST response
100+
101+
The existing `UIConfigResponse` in the registration service is extended with a `disabledIntegrations` field that mirrors the CRD 1:1:
102+
103+
```go
104+
type UIConfigResponse struct {
105+
UICanaryDeploymentWeight int `json:"uiCanaryDeploymentWeight"`
106+
WorkatoWebHookURL string `json:"workatoWebHookURL"`
107+
DisabledIntegrations []toolchainv1alpha1.IntegrationName `json:"disabledIntegrations"`
108+
}
109+
```
110+
111+
Example JSON response from `GET /api/v1/uiconfig` when OpenShift is down:
112+
113+
```json
114+
{
115+
"uiCanaryDeploymentWeight": 20,
116+
"workatoWebHookURL": "https://...",
117+
"disabledIntegrations": ["openshift"]
118+
}
119+
```
120+
121+
Example when everything is healthy (field not set in CR):
122+
123+
```json
124+
{
125+
"uiCanaryDeploymentWeight": 20,
126+
"workatoWebHookURL": "https://...",
127+
"disabledIntegrations": []
128+
}
129+
```
130+
131+
The handler normalizes nil to an empty slice before serialization, so the response always contains `"disabledIntegrations": []` (never absent or `null`). The UI checks `disabledIntegrations.includes("x")` before rendering each integration entry point.
132+
133+
## Implementation Plan
134+
135+
Changes span **three repositories** in the codeready-toolchain ecosystem:
136+
137+
### Phase 1: API types (`codeready-toolchain/api`)
138+
139+
1. Define the `IntegrationName` type with a `+kubebuilder:validation:Enum` marker and the associated constants in `api/v1alpha1/toolchainconfig_types.go`.
140+
2. Add `DisabledIntegrations []IntegrationName` to `RegistrationServiceConfig` with `+listType=set` and `+optional` markers.
141+
3. Run code generation (`make generate`) to update DeepCopy methods and CRD manifests.
142+
143+
### Phase 2: Host operator (`codeready-toolchain/host-operator`)
144+
145+
1. Bump the `codeready-toolchain/api` dependency in `go.mod` to pick up the new field, then run `make generate` to regenerate the CRD YAML in `config/crd/bases/`.
146+
147+
### Phase 3: Registration service (`codeready-toolchain/registration-service`)
148+
149+
1. Add a `DisabledIntegrations() []IntegrationName` accessor method on `RegistrationServiceConfig` in `pkg/configuration/configuration.go`. Note: unlike pointer-based fields (e.g. `UICanaryDeploymentWeight`, `WorkatoWebHookURL`) that use `commonconfig.Get*` helpers, this accessor returns the `[]IntegrationName` field directly — nil and empty both mean "nothing disabled," so no default-value wrapper is needed.
150+
2. Extend `UIConfigResponse` in `pkg/controller/uiconfig.go` to include the `DisabledIntegrations` field (typed as `[]toolchainv1alpha1.IntegrationName`).
151+
3. Update the `GetHandler` to populate the new field from configuration, normalizing nil to an empty slice.
152+
4. Add unit tests for the updated handler.
153+
154+
### Phase 4: UI (out of scope for this design)
155+
156+
The UI team will consume the `disabledIntegrations` field from the `/api/v1/uiconfig` response and hide any integration present in the array.
157+
158+
## Decisions Summary
159+
160+
| # | Question | Decision |
161+
|---|----------|----------|
162+
| Q1 | Allowlist vs. denylist | Denylist (`disabledIntegrations`) — only list broken integrations |
163+
| Q2 | Field type | Typed enum `[]IntegrationName` with kubebuilder validation |
164+
| Q3 | Field placement | Under `spec.host.registrationService` |
165+
| Q4 | Endpoint | Extend existing `GET /api/v1/uiconfig` |
166+
| Q5 | Authentication | Keep JWT-secured (no change) |
167+
| Q6 | CRD field name | `disabledIntegrations` |
168+
| Q7 | REST response property | Mirror CRD — `disabledIntegrations` |
169+
170+
Full decision rationale: [enabled-integrations-questions.md](enabled-integrations-questions.md)

‎docs/proposals/enabled-integrations-questions.md‎

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
# Integration Visibility — Design Questions
2+
3+
**Status:** Resolved — all decisions made
4+
**Related:** [Design document](enabled-integrations-design.md)
5+
6+
Each question has options with trade-offs and a recommendation. Go through them one by one to form the design, then update the design document.
7+
8+
---
9+
10+
## Q1: Allowlist vs. denylist approach?
11+
12+
When a cluster admin wants to hide a broken integration, the system needs a mechanism. This affects the operational model and how much config admins need to maintain.
13+
14+
### Option C: Denylist approach (`disabledIntegrations`)
15+
16+
List what is disabled. Everything not in the list is enabled.
17+
18+
- **Pro:** Backward compatible — existing deployments show everything.
19+
- **Pro:** Operationally simpler — the common case (everything healthy) is an empty field; admins only add the 1-2 broken integrations.
20+
- **Pro:** Adding a new integration requires no config change — it's enabled by default.
21+
- **Con:** The UI must check each integration against the disabled list (trivial).
22+
23+
**Decision:** Option C — denylist approach. With an allowlist, admins must enumerate every integration; with a denylist the normal state is empty and only broken integrations are listed.
24+
25+
_Considered and rejected: Option A — allowlist when populated (requires listing all integrations, higher operational overhead), Option B — explicit allowlist with nothing enabled by default (breaking change for existing deployments)._
26+
27+
---
28+
29+
## Q2: Should integrations be simple string identifiers or richer structs?
30+
31+
This determines the shape of the array items in the CRD.
32+
33+
### Option A: Simple `[]string`
34+
35+
```yaml
36+
disabledIntegrations:
37+
- "openshift"
38+
```
39+
40+
- **Pro:** Minimal API surface, easy to understand.
41+
- **Pro:** Follows the pattern used by comma-separated string lists elsewhere in the config (e.g. `domains`, `forbiddenUsernamePrefixes`), but as a proper array.
42+
- **Con:** No room for per-integration metadata (e.g. a display name or URL) without a future API change.
43+
- **Con:** Free-form strings are prone to typos and mismatches between backend and UI — a misspelled identifier is silently ignored.
44+
45+
### Option B: Typed enum (`IntegrationName`) with kubebuilder validation
46+
47+
Define a named string type with a `+kubebuilder:validation:Enum` marker and Go constants for each known integration. The CRD field becomes `[]IntegrationName` instead of `[]string`.
48+
49+
```go
50+
// +kubebuilder:validation:Enum=openshift;openshift-ai;devspaces;ansible-automation-platform;openshift-virtualization
51+
type IntegrationName string
52+
53+
const (
54+
IntegrationOpenShift IntegrationName = "openshift"
55+
IntegrationOpenShiftAI IntegrationName = "openshift-ai"
56+
IntegrationDevSpaces IntegrationName = "devspaces"
57+
IntegrationAnsibleAutomationPlatform IntegrationName = "ansible-automation-platform"
58+
IntegrationOpenShiftVirtualization IntegrationName = "openshift-virtualization"
59+
)
60+
```
61+
62+
```yaml
63+
disabledIntegrations:
64+
- "openshift"
65+
```
66+
67+
- **Pro:** Kubernetes rejects invalid values at admission time — immediate feedback on typos.
68+
- **Pro:** `kubectl explain` shows the valid values — self-documenting.
69+
- **Pro:** Go constants provide a canonical reference for backend and UI teams.
70+
- **Con:** Adding a new integration requires an API module change, CRD regeneration, and rollout.
71+
72+
**Decision:** Option B — typed enum with kubebuilder validation. The safety of admission-time validation outweighs the cost of requiring an API change for new integrations. That cost is actually desirable — new integrations should be a deliberate, reviewable change.
73+
74+
_Considered and rejected: Option A — simple `[]string` (too error-prone, no validation), Option C — array of structs (over-engineered for current needs), Option D — comma-separated string (less idiomatic for CRDs)._
75+
76+
---
77+
78+
## Q3: Where should the field live in the config hierarchy?
79+
80+
The `ToolchainConfig` has a deep nested structure. The field placement affects who "owns" this configuration conceptually.
81+
82+
### Option A: Under `spec.host.registrationService`
83+
84+
```yaml
85+
spec:
86+
host:
87+
registrationService:
88+
disabledIntegrations:
89+
- "openshift"
90+
```
91+
92+
- **Pro:** The registration service is the component that exposes this data; co-locating it with other registration service config is natural.
93+
- **Pro:** Follows the pattern of `uiCanaryDeploymentWeight` and `workatoWebHookURL` which are UI-facing config under `registrationService`.
94+
- **Con:** Conceptually, "which integrations are disabled" could be considered a broader host-level concern, not specific to the registration service.
95+
96+
**Decision:** Option A — co-locate with other UI-facing config under `registrationService`, following existing precedent.
97+
98+
_Considered and rejected: Option B — under `spec.host` directly (only registration service needs it, would bloat HostConfig)._
99+
100+
---
101+
102+
## Q4: Should this be a new endpoint or added to the existing `/api/v1/uiconfig`?
103+
104+
The registration service already has `GET /api/v1/uiconfig` that returns UI-facing configuration (canary weight, Workato URL).
105+
106+
### Option A: Extend `/api/v1/uiconfig`
107+
108+
Add the integration visibility field to the existing `UIConfigResponse`.
109+
110+
- **Pro:** No new endpoint to maintain; the UI already calls `/api/v1/uiconfig`.
111+
- **Pro:** Keeps UI configuration consolidated in one place.
112+
- **Con:** Makes the uiconfig response grow over time; it's currently JWT-secured, which means unauthenticated callers cannot access it.
113+
114+
**Decision:** Option A — add to existing `UIConfigResponse`. Avoids an extra HTTP round-trip and keeps UI configuration consolidated.
115+
116+
_Considered and rejected: Option B — new dedicated endpoint (extra maintenance burden, extra HTTP call from the UI)._
117+
118+
---
119+
120+
## Q5: Should the endpoint be secured (JWT) or unsecured?
121+
122+
Currently `/api/v1/uiconfig` requires JWT authentication. The list of disabled integrations is not sensitive, but access control is a consideration.
123+
124+
### Option A: Keep it secured (current behavior for `/api/v1/uiconfig`)
125+
126+
- **Pro:** No security posture change.
127+
- **Pro:** Only authenticated users see what integrations are available.
128+
- **Con:** The UI must have a valid token before it can determine what to render, which may delay the initial page load or require the UI to handle a two-phase render.
129+
130+
**Decision:** Option A — keep the endpoint JWT-secured. No security posture change needed; the UI already has a token when it renders integration entry points.
131+
132+
_Considered and rejected: Option B — unsecured (would also expose workatoWebHookURL and canary weight), Option C — split into new unsecured endpoint (contradicts Q4 decision)._
133+
134+
---
135+
136+
## Q6: What should the CRD field be named?
137+
138+
Naming affects clarity, consistency with the codebase, and how the field appears in the CRD YAML.
139+
140+
### Option A: `disabledIntegrations`
141+
142+
```yaml
143+
disabledIntegrations:
144+
- "openshift"
145+
```
146+
JSON tag: `disabledIntegrations`
147+
148+
- **Pro:** Directly communicates the denylist semantics — these are the integrations that are turned off.
149+
- **Pro:** Consistent with the naming pattern of the approach (listing what's disabled).
150+
- **Con:** Slightly long.
151+
152+
**Decision:** Option A (`disabledIntegrations`) — unambiguous, directly reflects the denylist semantics.
153+
154+
_Considered and rejected: Option B — `integrations` (ambiguous), Option C — `hiddenIntegrations` (UI term out of place in a CRD)._
155+
156+
---
157+
158+
## Q7: What should the REST response JSON property be named?
159+
160+
The CRD field is `disabledIntegrations` (a denylist), but the REST response could either mirror the CRD directly or translate the semantics for the UI's convenience. This affects where the "is this integration disabled?" logic lives.
161+
162+
### Option A: Mirror the CRD — expose `disabledIntegrations` in the JSON response
163+
164+
```json
165+
{
166+
"uiCanaryDeploymentWeight": 20,
167+
"workatoWebHookURL": "https://...",
168+
"disabledIntegrations": ["openshift"]
169+
}
170+
```
171+
172+
- **Pro:** 1:1 mapping with the CRD — simple, transparent, no translation layer.
173+
- **Pro:** The UI can trivially check `disabledIntegrations.includes("x")` before rendering each integration.
174+
- **Pro:** Empty array `[]` unambiguously means "nothing is disabled" (everything enabled).
175+
- **Con:** The UI works with a negative list, which some may find less intuitive than a positive one.
176+
177+
**Decision:** Option A — mirror the CRD directly. Simple, no master list needed, trivial UI check.
178+
179+
_Considered and rejected: Option B — translate to `enabledIntegrations` (requires backend to maintain a hardcoded master list of all integrations, adds a translation layer that can drift)._

0 commit comments

Comments
 (0)