This repository was archived by the owner on Jun 11, 2025. It is now read-only.
Feat/workspace and workmachine #234
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…/operator into feat/workspace-and-workmachine
Reviewer's GuideThis PR implements two new CRDs, Workspace and WorkMachine, complete with controllers, reconciler logic, templates and RBAC, while also refactoring the existing router controller and request/YAML client infrastructure to simplify checklist handling, modularize domain and ingress logic, standardize logging, and bump dependencies and CRD schemas. Sequence Diagram for Updated Router Controller ReconciliationsequenceDiagram
participant Reconciler as RouterReconciler
participant Request
participant K8sAPI
RouterReconciler->>Request: EnsureCheckList(["EnsuringHttpsCertsIfEnabled", "SettingUpBasicAuthIfEnabled", "CreateIngressResource"])
Request-->>RouterReconciler: Proceed
RouterReconciler->>RouterReconciler: reconBasicAuth(req)
alt Basic Auth Enabled and SecretName not set
RouterReconciler->>K8sAPI: Update Router CR (set .Spec.BasicAuth.SecretName)
K8sAPI-->>RouterReconciler: Updated Router CR
end
alt Basic Auth Enabled
RouterReconciler->>K8sAPI: CreateOrUpdate Secret (basic-auth)
K8sAPI-->>RouterReconciler: Secret
RouterReconciler->>Request: AddToOwnedResources(Secret)
end
RouterReconciler->>RouterReconciler: ensureIngresses(req)
alt IngressClass not set in Spec
RouterReconciler->>RouterReconciler: findIngressClass(req)
RouterReconciler->>K8sAPI: List IngressClassList
K8sAPI-->>RouterReconciler: IngressClassList (or error)
RouterReconciler->>K8sAPI: Update Router CR (set .Spec.IngressClass)
K8sAPI-->>RouterReconciler: Updated Router CR
end
alt HTTPS enabled and ClusterIssuer not set in Spec
RouterReconciler->>RouterReconciler: findClusterIssuer(req)
RouterReconciler->>K8sAPI: List ClusterIssuerList / Get ClusterIssuer
K8sAPI-->>RouterReconciler: ClusterIssuer (or error)
RouterReconciler->>K8sAPI: Update Router CR (set .Spec.Https.ClusterIssuer)
K8sAPI-->>RouterReconciler: Updated Router CR
end
RouterReconciler->>RouterReconciler: groupHostsByKind(issuer, obj) # new helper
RouterReconciler->>RouterReconciler: templates.ParseBytes(templateIngress, ...)
RouterReconciler->>K8sAPI: ApplyYAML (Ingress)
K8sAPI-->>RouterReconciler: Ingress ResourceRefs
RouterReconciler->>Request: AddToOwnedResources(Ingress)
Sequence Diagram for WorkMachine Creation FlowsequenceDiagram
participant User/System
participant WMR as WorkMachineReconciler
participant K8sAPI
participant LFCRD as LifecycleCRD
participant IACJobPod
participant CloudProviderAPI
User/System->>K8sAPI: Create WorkMachine CR
K8sAPI->>WMR: Reconcile WorkMachine
WMR->>WMR: createWorkMachineCreationJob(req)
WMR->>WMR: parseSpecIntoTFValues()
WMR->>K8sAPI: CreateOrUpdate Lifecycle CR (IAC job spec)
K8sAPI-->>WMR: Lifecycle CR
note right of K8sAPI: Lifecycle Controller reconciles LFCRD,
note right of K8sAPI: creates Job which runs IACJobPod.
IACJobPod->>IACJobPod: Run Terraform (apply)
IACJobPod->>CloudProviderAPI: Provision Resources (e.g., EC2 instance)
CloudProviderAPI-->>IACJobPod: Resources Provisioned
IACJobPod->>K8sAPI: Update Lifecycle CR Status (Completed)
K8sAPI->>WMR: Reconcile WorkMachine (Lifecycle CR completed)
WMR->>WMR: createTargetNamespace(req)
WMR->>K8sAPI: CreateOrUpdate Namespace
WMR->>WMR: createSSHPublicKeysSecret(req)
WMR->>K8sAPI: CreateOrUpdate Secret (with user + machine SSH keys)
K8sAPI-->>WMR: Secret
WMR->>K8sAPI: Update WorkMachine Status (IsReady=true)
Class Diagram for Updated RouterSpec and Route TypesclassDiagram
class RouterSpec {
-IngressClass string
-BackendProtocol *string
-Https *Https
-RateLimit *RateLimit
-MaxBodySizeInMB *int
-BasicAuth *BasicAuth
-Cors *Cors
+NginxIngressAnnotations map~string,string~
+Routes []Route
}
class Route {
+Host string
+Service string
+Path string
+Port uint16
+Rewrite bool
}
RouterSpec *-- "1..*" Route : contains
note for RouterSpec "Removed 'Domains []string' field, added 'NginxIngressAnnotations'"
Class Diagram for New WorkMachine CRDclassDiagram
class WorkMachine {
+Spec WorkMachineSpec
+Status common_types.Status
}
class WorkMachineSpec {
+SSHPublicKeys []string
+JobParams WorkMachineJobParams
+AWSMachineConfig *AWSMachineConfig
+State crdsv1.WorkMachineState
+TargetNamespace string
GetCloudProvider() common_types.CloudProvider
}
class AWSMachineConfig {
+AMI string
+InstanceType string
+RootVolumeSize int
+RootVolumeType string
+ExternalVolumeSize int
+ExternalVolumeType string
+IAMInstanceProfileRole *string
}
class WorkMachineJobParams {
+NodeSelector map~string,string~
+Tolerations []corev1.Toleration
}
WorkMachine *-- WorkMachineSpec
WorkMachineSpec *-- WorkMachineJobParams
WorkMachineSpec *-- AWSMachineConfig
Class Diagram for New Workspace CRDclassDiagram
class Workspace {
+Spec WorkspaceSpec
+Status common_types.Status
}
class WorkspaceSpec {
+WorkMachine string
+ServiceAccountName string
+State crdsv1.WorkspaceState
+EnableTTYD bool
+EnableJupyterNotebook bool
+EnableCodeServer bool
+EnableVSCodeServer bool
+ImagePullPolicy string
}
Workspace *-- WorkspaceSpec
Class Diagram for Modified RouterReconcilerclassDiagram
class RouterReconciler {
+Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error)
+findClusterIssuer(req *reconciler.Request~*crdsv1.Router~) (*certmanagerv1.ClusterIssuer, error)
+findIngressClass(req *reconciler.Request~*crdsv1.Router~) (string, error)
+groupHostsByKind(issuer *certmanagerv1.ClusterIssuer, obj *crdsv1.Router) (wildcardHosts []string, nonWildcardHosts []string)
+reconBasicAuth(req *reconciler.Request~*crdsv1.Router~) stepResult.Result
+ensureIngresses(req *reconciler.Request~*crdsv1.Router~) stepResult.Result
- Yanked patchDefaults()
- Commented EnsuringHttpsCerts()
}
note for RouterReconciler "Reconcile method logic changed, new checklist: CreateIngressResource"
Class Diagram for New WorkMachineReconcilerclassDiagram
class WorkMachineReconciler {
+Client client.Client
+Scheme *runtime.Scheme
+Env *env.Env
+YAMLClient kubectl.YAMLClient
+Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error)
-finalize(req *rApi.Request~crdsv1.WorkMachine~) step_result.Result
-parseSpecIntoTFValues(ctx context.Context, obj *crdsv1.WorkMachine) ([]byte, error)
-createWorkMachineCreationJob(req *rApi.Request~crdsv1.WorkMachine~) step_result.Result
-createTargetNamespace(req *rApi.Request~crdsv1.WorkMachine~) step_result.Result
-createSSHPublicKeysSecret(req *rApi.Request~crdsv1.WorkMachine~) step_result.Result
+SetupWithManager(mgr ctrl.Manager) error
}
Class Diagram for New WorkspaceReconcilerclassDiagram
class WorkspaceReconciler {
+Client client.Client
+Scheme *runtime.Scheme
+Env *env.Env
+YAMLClient kubectl.YAMLClient
+Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error)
-createDeployment(req *rApi.Request~crdsv1.Workspace~) stepResult.Result
-finalize(req *rApi.Request~crdsv1.Workspace~) stepResult.Result
+SetupWithManager(mgr ctrl.Manager) error
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @nxtcoder17 - I've reviewed your changes - here's some feedback:
- There’s a lot of commented-out and duplicated logic in the router controller (e.g. commented EnsureHttpsCerts and inline patchDefaults removal) – please clean up or remove unused code blocks to keep the reconciler focused and maintainable.
- Several
EnsureCheckListcalls and constants likeCreateIngressResourceare defined inline in Reconcile; extracting those lists and check names into top‐level variables or constants would reduce noise and improve readability. - In operators/workspace/main.go you never call your controller’s RegisterInto – the workspace controller isn’t actually registered with the manager, so reconciliation won’t run; please add the registration there.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| if *oldRes.Status.IsReady != *newRes.Status.IsReady { | ||
| fireEvent(newObj, ReasonStatusIsReadyChanged, fmt.Sprintf("resource isReady changed from (%v) to (%v)", newRes.Status.IsReady, oldRes.Status.IsReady)) |
There was a problem hiding this comment.
nitpick: Event predicate now logs the new value as old and vice versa.
Swap the order of newRes.Status.IsReady and oldRes.Status.IsReady in the log message to accurately reflect the value change.
Comment on lines
14
to
16
| WorkspaceImageCodeServer string `env:"WORKSPACE_IMAGE_CODE_SERVER" default:"ghcr.io/kloudlite/iac/code-server:latest"` | ||
| WorkspcaeImageVscodeServer string `env:"WORKSPCAE_IMAGE_VSCODE_SERVER" default:"ghcr.io/kloudlite/iac/vscode-server:latest"` | ||
| } |
There was a problem hiding this comment.
issue (typo): Typo in field name and env tag for VSCode server image
Rename the field to 'WorkspaceImageVscodeServer' and update the env tag to 'WORKSPACE_IMAGE_VSCODE_SERVER'.
Suggested change
| WorkspaceImageCodeServer string `env:"WORKSPACE_IMAGE_CODE_SERVER" default:"ghcr.io/kloudlite/iac/code-server:latest"` | |
| WorkspcaeImageVscodeServer string `env:"WORKSPCAE_IMAGE_VSCODE_SERVER" default:"ghcr.io/kloudlite/iac/vscode-server:latest"` | |
| } | |
| WorkspaceImageCodeServer string `env:"WORKSPACE_IMAGE_CODE_SERVER" default:"ghcr.io/kloudlite/iac/code-server:latest"` | |
| WorkspaceImageVscodeServer string `env:"WORKSPACE_IMAGE_VSCODE_SERVER" default:"ghcr.io/kloudlite/iac/vscode-server:latest"` | |
| } |
| number: {{$route.Port}} | ||
|
|
||
| path: {{ if not hasPrefix "/" $route.Path }}/{{end}}{{$route.Path}} | ||
| ({{if hasPrefix "/" $route.Path }}{{substr 1 $x $route.Path}}{{else}}{{$route.Path}}{{end}}.*) |
There was a problem hiding this comment.
issue (bug_risk): Invalid YAML line in ingress template
The parentheses line is not valid YAML. Please remove it or incorporate its logic into a valid field such as path or regex.
Member
Author
There was a problem hiding this comment.
ingress resource v2 path is not used actually
There was a problem hiding this comment.
Thanks for the context—since that “(…).*” line isn’t actually used and, as Sourcery flagged, isn’t valid YAML, the simplest fix is to drop it entirely. If we later decide we need regex-style matching:
- You’ll need to add the appropriate nginx ingress annotation (e.g.
nginx.ingress.kubernetes.io/use-regex: "true") - Move the regex into the
pathfield itself (K8s’ nativePrefixpathType won’t accept the.*syntax).
But if we don’t need regex today, let’s just remove the parentheses line and keep the template strictly valid YAML. Let me know if you’d rather implement the regex route now or defer it.
toolkit/job-helper/job-runner.go
Outdated
|
|
||
| func (jr *JobTracker) HasJobFinished() bool { | ||
| for _, v := range jr.job.Status.Conditions { | ||
| if v.Type == batchv1.JobComplete && v.Status == "True" { |
There was a problem hiding this comment.
suggestion: Use constant for condition status
Use corev1.ConditionTrue (or metav1.ConditionTrue) instead of the string literal to improve clarity and reduce the risk of typos.
| ann[rApi.LastAppliedKey] = string(b) | ||
|
|
||
| // Check if the resource exists | ||
| cobj, err := resourceClient.Get(ctx, obj.GetName(), metav1.GetOptions{}) |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the resource apply logic to use early exits for errors and a helper function for metadata merging.
Here’s one way to collapse that tangled if err logic into a straightforward “get → create or update” flow and pull your annotation/label merging out into a small helper. You’ll end up with:
- A single early‐exit for
IsNotFound - A single early‐exit for any other
err - A clear “update” block that just calls your helper and then
Update
for _, obj := range objs {
// … marshal JSON and set LastApplied in ann …
cobj, err := resourceClient.Get(ctx, obj.GetName(), metav1.GetOptions{})
if apiErrors.IsNotFound(err) {
// create
obj.SetAnnotations(ann)
obj.SetLabels(labels)
if _, err := resourceClient.Create(ctx, &obj, metav1.CreateOptions{}); err != nil {
return resources, errors.NewEf(err, "resource: %s/%s", obj.GetNamespace(), obj.GetName())
}
logger.Info("created resource")
continue
}
if err != nil {
return nil, err
}
// update
mergeMeta(&obj, cobj, ann, labels)
obj.SetAnnotations(ann)
obj.SetLabels(labels)
if _, err := resourceClient.Update(ctx, &obj, metav1.UpdateOptions{}); err != nil {
return resources, errors.NewEf(err, "resource: %s/%s", obj.GetNamespace(), obj.GetName())
}
logger.Info("updated resource")
}And the helper could live just above:
// mergeMeta munges annotations+labels from the existing object and the last-applied snapshot
func mergeMeta(obj, existing *unstructured.Unstructured, ann, labels map[string]string) {
prev, ok := existing.GetAnnotations()[rApi.LastAppliedKey]
if ok && prev == ann[rApi.LastAppliedKey] {
return // nothing to do
}
// unmarshal the previous-applied to pick up its ann/labels
var prevObj unstructured.Unstructured
if err := json.Unmarshal([]byte(prev), &prevObj); err != nil {
return // or log if you prefer
}
// preserve any keys on existing that neither the new nor the prev-applied annotations had
for k, v := range existing.GetAnnotations() {
if !fn.MapHasKey(ann, k) && !fn.MapHasKey(prevObj.GetAnnotations(), k) {
ann[k] = v
}
}
// same for labels
for k, v := range existing.GetLabels() {
if !fn.MapHasKey(labels, k) && !fn.MapHasKey(prevObj.GetLabels(), k) {
labels[k] = v
}
}
// preserve kubernetes metadata fields (uid, resourceVersion…)
obj.Object["metadata"] = existing.Object["metadata"]
}This:
- Flattens your error‐handling to two simple branches
- Removes the dual
err+cobjtracking - Pushes the verbose merge logic into one short, focused function
- Leaves behavior exactly the same.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Implement Workspace and WorkMachine custom resources and controllers to enable dynamic provisioning of virtual workspaces and machines, refactor existing operators for improved consistency, and update CRD schemas and infrastructure utilities.
New Features:
Bug Fixes:
Enhancements: