Add OpenShift Test Extension (OTE) suite#122
Open
mdbooth wants to merge 2 commits into
Open
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThis PR adds a new ChangesOpenShift Tests Extension suite
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Main as main()
participant Framework as e2e Framework
participant GCE as gceFactory
participant K8sAPI as Kubernetes API
participant Subprocess as RunParallel subprocess
Main->>Framework: register gce provider, initFrameworkForTests
Framework->>GCE: NewFactory("gce", gceFactory)
GCE->>K8sAPI: read Infrastructure CR, node labels
K8sAPI-->>GCE: ProjectID, Region, zones
GCE-->>Framework: GCE cloud provider instance
Main->>Main: build & filter specs, decorate serial/parallel
Main->>Subprocess: RunParallel spawns test subprocess per spec
Subprocess-->>Main: test result
🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
openshift-tests/main.go (1)
232-314: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimilar function names
initFrameworkForTests/initFrameworkForTestare easy to mix up.One initializes framework state globally once at startup, the other per-suite-run via
AddBeforeAll. A more distinct naming (e.g.initFrameworkGlobal/initFrameworkPerRun) would reduce the chance of future contributors calling the wrong one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift-tests/main.go` around lines 232 - 314, The two helpers are too easy to confuse: initFrameworkForTests sets one-time global framework state, while initFrameworkForTest is the per-suite AddBeforeAll initializer. Rename them to clearly distinguish scope, such as initFrameworkGlobal and initFrameworkPerRun, and update the corresponding call sites so future changes don’t wire the wrong initializer.openshift-tests/go.mod (1)
108-108: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winBump
go.opentelemetry.io/otel/sdkpast the flagged CVE.OSV flags
go.opentelemetry.io/otel/sdk v1.41.0for GHSA-hfvc-g4fc-pqhx (CVE-2026-39883): "The fix for GHSA-9h8m-3fm2-qjrq (CVE-2026-24051) changed the Darwin ioreg command to use an absolute path but left the BSD kenv command using a bare name, allowing the same PATH hijacking attack on BSD and Solaris platforms." Fixed in 1.43.0. This is an indirect transitive dependency, and the attack surface is limited to BSD/Solaris hosts (unlikely for OpenShift/Linux), but bumping is low-cost and keeps the module clean for supply-chain scanners.🛡️ Suggested version bump
- go.opentelemetry.io/otel/sdk v1.41.0 // indirect + go.opentelemetry.io/otel/sdk v1.43.0 // indirect🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift-tests/go.mod` at line 108, The indirect dependency entry for go.opentelemetry.io/otel/sdk is pinned to a vulnerable version; update the module requirement in go.mod to a fixed release at or above 1.43.0 so the transitive dependency is no longer flagged by OSV. Keep the change scoped to the existing otel/sdk version reference in go.mod and ensure any resulting dependency lock or tidy updates stay consistent with that bump.Source: Linters/SAST tools
openshift-tests/sync-openshift-tests-k8s-deps.py (2)
120-146: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
-dropreplacebranch inbuild_edit_flagsis unreachable.
target_paths(line 134) is built assorted(set(root_replace_map) | set(openshift_tests_replace_map))— a union that always contains every key present inopenshift_tests_replace_map. Consequentlytarget_pairs(returned bytarget_replace_pairs) always has an entry for every path already inopenshift-tests's replace map, so inbuild_edit_flags,set(openshift_tests_map) - set(target_map)(lines 163-164) can never be non-empty — the-dropreplaceflag is generated by dead code. I verified this algebraically and against the "only_updates_changed_paths" unit test (the dropped-set is empty there too, matching the expected output with no-dropreplaceentries).Practical effect: the sync script can never actually remove a stray/obsolete
k8s.io/*replace fromopenshift-tests/go.mod— it will just keep re-versioning it against the fallback forever. If this cleanup capability wasn't intended, consider removing the dead branch for clarity; if it was intended,target_pathsneeds to be derived independently ofopenshift_tests_replace_map(e.g. fromroot_replace_maponly, plus explicitly-allowed extra pins) so obsolete entries can actually be dropped.♻️ One way to make target_paths independent of the child's own map
- target_paths = sorted(set(root_replace_map) | set(openshift_tests_replace_map)) + # Only root-owned k8s.io modules are kept; anything openshift-tests + # added on its own that the parent no longer replaces/requires will be + # dropped by build_edit_flags. + target_paths = sorted(set(root_replace_map) | set(root_require_map))Also applies to: 163-165
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift-tests/sync-openshift-tests-k8s-deps.py` around lines 120 - 146, The -dropreplace path in build_edit_flags is unreachable because target_replace_pairs always includes every k8s.io replace already present in openshift_tests. Update target_replace_pairs so target_paths is derived independently of openshift_tests_replace_map (for example from root_replace_map plus any explicitly allowed extras), and then let build_edit_flags compute removals from the resulting target_map. Keep the existing unique symbols target_replace_pairs and build_edit_flags in sync so obsolete replaces can actually be dropped instead of being preserved and re-versioned.
106-118: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
fallback_k8s_versionis computed eagerly even when unused.
target_replace_pairscallsfallback_k8s_version(...)unconditionally on line 135, which raisesValueErrorif the parentgo.moddoesn't require/replacek8s.io/cloud-provider— even in runs where everyopenshift-testspath already has an explicit root replace/require and the fallback would never be consulted. This is a minor robustness gap in an otherwise defensively-written script.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift-tests/sync-openshift-tests-k8s-deps.py` around lines 106 - 118, Make the fallback version lookup lazy in target_replace_pairs so fallback_k8s_version is only called when a path actually needs a fallback. Right now it is evaluated eagerly and can raise ValueError even when all openshift-tests entries already have an explicit root replace/require. Update the logic around target_replace_pairs to defer calling fallback_k8s_version until after checking whether the current dependency pair still needs it, while keeping the existing fallback_k8s_version behavior unchanged.openshift-hack/update-ote.sh (2)
9-12: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNo guard for unset
REBASEBOT_WORKING_DIR.Without
set -u, an unsetREBASEBOT_WORKING_DIRsilently resolvesOTE_DIRto/openshift-tests.set -ewill catch the resultingpushdfailure, but a clearer upfront check would give a more actionable error message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift-hack/update-ote.sh` around lines 9 - 12, The update-ote.sh setup currently relies on REBASEBOT_WORKING_DIR without checking that it is set, so add an explicit upfront validation before computing OTE_DIR. Use the existing shell setup near set -e and the OTE_DIR assignment to guard against an empty REBASEBOT_WORKING_DIR and exit with a clear, actionable error message if it is missing.
15-19: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winInconsistent path basis between
pushd/popdand latergitcommands.
OTE_DIR(an absolute path derived fromREBASEBOT_WORKING_DIR) is used forpushd, but afterpopdthe script switches to the relative literalopenshift-tests/forgit status/git add. This only works if the script's cwd at invocation happens to equalREBASEBOT_WORKING_DIR. If that assumption ever breaks (e.g. hook invoked from a different cwd),git status --porcelain openshift-tests/would silently produce no matches/error, and theelsebranch would just log "No changes" even thoughmake vendorchanged files — silently dropping vendor updates in the carry commit.Use
$OTE_DIR(orgit -C "$REBASEBOT_WORKING_DIR") consistently instead of the relative path.🔧 Suggested fix
-if [[ -n $(git status --porcelain openshift-tests/) ]]; then - git add openshift-tests/ +if [[ -n $(git -C "${REBASEBOT_WORKING_DIR}" status --porcelain openshift-tests/) ]]; then + git -C "${REBASEBOT_WORKING_DIR}" add openshift-tests/ git commit "${author_flag[@]}" \ -m "UPSTREAM: <carry>: Update OTE vendor dependencies" else echo "No changes to OTE vendor dependencies" fiAlso applies to: 27-29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift-hack/update-ote.sh` around lines 15 - 19, The path handling in update-ote.sh is inconsistent: pushd/popd runs in OTE_DIR, but the later git status/git add logic still uses the relative openshift-tests/ path, which can miss vendor changes when the script is invoked from a different cwd. Update the git status and git add calls to use OTE_DIR consistently, or run them with git -C from REBASEBOT_WORKING_DIR, so the carry-commit detection and staging logic always targets the same directory as make vendor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openshift-tests/main.go`:
- Around line 72-74: The comment near the provider registration and the GCE
factory setup is inaccurate: it says credentials are read in-memory from the
cluster Secret with no ADC/filesystem writes, but `gceFactory` actually uses
`google.FindDefaultCredentials` with a JSON key file source. Update the comment
to match the real credential flow, or change `gceFactory` and any related setup
to truly use in-memory Secret-based credentials if that was intended; make sure
the wording around `framework.RegisterProvider("gce", gceFactory)` and the
credential-loading logic are consistent.
- Around line 152-227: gceFactory currently uses context.Background(), so all
provider setup calls can hang forever without cancellation or timeout. Replace
it with a context that has a deadline/timeout and pass that same ctx through
google.FindDefaultCredentials, configClient.Infrastructures().Get,
e2enode.GetClusterZones, and gcecloud.CreateGCECloud so initialization fails
fast if the API server or GCP services are unresponsive. Keep the change
localized to gceFactory and ensure the timeout is appropriate for provider
startup.
In `@openshift-tests/README.md`:
- Line 3: Update the README description for the OTE suite so it matches the
current scope: it should describe that this suite runs the upstream GCP e2es
from the parent suite with the specified exclusion, and remove or soften the
claim that it adds downstream OpenShift-specific tests. Use the existing README
wording in openshift-tests/README.md to keep the summary accurate and aligned
with the current behavior.
---
Nitpick comments:
In `@openshift-hack/update-ote.sh`:
- Around line 9-12: The update-ote.sh setup currently relies on
REBASEBOT_WORKING_DIR without checking that it is set, so add an explicit
upfront validation before computing OTE_DIR. Use the existing shell setup near
set -e and the OTE_DIR assignment to guard against an empty
REBASEBOT_WORKING_DIR and exit with a clear, actionable error message if it is
missing.
- Around line 15-19: The path handling in update-ote.sh is inconsistent:
pushd/popd runs in OTE_DIR, but the later git status/git add logic still uses
the relative openshift-tests/ path, which can miss vendor changes when the
script is invoked from a different cwd. Update the git status and git add calls
to use OTE_DIR consistently, or run them with git -C from REBASEBOT_WORKING_DIR,
so the carry-commit detection and staging logic always targets the same
directory as make vendor.
In `@openshift-tests/go.mod`:
- Line 108: The indirect dependency entry for go.opentelemetry.io/otel/sdk is
pinned to a vulnerable version; update the module requirement in go.mod to a
fixed release at or above 1.43.0 so the transitive dependency is no longer
flagged by OSV. Keep the change scoped to the existing otel/sdk version
reference in go.mod and ensure any resulting dependency lock or tidy updates
stay consistent with that bump.
In `@openshift-tests/main.go`:
- Around line 232-314: The two helpers are too easy to confuse:
initFrameworkForTests sets one-time global framework state, while
initFrameworkForTest is the per-suite AddBeforeAll initializer. Rename them to
clearly distinguish scope, such as initFrameworkGlobal and initFrameworkPerRun,
and update the corresponding call sites so future changes don’t wire the wrong
initializer.
In `@openshift-tests/sync-openshift-tests-k8s-deps.py`:
- Around line 120-146: The -dropreplace path in build_edit_flags is unreachable
because target_replace_pairs always includes every k8s.io replace already
present in openshift_tests. Update target_replace_pairs so target_paths is
derived independently of openshift_tests_replace_map (for example from
root_replace_map plus any explicitly allowed extras), and then let
build_edit_flags compute removals from the resulting target_map. Keep the
existing unique symbols target_replace_pairs and build_edit_flags in sync so
obsolete replaces can actually be dropped instead of being preserved and
re-versioned.
- Around line 106-118: Make the fallback version lookup lazy in
target_replace_pairs so fallback_k8s_version is only called when a path actually
needs a fallback. Right now it is evaluated eagerly and can raise ValueError
even when all openshift-tests entries already have an explicit root
replace/require. Update the logic around target_replace_pairs to defer calling
fallback_k8s_version until after checking whether the current dependency pair
still needs it, while keeping the existing fallback_k8s_version behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+72
to
+74
| // Register the GCE provider before framework init. Our factory reads | ||
| // credentials in-memory from the cluster Secret (no ADC / filesystem writes). | ||
| framework.RegisterProvider("gce", gceFactory) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Comment contradicts the implementation's credential source.
The main() comment states the factory "reads credentials in-memory from the cluster Secret (no ADC / filesystem writes)," but gceFactory actually calls google.FindDefaultCredentials — Application Default Credentials — pointed at a JSON key file (GOOGLE_APPLICATION_CREDENTIALS or $CLUSTER_PROFILE_DIR/gce.json), which is exactly the ADC/filesystem-based flow the comment says is avoided. This mismatch could mislead future maintainers about the actual credential model.
📝 Suggested comment fix
- // Register the GCE provider before framework init. Our factory reads
- // credentials in-memory from the cluster Secret (no ADC / filesystem writes).
+ // Register the GCE provider before framework init. Our factory locates
+ // GCP Application Default Credentials from a file (typically the Prow
+ // cluster-profile secret mounted at $CLUSTER_PROFILE_DIR/gce.json).Also applies to: 148-167
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift-tests/main.go` around lines 72 - 74, The comment near the provider
registration and the GCE factory setup is inaccurate: it says credentials are
read in-memory from the cluster Secret with no ADC/filesystem writes, but
`gceFactory` actually uses `google.FindDefaultCredentials` with a JSON key file
source. Update the comment to match the real credential flow, or change
`gceFactory` and any related setup to truly use in-memory Secret-based
credentials if that was intended; make sure the wording around
`framework.RegisterProvider("gce", gceFactory)` and the credential-loading logic
are consistent.
Comment on lines
+152
to
+227
| func gceFactory() (framework.ProviderInterface, error) { | ||
| ctx := context.Background() | ||
|
|
||
| // Use GOOGLE_APPLICATION_CREDENTIALS_FILE if it is already set | ||
| if os.Getenv("GOOGLE_APPLICATION_CREDENTIALS") == "" { | ||
| // If it looks like we're executing in Prow, use the cluster profile credentials file. | ||
| if clusterProfileDir := os.Getenv("CLUSTER_PROFILE_DIR"); clusterProfileDir != "" { | ||
| os.Setenv("GOOGLE_APPLICATION_CREDENTIALS", filepath.Join(clusterProfileDir, "gce.json")) | ||
| } | ||
| } | ||
|
|
||
| // FindDefaultCredentials falls back to user credential locations if GOOGLE_APPLICATION_CREDENTIALS is not set. | ||
| creds, err := google.FindDefaultCredentials(ctx, "https://www.googleapis.com/auth/compute") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to find GCP credentials: %w", err) | ||
| } | ||
|
|
||
| clientConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig( | ||
| &clientcmd.ClientConfigLoadingRules{ExplicitPath: os.Getenv("KUBECONFIG")}, | ||
| &clientcmd.ConfigOverrides{}, | ||
| ) | ||
| restConfig, err := clientConfig.ClientConfig() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to build REST config: %w", err) | ||
| } | ||
|
|
||
| // Read project and region from the Infrastructure CR. | ||
| configClient, err := configv1client.NewForConfig(restConfig) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create OpenShift config client: %w", err) | ||
| } | ||
| infra, err := configClient.Infrastructures().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get Infrastructure CR: %w", err) | ||
| } | ||
| if infra.Status.PlatformStatus == nil || infra.Status.PlatformStatus.GCP == nil { | ||
| return nil, fmt.Errorf("Infrastructure CR does not contain GCP platform status") | ||
| } | ||
| kubeClient, err := kclientset.NewForConfig(restConfig) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create Kubernetes client: %w", err) | ||
| } | ||
| projectID := infra.Status.PlatformStatus.GCP.ProjectID | ||
| region := infra.Status.PlatformStatus.GCP.Region | ||
| managedZones, err := e2enode.GetClusterZones(ctx, kubeClient) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to determine cluster zones from node labels: %w", err) | ||
| } | ||
| if len(managedZones) == 0 { | ||
| return nil, fmt.Errorf("failed to determine cluster zones from node labels: no zones found") | ||
| } | ||
| zone := managedZones.List()[0] | ||
| // On OpenShift GCP the VPC network is named after the cluster infrastructure name. | ||
| networkName := infra.Status.InfrastructureName | ||
|
|
||
| gceCloud, err := gcecloud.CreateGCECloud(&gcecloud.CloudConfig{ | ||
| ProjectID: projectID, | ||
| NetworkName: networkName, | ||
| Region: region, | ||
| Zone: zone, | ||
| ManagedZones: managedZones.List(), | ||
| TokenSource: creds.TokenSource, | ||
| UseMetadataServer: false, | ||
| AlphaFeatureGate: gcecloud.NewAlphaFeatureGate([]string{}), | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create GCE Cloud client: %w", err) | ||
| } | ||
|
|
||
| // Populate the framework CloudConfig so that GetGCECloud() and zone-aware | ||
| // tests can look up provider metadata without re-fetching the Infrastructure CR. | ||
| testContext.CloudConfig.ProjectID = projectID | ||
| testContext.CloudConfig.Region = region | ||
| testContext.CloudConfig.Zone = zone | ||
|
|
||
| return gcee2e.NewProvider(gceCloud), nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout/deadline to gceFactory's context.
ctx := context.Background() is used for every downstream network call in this function (google.FindDefaultCredentials, configClient.Infrastructures().Get, kubeClient calls, e2enode.GetClusterZones, gcecloud.CreateGCECloud). None of these have a deadline, so an unresponsive OpenShift API server or GCP metadata/IAM endpoint would hang the test binary's provider initialization indefinitely with no way to recover.
As per path instructions, **/*.go Go security guidance requires "context.Context for cancellation and timeouts."
⏱️ Suggested fix
-func gceFactory() (framework.ProviderInterface, error) {
- ctx := context.Background()
+func gceFactory() (framework.ProviderInterface, error) {
+ ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
+ defer cancel()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func gceFactory() (framework.ProviderInterface, error) { | |
| ctx := context.Background() | |
| // Use GOOGLE_APPLICATION_CREDENTIALS_FILE if it is already set | |
| if os.Getenv("GOOGLE_APPLICATION_CREDENTIALS") == "" { | |
| // If it looks like we're executing in Prow, use the cluster profile credentials file. | |
| if clusterProfileDir := os.Getenv("CLUSTER_PROFILE_DIR"); clusterProfileDir != "" { | |
| os.Setenv("GOOGLE_APPLICATION_CREDENTIALS", filepath.Join(clusterProfileDir, "gce.json")) | |
| } | |
| } | |
| // FindDefaultCredentials falls back to user credential locations if GOOGLE_APPLICATION_CREDENTIALS is not set. | |
| creds, err := google.FindDefaultCredentials(ctx, "https://www.googleapis.com/auth/compute") | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to find GCP credentials: %w", err) | |
| } | |
| clientConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig( | |
| &clientcmd.ClientConfigLoadingRules{ExplicitPath: os.Getenv("KUBECONFIG")}, | |
| &clientcmd.ConfigOverrides{}, | |
| ) | |
| restConfig, err := clientConfig.ClientConfig() | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to build REST config: %w", err) | |
| } | |
| // Read project and region from the Infrastructure CR. | |
| configClient, err := configv1client.NewForConfig(restConfig) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create OpenShift config client: %w", err) | |
| } | |
| infra, err := configClient.Infrastructures().Get(ctx, "cluster", metav1.GetOptions{}) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get Infrastructure CR: %w", err) | |
| } | |
| if infra.Status.PlatformStatus == nil || infra.Status.PlatformStatus.GCP == nil { | |
| return nil, fmt.Errorf("Infrastructure CR does not contain GCP platform status") | |
| } | |
| kubeClient, err := kclientset.NewForConfig(restConfig) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create Kubernetes client: %w", err) | |
| } | |
| projectID := infra.Status.PlatformStatus.GCP.ProjectID | |
| region := infra.Status.PlatformStatus.GCP.Region | |
| managedZones, err := e2enode.GetClusterZones(ctx, kubeClient) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to determine cluster zones from node labels: %w", err) | |
| } | |
| if len(managedZones) == 0 { | |
| return nil, fmt.Errorf("failed to determine cluster zones from node labels: no zones found") | |
| } | |
| zone := managedZones.List()[0] | |
| // On OpenShift GCP the VPC network is named after the cluster infrastructure name. | |
| networkName := infra.Status.InfrastructureName | |
| gceCloud, err := gcecloud.CreateGCECloud(&gcecloud.CloudConfig{ | |
| ProjectID: projectID, | |
| NetworkName: networkName, | |
| Region: region, | |
| Zone: zone, | |
| ManagedZones: managedZones.List(), | |
| TokenSource: creds.TokenSource, | |
| UseMetadataServer: false, | |
| AlphaFeatureGate: gcecloud.NewAlphaFeatureGate([]string{}), | |
| }) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create GCE Cloud client: %w", err) | |
| } | |
| // Populate the framework CloudConfig so that GetGCECloud() and zone-aware | |
| // tests can look up provider metadata without re-fetching the Infrastructure CR. | |
| testContext.CloudConfig.ProjectID = projectID | |
| testContext.CloudConfig.Region = region | |
| testContext.CloudConfig.Zone = zone | |
| return gcee2e.NewProvider(gceCloud), nil | |
| func gceFactory() (framework.ProviderInterface, error) { | |
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) | |
| defer cancel() | |
| // Use GOOGLE_APPLICATION_CREDENTIALS_FILE if it is already set | |
| if os.Getenv("GOOGLE_APPLICATION_CREDENTIALS") == "" { | |
| // If it looks like we're executing in Prow, use the cluster profile credentials file. | |
| if clusterProfileDir := os.Getenv("CLUSTER_PROFILE_DIR"); clusterProfileDir != "" { | |
| os.Setenv("GOOGLE_APPLICATION_CREDENTIALS", filepath.Join(clusterProfileDir, "gce.json")) | |
| } | |
| } | |
| // FindDefaultCredentials falls back to user credential locations if GOOGLE_APPLICATION_CREDENTIALS is not set. | |
| creds, err := google.FindDefaultCredentials(ctx, "https://www.googleapis.com/auth/compute") | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to find GCP credentials: %w", err) | |
| } | |
| clientConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig( | |
| &clientcmd.ClientConfigLoadingRules{ExplicitPath: os.Getenv("KUBECONFIG")}, | |
| &clientcmd.ConfigOverrides{}, | |
| ) | |
| restConfig, err := clientConfig.ClientConfig() | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to build REST config: %w", err) | |
| } | |
| // Read project and region from the Infrastructure CR. | |
| configClient, err := configv1client.NewForConfig(restConfig) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create OpenShift config client: %w", err) | |
| } | |
| infra, err := configClient.Infrastructures().Get(ctx, "cluster", metav1.GetOptions{}) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get Infrastructure CR: %w", err) | |
| } | |
| if infra.Status.PlatformStatus == nil || infra.Status.PlatformStatus.GCP == nil { | |
| return nil, fmt.Errorf("Infrastructure CR does not contain GCP platform status") | |
| } | |
| kubeClient, err := kclientset.NewForConfig(restConfig) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create Kubernetes client: %w", err) | |
| } | |
| projectID := infra.Status.PlatformStatus.GCP.ProjectID | |
| region := infra.Status.PlatformStatus.GCP.Region | |
| managedZones, err := e2enode.GetClusterZones(ctx, kubeClient) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to determine cluster zones from node labels: %w", err) | |
| } | |
| if len(managedZones) == 0 { | |
| return nil, fmt.Errorf("failed to determine cluster zones from node labels: no zones found") | |
| } | |
| zone := managedZones.List()[0] | |
| // On OpenShift GCP the VPC network is named after the cluster infrastructure name. | |
| networkName := infra.Status.InfrastructureName | |
| gceCloud, err := gcecloud.CreateGCECloud(&gcecloud.CloudConfig{ | |
| ProjectID: projectID, | |
| NetworkName: networkName, | |
| Region: region, | |
| Zone: zone, | |
| ManagedZones: managedZones.List(), | |
| TokenSource: creds.TokenSource, | |
| UseMetadataServer: false, | |
| AlphaFeatureGate: gcecloud.NewAlphaFeatureGate([]string{}), | |
| }) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create GCE Cloud client: %w", err) | |
| } | |
| // Populate the framework CloudConfig so that GetGCECloud() and zone-aware | |
| // tests can look up provider metadata without re-fetching the Infrastructure CR. | |
| testContext.CloudConfig.ProjectID = projectID | |
| testContext.CloudConfig.Region = region | |
| testContext.CloudConfig.Zone = zone | |
| return gcee2e.NewProvider(gceCloud), nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift-tests/main.go` around lines 152 - 227, gceFactory currently uses
context.Background(), so all provider setup calls can hang forever without
cancellation or timeout. Replace it with a context that has a deadline/timeout
and pass that same ctx through google.FindDefaultCredentials,
configClient.Infrastructures().Get, e2enode.GetClusterZones, and
gcecloud.CreateGCECloud so initialization fails fast if the API server or GCP
services are unresponsive. Keep the change localized to gceFactory and ensure
the timeout is appropriate for provider startup.
Source: Path instructions
|
@mdbooth: 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. |
|
Hi, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an OTE suite. The initial version simply calls the upstream e2e suite. We mirror the factory code for initialisation, pulling required configuration from OpenShift Prow.
Excludes a single tests which expects to run in-cluster, and therefore will not work on OpenShift Prow.
Summary by CodeRabbit
New Features
Documentation
Chores