AROSLSRE-1158: add --install-scope flag to hypershift install - #8725
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@shubhadapaithankar: This pull request references AROSLSRE-1158 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Please specify an area label DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds Sequence Diagram(s)sequenceDiagram
participant CLI
participant Options
participant InstallHyperShiftOperator
participant KubernetesAPI
participant ClusterCAPI
CLI->>Options: set --install-scope
Options->>InstallHyperShiftOperator: resolved InstallScope
alt scope includes CRDs
InstallHyperShiftOperator->>KubernetesAPI: dry-run validate CRDs
InstallHyperShiftOperator->>ClusterCAPI: coordinate unmanaged CRDs
InstallHyperShiftOperator->>KubernetesAPI: apply CRDs
InstallHyperShiftOperator->>KubernetesAPI: wait for CRD establishment
end
alt scope includes resources
InstallHyperShiftOperator->>KubernetesAPI: apply operator resources
InstallHyperShiftOperator->>KubernetesAPI: wait until available
end
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
fe80425 to
ed5f298
Compare
|
/area hypershift-operator |
openshift-ci
Bot
added
the
area/hypershift-operator
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8725 +/- ##
==========================================
+ Coverage 43.37% 43.53% +0.15%
==========================================
Files 771 771
Lines 95718 95753 +35
==========================================
+ Hits 41520 41683 +163
+ Misses 51313 51172 -141
- Partials 2885 2898 +13
... and 2 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ed5f298 to
ff45f66
Compare
celebdor
left a comment
celebdor
left a comment
There was a problem hiding this comment.
Thanks for the PR! The approach makes sense given the container constraints (no kubectl available in the Helm hook image).
The hypershift install render subcommand already defines an Outputs type with OutputAll, OutputCRDs, and OutputResources constants, along with validation logic in ValidateRender(). This PR duplicates those values as raw strings in both the validation switch and the scope conditionals.
I'd suggest reusing the existing type by adding methods to it:
func (o Outputs) IsValid() bool {
switch o {
case OutputAll, OutputCRDs, OutputResources:
return true
default:
return false
}
}
func (o Outputs) IncludesCRDs() bool {
return o == OutputAll || o == OutputCRDs
}
func (o Outputs) IncludesResources() bool {
return o == OutputAll || o == OutputResources
}Then the install path becomes:
scope := Outputs(opts.InstallScope)
if scope == "" {
scope = OutputAll
}
if scope.IncludesCRDs() {
// ...
}
if scope.IncludesResources() {
// ...
}And the validation simplifies to:
if o.InstallScope != "" && !Outputs(o.InstallScope).IsValid() {
errs = append(errs, fmt.Errorf("invalid --install-scope value %q: must be '%s', '%s', or '%s'", o.InstallScope, OutputAll, OutputCRDs, OutputResources))
}This also benefits the render path — its switch and validation can use the same methods, and the sets import can be dropped.
Additionally, the --outputs scope filtering on the render side has no test coverage today either. Since both paths would now share the same methods, adding a test that calls RenderHyperShiftOperator with each scope value and asserts CRDs vs resources in the output (using *apiextensionsv1.CustomResourceDefinition type checks) would cover the logic for both commands. I prototyped this locally — happy to share the test if helpful.
|
I prototyped the aforementioned refactor and tests at |
90f713b to
67b79f6
Compare
|
Thanks @celebdor for the review and the prototype! Addressed all feedback:
|
|
@Nirshal @sdminonne friendly ping - could you take a look when you get a chance? Small change adding |
917d4d5 to
a9f2c29
Compare
celebdor
left a comment
celebdor
left a comment
There was a problem hiding this comment.
/lgtm
Thanks for the changes
|
Oh. I just realized that it decreases the codecov |
|
/retest e2e-azure-v2-self-managed |
|
/retest |
|
/retest e2e-azure-v2-self-managed |
|
/verified by @shubhadapaithankar |
openshift-ci-robot
added
the
verified
|
@shubhadapaithankar: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Add a new --install-scope flag to the hypershift install command that controls which subset of manifests are applied: - "all" (default): installs CRDs and resources (existing behavior) - "crds": installs only CRDs - "resources": installs only resources (operator deployment and RBAC) Reuses the existing Outputs type from the render subcommand, adding IsValid(), IncludesCRDs(), and IncludesResources() helper methods. Both the install and render paths now use the shared methods, and the render path's switch statement is replaced with the same conditional pattern. The sets import is dropped. Includes unit tests for validation (valid/invalid scope values), Outputs helper methods (all enum values + edge cases), and render output filtering (verifying CRDs vs resources in output). This enables consumers to split the hypershift install into phases, allowing CRD-dependent manifests to be applied between the CRD installation and operator startup. Ref: https://redhat.atlassian.net/browse/AROSLSRE-1158 Co-authored-by: celebdor <celebdor@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
48338ef to
2dd1478
Compare
openshift-ci-robot
removed
the
verified
|
/lgtm |
|
Scheduling tests matching the |
|
/verified |
|
@shubhadapaithankar: The DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest e2e-aks |
|
/verified by @shubhadapaithankar |
|
/retest e2e-aks |
openshift-ci-robot
added
the
verified
|
@shubhadapaithankar: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-aks |
|
Now I have the complete picture. Here is the final report: Test Failure Analysis:
|
| Test | Duration | Failure Mode |
|---|---|---|
| TestCreateCluster | 1460s | ExternalDNSHostNotReachable |
| TestCreateClusterCustomConfig | 1447s | ExternalDNSHostNotReachable |
| TestCreateClusterHABreakGlassCredentials | 1529s | ExternalDNSHostNotReachable |
| TestAutoscaling | 1533s | ExternalDNSHostNotReachable |
| TestAzureScheduler | 1518s | ExternalDNSHostNotReachable |
| TestUpgradeControlPlane | 1558s | ExternalDNSHostNotReachable |
| TestNodePool | 0s | ExternalDNSHostNotReachable |
Passing Tests (do not require DNS resolution or worker nodes)
- TestHAEtcdChaos (667s) — uses zero workers
- TestPullSecretUnavailable (687s) — uses zero workers
- TestCreateClusterDefaultSecurityContextUID (241s)
- TestNodePoolMultiArch (0s)
Root Cause
Expired/invalid Azure AD client secret for ExternalDNS service principal.
The external-dns pod was running but continuously failing to authenticate to Azure DNS API with error code AADSTS7000215: Invalid client secret provided. Every 60 seconds from 20:16:02Z through 20:34:16Z (18+ minutes of continuous failure), external-dns attempted to refresh its OAuth2 token against Azure AD endpoint login.microsoftonline.com for app ID 9de86ce0-995b-4731-93d2-24849ecae5e3 in tenant 520cf09d-78ff-44ed-a731-abd623e73b09, and every attempt was rejected with HTTP 401.
Because external-dns could never authenticate, it could never create DNS A/CNAME records in Azure DNS zone aks-e2e.hypershift.azure.devcluster.openshift.com. Without DNS records, every hosted cluster's API endpoint (e.g., api-control-plane-upgrade-gsckp.aks-e2e.hypershift.azure.devcluster.openshift.com) resolved to no such host, causing the ExternalDNSReachable=False and Available=False (KASLoadBalancerNotReachable) conditions on all hosted clusters.
This is a CI infrastructure credential issue, NOT a regression caused by PR #8725. The PR only adds a new --install-scope flag to hypershift install with a default value of all, which preserves the existing behavior (install both CRDs and resources). The code changes are purely additive — the filterManifestsByScope() function returns all CRDs and all resources when scope is all. The install log confirms the operator deployed successfully and external-dns was configured with the correct provider/domain-filter settings. The failure is that the credential secret itself (mounted at /etc/provider/credentials in the external-dns pod) contains an expired or rotated client secret.
Recommendations
-
Rotate the Azure AD client secret for app registration
9de86ce0-995b-4731-93d2-24849ecae5e3in the CI infrastructure. Update the corresponding Prow secret that provides--external-dns-credentialsto thehypershift installstep. -
Re-run the
e2e-aksjob after the credential is rotated —/reteston the PR should be sufficient once the secret is fixed. -
Verify the secret is not expired by checking the App Registration in Azure Portal → Certificates & secrets → Client secrets for the app
9de86ce0-995b-4731-93d2-24849ecae5e3in tenant520cf09d-78ff-44ed-a731-abd623e73b09. -
Consider adding credential validity monitoring — an alert on
AADSTS7000215errors in external-dns logs would catch this sooner in CI. -
This failure is unrelated to PR AROSLSRE-1158: add --install-scope flag to hypershift install #8725 — the PR can be merged once the credential issue is fixed and
e2e-akspasses on re-run.
Evidence
| # | Source | Detail |
|---|---|---|
| 1 | external-dns-87c5d7ff5-5k6ps container log |
27 consecutive AADSTS7000215: Invalid client secret provided errors from 20:16:02Z to 20:34:16Z, one per minute. Azure AD rejected every token refresh attempt for app 9de86ce0-995b-4731-93d2-24849ecae5e3. |
| 2 | build-log.txt |
All 7 failing tests report identical condition: ExternalDNSReachable=False: ExternalDNSHostNotReachable(lookup api-{name}.aks-e2e.hypershift.azure.devcluster.openshift.com ... no such host) |
| 3 | build-log.txt |
All 7 failing tests also report: Available=False: KASLoadBalancerNotReachable — DNS resolution fails so the API server can never be reached |
| 4 | hypershift-install build-log.txt |
Operator install succeeded: deployment "operator" successfully rolled out (line 405). External-dns deployment was applied with correct config: --external-dns-provider=azure, --external-dns-domain-filter=aks-e2e.hypershift.azure.devcluster.openshift.com |
| 5 | junit_operator.xml |
Test phase failed: pod e2e-aks-hypershift-azure-run-e2e exited with code 1. 17 failures out of 91 tests. |
| 6 | PR #8725 diff | Changes are purely additive: adds --install-scope flag with default all. filterManifestsByScope(crds, objects, OutputAll) returns all CRDs and all objects — identical behavior to pre-PR code. No changes to external-dns configuration, credentials handling, or DNS logic. |
| 7 | Passing tests | TestHAEtcdChaos and TestPullSecretUnavailable passed — both use zero worker nodes and don't validate ExternalDNSReachable, confirming the operator itself works correctly. |
|
/retest-required |
|
/retest |
|
@shubhadapaithankar: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
What this PR does / why we need it
Adds a new
--install-scopeflag to thehypershift installcommand that controls which subset of manifests are applied:all(default): installs CRDs and resources (existing behavior, fully backwards compatible)crds: installs only CRDsresources: installs only resources (operator deployment and RBAC)Why we need it
In ARO-HCP, the HyperShift Helm chart uses post-install hooks to run
hypershift install. There's a race condition where the operator starts and creates aClusterSizingConfigurationbefore the Helm hook can apply its own version, causing apply conflicts.Splitting the install into phases allows:
--install-scope=crds)ClusterSizingConfiguration) to be applied by Helm--install-scope=resources)Which issue(s) this PR fixes
Fixes: https://redhat.atlassian.net/browse/AROSLSRE-1158
Related: https://redhat.atlassian.net/browse/AROSLSRE-313
Checklist
Summary by CodeRabbit
New Features
--install-scope(default:all) to control whether CRDs, operator resources, or both are installed.Refactor
--outputsvalidation and manifest selection using explicit CRD-vs-resource scoping rules.Tests
--install-scope.