-
Notifications
You must be signed in to change notification settings - Fork 10
operator: basedomain autodetect #204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
operator: basedomain autodetect #204
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (2)
Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding WalkthroughThis pull request adds OpenShift base domain auto-detection capabilities to the Jumpstarter operator. It extends RBAC permissions, introduces a discovery function to query OpenShift ingress configuration, and updates the Reconciler to auto-populate the base domain when not explicitly provided. Changes
Sequence DiagramsequenceDiagram
participant Reconciler
participant DynamicClient as Dynamic Client
participant OpenShiftAPI as OpenShift API<br/>(ingresses.config)
participant JumpstarterSpec as JumpstarterSpec
Reconciler->>DynamicClient: NewReconciler() creates client
Reconciler->>OpenShiftAPI: detectOpenShiftBaseDomain()<br/>GET cluster ingress config
alt OpenShift Cluster Found
OpenShiftAPI-->>Reconciler: Returns spec.domain<br/>(e.g., "apps.example.com")
Reconciler->>Reconciler: Store as DefaultBaseDomain
else Non-OpenShift or Missing
OpenShiftAPI-->>Reconciler: Empty/Error (log & continue)
Reconciler->>Reconciler: DefaultBaseDomain = ""
end
Reconciler->>JumpstarterSpec: ApplyDefaults(spec, namespace)
alt BaseDomain Empty && DefaultBaseDomain Set
JumpstarterSpec->>JumpstarterSpec: Auto-generate BaseDomain<br/>= "jumpstarter.{namespace}.{defaultBaseDomain}"
else BaseDomain Provided or No Default
JumpstarterSpec->>JumpstarterSpec: Keep existing/empty
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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. Comment |
d6caaa2 to
bcc971a
Compare
| } | ||
|
|
||
| // Add jumpstarter prefix to the cluster's apps domain | ||
| baseDomain := fmt.Sprintf("jumpstarter.%s", domain) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do jumpstarter.$namespace.$domain
because we support multiple installations of jumpstarter in different namespaces :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the literal "jumpstarter" or maybe the resource name $jumpstarter.Name ?
a7d95ad to
b9f7d54
Compare
b9f7d54 to
9ed7646
Compare
9ed7646 to
ac05654
Compare
|
@mangelajo hmm just seen #181 isn't this a duplicate effort? 🤔 |
33f573b to
ca41e7e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
deploy/operator/internal/controller/jumpstarter/endpoints/discovery.go (1)
58-94: Consider accepting a context parameter.The function uses
context.Background()on Line 78, which is acceptable for initialization. However, for consistency and to enable proper cancellation/timeout propagation, consider accepting a context parameter:func detectOpenShiftBaseDomain(ctx context.Context, config *rest.Config) stringThis would allow callers to control timeouts and cancellation during the detection process.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
deploy/operator/dist/install.yamlis excluded by!**/dist/**
📒 Files selected for processing (6)
deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml(2 hunks)deploy/operator/config/rbac/role.yaml(1 hunks)deploy/operator/internal/controller/jumpstarter/endpoints/discovery.go(2 hunks)deploy/operator/internal/controller/jumpstarter/endpoints/discovery_test.go(1 hunks)deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go(2 hunks)deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/rbac.go:193-196
Timestamp: 2025-10-24T11:57:13.484Z
Learning: In the Jumpstarter operator codebase (deploy/operator/internal/controller/jumpstarter/rbac.go), the Role created by `createRole()` defines RBAC permissions for the managed Jumpstarter controller application, not for the operator itself. The managed controller needs delete permissions on secrets for its runtime operations.
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go:328-333
Timestamp: 2025-10-24T11:57:23.796Z
Learning: In the jumpstarter-controller operator (deploy/operator/), the design allows only one Jumpstarter CR per namespace, which will be enforced by a validation webhook. This constraint eliminates concerns about resource name collisions within a namespace.
📚 Learning: 2025-10-24T11:57:13.484Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/rbac.go:193-196
Timestamp: 2025-10-24T11:57:13.484Z
Learning: In the Jumpstarter operator codebase (deploy/operator/internal/controller/jumpstarter/rbac.go), the Role created by `createRole()` defines RBAC permissions for the managed Jumpstarter controller application, not for the operator itself. The managed controller needs delete permissions on secrets for its runtime operations.
Applied to files:
deploy/operator/config/rbac/role.yamldeploy/operator/internal/controller/jumpstarter/jumpstarter_controller.godeploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml
📚 Learning: 2025-10-24T11:57:23.796Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 170
File: deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go:328-333
Timestamp: 2025-10-24T11:57:23.796Z
Learning: In the jumpstarter-controller operator (deploy/operator/), the design allows only one Jumpstarter CR per namespace, which will be enforced by a validation webhook. This constraint eliminates concerns about resource name collisions within a namespace.
Applied to files:
deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.godeploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yamldeploy/operator/internal/controller/jumpstarter/endpoints/discovery.go
📚 Learning: 2025-11-14T15:47:36.325Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter-controller PR: 190
File: api/v1alpha1/exporter_helpers.go:16-24
Timestamp: 2025-11-14T15:47:36.325Z
Learning: In the jumpstarter-controller project, migration annotations (jumpstarter.dev/migrated-namespace and jumpstarter.dev/migrated-uid) that override namespace and UID values in authentication tokens are acceptable without additional validation webhooks because the security model assumes only administrators have write access to Exporter and Client resources via K8s RBAC.
Applied to files:
deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.godeploy/operator/internal/controller/jumpstarter/endpoints/discovery.go
📚 Learning: 2025-05-13T19:57:56.811Z
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 137
File: hack/deploy_with_helm.sh:26-34
Timestamp: 2025-05-13T19:57:56.811Z
Learning: The jumpstarter project uses a custom kind cluster configuration with an expanded NodePort range (3000-32767) and explicit port mappings for ingress (5080/5443) and gRPC services (30010/30011 mapped to 8082/8083).
Applied to files:
deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go
📚 Learning: 2025-05-13T19:57:56.811Z
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 137
File: hack/deploy_with_helm.sh:26-34
Timestamp: 2025-05-13T19:57:56.811Z
Learning: The jumpstarter-dev repository uses a custom kind cluster configuration that allows NodePort services to use non-standard ports 5080 and 5443, outside the default Kubernetes NodePort range (30000-32767).
Applied to files:
deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go
📚 Learning: 2025-05-13T19:56:27.924Z
Learnt from: NickCao
Repo: jumpstarter-dev/jumpstarter-controller PR: 137
File: deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/router-deployment.yaml:23-26
Timestamp: 2025-05-13T19:56:27.924Z
Learning: In the jumpstarter-controller project, the router service uses the same ConfigMap as the controller service (controller-cm.yaml) even though it has been moved to its own separate deployment.
Applied to files:
deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go
🧬 Code graph analysis (1)
deploy/operator/internal/controller/jumpstarter/endpoints/discovery_test.go (2)
deploy/operator/api/v1alpha1/jumpstarter_types.go (1)
JumpstarterSpec(130-156)deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go (1)
NewReconciler(45-70)
🔇 Additional comments (6)
deploy/operator/config/rbac/role.yaml (1)
57-64: LGTM! Read-only permissions for OpenShift baseDomain detection.The added RBAC permissions appropriately grant read-only access (get, list, watch) to OpenShift ingress configuration for baseDomain auto-detection.
deploy/operator/internal/controller/jumpstarter/jumpstarter_controller.go (2)
89-90: LGTM! RBAC annotation for baseDomain auto-detection.The kubebuilder RBAC annotation correctly grants read-only access to OpenShift ingress config, matching the permissions in role.yaml.
136-136: LGTM! Namespace-aware defaulting.Passing the namespace to
ApplyDefaultsenables the generation of namespace-scoped baseDomain values.deploy/operator/bundle/manifests/jumpstarter-operator.clusterserviceversion.yaml (1)
111-118: LGTM! ClusterServiceVersion permissions match role.yaml.The added permissions for OpenShift ingress config access are consistent with the role.yaml changes.
deploy/operator/internal/controller/jumpstarter/endpoints/endpoints.go (2)
52-56: LGTM! OpenShift baseDomain auto-detection.The logic correctly attempts auto-detection only when Routes are available (OpenShift), which is an appropriate optimization.
74-79: LGTM! Namespace-scoped baseDomain defaulting.The implementation correctly:
- Only applies defaults when
spec.BaseDomainis empty (preserves user input)- Uses the pattern
jumpstarter.$namespace.$clusterDomainwhich supports multiple installations per cluster- Falls back gracefully when
DefaultBaseDomainis empty
deploy/operator/internal/controller/jumpstarter/endpoints/discovery_test.go
Show resolved
Hide resolved
656fa0f to
83e152d
Compare
Summary
Adds automatic baseDomain detection for OpenShift clusters, eliminating the need to manually specify
baseDomainin the Jumpstarter CR when deploying on OpenShift.Changes
BaseDomain Auto-Detection
ingresses.config.openshift.io/clusteron OpenShiftjumpstarter.$namespace.$clusterDomainImplementation
detectOpenShiftBaseDomain()indiscovery.goto query OpenShift config APIconfig.openshift.io/ingresses(read-only)ApplyDefaults()now accepts namespace parameter to construct the full baseDomainSummary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.