SDCICD-1958 move leftover unreferenced base manifests from argo centr… - #236
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR adds six base manifests (api-server, control-plane, RBAC, service, configmap, network policies) copied from the Argo central repo into deploy/base/, with correct restrictive SecurityContexts and no Go code changes. My concerns are structural: the new files duplicate the existing kustomize-referenced base manifests with divergent values, are not wired into any kustomization, and are missing the ServiceAccount/ClusterRoleBinding that would make the control-plane copy functional if applied as-is. Because these are unreferenced reference manifests owned externally, I'm landing on COMMENT rather than blocking, but the duplication/drift and the RBAC gap deserve a maintainer decision before merge.
Findings
[Major] Second, unsynchronized source of truth for base manifests. The new deploy/base/api-server/deployment.yaml, deploy/base/control-plane/deployment.yaml, deploy/base/control-plane/service.yaml, deploy/base/control-plane/rbac/clusterrole.yaml, and deploy/base/platform/networkpolicies.yaml duplicate the already-referenced deploy/base/api-server.yaml, controller.yaml, controller-rbac.yaml, and networkpolicies.yaml, but with divergent values: service/deployment name hypershell-control-plane vs hypershell-controller; provisioner target hypershell-control-plane:9443 vs hypershell-controller:9443; secret hypershell-db vs hypershell-db-app; health port 4434 vs 8434; db-name file /secrets/name vs /secrets/dbname; and the new ClusterRole omits the route.openshift.io/routes/custom-host rule present in controller-rbac.yaml. None of the new files are listed in deploy/base/kustomization.yaml, so they are orphaned in-repo. Two divergent copies of the same components will drift. Please document which set is authoritative (or consolidate).
[Major] Control-plane copy is missing its ServiceAccount and ClusterRoleBinding. deploy/base/control-plane/deployment.yaml sets serviceAccountName: hypershell-control-plane, and deploy/base/control-plane/rbac/clusterrole.yaml defines a ClusterRole named hypershell-controller, but this PR adds no ServiceAccount and no ClusterRoleBinding tying the role to the SA. Applied as-is, the control-plane pod would run with an unbound service account and every reconcile would be RBAC-denied. Confirm the SA and binding are provided in the owner repo (and that the binding subject/namespace match hypershell-control-plane).
[Minor] Environment-specific values baked into a base layer. deploy/base/control-plane/deployment.yaml hardcodes HYPERSHELL_NAMESPACE: hypershell-stage and GATEWAY_API_BASE_DOMAIN: openshell.stage.devshift.net. Base manifests should be environment-agnostic and set these via overlays / downward API, per the config-separate-from-code convention.
[Minor] Mutable :latest image tags. The api-server and control-plane deployments pin hypershell-api-server-main:latest / hypershell-control-plane-main:latest, while the gateway images are digest-pinned. Mutable tags make rollouts non-reproducible; prefer digest pinning.
Cross-PR coordination
Two open pull requests are concurrently editing the exact base manifests this PR duplicates, which forces a source-of-truth decision:
- #232 adds the
bindverb todeploy/base/controller-rbac.yamland switchesHYPERSHELL_NAMESPACEto a downward-APIfieldRefindeploy/base/controller.yaml. This PR's parallel copies (control-plane/rbac/clusterrole.yaml,control-plane/deployment.yaml) do not include thebindverb and hardcodehypershell-stage. Maintainers must decide which copy is authoritative; otherwise #232's RBAC fix and namespace change are silently absent from these new manifests. - #211 adds
--metrics-server-bindaddresstodeploy/base/api-server.yamland an Egress rule to the provisioner NetworkPolicy indeploy/base/networkpolicies.yaml. The corresponding new copies here (platform/networkpolicies.yaml) lack the Egress rule, so the same divergence applies.
These are not mere text merge conflicts: they are competing copies of the same components, and a decision is needed on which layout is canonical and whether concurrent edits must be replicated into the new files (and in what merge order).
Findings Summary (highest severity first)
- [Major] Duplicate, divergent, unreferenced base manifests create a second source of truth - Spec Consistency / Maintainability
- [Major] Control-plane copy missing ServiceAccount + ClusterRoleBinding for
hypershell-control-plane- Reconciliation / RBAC - [Minor] Stage-specific values hardcoded in base layer - Config separate from code
- [Minor] Mutable
:latestimage tags - Reproducibility
Convention Checklist
| Convention | Result |
|---|---|
| Restricted SecurityContext on all containers | Pass |
| Image references consistent across the stack | Fail |
| Config separate from code | Fail |
| Base manifests wired into kustomization | Fail |
| RBAC binding present for service account | Fail |
| labels: | ||
| app: hypershell-control-plane | ||
| spec: | ||
| serviceAccountName: hypershell-control-plane |
There was a problem hiding this comment.
This sets serviceAccountName: hypershell-control-plane, but this PR adds no ServiceAccount by that name and no ClusterRoleBinding tying the hypershell-controller ClusterRole (in rbac/clusterrole.yaml) to it. Applied as-is, the control-plane pod would run unbound and every reconcile would be RBAC-denied. Confirm the SA + binding exist in the owner repo with matching name/namespace.
| - name: HYPERSHELL_API_SERVER_URL | ||
| value: "http://hypershell-api-server:8000" | ||
| - name: HYPERSHELL_NAMESPACE | ||
| value: "hypershell-stage" |
There was a problem hiding this comment.
Stage-specific values (hypershell-stage here, openshell.stage.devshift.net below) are hardcoded into a base manifest. Base layers should be environment-agnostic; set namespace via the downward API (as #232 does for the referenced controller.yaml) and domain via an overlay, per config-separate-from-code.
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRole | ||
| metadata: | ||
| name: hypershell-controller |
There was a problem hiding this comment.
This ClusterRole duplicates deploy/base/controller-rbac.yaml but has already diverged: it omits the route.openshift.io/routes/custom-host rule the referenced copy has, and there is no accompanying ClusterRoleBinding or ServiceAccount here. If this copy becomes authoritative, the controller loses the ability to set spec.host on Routes and runs unbound. Note #232 also adds a bind verb to the referenced copy that is missing here.
| volumes: | ||
| - name: db-secrets | ||
| secret: | ||
| secretName: hypershell-db |
There was a problem hiding this comment.
Divergence from the referenced deploy/base/api-server.yaml: secret name is hypershell-db here vs hypershell-db-app there, health port is 4434 vs 8434, and the db-name file is /secrets/name vs /secrets/dbname. Two copies of the same Deployment with different values will drift. Please clarify which is canonical.
| serviceAccountName: hypershell-api-server | ||
| initContainers: | ||
| - name: migrate | ||
| image: quay.io/redhat-services-prod/hcm-eng-prod-tenant/hypershell-main/hypershell-api-server-main:latest |
There was a problem hiding this comment.
Mutable :latest tag makes rollouts non-reproducible; the gateway images in the control-plane manifest are digest-pinned. Prefer pinning by digest here too (and in the control-plane deployment).
| @@ -0,0 +1,125 @@ | |||
| --- | |||
There was a problem hiding this comment.
This duplicates deploy/base/networkpolicies.yaml (which is kustomization-referenced) with divergences: no namespace, and the provisioner policy targets app: hypershell-control-plane instead of hypershell-controller. It also lacks the Egress rule #211 adds to the referenced copy. Confirm which network-policy set is authoritative to avoid drift.
3 participants
Summary
Move leftover unreferenced base manifests to owner repo.