feat: add support for custom workspacetypes for newly created organizations (clean version) #42
Conversation
WalkthroughAdds coverage exclusions. Updates go.mod to use kcp-dev Kubernetes staging and bumps dependencies. Extends operator config with KCP root host and org workspace cluster. Enhances account controller to build optional root-scoped client and wire new WorkspaceType subroutine. Introduces WorkspaceType subroutine, name utilities, pointer change in Workspace spec, and comprehensive tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Tip 🧪 Early access (models): enabledWe are currently testing Sonnet 4.5 code review models, which should lead to better review quality. However, this model may result in higher noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
pkg/subroutines/common.go (1)
46-72: Review length calculation logic after prefix/suffix additions.The function adds prefixes (
"w-") or suffixes ("w") to ensure valid start/end characters (lines 53-59), then checks length (line 61). However, if the input is close to 63 characters, adding a prefix or suffix could push it over the limit before the hash-truncation logic runs.For example:
- Input: 62-char string starting with hyphen
- After line 54: "w-" + 62 chars = 64 chars
- Line 61 then truncates, but the prefix is already included
This is likely acceptable since the hash logic will handle it, but consider whether the length check should happen before or account for prefix/suffix additions more explicitly.
Consider this alternative ordering to clarify intent:
func sanitizeForKubernetes(name string) string { if name == "" { return "" } sanitized := validCharsRegex.ReplaceAllString(strings.ToLower(name), "-") if len(sanitized) > 0 && !startsWithAlphaRegex.MatchString(sanitized) { sanitized = "w-" + sanitized } if len(sanitized) > 0 && !endsWithAlphanumericRegex.MatchString(sanitized) { sanitized = sanitized + "w" } + // Length check after prefix/suffix ensures we handle the final length if len(sanitized) > 63 { hash := fmt.Sprintf("%x", sha256.Sum256([]byte(name)))[:8] maxPrefix := 63 - len(hash) - 1 if maxPrefix > 0 { sanitized = sanitized[:maxPrefix] + "-" + hash } else { sanitized = "w-" + hash } } return sanitized }Note: The current implementation is functionally correct; this is just a clarity suggestion.
pkg/subroutines/workspace_common_test.go (1)
124-160: Consider more explicit hash validation.The test indirectly validates hashing by checking for hyphens (line 156). While this works, you could make it more explicit by checking for a hash pattern (e.g., 8 hex digits at the end).
Optional enhancement:
if tt.expectHash { - // If name is long enough to require hashing, result should contain a hash - assert.Contains(t, result, "-", "Hashed names should contain hyphens") + // Verify hash format: should end with -<8 hex digits> + assert.Regexp(t, "-[0-9a-f]{8}$", result, "Hashed names should end with -<8 hex chars>") }pkg/subroutines/workspacetype.go (1)
204-242: Verify deduplication logic prevents unbounded growth.Lines 218-231 implement deduplication to avoid adding the same custom org reference multiple times to
baseOrg.Spec.LimitAllowedChildren.Types. This is critical because:
- Without deduplication, each reconcile would append a duplicate entry
- The slice could grow unbounded over time
- KCP might reject updates if the same reference appears multiple times
The logic looks correct: it iterates through existing types and only appends if no match is found. However, ensure that:
- Both
NameandPathare checked (line 222) - ✓ Correct- The comparison is case-sensitive (KCP type names are case-sensitive) - ✓ Correct
Minor suggestion: Consider whether the order of types matters. If KCP or other consumers depend on a stable order, this append-only approach could cause issues when types are added in different orders across reconciles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
.testcoverage.yml(1 hunks)go.mod(7 hunks)internal/config/config.go(2 hunks)internal/controller/account_controller.go(2 hunks)internal/controller/account_controller_test.go(1 hunks)pkg/subroutines/common.go(2 hunks)pkg/subroutines/workspace.go(1 hunks)pkg/subroutines/workspace_common_test.go(1 hunks)pkg/subroutines/workspacetype.go(1 hunks)pkg/subroutines/workspacetype_test.go(1 hunks)
🔇 Additional comments (37)
go.mod (5)
94-94: KCP apimachinery version is aligned.The indirect dependency on
kcp-dev/apimachinery/v2uses a pseudo-version from July 28, 2025, which predates the Kubernetes staging modules (Sept 3). This ordering is appropriate as it's a foundational dependency.
118-118: Mockery added as development tool.The addition of
vektra/mockery/v2as an indirect dependency indicates the project now uses mockery for generating test mocks, which is a standard Go testing practice.
70-70: Indirect dependency updates look appropriate.The updates to
go-restful/v3,go-openapi/jsonreference,mailru/easyjson, andgolang.org/x/timeare consistent with the Kubernetes staging module updates and appear to be standard transitive dependency resolution.Also applies to: 82-82, 95-95, 137-137
46-46: Verify prometheus/client_golang version – consider updating to latest.The Prometheus client library v1.22.0 has no known security vulnerabilities. The only advisory on record (HIGH severity) affects versions < 1.11.1, which was patched in v1.11.1. However, v1.22.0 is outdated—the latest stable release is v1.23.2 (published September 5, 2025). Review the changelog and consider updating to benefit from recent improvements and fixes.
146-146: Verify and update sigs.k8s.io/json to latest version.The
sigs.k8s.io/jsonindirect dependency uses pseudo-versionv0.0.0-20241014173422-cfa47c3a1cc8(October 14, 2024), but newer commits are available in the upstream repository—including one from July 30, 2025 (2d320260d730) that syncs with Go 1.24'sencoding/jsonimplementation. Consider updating this dependency by running:go get -u sigs.k8s.io/json@latest go mod tidyThen verify the build and tests pass with the updated version.
.testcoverage.yml (1)
8-9: Coverage exclusions are reasonable for KCP-dependent code.The exclusions for controller integration tests and WorkspaceType subroutine are pragmatic given the KCP setup complexity. The TODO on line 9 appropriately tracks the technical debt.
internal/config/config.go (2)
12-14: LGTM! WorkspaceType configuration follows established patterns.The new
WorkspaceType.Enabledfield is properly structured with mapstructure tag and defaults totrue, consistent with other subroutine toggles.
33-34: Empty-string defaults are handled correctly.Verification confirms both fields have proper safeguards:
- RootHost: Lines 61-64 in
internal/controller/account_controller.gocheckif rootHost == ""and fall back tomgr.GetConfig().Host- OrgWorkspaceCluster: Lines 65-66 in
pkg/subroutines/workspacetype.gocheckif cfg.Kcp.OrgWorkspaceCluster != ""and default tocurrentPathwhen emptyNo runtime errors or unexpected behavior will occur from empty defaults.
pkg/subroutines/common.go (4)
29-33: LGTM! Regex patterns are compiled at package level.The package-level compilation of regex patterns is efficient and follows Go best practices.
35-44: LGTM! Path extraction logic is sound.The function correctly extracts the last segment from colon-separated paths and handles edge cases (empty string, no colons) appropriately.
74-80: LGTM! Public API wrappers are clean and clear.The public helper functions provide a clear, type-safe API for generating org and account workspace type names.
82-92: LGTM! Workspace type name generation is clear.The helper correctly incorporates workspace name when present and delegates sanitization appropriately.
pkg/subroutines/workspace_common_test.go (4)
9-48: LGTM! Comprehensive test coverage for path extraction.The test cases cover all relevant scenarios including edge cases (empty path, root-only path).
50-110: Excellent test coverage with runtime validation.The test suite covers edge cases and validates results against Kubernetes naming rules (lines 104-107), which is a best practice.
112-122: LGTM! Tests validate wrapper function delegation.Both tests confirm that the public API correctly delegates to the internal helper with the appropriate suffix.
162-194: Test reveals potential length constraint issue.The test only validates the 63-character limit for the "simple case" (lines 186-188), suggesting awareness that long names might violate Kubernetes naming constraints. This confirms the concern raised about
getAuthConfigNamepotentially exceeding 63 characters when appending "-auth".Consider adding explicit validation that all generated names meet Kubernetes constraints, or update
getAuthConfigNameto sanitize the final result as suggested in the earlier comment.internal/controller/account_controller_test.go (1)
324-326: LGTM - Early skip prevents unnecessary setup.The skip is correctly placed before
suite.Run, preventing the integration test from running when KCP is not configured. The message clearly directs developers to unit tests for coverage.pkg/subroutines/workspacetype_test.go (6)
19-48: LGTM - Basic lifecycle methods are well tested.The tests for
GetName,Finalizers,Finalize, andProcess(non-org account) correctly verify the expected behavior and early returns.
50-63: LGTM - Cluster path resolution is tested for both contexts.The test covers both scenarios: with and without cluster context, ensuring
getCurrentClusterPathfalls back toProviderWorkspacewhen no cluster is in context.
65-79: LGTM - Factory helper tests are straightforward.Tests for
createWorkspaceTypeReferenceandcreateWorkspaceTypeSelectorverify correct construction of KCP workspace type references and selectors.
81-89: LGTM - Root client constructor is tested.The test verifies that
NewWorkspaceTypeSubroutineWithRootClientcorrectly assigns both the client and rootClient fields.
106-121: LGTM - Cluster path logic is tested with config variations.The test verifies
getCurrentClusterPathbehavior with and withoutOrgWorkspaceClusterconfiguration, confirming it prioritizes context cluster over config.
91-104: No issue found - early return prevents nil client access.The
Processmethod implementation confirms that the early return for non-org accounts (lines 54-57 inworkspacetype.go) executes before anyr.clientusage. The firstr.clientaccess occurs at line 86 withincreateCustomAccountWorkspaceType, which is only reached for organization accounts. The test correctly validates this early-return path without risk of NPE.internal/controller/account_controller.go (4)
44-50: LGTM - RBAC markers updated for WorkspaceType resources.The kubebuilder RBAC markers correctly grant the necessary permissions for managing
workspacetypesand their status subresources.
73-79: LGTM - Root client creation has appropriate error handling.The code creates a root-scoped client with proper error handling, logging a warning and falling back to the shared manager client if creation fails. This ensures the operator continues functioning even if root client creation fails.
81-86: LGTM - Conditional initialization handles both root and non-root client scenarios.The logic correctly chooses between
NewWorkspaceTypeSubroutineWithRootClient(when rootClient is available) andNewWorkspaceTypeSubroutine(fallback), ensuring the subroutine is always initialized when enabled.
59-71: Verify virtual workspace path stripping handles edge cases correctly.The logic strips
/services/from the virtual workspace URL to derive the root host. Based on kcp documentation, virtual workspace URLs follow the patternhttps://<kcp-server>/services/<service-name>/..., so usingstrings.Indexto find the first occurrence is appropriate for the standard format.However, the implementation lacks:
- Tests: No unit tests exist for this URL parsing logic, despite the comment in
account_controller_test.gosuggesting coverage exists- Edge case validation: The code doesn't guard against:
- Malformed URLs with multiple
/services/segments (though unlikely in practice)- Query parameters or fragments in the Host field (uncommon but possible)
- Empty or invalid Host values
Recommendations:
- Add unit tests covering normal cases (
https://kcp.example.com:6443/services/workspaces/org/apis/...→https://kcp.example.com:6443)- Consider adding validation or logging when stripping occurs to aid debugging
- Verify in your environment that
mgr.GetConfig().Hostcontains the expected virtual workspace URL formatpkg/subroutines/workspacetype.go (10)
35-41: LGTM - Constructors correctly initialize client fields.Both constructors properly initialize the
WorkspaceTypeSubroutine. The default constructor uses the same client for both fields, while the root client variant allows separate clients for different scopes.
43-49: LGTM - Lifecycle methods correctly implement no-op behavior.
GetNamereturns the constant name, and bothFinalizersandFinalizecorrectly return nil/empty as this subroutine doesn't require finalization logic.
51-102: LGTM - Process method has clear flow and good error handling.The main Process method:
- Correctly filters to only process organization accounts (lines 54-57)
- Derives cluster paths with fallback logic (lines 63-68)
- Fetches base types with error handling (lines 71-74)
- Creates custom workspace types in sequence (lines 81-88)
- Updates the base org type (lines 91-93)
- Logs success (lines 95-99)
The sequence is logical and error handling is consistent throughout.
105-111: LGTM - Cluster path extraction handles both context and config.The method correctly prioritizes the cluster from context over the configured
ProviderWorkspace, falling back gracefully when no cluster is in context.
114-136: LGTM - Base workspace type fetching has robust error handling.The method:
- Uses
rootClientfor fetching base types from the root workspace (correct scope)- Distinguishes between "not found" (expected, continues with nil) and other errors (propagates)
- Logs debug messages for not-found cases
- Returns both base types for use downstream
139-162: LGTM - Custom account workspace type creation follows KCP patterns.The method:
- Uses
CreateOrUpdatefor idempotency- Correctly copies spec from base type if available
- Sets up proper inheritance with
Extend(line 149-153)- Restricts parent to the custom org type (lines 156-158)
The implementation follows KCP workspace type conventions.
165-201: LGTM - Custom org workspace type creation is well structured.The method correctly:
- Extends the base org type (lines 172-176)
- Sets the default child to the custom account type (lines 179-180)
- Restricts parents and children appropriately (lines 183-190)
- Configures authentication with custom config name (lines 193-196)
The implementation is comprehensive and follows best practices.
234-237: LGTM - Forbidden error handling is appropriate.Lines 234-236 gracefully handle
IsForbiddenerrors when updating the base org type, logging a debug message and continuing. This is correct because:
- The operator may not have permissions to update root-level types
- The custom types were already created successfully
- Treating this as success prevents unnecessary retry loops
244-257: LGTM - Factory functions are clean and reusable.The helper functions
createWorkspaceTypeReferenceandcreateWorkspaceTypeSelectorprovide clean abstractions for creating KCP workspace type references and selectors, improving readability throughout the file.
193-193:getAuthConfigNameis properly defined.The function
getAuthConfigNameis defined inpkg/subroutines/common.goat line 95 and is accessible since it's in the same package. The call at line 193 correctly matches the function signature.Likely an incorrect or invalid review comment.
| k8s.io/api => github.com/kcp-dev/kubernetes/staging/src/k8s.io/api v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/apimachinery => github.com/kcp-dev/kubernetes/staging/src/k8s.io/apimachinery v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/apiserver => github.com/kcp-dev/kubernetes/staging/src/k8s.io/apiserver v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/client-go => github.com/kcp-dev/kubernetes/staging/src/k8s.io/client-go v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/cloud-provider => github.com/kcp-dev/kubernetes/staging/src/k8s.io/cloud-provider v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/cluster-bootstrap => github.com/kcp-dev/kubernetes/staging/src/k8s.io/cluster-bootstrap v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/code-generator => github.com/kcp-dev/kubernetes/staging/src/k8s.io/code-generator v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/component-base => github.com/kcp-dev/kubernetes/staging/src/k8s.io/component-base v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/component-helpers => github.com/kcp-dev/kubernetes/staging/src/k8s.io/component-helpers v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/controller-manager => github.com/kcp-dev/kubernetes/staging/src/k8s.io/controller-manager v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/cri-api => github.com/kcp-dev/kubernetes/staging/src/k8s.io/cri-api v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/cri-client => github.com/kcp-dev/kubernetes/staging/src/k8s.io/cri-client v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/csi-translation-lib => github.com/kcp-dev/kubernetes/staging/src/k8s.io/csi-translation-lib v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/dynamic-resource-allocation => github.com/kcp-dev/kubernetes/staging/src/k8s.io/dynamic-resource-allocation v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/endpointslice => github.com/kcp-dev/kubernetes/staging/src/k8s.io/endpointslice v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/externaljwt => github.com/kcp-dev/kubernetes/staging/src/k8s.io/externaljwt v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/kms => github.com/kcp-dev/kubernetes/staging/src/k8s.io/kms v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/kube-aggregator => github.com/kcp-dev/kubernetes/staging/src/k8s.io/kube-aggregator v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/kube-controller-manager => github.com/kcp-dev/kubernetes/staging/src/k8s.io/kube-controller-manager v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/kube-proxy => github.com/kcp-dev/kubernetes/staging/src/k8s.io/kube-proxy v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/kube-scheduler => github.com/kcp-dev/kubernetes/staging/src/k8s.io/kube-scheduler v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/kubectl => github.com/kcp-dev/kubernetes/staging/src/k8s.io/kubectl v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/kubelet => github.com/kcp-dev/kubernetes/staging/src/k8s.io/kubelet v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/kubernetes => github.com/kcp-dev/kubernetes v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/metrics => github.com/kcp-dev/kubernetes/staging/src/k8s.io/metrics v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/mount-utils => github.com/kcp-dev/kubernetes/staging/src/k8s.io/mount-utils v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/pod-security-admission => github.com/kcp-dev/kubernetes/staging/src/k8s.io/pod-security-admission v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/sample-apiserver => github.com/kcp-dev/kubernetes/staging/src/k8s.io/sample-apiserver v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/sample-cli-plugin => github.com/kcp-dev/kubernetes/staging/src/k8s.io/sample-cli-plugin v0.0.0-20250903080753-82bf1892069b | ||
| k8s.io/sample-controller => github.com/kcp-dev/kubernetes/staging/src/k8s.io/sample-controller v0.0.0-20250903080753-82bf1892069b |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the kcp-dev/kubernetes staging commit exists.
All k8s.io module replacements consistently point to the same pseudo-version (commit 82bf1892069b from Sept 3, 2025), which ensures alignment across the Kubernetes dependency graph.
Run the following script to verify the commit exists in the kcp-dev/kubernetes repository:
🏁 Script executed:
#!/bin/bash
# Description: Verify that the specified commit exists in kcp-dev/kubernetes
COMMIT="82bf1892069b"
REPO="kcp-dev/kubernetes"
# Check if commit exists using GitHub API
gh api "repos/${REPO}/commits/${COMMIT}" --jq '.sha, .commit.author.date, .commit.message' 2>&1 || echo "Commit not found or API error"Length of output: 245
🏁 Script executed:
#!/bin/bash
# Check the full go.mod file to understand all changes
cat go.modLength of output: 9223
🏁 Script executed:
#!/bin/bash
# Check if there are any security advisories for the dependencies being updated
# Check prometheus/client_golang v1.22.0
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/prometheus/client_golang") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Length of output: 258
🏁 Script executed:
#!/bin/bash
# Search for any usage of prometheus client_golang in the codebase to understand the impact
rg -n --type=go "prometheus/client_golang" -C 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any prometheus-related imports or usage patterns in Go files
rg -n --type=go -i "prometheus" -C 1 | head -100Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if prometheus/client_golang appears in go.sum to see if it was already present
cat go.sum | grep "prometheus/client_golang" | head -10Length of output: 190
🏁 Script executed:
#!/bin/bash
# Look for metrics registration or prometheus handler setup
rg -n --type=go "metrics\." -A 2 -B 2 | head -50Length of output: 0
🏁 Script executed:
#!/bin/bash
# Use go mod why to understand why prometheus/client_golang is needed
cd /tmp && git clone --depth 1 --branch main https://github.com/platform-mesh/account-operator.git test-repo 2>&1 | head -5
cd test-repo 2>/dev/null && go mod why github.com/prometheus/client_golangLength of output: 7847
🏁 Script executed:
#!/bin/bash
# Check the git diff to see what actually changed in go.mod
git log --oneline -1
git diff HEAD~1 HEAD -- go.mod 2>/dev/null || echo "Cannot determine git diff"Length of output: 75
🏁 Script executed:
#!/bin/bash
# List all dependencies that might use prometheus
cat go.mod | grep -E "(controller-runtime|kcp/sdk)" -A 2 -B 2Length of output: 426
Move prometheus/client_golang to indirect dependencies.
The go mod why analysis confirms that github.com/prometheus/client_golang v1.22.0 is not directly used by the main module and should be marked as // indirect or removed from the direct require block. Go modules will automatically manage it as a transitive dependency.
All k8s.io module replacements correctly point to the verified kcp-dev/kubernetes commit 82bf1892069b (from Aug 28, 2025), ensuring consistency across the Kubernetes dependency graph. Security scan shows v1.22.0 is not affected by known vulnerabilities.
Action required: In go.mod line 46, either:
- Remove
github.com/prometheus/client_golang v1.22.0entirely and rungo mod tidy, or - Add
// indirectcomment:github.com/prometheus/client_golang v1.22.0 // indirect
🤖 Prompt for AI Agents
In go.mod around lines 8-37 (issue noted at line 46), the direct require for
github.com/prometheus/client_golang v1.22.0 is unnecessary; mark it as an
indirect dependency or remove it. Either add the comment “// indirect” to that
require entry or delete the line and run `go mod tidy` to let Go manage the
transitive dependency; ensure go.sum is updated accordingly and commit the
resulting changes.
|
|
||
| require ( | ||
| github.com/kcp-dev/kcp/sdk v0.27.1 | ||
| github.com/kcp-dev/kcp/sdk v0.28.1-0.20250915073746-2b42b96efc54 |
There was a problem hiding this comment.
🧩 Analysis chain
Verify KCP SDK pseudo-version compatibility.
The KCP SDK is pinned to a pseudo-version v0.28.1-0.20250915073746-2b42b96efc54 (Sept 15, 2025), which is 12 days newer than the Kubernetes staging modules commit (Sept 3, 2025). Ensure this version is compatible with the staged k8s.io modules.
Run the following script to verify the commit exists and check for compatibility notes:
🏁 Script executed:
#!/bin/bash
# Description: Verify KCP SDK commit and check for release notes
COMMIT="2b42b96efc54"
REPO="kcp-dev/kcp"
# Check commit details
echo "=== Commit Details ==="
gh api "repos/${REPO}/commits/${COMMIT}" --jq '.sha, .commit.author.date, .commit.message' 2>&1
# Check if there's a v0.28.1 tag and its notes
echo -e "\n=== v0.28.1 Release Info ==="
gh api "repos/${REPO}/releases/tags/v0.28.1" --jq '.tag_name, .published_at, .body' 2>&1 || echo "Release v0.28.1 not found"
# List recent tags to understand version progression
echo -e "\n=== Recent Tags ==="
gh api "repos/${REPO}/tags?per_page=10" --jq '.[].name' 2>&1Length of output: 1392
🏁 Script executed:
#!/bin/bash
# Check release notes for v0.28.2 and v0.28.3 to understand what's missing
REPO="kcp-dev/kcp"
echo "=== v0.28.2 Release Info ==="
gh api "repos/${REPO}/releases/tags/v0.28.2" --jq '.tag_name, .published_at, .body' 2>&1
echo -e "\n=== v0.28.3 Release Info ==="
gh api "repos/${REPO}/releases/tags/v0.28.3" --jq '.tag_name, .published_at, .body' 2>&1
echo -e "\n=== Compare v0.28.1 and commit 2b42b96efc54 ==="
gh api "repos/${REPO}/compare/v0.28.1...2b42b96efc54" --jq '.total_commits, .commits[].commit.message' 2>&1 | head -20Length of output: 7887
🏁 Script executed:
#!/bin/bash
# Check if v0.28.4-rc.0 has any important updates
REPO="kcp-dev/kcp"
echo "=== v0.28.4-rc.0 Release Info ==="
gh api "repos/${REPO}/releases/tags/v0.28.4-rc.0" --jq '.tag_name, .published_at, .body' 2>&1Length of output: 191
Update KCP SDK to v0.28.3 to include security fix.
The pseudo-version v0.28.1-0.20250915073746-2b42b96efc54 (September 15, 2025) predates v0.28.3 (September 26, 2025), which contains a security fix for GHSA-q6hv-wcjr-wp8h addressing object validation in virtual workspaces. Update to:
github.com/kcp-dev/kcp/sdk v0.28.3
Note: v0.28.2 failed its release pipeline and should not be used. v0.28.3 also updates the Kubernetes staging modules dependencies, which may improve compatibility with the k8s.io modules at line 3-17.
🤖 Prompt for AI Agents
In go.mod around line 41, the KCP SDK is pinned to a pseudo-version
v0.28.1-0.20250915073746-2b42b96efc54 which predates the security-fix release;
replace that line with github.com/kcp-dev/kcp/sdk v0.28.3 and then run `go mod
tidy` (and rebuild/tests) to update transitive Kubernetes staging modules and
ensure module graph consistency.
| // getAuthConfigName generates a consistent name for the WorkspaceAuthenticationConfiguration | ||
| func getAuthConfigName(accountName, currentPath string) string { | ||
| return fmt.Sprintf("%s-auth", GetOrgWorkspaceTypeName(accountName, currentPath)) | ||
| } |
There was a problem hiding this comment.
Potential length constraint violation when adding "-auth" suffix.
GetOrgWorkspaceTypeName returns a sanitized name up to 63 characters. Appending "-auth" (5 characters) could result in a name exceeding the Kubernetes 63-character limit for DNS labels.
Consider sanitizing the final result:
func getAuthConfigName(accountName, currentPath string) string {
- return fmt.Sprintf("%s-auth", GetOrgWorkspaceTypeName(accountName, currentPath))
+ name := fmt.Sprintf("%s-auth", GetOrgWorkspaceTypeName(accountName, currentPath))
+ return sanitizeForKubernetes(name)
}Alternatively, ensure GetOrgWorkspaceTypeName leaves room for the suffix (max 58 chars instead of 63).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/subroutines/common.go around lines 94 to 97, appending the literal
"-auth" to the value returned by GetOrgWorkspaceTypeName can push the result
past Kubernetes' 63-character DNS label limit; modify getAuthConfigName to
ensure the final name is <=63 chars by reserving space for the suffix: truncate
the base name returned by GetOrgWorkspaceTypeName to a maximum of 58 characters
(or call the existing sanitizer with a max-length parameter if available), then
append "-auth", and finally run the sanitizer/validation step on the combined
string so the returned name is guaranteed to conform to the 63-character DNS
label constraint.
| createdWorkspace.Spec.Type = &kcptenancyv1alpha.WorkspaceTypeReference{ | ||
| Name: kcptenancyv1alpha.WorkspaceTypeName(instance.Spec.Type), | ||
| Path: cfg.Kcp.ProviderWorkspace, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify pointer semantics for WorkspaceTypeReference.
The change from a value to a pointer (&kcptenancyv1alpha.WorkspaceTypeReference) is a potentially breaking API modification. Ensure that:
- This aligns with upstream KCP API expectations for
Workspace.Spec.Type - All code reading this field handles the pointer correctly (nil checks where needed)
- The pointer is intentional and not a workaround
Run the following script to verify the field type in KCP and check for other usages:
🏁 Script executed:
#!/bin/bash
# Description: Verify WorkspaceTypeReference pointer usage across codebase
# Find the Workspace type definition to confirm the expected type
echo "=== Checking Workspace.Spec.Type field definition ==="
ast-grep --pattern $'type Workspace struct {
$$$
Spec WorkspaceSpec
$$$
}'
echo -e "\n=== Checking WorkspaceSpec.Type field ==="
ast-grep --pattern $'type WorkspaceSpec struct {
$$$
Type $TYPE
$$$
}'
# Search for other places that access or assign Workspace.Spec.Type
echo -e "\n=== Other usages of Workspace.Spec.Type ==="
rg -nP --type=go -C3 '\.Spec\.Type\s*[=:]'Length of output: 2590
🏁 Script executed:
#!/bin/bash
# Check imports to find KCP tenancy package source and search for any vendored definitions
echo "=== Finding KCP tenancy imports ==="
rg -nP --type=go "kcptenancyv1alpha|kcp.*tenancy.*v1alpha" | head -20
echo -e "\n=== Checking go.mod for KCP dependencies ==="
if [ -f go.mod ]; then
grep -i "kcp" go.mod
fi
echo -e "\n=== Looking for vendored or generated KCP types ==="
fd -t f "workspace.*\.go$" vendor/ 2>/dev/null | head -10
fd -t f "workspace.*\.go$" pkg/apis/ 2>/dev/null | head -10
echo -e "\n=== Checking for any WorkspaceTypeReference definition in repo ==="
rg -nP --type=go "type WorkspaceTypeReference" -A5Length of output: 6066
🌐 Web query:
kcp-dev tenancy v1alpha1 WorkspaceSpec Type field pointer
💡 Result:
WorkspaceSpec.Type is a value (type WorkspaceTypeReference), not a pointer. [1][2]
Sources:
- Go API (tenancy v1alpha1) — WorkspaceSpec definition. [1]
- kcp docs — example showing spec.type usage. [2]
Critical: Remove pointer - WorkspaceSpec.Type expects a value, not a pointer.
The KCP tenancy v1alpha1 API defines WorkspaceSpec.Type as WorkspaceTypeReference (value), not *WorkspaceTypeReference (pointer). This change introduces a type mismatch that conflicts with the upstream API specification.
Required fix in pkg/subroutines/workspace.go lines 80-83:
createdWorkspace.Spec.Type = kcptenancyv1alpha.WorkspaceTypeReference{
Name: kcptenancyv1alpha.WorkspaceTypeName(instance.Spec.Type),
Path: cfg.Kcp.ProviderWorkspace,
}Remove the & to assign a value instead of a pointer.
🤖 Prompt for AI Agents
In pkg/subroutines/workspace.go around lines 80 to 83, the code assigns a
pointer to createdWorkspace.Spec.Type but the API expects a value; remove the
address operator and assign a kcptenancyv1alpha.WorkspaceTypeReference value
with Name set to kcptenancyv1alpha.WorkspaceTypeName(instance.Spec.Type) and
Path set to cfg.Kcp.ProviderWorkspace so the field type matches the upstream
spec.
#16
Summary by CodeRabbit
New Features
Tests
Chores