-
Notifications
You must be signed in to change notification settings - Fork 13
add HyperShift control plane adapter spike doc #43
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
add HyperShift control plane adapter spike doc #43
Conversation
WalkthroughAdds two new design documents: a HyperShift control-plane adapter spike for GCP at hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md and a Maestro integration guide at hyperfleet/components/adapter/framework/maestro-integration-guide.md. The spike details prerequisites, HostedCluster CR templates and spec→CR mappings, config-driven templating, certificate and kubeconfig handling, an event-driven two-container Job pattern with a status-reporter sidecar, status mapping to HyperFleet, Maestro-based ManifestWork delivery and feedback, RBAC and management-cluster selection considerations, MVP vs post‑MVP behavior, implementation milestones, and testing/validation guidance. The Maestro guide documents ManifestWork structure, FeedbackRules, SDK usage patterns (idempotent create/patch), status feedback parsing, authentication/error handling, targeting strategies for management clusters, and end-to-end examples. Sequence Diagram(s)sequenceDiagram
autonumber
participant HF as HyperFleet controller
participant Maestro as Maestro SDK
participant MGMT as Management cluster API
participant HS as HyperShift operator
participant GCP as GCP control plane
Note over HF,Maestro: Adapter composes ManifestWork (HostedCluster, Job, FeedbackRules)
HF->>Maestro: Create/Apply ManifestWork (manifests + FeedbackRules)
Maestro->>MGMT: Deliver ManifestWork to target management cluster
MGMT-->>HS: HostedCluster CR + Job observed by HyperShift operator
HS->>GCP: Provision control-plane resources (VMs, IAM, networking)
GCP-->>HS: Provisioning status, credentials, certificates
HS->>MGMT: Update HostedCluster status and Job status
MGMT-->>Maestro: ManifestWork feedback populated (FeedbackRules extract status)
Maestro-->>HF: Return feedback/status from ManifestWork
HF->>HF: Map feedback to HyperFleet status, trigger retries/finalizers as needed
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
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 (4)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md (4)
725-725: Use markdown headings instead of bold emphasis for task titles.Section 11.2 uses bold emphasis (
**text**) for task titles, but the Markdown linter correctly flags these as violations (MD036). Use proper heading syntax (### heading) instead to improve document structure and enable automatic table-of-contents generation.Apply this pattern for each task (example for first task):
-**HYPERFLEET-XXX: Create HyperShift Control Plane Adapter Configuration** +### HYPERFLEET-XXX: Create HyperShift Control Plane Adapter Configuration -**Deliverable**: ... +**Deliverable**: ...Repeat this change for all 9 tasks on the flagged lines.
Also applies to: 730-730, 735-735, 740-740, 745-745, 750-750, 755-755, 760-760, 765-765
721-776: Task breakdown is detailed but testing strategy needs clarity.The 9-story breakdown totaling 25 story points is well-scoped. However, the integration testing approach (line 751) mentions "mocked management cluster or test environment" without specifying which approach is recommended. Similarly, E2E testing (line 755) is heavily dependent on PR #6938 merge, which creates scheduling risk.
Clarify the preferred testing strategy (mocked vs. real management cluster) to help the implementation team plan resource allocation. Consider documenting a fallback plan if HyperShift GCP support is delayed beyond current estimates.
Consider expanding lines 750-758 to specify:
- Whether integration tests will use mocks or a test GCP environment
- Timeline assumptions for HyperShift PR #6938 availability
- Fallback plan if GCP support is delayed (e.g., AWS implementation first)
836-838: Clarify internal documentation references.The References section links to internal HyperFleet documentation using relative paths:
- "../../framework/adapter-frame-design.md"
- "../../framework/adapter-status-contract.md"
- "../../validation/GCP/gcp-validation-adapter-spike-report.md"
Verify these documents exist and are accessible to the implementation team. Consider adding absolute URLs or repository links if these are shared resources.
Update the references to use consistent, absolute URLs or clarify the document repository structure for readers unfamiliar with the codebase layout.
1-6: Update spike review status and assignees.The document header lists "Status: Reviewing" and "Reviewers: TBD". Once you complete the design review:
- Confirm the technical approach with stakeholders
- Assign reviewers who will approve proceeding to implementation
- Update the status and reviewer fields
This spike appears ready for technical review based on comprehensiveness. The next step should be scheduling the team review mentioned in line 795.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
3-3: Bare URL used
(MD034, no-bare-urls)
725-725: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
730-730: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
735-735: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
740-740: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
745-745: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
750-750: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
755-755: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
760-760: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
765-765: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (2)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md (2)
366-407: ✓ Well-reasoned decision on Direct Kubernetes API vs Maestro SDK.The rationale for using direct Kubernetes API instead of Maestro SDK (lines 373-398) is sound for same-cluster operations. The reasoning around framework simplicity, standard RBAC patterns, and avoiding unnecessary cross-cluster complexity is well justified. The acknowledgment of future Maestro use cases (lines 396-398) shows good forward planning.
318-323: ✓ Clear and implementable adapter workflow.The workflow description (idempotency check, creation, monitoring, reporting) follows best practices for resource adapters. The sequence is logical and should work well within the config-driven framework.
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
0a99733 to
4ebe72a
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: 2
🧹 Nitpick comments (1)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md (1)
32-107: Consider whether Background section aligns with document purpose.The "Background: HyperShift Architecture" section (lines 32–107) provides valuable context for readers unfamiliar with HyperShift. However, previous feedback questioned its necessity for this spike.
Evaluation:
- Pros: Helps readers unfamiliar with HyperShift understand the architecture quickly; improves accessibility
- Cons: Spike document is already comprehensive; could defer architectural background to separate onboarding docs
Recommendation: Retain this section if the intended audience includes readers new to HyperShift. If the spike is for a team already familiar with HyperShift concepts, consider moving this to a separate "HyperShift Primer" document or ADR and linking it, reducing this spike's scope to focus on GCP-specific adapter design.
Decision is yours based on target audience for this spike.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
3-3: Bare URL used
(MD034, no-bare-urls)
820-820: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
825-825: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
830-830: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
835-835: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
840-840: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
845-845: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
850-850: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
855-855: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
860-860: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (1)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md (1)
2-2: Wrap bare URL in link syntax for markdown compliance.Line 3 contains a reference to the JIRA story as a bare link. While it may be functional, markdown linting prefers URLs to be wrapped in link syntax or angle brackets (MD034).
Current:
**JIRA Story**: [HYPERFLEET-63](https://issues.redhat.com/browse/HYPERFLEET-63)This is actually correct markdown syntax and should not trigger the linter. However, if you receive MD034 warnings, ensure all URLs are wrapped in
[text](url)or<url>format, which this example already follows.
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
| } | ||
| ], | ||
| "data": { | ||
| "controlPlaneEndpoint": "https://api.cluster.example.com:6443", // Empty while provisioning |
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.
snake_case here also?
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
|
|
||
| ## 1. Executive Summary | ||
|
|
||
| This spike defines the implementation approach for a GCP HyperShift Control Plane adapter that creates and manages HostedCluster CRs in a management cluster to provision OpenShift control planes. The solution leverages the HyperFleet config-driven adapter framework and integrates with HyperShift's GCP platform support. |
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.
I am thinking about if we should put control plane multi cloud applicable or just fit GCP? As we are going to use Maestro, we will have the unified interface of transportation. We can handle code of cluster spec in a generic way so it can be multiple platform applied.
Or we should distinguish our adapter by transportation, like control-plane via maestro, control-plane via acm, control-plane mc directly?
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.
This is just an open question, we need discussion here.
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.
yes I agree with this, we should scope this to taking a cluster spec to hosted cluster CR, in GCP it will interface with Maestro, but other envs it might interface with ACM.
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.
I need to update a lot of this document then - I was under the assumption that our adapters were on the MCs alongside the hostedcluster CRs - that's my mistake.
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.
Given the significant differences in platform specs (GCP WIF vs AWS IAM, different network configs, etc) I think per-cloud adapter configs makes the most sense here. The adapter FRAMEWORK is cloud-agnostic, but each cloud needs its own adapter configuration YAML.
For example - in GCP we have...
...
platform:
gcp:
project: "my-project"
projectNumber: "123456"
workloadIdentity:
poolID: "..."
providerID: "..."
serviceAccountsEmails: {...}
networkConfig:
privateServiceConnectSubnet: {...}
...
While in AWS it's something like...
platform:
aws:
accountID: "123456789"
region: "us-east-1"
credentials:
name: "my-creds"
vpc:
id: "vpc-123"
subnet: "subnet-456"
kmsKeyArn: "arn:aws:kms:..."
If we tried to make this generic, we'd get a nightmare of yaml template comparisons. The runtime, status reporting pattern, and overall adapter framework remain shared across all clouds - only the platform-specific HostedCluster templates and transport tool (Maestro vs. ACM vs. whatever else) differ per offering.
Want me to add this to the doc explicitly?
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.
What information does the adapter SDK need that is specific for a cloud provider?
I think, that specific information is for an Adapter Task, so it will be passed "as-is" from the adapter to the Adapter Task.
If that is the case, does it make sense to have a generic data field that will be passed to the Adapter Task?
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.
🤔 Answering to my initial question, the adapter still needs cloud-specific data to connect to the broker.
And that is now managed in the broker config, in a similar way you are proposing here.
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.
@rh-amarin yeah exactly - the adapter framework is generic, and the cloud-specific data lives in the config. Each cloud gets its own adapter config YAML, but the runtime is shared across all clouds.
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.
I think it makes most sense to scope this to maestro, we are creating a maestro resource to be consumed, so it will be a maestro-hosted-control-plane-adapter :) and if/when the need to integrate with ACM or something else we will create a different adapter spec.
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
| publicZoneID: "{{ .public_zone_id }}" | ||
|
|
||
| # Infrastructure ID | ||
| infraID: "{{ .cluster_id }}" |
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.
Is this required by HO CR?
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.
No, it's optional. From the HostedCluster CRD definition:
// If a value is not specified, a random infraID will be generated and set by the controller.
// +optional
I think we should set it on our own to get predictable GCP resource naming, but I'm open to suggestions!
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
| name: "{{ .ssh_key_secret_name }}" | ||
| ``` | ||
|
|
||
| **Status Conditions to Monitor**: |
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.
Any idea how to organize them to have status aggression to hyperfleet-api?
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.
+1 to seeing a matrix of translation between status conditions and hyperfleet status
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.
Does the section on "Status Reporting Patterns" answer your concerns here?
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
4ebe72a to
345eea3
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: 0
♻️ Duplicate comments (1)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md (1)
548-591: Convert task items from emphasis to proper markdown headings (MD036).Lines 548–591 format task items using bold emphasis (e.g.,
**HYPERFLEET-XXX: Create HyperShift Control Plane Adapter Configuration**) instead of proper markdown headings. This violates the MD036 linting rule and reduces document structure clarity for navigation and outline purposes.Apply this pattern to all 9 task items (replace
**with###heading markers):-**HYPERFLEET-XXX: Create HyperShift Control Plane Adapter Configuration** - - **Deliverable**: ... - - **Estimate**: 3 story points - - **Dependencies**: ... +### HYPERFLEET-XXX: Create HyperShift Control Plane Adapter Configuration + - **Deliverable**: ... + - **Estimate**: 3 story points + - **Dependencies**: ...Apply the same heading conversion to lines 553, 558, 563, 568, 573, 578, 583, and 588.
🧹 Nitpick comments (5)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md (5)
15-27: Tech Preview and feature gate requirements are well-documented; confirm testing strategy accounts for alpha status.The spike correctly identifies GCP support as Tech Preview (requires
TechPreviewNoUpgradefeature set) and notes this in risks (lines 22, 484, 508, 596). The acceptance criteria include testing strategy (unit, integration, E2E on real cluster) and next steps call for non-production environment validation.Ensure implementation planning explicitly accounts for potential API changes between releases. Consider documenting:
- Pinned HyperShift version for initial implementation
- Deprecation/upgrade checklist for future HyperShift releases
- Test coverage for feature gate enablement validation
Would you like me to suggest a template for documenting version compatibility and feature gate validation in the implementation stories?
432-454: Deletion strategy is sound; note that MVP doesn't implement deletion to avoid incomplete cleanup logic.The strategy correctly:
- Identifies cluster phase as trigger
- Requires NodePool deletion before HostedCluster (documented dependency)
- Monitors finalizers (30m timeout for stuck finalizers)
- Reports status during deletion lifecycle
MVP scope (lines 459–476) focuses on creation only and explicitly defers deletion to post-MVP. This is appropriate because:
- Deletion requires NodePool adapter coordination
- Finalizer handling is complex and benefits from separate story/testing
- MVP can prove the adapter framework works without full lifecycle
Document in implementation stories that post-MVP deletion work should include:
- Explicit ordering: wait for NodePools → delete HostedCluster → monitor finalizers → confirm deletion
- Timeout and escalation strategy if finalizers are stuck > 30m
- Test scenarios (normal cleanup, stuck finalizers, orphaned HostedClusters)
Consider adding a brief section (post-MVP bullet) documenting the deletion testing scenarios and finalizer edge cases, so post-MVP implementation team has context.
479-509: Dependencies and risks are comprehensive; highlight Tech Preview maturity and WIF setup complexity as critical blockers.Risk matrix covers:
- Tech Preview stability (HIGH): Mitigated by testing in non-production, planning for API changes
- WIF configuration (HIGH): Mitigated by validation adapter
- Network prerequisites (HIGH): Mitigated by validation adapter
- Management cluster access (HIGH): Mitigated by early RBAC design
- API changes (MEDIUM): Mitigated by versioned API and upgrade testing
- Framework support for CRs (MEDIUM): Mitigated by early framework validation
- Feature gate not enabled (HIGH): Mitigated by deployment checks
This is well-structured. One addition: The "Internal Dependencies" section (lines 493–497) mentions "Adapter framework must support Custom Resource creation (not just Jobs)." This is flagged as a critical blocker in next steps (line 629–632). Consider escalating this to the risks table as a BLOCKER risk, since framework limitations could derail the entire MVP.
Add framework support as an explicit blocker risk to the risk matrix, marking it as critical-path dependency that must be validated before implementation begins.
512-599: Acceptance criteria and task breakdown are thorough; story estimates and dependencies support parallel execution.Strengths:
- 9 stories (25 story points) with clear deliverables, dependencies, and estimates
- Stories decomposed by concern: config, RBAC, monitoring, validation, testing, documentation
- Critical path identified: framework support → config → status monitoring → validation → testing → docs
- Next steps include team review and environment setup before implementation
One clarification needed (lines 548–591): Task item format. Lines currently use bold emphasis instead of markdown headings (MD036 violation flagged separately). Once converted to headings (
###), the structure will be clearer.Suggestion: Consider adding story link/reference format if JIRA integration exists (e.g.,
### HYPERFLEET-70: Create HyperShift Control Plane Adapter Configuration). PlaceholderHYPERFLEET-XXXis appropriate for spike, but implementation should replace with actual JIRA issue keys.Update task breakdown after JIRA stories are created to link to actual issue keys for traceability.
603-655: Next steps are well-prioritized and actionable; success criteria provide clear completion gates.Immediate Actions are sensible:
- Update epic (HYPERFLEET-58) with findings
- Create implementation stories from task breakdown
- Schedule technical review with team
- Validate adapter framework support (critical blocker)
- Setup WIF infrastructure (precondition for testing)
- Prepare test environment (management cluster, GKE, TechPreviewNoUpgrade)
Success Criteria (lines 648–654) are clear checkpoints.
Minor enhancements to consider:
- Line 624 mentions "discussing Tech Preview implications for production readiness" — consider adding a follow-up action: document production readiness criteria and timeline for moving GCP support from Tech Preview → GA (once HyperShift upstream moves it out of Tech Preview)
- Line 632 validates "framework supports Custom Resource creation" — consider adding a sub-task: if framework doesn't support CRs, document the workaround or timeline for framework enhancement
Add post-review actions for production readiness planning and framework enhancement (if needed) to the success criteria checklist.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: AlexVulaj
Repo: openshift-hyperfleet/architecture PR: 43
File: hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md:17-17
Timestamp: 2025-12-08T22:57:33.164Z
Learning: HyperShift's GCPPlatformSpec exists in the API at api/hypershift/v1beta1/hostedcluster_types.go and is behind the GCPPlatform feature gate. The field is marked immutable and is a valid platform type when the feature gate is enabled.
📚 Learning: 2025-12-08T22:57:33.164Z
Learnt from: AlexVulaj
Repo: openshift-hyperfleet/architecture PR: 43
File: hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md:17-17
Timestamp: 2025-12-08T22:57:33.164Z
Learning: HyperShift's GCPPlatformSpec exists in the API at api/hypershift/v1beta1/hostedcluster_types.go and is behind the GCPPlatform feature gate. The field is marked immutable and is a valid platform type when the feature gate is enabled.
Applied to files:
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
🪛 markdownlint-cli2 (0.18.1)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
3-3: Bare URL used
(MD034, no-bare-urls)
548-548: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
553-553: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
558-558: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
563-563: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
568-568: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
573-573: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
578-578: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
583-583: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
588-588: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (4)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md (4)
1-50: Document structure and technical accuracy are sound; architecture aligns with Maestro-based cross-cluster pattern.The spike correctly documents:
- GCPPlatformSpec as Tech Preview behind GCPPlatform feature gate (confirmed in HyperShift v1beta1 API)
- Clear prerequisite boundary (WIF pools, VPC, PSC subnet created externally; adapter maps them to HostedCluster CR)
- Maestro SDK as the transportation layer for cross-cluster resource management (addressing concerns about adapter placement and multi-cloud integration)
- Certificate management as HyperShift-owned; adapter monitors only
- 3-condition status pattern (Applied/Available/Health) consistent with adapter framework
The document provides comprehensive coverage of config-driven adapter requirements, implementation approach, error handling, and risk mitigation.
74-144: HostedCluster CR template is comprehensive and correctly maps HyperFleet fields; validate GCP-specific constraints are tested.The YAML template covers:
- Platform type and GCP-specific config (project, region, networkConfig, workloadIdentity)
- Network requirements (VPC network, Private Service Connect subnet)
- WIF configuration (poolID, providerID, service account emails)
- Release image, networking (cluster/service/machine CIDRs), pull secret, DNS, infraID, SSH key
The field mappings (lines 278–296) are clear and correctly document the transformation from HyperFleet spec to HostedCluster CR. The template includes all required fields per GCP platform requirements.
One consideration: The template shows
infraIDas optional in HostedCluster CRD (line 139 comment references this), but it's set explicitly for deterministic GCP resource naming. Document why deterministic infraID is preferred (vs letting HyperShift auto-generate) — this affects GCP resource naming predictability and should be validated in integration tests.Confirm in implementation stories that integration tests validate:
- infraID behavior when explicitly set vs auto-generated
- GCP resource naming follows expected pattern with deterministic infraID
- If HyperShift requires infraID for some WIF or network binding, document this constraint
166-194: Preconditions logic is well-structured; validate expression evaluation in adapter framework.The preconditions section defines:
fetch_cluster_details: API call to HyperFleet to fetch spec and check adapter statuses- Capture expressions to extract GCP config, phase, and availability of validation/DNS/pull secret adapters
- Expression-based gate: all three adapters (validation, DNS, pullsecret) must report Available=True and cluster in Provisioning/Installing phase
This is a sound pattern. Ensure the adapter framework's expression evaluator handles:
- Nested
.filter()and array access (e.g.,status.adapters.filter(...)[0].conditions.filter(...)[0])- Null/missing fields (e.g., if adapters array is empty or condition doesn't exist)
- Type coercion for string comparisons ("True" vs boolean true)
Confirm in implementation stories or adapter framework documentation that:
- Expression language supports the filter/array patterns shown (lines 182–188)
- Error handling for missing/null fields is documented
- Type safety for condition status comparisons (string "True" vs boolean)
322-400: Maestro SDK integration approach is well-defined; status polling strategy includes good tuning for resource lifecycle.The section covers:
- Cross-cluster architecture (regional clusters run adapters; management clusters host HostedCluster CRs)
- ManifestWork as transportation unit (wrapping HostedCluster CRs)
- Authentication via ServiceAccount token
- Authorization (ClusterRole for HostedCluster CRUD + status read)
- Error classification with retry strategies (transient network/resource vs permanent auth/validation)
- Status polling intervals tuned by resource state (10s Creating, 30s Provisioning, 5m Available, 1m Degraded, 30s Deleting)
One note: Line 344 shows
targetCluster: "{{ .managementClusterName }}"in the adapter configuration example, but line 360 mentions "adapter configuration fetches available management clusters from HyperFleet API and filters by region." Ensure:
- Implementation decides: Is management cluster name/region passed in HyperFleet spec, or does adapter discover it?
- If discovered, add this to the preconditions section or clarify in resource template
In implementation stories, clarify:
- Whether management cluster selection is hardcoded in config, fetched from HyperFleet spec, or discovered via API call
- If discovered, add precondition/logic to validate management cluster availability and RBAC before attempting HostedCluster creation
| - as: "gcpProjectId" | ||
| field: "spec.gcp_project_id" | ||
| - as: "gcpRegion" | ||
| field: "spec.gcp_region" |
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.
If these are only used later in the template, and will grow, maybe we should only capture spec, and then the template will use them as spec.gcp_project_id directly
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.
Good call - we lose some explicitness here, but it's a lot cleaner. I'll update the config here to capture the entire spec object and reference fields with nested properties.
| ### 5.3 Authentication and Authorization Patterns | ||
|
|
||
| **Maestro Authentication**: | ||
| The adapter authenticates to Maestro API using a ServiceAccount token stored in a Secret (`maestro-adapter-token`). |
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.
Is this a Kubernetes Service Account?
How do Adapters running in one namespace get access to the secret in another namespace? or is this copied "somehow" ?
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.
Yeah it's a K8s ServiceAccount token. The adapters and the Maestro auth secret would be in the same namespace, so there's no cross-namespace access needed.
| | Available=False | Progressing=True | Available=False | Control plane provisioning | | ||
| | Degraded=True | Degraded=True | Health=False | Control plane has issues | | ||
|
|
||
| **Status Polling Strategy**: |
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 GCP mentioned that there is the alternate possibility of using Maestro gRPC that provides real-time push notification so status changes.
We could have this recorded as an alternative, and why we are selecting polling for now in MVP
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.
I dug through the Maestro code/docs a bit here - I had a misunderstanding with different parts of the architecture. Let me fix up this section a bit, good catch.
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
|
|
||
| ### 7.1 Configuration Updates Strategy | ||
|
|
||
| HyperShift HostedCluster CRs have both mutable and immutable fields. The adapter must validate updates and reject changes to immutable fields. |
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.
This showcases a gap in our design.
If customer POSTs a change, the Hyperfleet API will not validate it and will store in the DB.
Should an "invalid change" update generation?
If the validation only happens in the Adapter Task, there is little the Hyperfleet API can do
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.
Yeah, this is a real gap. My thought was that the adapter job/task would fetch the existing MFW via Maestro (if it exists) and compare it to the incoming desired state. Not great to have validation happening in the adapters, especially since the API has already stored the change and bumped the generation.
I think this is something we should try and address further post-MVP, because it'll be a growing problem with other adapters. Thoughts?
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
|
I'm marking this as a WIP now while we discuss:
I'm also going to break out a separate doc on Maestro specifically, since it's cross-cutting knowledge that multiple adapters may need. |
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
| name: "{{ .ssh_key_secret_name }}" | ||
| ``` | ||
|
|
||
| **Status Conditions to Monitor**: |
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.
+1 to seeing a matrix of translation between status conditions and hyperfleet status
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Outdated
Show resolved
Hide resolved
|
|
||
| ## 1. Executive Summary | ||
|
|
||
| This spike defines the implementation approach for a GCP HyperShift Control Plane adapter that creates and manages HostedCluster CRs in a management cluster to provision OpenShift control planes. The solution leverages the HyperFleet config-driven adapter framework and integrates with HyperShift's GCP platform support. |
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.
I think it makes most sense to scope this to maestro, we are creating a maestro resource to be consumed, so it will be a maestro-hosted-control-plane-adapter :) and if/when the need to integrate with ACM or something else we will create a different adapter spec.
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
| ### Primary Risks | ||
| - **Tech Preview Status**: GCP platform support is Tech Preview (alpha), behind TechPreviewNoUpgrade feature gate | ||
| - **WIF Prerequisites**: Requires pre-created Workload Identity pools, providers, and service accounts with IAM bindings | ||
| - **Network Prerequisites**: Requires pre-created VPC network and Private Service Connect subnet |
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.
Are some of these risks, or just prerequisites, which for the most part should land on the validation adapter. So might be worth mapping these risks to other adapters and why this adapter is dependent on them, to cover these prerequisites
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.
I'll remove what I consider pre-requisites here. I agree that they aren't really "risks."
|
|
||
| # Pull secret | ||
| pullSecret: | ||
| name: "{{ .pull_secret_name }}" |
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.
Think this could be a gap we need to catch. Will there be a separate adapter creating the pull secret secret on the cluster 🤔 or will this adapter create all the maestro resources? It is worth discussion
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.
Good catch - I believe it should fall on the Pull Secret adapter to create the ExternalSecret in the management cluster (via Maestro). ESO then reconciles it and creates the actual Secret. Control Plane adapter's precondition verifies the Secret exists before running, then just references it by name.
345eea3 to
abdc302
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: 5
♻️ Duplicate comments (1)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md (1)
998-1058: Use headings (or list items) instead of bold for task titles (MD036)
This was already called out in prior review comments; still applies to the current content.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hyperfleet/components/adapter/framework/maestro-integration-guide.md(1 hunks)hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- hyperfleet/components/adapter/framework/maestro-integration-guide.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-08T22:57:33.164Z
Learnt from: AlexVulaj
Repo: openshift-hyperfleet/architecture PR: 43
File: hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md:17-17
Timestamp: 2025-12-08T22:57:33.164Z
Learning: HyperShift's GCPPlatformSpec exists in the API at api/hypershift/v1beta1/hostedcluster_types.go and is behind the GCPPlatform feature gate. The field is marked immutable and is a valid platform type when the feature gate is enabled.
Applied to files:
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
🪛 markdownlint-cli2 (0.18.1)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
4-4: Bare URL used
(MD034, no-bare-urls)
22-22: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
695-695: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
998-998: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1005-1005: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1011-1011: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1017-1017: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1023-1023: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1029-1029: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1035-1035: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1041-1041: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1048-1048: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1054-1054: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
abdc302 to
1099cdf
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: 5
♻️ Duplicate comments (5)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md (5)
1005-1074: Convert “bold-as-heading” task items into real headings (MD036)This was already flagged previously; still applies to the task breakdown block. Convert each
**HYPERFLEET-XXX: ...**into### HYPERFLEET-XXX: ...(or list items) consistently.
221-263: Makeresources.hostedClusterhandle consistent across examplesSection 3.1 defines a HostedCluster resource without a
name, but post-processing referencesresources.hostedCluster.... Section 5.2 does introducename: "hostedCluster".resources: - - kind: "HostedCluster" + - name: "hostedCluster" + kind: "HostedCluster" apiVersion: "hypershift.openshift.io/v1beta1"Also applies to: 268-291, 768-777
4-5: Fix bare email (MD034) and keep metadata consistent formatting-**Prepared By**: avulaj@redhat.com +**Prepared By**: `avulaj@redhat.com`
761-763: Fix malformed bold link line (renders broken)-**For detailed Maestro integration patterns, see -**: [Maestro Integration Guide](../../framework/maestro-integration-guide.md) +**For detailed Maestro integration patterns, see**: [Maestro Integration Guide](../../framework/maestro-integration-guide.md)
97-165: Verify GCPPlatformSpec field names and resourceLabels constraints against target HyperShift versionThe document's GCP platform configuration requires validation against your specific HyperShift deployment, particularly:
spec.platform.gcp.endpointAccess— Confirm this field exists in GCPPlatformSpec (GCP support is behind a feature gate; AWS uses this pattern, but GCP field names may differ).spec.platform.gcp.resourceLabels— Verify the array-of-objects structure (key/value pairs) matches GCPPlatformSpec; also note GCP enforces a 64-label maximum per resource (document states max 60).spec.release.image— Formatquay.io/openshift-release-dev/ocp-release:{{ .openshift_version }}is correct (must be full OCP release payload URL or digest, e.g.,4.14.0-x86_64), but ensure your templating resolves{{ .openshift_version }}to a valid, supported release tag.Also check lines 221–263, 307–326, 469–523 for the same field mappings and templating patterns.
🧹 Nitpick comments (1)
hyperfleet/components/adapter/framework/maestro-integration-guide.md (1)
305-354: Consumer naming: align terminology (consumerNamevstargetCluster) with framework docsThe guide uses
metadata.namespaceas consumer name (correct for ManifestWork routing), while the spike doc also introducestargetCluster. Consider adding a short “Terminology” note mapping:
- framework
targetCluster→ ManifestWorkmetadata.namespace(“consumer name”)
to prevent confusion when moving between docs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hyperfleet/components/adapter/framework/maestro-integration-guide.md(1 hunks)hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-08T22:57:33.164Z
Learnt from: AlexVulaj
Repo: openshift-hyperfleet/architecture PR: 43
File: hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md:17-17
Timestamp: 2025-12-08T22:57:33.164Z
Learning: HyperShift's GCPPlatformSpec exists in the API at api/hypershift/v1beta1/hostedcluster_types.go and is behind the GCPPlatform feature gate. The field is marked immutable and is a valid platform type when the feature gate is enabled.
Applied to files:
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
🪛 markdownlint-cli2 (0.18.1)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
4-4: Bare URL used
(MD034, no-bare-urls)
23-23: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
700-700: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1005-1005: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1012-1012: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1018-1018: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1025-1025: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1031-1031: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1037-1037: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1043-1043: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1049-1049: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1056-1056: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1062-1062: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1068-1068: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
|
Something that @rh-amarin and I discussed once is whether or not we should have code blocks in our architecture documents. I agreed that architecture documents should be more high level. Code blocks make documents larger and hard to read. Do we still have that concensus? |
|
@rafabene I'm happy to remove the code blocks if we think that makes the document more clear! |
|
@AlexVulaj Let's wait more people to confirm this approach. |
|
|
||
| ## Authentication | ||
|
|
||
| Set `serviceAccountName: maestro-adapter-sa` in your Job spec (Maestro team creates the ServiceAccount). Kubernetes |
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.
This means the pod will run as maestro-adapter-sa
My question is.... why should we run a pod in our system with a service account created by another team?
Can this come with any risk?
- e.g.
if we need to grant permissions to access other infrastructure (imagine publishing to a topic), then the service account from another team gets that privilege- E.g. in GCP when using WIF, the grant is for a service account in an specific namespace of a cluster. That means that workloads from another namespace won't be able to access infrastructure even if using the same service account
What is what make these tokens valid for maestro? does it have an expected assertion?
- e.g. the k8s tokens contain the
subassertion with the name of the namespace and the service account - something like
"sub": "system:serviceaccount:amarin:default" - Since the token is signed from the cluster, any workload with access to the public key of the cluster can validate and accept it
- Is this how Maestro accepts the token?
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.
This is a valid concern - from what I was told, another team is handling Maestro setup but I'm not sure who that is. I tried my best to fill in gaps here with what we can control, but if things are different than I can change up this doc. Do you know who would be best to ask here?
| Sentinel Pulse → Adapter Framework creates Job | ||
| ↓ | ||
| [Main Container] Maestro SDK calls → Write JSON → Exit | ||
| ↓ | ||
| [Sidecar] Read JSON → Patch Job.status.conditions → Exit | ||
| ↓ | ||
| Adapter Framework reads Job.status → Reports to HyperFleet API | ||
| ↓ | ||
| Job auto-deleted after TTL expires (60 seconds) | ||
| ↓ | ||
| Next Sentinel Pulse → Job doesn't exist → Create fresh Job (repeat) |
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.
This makes the assumption that the Adapter Framework waits for the job to finish
Which is not the case in the current design AFAIK:
- The adapter framework creates the job
- The adapter framework reports the job has been created (Applied=true, Available=false)
Is not until the next sentinel pulses that the adapter will read the job status, and therefore Maestro responses
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.
Yeah I wrote this a little confusing - I'll change it up to be more clear. Essentially:
First pulse - we just create the job
Subsequent pulses:
- if the job exists, read its status and report
- if the job doesn't exist (ttl deleted), the next pulse sees no job and creates a fresh one.
With the job running async between Sentinel pulses, just like other adapters.
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
1099cdf to
f5ffc99
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: 3
♻️ Duplicate comments (2)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md (2)
201-213: Guardfilter(...)[0]expressions to prevent runtime errors on empty arrays.These expressions will throw if adapters haven't reported status yet. Add existence checks before indexing:
- as: "validationAvailable" expression: | - status.adapters.filter(a, a.name == 'validation')[0].conditions.filter(c, c.type == 'Available')[0].status == "True" + has(status.adapters.find(a, a.name == 'validation')) && + has(status.adapters.find(a, a.name == 'validation').conditions.find(c, c.type == 'Available')) && + status.adapters.find(a, a.name == 'validation').conditions.find(c, c.type == 'Available').status == "True"Apply the same pattern to lines 204–209 (dns, pullsecret expressions).
124-126: RemoveendpointAccessfield from GCP platform spec example.The
endpointAccessfield is AWS-only and is not supported byGCPPlatformSpec, which is an empty struct. This field will cause validation errors if applied to a GCP-based HostedCluster. Remove lines 124-126 or replace with valid GCP-only configuration (projectID and region are available via platform status, not spec).
🧹 Nitpick comments (1)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md (1)
756-759: Simplify Maestro link formatting.Lines 756–759 have awkward bold/link formatting. Simplify to a single line:
-**For detailed Maestro integration patterns, see -**: [Maestro Integration Guide](../../framework/maestro-integration-guide.md) +**For detailed Maestro integration patterns, see**: [Maestro Integration Guide](../../framework/maestro-integration-guide.md)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hyperfleet/components/adapter/framework/maestro-integration-guide.md(1 hunks)hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- hyperfleet/components/adapter/framework/maestro-integration-guide.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-08T22:57:33.164Z
Learnt from: AlexVulaj
Repo: openshift-hyperfleet/architecture PR: 43
File: hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md:17-17
Timestamp: 2025-12-08T22:57:33.164Z
Learning: HyperShift's GCPPlatformSpec exists in the API at api/hypershift/v1beta1/hostedcluster_types.go and is behind the GCPPlatform feature gate. The field is marked immutable and is a valid platform type when the feature gate is enabled.
Applied to files:
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
🪛 markdownlint-cli2 (0.18.1)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
4-4: Bare URL used
(MD034, no-bare-urls)
23-23: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
697-697: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1002-1002: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1009-1009: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1015-1015: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1022-1022: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1028-1028: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1034-1034: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1040-1040: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1046-1046: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1053-1053: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1059-1059: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1065-1065: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (12)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md (12)
4-4: Format bare email as inline code to satisfy markdown linting.Line 4 contains a bare email address which triggers MD034. Wrap it in backticks.
-**Prepared By**: avulaj@redhat.com +**Prepared By**: `avulaj@redhat.com`
23-25: Fix broken emphasis and list indentation (MD007 + formatting).The nested note has mismatched asterisks and incorrect indentation, causing rendering issues.
- **Workload Identity Federation**: GCP HostedClusters require WIF configuration (pools, providers, service accounts) - - **Note:** This refers to WIF for the **HostedCluster CR** (worker node authentication to GCP). For WIF enabling * - *CLM adapters** to access customer GCP resources, see [WIF Spike](../../../docs/wif-spike.md). These are two - separate WIF configurations. + - **Workload Identity Federation**: GCP HostedClusters require WIF configuration (pools, providers, service accounts) + - **Note:** This refers to WIF for the **HostedCluster CR** (worker node authentication to GCP). For WIF enabling + **CLM adapters** to access customer GCP resources, see [WIF Spike](../../../docs/wif-spike.md). These are two + separate WIF configurations.
136-136: Clarifyrelease.imagetemplate variable format.The template uses
{{ .openshift_version }}as an image tag. Verify that this variable expands to a valid image tag format (e.g.,4.14.0is fine if the full URL isquay.io/openshift-release-dev/ocp-release:4.14.0). If the variable is a semantic version string, document this assumption; otherwise, update the template to use a full release image reference. Reference HyperShift's supported-versions mapping for production deployments.
697-709: Add language identifier to fenced code block (MD040).-``` +```text Sentinel Pulse → Adapter Framework creates Job
1-40: Approve executive summary and key decisions section.The executive summary clearly outlines the adapter approach, key decisions, and primary risks. The prerequisites vs. adapter scope is well-defined. Foundation for the spike is solid.
84-170: Approve HostedCluster CRD requirements and certificate management sections.These sections accurately describe the HostedCluster API requirements, GCP platform support status, and the adapter's monitoring-only role for certificate management. Certificate handling approach is correct (HyperShift operator manages certs, adapter monitors). Based on learnings, GCPPlatformSpec tech preview status and feature gate requirement are correctly documented.
326-462: Approve Job container pattern section with minor concern on filter safety.The two-container sidecar pattern is well-explained with clear responsibilities for main adapter and status reporter. The pseudocode and helper functions provide good implementation guidance. The JSON schema for results is clear. Note: filter safety issues flagged elsewhere also apply to status mapping logic here; ensure runtime guards are added during implementation.
719-743: Approve adapter workflow and status reporting sections.Idempotency approach, monitoring strategy, and state transitions are clearly documented. Job TTL pattern for fresh status checks on each pulse is sound. Status reporting conditions (Applied/Available/Health) follow HyperFleet pattern correctly.
745-820: Approve Maestro integration and error handling sections.Cross-cluster architecture is clearly explained. Maestro's role as transportation layer is well-justified. Consumer targeting approach (MVP: static, post-MVP: dynamic) is pragmatic. Error handling table covers key scenarios (transient, permanent auth, validation, conflicts) with appropriate retry strategies.
852-932: Approve update/deletion strategy and MVP vs post-MVP sections.The document clearly separates MVP scope (creation only) from post-MVP work (updates, deletion). Update strategy appropriately identifies immutable field validation requirements. Deletion strategy correctly requires coordination with NodePool adapter and finalizer monitoring. Clear boundaries between MVP and future work are helpful for implementation planning.
934-1000: Approve dependencies, risks, and acceptance criteria sections.Dependencies are comprehensive and include adapter framework, management cluster, GCP prerequisites, RBAC, and adapter preconditions. Risk table maps high-impact concerns (Tech Preview stability, WIF complexity, network prerequisites, API evolution) to mitigations. Acceptance criteria checkboxes provide clear implementation targets.
1082-1135: Approve next steps and action items.Post-spike actions are well-organized (epic update, story creation, team review, validation, infrastructure setup, environment prep). Success criteria checklist ensures completion tracking. Technical review agenda and dependencies are clearly outlined.
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
For me, the reason not to have code in the architecture docs is because they will drift from the implementation, or maintaining them aligned will be costly. But in this case, there is not implementation yet, so it may be acceptable to have some code.... which could be removed later, with a link to the implementation. One way to declutter the documents is to put the code between That way, if you don't want to deep into the details, reading through the docs, you don't face the "wall of code", which for me is always distracting Tip: you can ask Claude to wrap the code blocks longer than X lines of code automatically in sections, and it will automatically create also the |
f5ffc99 to
3c9909b
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: 2
♻️ Duplicate comments (6)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md (5)
22-26: Fix broken Markdown emphasis and list indentation (MD007).Lines 22–26 have incorrect list indentation and broken emphasis across line breaks. The text
* *CLM adapters**splits emphasis markers across lines, creating malformed markdown.Apply this fix:
- **Workload Identity Federation**: GCP HostedClusters require WIF configuration (pools, providers, service accounts) - - **Note:** This refers to WIF for the **HostedCluster CR** (worker node authentication to GCP). For WIF enabling * - *CLM adapters** to access customer GCP resources, see [WIF Spike](../../../docs/wif-spike.md). These are two - separate WIF configurations. + - **Note:** This refers to WIF for the **HostedCluster CR** (worker node authentication to GCP). For WIF enabling + **CLM adapters** to access customer GCP resources, see [WIF Spike](../../../docs/wif-spike.md). These are two + separate WIF configurations.
1002-1065: Convert task items from emphasis to proper H3 headings (MD036).Lines 1002, 1009, 1015, 1022, 1028, 1034, 1040, 1046, 1053, 1059, and 1065 use bold emphasis for task titles instead of proper Markdown headings. Replace each with an H3 heading:
-**HYPERFLEET-XXX: Create HyperShift Control Plane Adapter Configuration** +### HYPERFLEET-XXX: Create HyperShift Control Plane Adapter ConfigurationApply this to all 11 task lines to improve document structure and pass linting.
216-260: Add missingnamefield to resource definition — post-processing referencesresources.hostedCluster.The
resourcesblock (lines 219–260) defines a resource without a top-levelnamefield, but post-processing at lines 272 and 276 referencesresources.hostedCluster. This mismatch will cause template rendering to fail.Add a
namefield to make the resource referenceable:resources: + - name: "hostedCluster" - - kind: "HostedCluster" + kind: "HostedCluster" apiVersion: "hypershift.openshift.io/v1beta1"
272-282: Guard unsafe array index access in post-processing expressions.Lines 276–277 and 281–282 call
.filter(...)[0]without checking if the filtered array is non-empty. This will crash if a condition is missing. Add existence checks before indexing:available: status: expression: | has(resources.hostedCluster.status.conditions) && + resources.hostedCluster.status.conditions.exists(c, c.type == 'Available') && resources.hostedCluster.status.conditions.filter(c, c.type == 'Available')[0].status == "True" health: status: expression: | !has(resources.hostedCluster.status.conditions) || + resources.hostedCluster.status.conditions.exists(c, c.type == 'Degraded') && resources.hostedCluster.status.conditions.filter(c, c.type == 'Degraded')[0].status != "True"Apply the same pattern to preconditions at lines 203, 206, and 209.
94-162: Remove invalidendpointAccessfield from GCP platform spec — this is AWS-only.Line 125 specifies
endpointAccess: Private, but this field does not exist in HyperShift's GCPPlatformSpec. It is an AWS-only field (AWSEndpointAccessType). Including it will silently be ignored or cause validation errors depending on the HyperShift version.Remove this line from the HostedCluster spec template:
workloadIdentity: projectNumber: "{{ .gcp_project_number }}" poolID: "{{ .wif_pool_id }}" providerID: "{{ .wif_provider_id }}" serviceAccountsEmails: controlPlane: "{{ .control_plane_sa_email }}" nodePool: "{{ .nodepool_sa_email }}" - - # API endpoint access (optional, defaults to Private) - endpointAccess: PrivateAlso verify that line 242 in the resource template (which references
{{ .spec.endpoint_access }}) is removed or the configuration field is not used.hyperfleet/components/adapter/framework/maestro-integration-guide.md (1)
147-153: Add missing imports to Go SDK usage example.The code block lacks the
contextimport, which is used incontext.Background()at line 156. Add to the imports:import ( + "context" "github.com/openshift-online/maestro/pkg/api/openapi" "github.com/openshift-online/maestro/pkg/client/cloudevents/grpcsource" "github.com/openshift-online/ocm-sdk-go/logging" workv1 "open-cluster-management.io/api/work/v1" "open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc" )
🧹 Nitpick comments (1)
hyperfleet/components/adapter/framework/maestro-integration-guide.md (1)
23-93: Verify Go code example has all required imports for clarity.The Go code block starting at line 23 imports standard packages but omits
context, which is used in the example. While this may be intentional as "illustrative," readers may struggle to understand whycontext.Background()is used without an import. Consider either:
- Adding the missing imports for completeness, or
- Explicitly labeling the snippet as "illustrative (not compilable)" to set expectations
This applies to all Go code snippets in the guide.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hyperfleet/components/adapter/framework/maestro-integration-guide.md(1 hunks)hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-08T22:57:33.164Z
Learnt from: AlexVulaj
Repo: openshift-hyperfleet/architecture PR: 43
File: hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md:17-17
Timestamp: 2025-12-08T22:57:33.164Z
Learning: HyperShift's GCPPlatformSpec exists in the API at api/hypershift/v1beta1/hostedcluster_types.go and is behind the GCPPlatform feature gate. The field is marked immutable and is a valid platform type when the feature gate is enabled.
Applied to files:
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
🪛 markdownlint-cli2 (0.18.1)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
4-4: Bare URL used
(MD034, no-bare-urls)
23-23: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
697-697: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1002-1002: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1009-1009: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1015-1015: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1022-1022: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1028-1028: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1034-1034: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1040-1040: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1046-1046: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1053-1053: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1059-1059: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1065-1065: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (2)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md (1)
402-403: Verify indentation in Go pseudocode block.Past reviews noted indentation issues in Go code blocks. Ensure this pseudocode block at lines 402–462 has consistent indentation matching the guide's style. If lines are unindented, add proper spacing.
hyperfleet/components/adapter/framework/maestro-integration-guide.md (1)
290-302: [Your rewritten review comment text here]
[Exactly ONE classification tag]
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
Show resolved
Hide resolved
3c9909b to
e2482be
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: 0
♻️ Duplicate comments (9)
hyperfleet/components/adapter/framework/maestro-integration-guide.md (2)
146-203: Clarify whether Go snippets are illustrative or make them copy/paste‑readyThe shorter SDK examples omit imports (
context,os,encoding/json,fmt,log,errors,metav1, etc.) and use placeholders in comments. That’s fine if they’re illustrative only, but today it’s ambiguous.Consider either:
- adding the missing imports so the snippets compile as-is, or
- adding a short note above these blocks like “illustrative only; not a complete program.”
Also applies to: 220-244
192-203: Revisit error classification: Maestro SDK errors are likely gRPC status codes, not k8s API errorsHere you suggest using
errors.IsServerTimeout,errors.IsServiceUnavailable, anderrors.IsTooManyRequestsfromk8s.io/apimachinery/pkg/api/errorsto detect retryable errors. For the gRPC‑based Maestro client (grpcsource.NewMaestroGRPCSourceWorkClient), errors are typically surfaced as gRPC status codes rather than Kubernetes API errors, so these helpers may never match.It would be more accurate to:
- inspect errors via
status.Code(err)(gRPC) and- treat codes like
Unavailable,DeadlineExceeded, andResourceExhaustedas retriable, while failing fast on things likeInvalidArgument,AlreadyExists,PermissionDenied,NotFound.You can keep the high‑level table but should align the concrete helpers with how the SDK actually returns errors.
Check how the Maestro gRPC source work client (`github.com/openshift-online/maestro/pkg/client/cloudevents/grpcsource`) surfaces errors from `ManifestWorks().Get/Create/Patch` — are they plain gRPC status errors, or are they wrapped as `k8s.io/apimachinery/pkg/api/errors`? Provide example error types/call stacks if available.Also applies to: 290-301
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md (7)
20-25: Fix WIF note markdown (broken emphasis + nested list indent)The WIF note under “Workload Identity Federation” has broken emphasis (
* *CLM adapters**) and extra indentation, which renders oddly and violates MD007.Consider rewriting as:
- **Workload Identity Federation**: ... - **Note:** This refers to WIF for the **HostedCluster CR** (worker node authentication to GCP). For WIF enabling **CLM adapters** to access customer GCP resources, see [WIF Spike](../../../docs/wif-spike.md). These are two separate WIF configurations.
4-4: Wrap bare email to satisfy markdownlint (MD034)Line 4 has a bare email address, which markdownlint treats like a bare URL. To keep the metadata but satisfy lint, wrap it, e.g.:
**Prepared By**: `avulaj@redhat.com`(or
<avulaj@redhat.com>/mailto:link if you prefer).
695-714: Add language to flow-summary code fence (MD040) and keep it on one lineThe flow diagram fence currently has no language:
Sentinel Pulse → ...
To satisfy MD040 and improve readability, switch to: ```markdown ```text Sentinel Pulse → Framework checks: Does Job exist? ...--- `765-766`: **Fix bold formatting around Maestro integration guide link** The text currently splits the bold markers: ```markdown **For detailed Maestro integration patterns, see **: [Maestro Integration Guide](...)This renders oddly. Consider:
**For detailed Maestro integration patterns, see**: [Maestro Integration Guide](../../framework/maestro-integration-guide.md)
1009-1078: Use headings instead of bold for task titles (MD036)The implementation tasks are formatted as bold paragraphs rather than headings:
**HYPERFLEET-XXX: Create HyperShift Control Plane Adapter Configuration**Switching them to
###headings improves structure and satisfies MD036:### HYPERFLEET-XXX: Create HyperShift Control Plane Adapter ConfigurationApply to all similar items in this section.
190-214: Guardfilter(...)[0]in precondition captures to avoid runtime failuresThe expressions for
validationAvailable,dnsAvailable, andpullSecretAvailableassume the filtered arrays are non‑empty:status.adapters.filter(a, a.name == 'validation')[0].conditions.filter(c, c.type == 'Available')[0].status == "True"If an adapter entry or condition is missing (early reconciliation, rename, typo), this will throw on
[0].Prefer an existence+status pattern, for example:
- as: "validationAvailable" expression: | status.adapters.exists(a, a.name == 'validation') && status.adapters.filter(a, a.name == 'validation')[0].conditions.exists(c, c.type == 'Available') && status.adapters.filter(a, a.name == 'validation')[0].conditions.filter(c, c.type == 'Available')[0].status == "True"Apply the same pattern to
dnsAvailableandpullSecretAvailable.
218-260: Alignresources.hostedClusterreference with resource definition and guard condition filtersTwo related issues here:
Resource alias vs reference
postexpressions readresources.hostedCluster.status..., but theresourceslist doesn’t assign a static alias name (it only sets the K8snamefield):resources: - kind: "HostedCluster" apiVersion: "hypershift.openshift.io/v1beta1" namespace: "clusters" name: "{{ .clusterName }}" spec: ...If the adapter framework expects
resources.<alias>(not the K8smetadata.name), you likely need:resources: - name: "hostedCluster" # alias used in post-processing kind: "HostedCluster" apiVersion: "hypershift.openshift.io/v1beta1" namespace: "clusters" name: "{{ .clusterName }}" # actual K8s name spec: ...Otherwise the
resources.hostedClusterlookups may never resolve.Unsafe
filter(...)[0]on conditions
The post‑processing expressions again index[0]without checking:resources.hostedCluster.status.conditions.filter(c, c.type == 'Available')[0].status == "True"If the condition is missing, this will error. Safer example:
available: status: expression: | has(resources.hostedCluster.status.conditions) && resources.hostedCluster.status.conditions.exists(c, c.type == 'Available') && resources.hostedCluster.status.conditions.filter(c, c.type == 'Available')[0].status == "True" health: status: expression: | !has(resources.hostedCluster.status.conditions) || ( resources.hostedCluster.status.conditions.exists(c, c.type == 'Degraded') && resources.hostedCluster.status.conditions.filter(c, c.type == 'Degraded')[0].status != "True" )Also applies to: 265-283
🧹 Nitpick comments (1)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md (1)
337-388: Clarify ownership and scope ofmaestro-adapter-saThe Job manifest hard‑codes
serviceAccountName: maestro-adapter-sa. Since this SA is created by another team, it would help to explicitly note:
- which namespace it lives in,
- which cluster team owns and maintains its RBAC, and
- that this adapter should not grant extra infra access (e.g., GCP WIF bindings) via this shared SA.
A short comment or subsection in RBAC/Dependencies calling this out would prevent accidental privilege creep.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hyperfleet/components/adapter/framework/maestro-integration-guide.md(1 hunks)hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-08T22:57:33.164Z
Learnt from: AlexVulaj
Repo: openshift-hyperfleet/architecture PR: 43
File: hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md:17-17
Timestamp: 2025-12-08T22:57:33.164Z
Learning: HyperShift's GCPPlatformSpec exists in the API at api/hypershift/v1beta1/hostedcluster_types.go and is behind the GCPPlatform feature gate. The field is marked immutable and is a valid platform type when the feature gate is enabled.
Applied to files:
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
🪛 markdownlint-cli2 (0.18.1)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md
4-4: Bare URL used
(MD034, no-bare-urls)
23-23: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
697-697: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1009-1009: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1016-1016: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1022-1022: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1029-1029: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1035-1035: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1041-1041: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1047-1047: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1053-1053: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1060-1060: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1066-1066: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1072-1072: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (4)
hyperfleet/components/adapter/hypershift/GCP/hypershift-controlplane-adapter-spike.md (4)
401-462: Overall Maestro + HostedCluster flow is consistent and well‑scopedThe pseudocode and helpers (
buildHostedClusterManifestWork,extractStatusFromManifestWorkFeedback,checkAvailableCondition,checkHealthCondition) show a coherent pattern:
- idempotent GET‑before‑CREATE,
- feedback via JSONPath
"status",- clear mapping to Applied/Available/Health, and
- structured adapter report JSON.
No functional issues jump out here; this section should be a solid reference once the surrounding config examples are wired up.
Also applies to: 467-645
648-680: Status reporter sidecar contract is clear and reusableThe JSON schema and sidecar behavior (poll file → patch Job.status.conditions → exit) are described cleanly and match the two‑container pattern used elsewhere. This should be straightforward to reuse across adapters.
Also applies to: 682-694
171-188: High-level adapter architecture and MVP/post‑MVP split read solidThe separation of concerns (prereqs vs adapter scope, creation‑only MVP vs later update/deletion, Maestro as transport, two‑container Job pattern, and clear mappings to HyperFleet conditions) is well articulated and internally consistent. Nothing blocking here from a design perspective.
Also applies to: 326-356, 739-757, 859-938, 968-1046, 1089-1142
95-162: HostedCluster spec.release.image must be a full OpenShift release payloadThe
spec.release.imagefield (e.g.,quay.io/openshift-release-dev/ocp-release:4.y.z-x86_64) must reference a supported OCP release for the running HyperShift Operator; ensure{{ .openshift_version }}resolves to a valid image tag. For multi-arch, use the-multirelease tag where applicable.However, note that
GCPPlatformSpecin the current HyperShift API is an empty struct and is feature-gated. The fields shown inspec.platform.gcp(project, region, networkConfig, workloadIdentity, endpointAccess, resourceLabels) do not exist in the current API—this YAML appears to be a design proposal rather than aligned with the current upstream HostedCluster specification.
|
/lgtm |
Summary
Spike report for HYPERFLEET-63 defining the HyperShift Control Plane adapter implementation approach for GCP.
Key Decisions
What's Included
Links
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.