Skip to content

Commit efa4b67

Browse files
akurinnoyclaude
andauthored
docs: establish ADR framework with template and agent guidelines (#1633)
* docs: establish ADR framework with template and agent guidelines Add README, template, and agent instructions for Architecture Decision Records. Update root AGENTS.md and .claude/CLAUDE.md so agents know when and how to create ADRs without needing to be in the adr/ directory. Assisted-by: Claude Code Signed-off-by: Oleksii Kurinnyi <okurynny@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> * docs: add backup auth secret lifecycle ADR ADR documenting why the backup registry auth secret must not be owned by any workspace, including alternatives considered and trade-offs accepted. Assisted-by: Claude Code Signed-off-by: Oleksii Kurinnyi <okurynny@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> * chore: update AGENTS.md last updated date Signed-off-by: Oleksii Kurinnyi <okurynny@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> --------- Signed-off-by: Oleksii Kurinnyi <okurynny@redhat.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent a15632e commit efa4b67

6 files changed

Lines changed: 246 additions & 1 deletion

File tree

.claude/CLAUDE.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,7 @@
33
See @../AGENTS.md for project guidelines.
44

55
See @../redhat-compliance-and-responsible-ai.md for Red Hat compliance and responsible AI rules.
6+
7+
## Architecture Decision Records
8+
9+
Before marking work complete, check: did you reject an alternative or accept a trade-off? If yes, suggest creating an ADR in `adr/`. Before proposing changes, check existing ADRs for prior decisions that might be affected. See [adr/README.md](../adr/README.md) for conventions.

AGENTS.md

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -748,16 +748,38 @@ Use this to trigger a reconciliation without changing the spec.
748748
kubectl patch dw <name> --type merge -p "{\"metadata\": {\"annotations\": {\"force-update\": \"$(date +%s)\"}}}"
749749
```
750750

751+
## Architecture Decision Records (ADRs)
752+
753+
ADRs live in the [`adr/`](adr/) directory. They capture the **why** behind non-trivial design decisions — information that is lost in code and PR descriptions over time.
754+
755+
### When to Create an ADR
756+
757+
Before marking work complete, ask: **did I reject an alternative or accept a trade-off?** If yes, create an ADR.
758+
759+
Specifically, write an ADR when your change involves any of these:
760+
761+
1. **You rejected an alternative** — There were 2+ reasonable approaches and you picked one
762+
2. **You accepted a trade-off** — Something got worse in exchange for something more important
763+
3. **You changed a resource lifecycle or ownership model** — Who creates, owns, or cleans up a Kubernetes resource
764+
4. **You changed an external contract** — API shape, CRD fields, Secret/ConfigMap naming, image paths
765+
766+
**Don't** write an ADR for: bug fixes, dependency bumps, refactors preserving behavior, test additions, docs, or performance optimizations with no trade-offs.
767+
768+
### How to Create an ADR
769+
770+
Copy [`adr/TEMPLATE.md`](adr/TEMPLATE.md) to `adr/YYYY-MM-DD-short-slug.md` and fill in all sections. See [`adr/AGENTS.md`](adr/AGENTS.md) for maintenance conventions.
771+
751772
## See Also
752773
753774
- **[README.md](README.md)** - Project overview, installation, what is DevWorkspace Operator
754775
- **[CONTRIBUTING.md](CONTRIBUTING.md)** - Development setup, debugging, testing, contribution guidelines
755776
- **[redhat-compliance-and-responsible-ai.md](redhat-compliance-and-responsible-ai.md)** - Red Hat compliance & responsible AI rules
777+
- **[adr/](adr/)** - Architecture Decision Records
756778
- **[docs/additional-configuration.adoc](docs/additional-configuration.adoc)** - DevWorkspace configuration options
757779
- **[docs/unsupported-devfile-api.adoc](docs/unsupported-devfile-api.adoc)** - Unsupported Devfile API features
758780
- **[Devfile Documentation](https://devfile.io/)** - Devfile specification
759781
- **[Kubebuilder Book](https://book.kubebuilder.io/)** - Controller patterns and best practices
760782
761783
---
762784
763-
**Last Updated**: 2025-12-24
785+
**Last Updated**: 2026-05-15
Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
# Backup Auth Secret Must Not Be Owned by Any Workspace
2+
3+
**Status**: Accepted
4+
**Date**: 2026-05-11
5+
**Deciders**: DevWorkspace Operator maintainers
6+
**Related Issue**: [CRW-10760](https://redhat.atlassian.net/browse/CRW-10760)
7+
8+
## Context
9+
10+
The backup system copies the registry authentication secret (e.g., quay.io credentials) from the operator namespace into each workspace namespace as `devworkspace-backup-registry-auth`. The original implementation set a Kubernetes controller ownerReference from this secret to the DevWorkspace that triggered the copy:
11+
12+
```go
13+
controllerutil.SetControllerReference(workspace, desiredSecret, scheme)
14+
```
15+
16+
This was likely intended to clean up the secret when no workspaces need it anymore — standard Kubernetes garbage collection pattern.
17+
18+
However, two properties of this secret make ownerReference-based lifecycle management incorrect:
19+
20+
1. **The secret is a namespace singleton**: All workspaces in a namespace share the same `devworkspace-backup-registry-auth` secret, but a Kubernetes object can have only one controller owner. Whichever workspace's backup job ran last "wins" ownership.
21+
22+
2. **The secret is needed after all workspaces are deleted**: The primary restore use case is creating a new workspace from a backup of a deleted one. If the auth secret is garbage-collected when the last workspace is deleted, the user cannot authenticate to the private registry to pull the backup image.
23+
24+
**Bug observed (CRW-10760)**: When using quay.io (private registry) for backups, deleting a workspace caused backup entries to disappear from the Dashboard backup list for ALL workspaces in the namespace. The auth secret was garbage-collected, and the Dashboard could no longer query the registry.
25+
26+
**Validated on CRC cluster** (DWO 0.40.1, quay.io/okurinny):
27+
- The `devworkspace-backup-registry-auth` secret was confirmed to have ownerReference to a single workspace (`nodejs`)
28+
- Deleting that workspace triggered K8s GC, removing the secret
29+
- The secret was not re-created by subsequent backup cycles (remaining workspaces already had recent backups)
30+
- Backup listing in the Dashboard failed for all workspaces
31+
32+
## Decision
33+
34+
**Remove the ownerReference from the backup registry auth secret.** The secret becomes a namespace-scoped resource with no ownership tie to any workspace.
35+
36+
### What Changes
37+
38+
- `pkg/secrets/backup.go`: Remove the `controllerutil.SetControllerReference()` call in `CopySecret()`
39+
- The secret is still created via `c.Create()` with `AlreadyExists` handling, just without an ownerReference
40+
- The restore path now resolves the operator namespace via `infrastructure.GetNamespace()` to copy the secret on demand when it is missing
41+
42+
### What Doesn't Change
43+
44+
- Per-workspace resources (job runner ServiceAccount, image-builder RoleBinding) retain their ownerReferences — their GC on workspace deletion is correct and expected
45+
- The backup Job itself retains its ownerReference (short-lived, TTL-cleaned)
46+
- The `CopySecret` function signature stays the same
47+
48+
## Considered Alternatives
49+
50+
### Alternative 1: Multi-owner references (non-controller)
51+
52+
Add each workspace as a non-controller owner of the secret. K8s GC would only delete the secret when ALL owning workspaces are gone.
53+
54+
**Rejected because**:
55+
- The secret must survive deletion of ALL workspaces (for restore)
56+
- Adds complexity to track and merge owner lists
57+
- Non-controller ownerReferences have subtle GC semantics
58+
59+
### Alternative 2: Finalizer-based cleanup
60+
61+
Remove ownerReference but add a cleanup mechanism (e.g., a controller that deletes the secret when the namespace has zero DevWorkspaces).
62+
63+
**Rejected because**:
64+
- Adds complexity for a marginal benefit (one small secret in an otherwise-empty namespace)
65+
- Could race with restore operations (user creates a workspace from backup right after the last one is deleted)
66+
- Namespace deletion already cleans up all resources
67+
68+
### Alternative 3: Per-workspace auth secrets
69+
70+
Create a unique auth secret per workspace (e.g., `devworkspace-backup-registry-auth-{workspace-id}`).
71+
72+
**Rejected because**:
73+
- Multiplies secrets unnecessarily (all contain the same credentials)
74+
- The restore path expects the predefined name `devworkspace-backup-registry-auth`
75+
- Still wouldn't survive workspace deletion for restore use case
76+
77+
## Consequences
78+
79+
### Positive
80+
81+
1. **Backups survive workspace deletion**: Users can delete all workspaces and still restore from backups
82+
2. **No cross-workspace interference**: Deleting one workspace no longer affects other workspaces' backup capabilities
83+
3. **Simpler lifecycle**: No ownership tracking needed for a namespace-scoped singleton
84+
85+
### Negative
86+
87+
1. **Secret persists in empty namespaces**: If all workspaces are deleted and the user never restores, the auth secret remains until the namespace is deleted. This is a minor leak — one small secret per namespace.
88+
89+
### Neutral
90+
91+
1. **Existing secrets on upgraded clusters**: Secrets created by older DWO versions will retain their stale ownerReference until the next `CopySecret` call overwrites them (via `SyncObjectWithCluster`). In the worst case, one more GC event occurs before the fix takes effect.
92+
93+
## References
94+
95+
- `pkg/secrets/backup.go``CopySecret()` function
96+
- `controllers/backupcronjob/rbac.go` — Per-workspace SA/RoleBinding (unchanged)
97+
- `pkg/constants/metadata.go:204``DevWorkspaceBackupAuthSecretName`

adr/AGENTS.md

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
# ADR Maintenance Guide for AI Agents
2+
3+
This guide covers how to create and maintain ADRs. For when to create an ADR, see the root [AGENTS.md](../AGENTS.md).
4+
5+
## Creating a New ADR
6+
7+
1. Copy [TEMPLATE.md](TEMPLATE.md) to a new file named `YYYY-MM-DD-short-slug.md`
8+
2. Use today's date and a descriptive lowercase slug with hyphens
9+
3. Fill in all sections — the "Considered Alternatives" section is the most valuable part
10+
4. Set status to **Proposed** if seeking feedback, or **Accepted** if the decision is final
11+
12+
## Naming Convention
13+
14+
`YYYY-MM-DD-short-description.md`
15+
16+
- Date prefix for chronological ordering
17+
- Lowercase, hyphens only, no special characters
18+
- Keep the slug short but descriptive
19+
20+
## Superseding an ADR
21+
22+
When a decision is replaced:
23+
24+
1. Update the old ADR's status line: `**Status**: Superseded by [New Title](YYYY-MM-DD-new-slug.md)`
25+
2. Create the new ADR with its own context explaining why the previous decision changed
26+
3. Reference the old ADR in the new one's Context section
27+
28+
## Format Rules
29+
30+
- Follow the structure in [TEMPLATE.md](TEMPLATE.md)
31+
- End every file with a trailing newline
32+
- Use the existing ADRs as examples for tone and level of detail

adr/README.md

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
# Architecture Decision Records (ADRs)
2+
3+
This directory contains Architecture Decision Records for the DevWorkspace Operator.
4+
5+
## What is an ADR?
6+
7+
An ADR captures a significant design decision along with its context, alternatives considered, and trade-offs accepted. ADRs preserve the **why** behind decisions — information that is lost in code, commit messages, and PR descriptions over time.
8+
9+
## When to Write an ADR
10+
11+
Write an ADR when your change involves any of these:
12+
13+
1. **You rejected an alternative** — There were 2+ reasonable approaches and you picked one. The code shows what you chose but not what you didn't.
14+
2. **You accepted a trade-off** — Something got worse (performance, complexity, a minor leak) in exchange for something more important.
15+
3. **You changed a resource lifecycle or ownership model** — Who creates, owns, or cleans up a Kubernetes resource.
16+
4. **You changed an external contract** — API shape, CRD fields, Secret/ConfigMap naming conventions, image paths.
17+
18+
**Don't** write an ADR for: bug fixes, dependency bumps, refactors preserving behavior, test additions, docs updates, or performance optimizations with no trade-offs.
19+
20+
## Lifecycle
21+
22+
| Status | Meaning |
23+
|--------|---------|
24+
| **Proposed** | Under discussion, not yet accepted |
25+
| **Accepted** | Decision is in effect |
26+
| **Deprecated** | Decision is outdated but not yet replaced |
27+
| **Superseded** | Replaced by a newer ADR (link to it in the status line) |
28+
29+
To supersede an ADR, update its status to `Superseded by [new ADR title](new-adr-file.md)` and create a new ADR.
30+
31+
## Naming Convention
32+
33+
Files use date-prefixed slugs: `YYYY-MM-DD-short-description.md`
34+
35+
Example: `2026-05-11-backup-auth-secret-lifecycle.md`
36+
37+
## Creating a New ADR
38+
39+
Copy [TEMPLATE.md](TEMPLATE.md) and fill in the sections. See existing ADRs for examples.

adr/TEMPLATE.md

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
# [Title of Decision]
2+
3+
**Status**: Proposed | Accepted | Deprecated | Superseded by [title](link)
4+
**Date**: YYYY-MM-DD
5+
**Deciders**: DevWorkspace Operator maintainers
6+
**Related Issue**: [ISSUE-ID](link) *(optional)*
7+
8+
## Context
9+
10+
What is the problem or need that prompted this decision? Include enough background for someone unfamiliar with the issue to understand why a decision was needed.
11+
12+
## Decision
13+
14+
What did we decide to do? Be specific about what changes and what stays the same.
15+
16+
## Considered Alternatives
17+
18+
### Alternative 1: [Name]
19+
20+
Brief description.
21+
22+
**Rejected because**:
23+
- Reason 1
24+
- Reason 2
25+
26+
### Alternative 2: [Name]
27+
28+
Brief description.
29+
30+
**Rejected because**:
31+
- Reason 1
32+
- Reason 2
33+
34+
## Consequences
35+
36+
### Positive
37+
38+
1. Benefit 1
39+
2. Benefit 2
40+
41+
### Negative
42+
43+
1. Downside or accepted trade-off
44+
45+
### Neutral
46+
47+
1. Side effects that are neither positive nor negative
48+
49+
## References
50+
51+
- Links to relevant code, docs, PRs, or external resources

0 commit comments

Comments
 (0)