OCPBUGS-84569: Fix node.kubernetes.io/exclude-from-external-load-balancers on masters#121
Conversation
openshift-ci-robot
added
jira/severity-important
|
@mdbooth: This pull request references Jira Issue OCPBUGS-84569, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR migrates instance-name set handling in GCE internal load balancer OpenShift logic from sets.String to the generic sets.Set[string], adds bootstrap-instance exclusion during external instance group reuse evaluation, changes reuse decision criteria, removes the prefix-based fallback, and adds corresponding tests. ChangesGCE external instance group reuse update
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller as filterNodesWithExistingExternalInstanceGroups
participant Extract as extractInstanceNamesFromGroup
participant Evaluate as evaluateExternalInstanceGroup
participant Zone as gceInstanceNamesInZone
Caller->>Extract: get instance names from external group
Extract-->>Caller: return sets.Set[string]
Caller->>Evaluate: evaluate group with instance set
Evaluate->>Evaluate: remove bootstrap instance name
Evaluate->>Zone: get host names for zone
Zone-->>Evaluate: return sets.Set[string]
Evaluate->>Evaluate: check HasAll and Len > 0
Evaluate-->>Caller: reuse decision
Compact metadata
Poem Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 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: 1
🤖 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 `@providers/gce/gce_loadbalancer_internal_openshift_test.go`:
- Around line 167-169: Tighten the assertion in evaluateExternalInstanceGroup
test coverage so it verifies the exact returned instance set, not just subset
membership. In the loop over tc.wantInSet, add a check against the full contents
of instanceNames in this test case so unwanted entries like the bootstrap
instance cause a failure, and keep the check anchored around tc.wantInSet and
instanceNames in gce_loadbalancer_internal_openshift_test.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 35323f1a-5a8d-45c7-85da-2d98487d3d9f
📒 Files selected for processing (2)
providers/gce/gce_loadbalancer_internal_openshift.goproviders/gce/gce_loadbalancer_internal_openshift_test.go
|
/test e2e-gcp-ovn |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
openshift-tests/Makefile (1)
14-18: 📐 Maintainability & Code Quality | 🔵 TrivialAdd a
testtarget and cover the whole module incheck.
make testis part of the repo’s documented workflow, but this Makefile doesn’t expose it yet. Also,go fmt ./go vet .only touch the root package, so any future subpackages inopenshift-testswould be skipped. As per coding guidelines, this repo expectsmake testfor local validation, and static analysis also flagged the missing target.♻️ Proposed fix
.PHONY: check check: - GOWORK=$(GOWORK) go fmt . - GOWORK=$(GOWORK) go vet . + GOWORK=$(GOWORK) go fmt ./... + GOWORK=$(GOWORK) go vet ./... + +.PHONY: test +test: + GOWORK=$(GOWORK) go test ./...🤖 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/Makefile` around lines 14 - 18, Add a new test target in the Makefile so `make test` is available for the documented workflow, and make `check` cover the entire Go module instead of only the root package. Update the existing `check` recipe and the new `test` target to use the module-wide package pattern from the `openshift-tests` Makefile context, keeping `GOWORK=$(GOWORK)` on each Go command. Use the `check` and `test` target names as the entry points to ensure future subpackages are included.Sources: Coding guidelines, Linters/SAST tools
🤖 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/go.mod`:
- Around line 149-182: The openshift-tests nested module is not resolving the
local dependency graph correctly, which leaves golangci-lint failing on missing
conversion symbols. Fix the module wiring in the go.mod replace blocks so the
local providers and test/e2e modules are used consistently and the pinned k8s.io
staging repos match the repo’s expected versions. Verify the openshift-tests
module can typecheck after the replace/pin setup is updated.
In `@openshift-tests/loadbalancer_openshift.go`:
- Around line 59-121: The ILB verification in loadbalancer_openshift.go is
asserting too early and can race both node-label propagation and backend
reconciliation. Update the control-plane exclusion check in the test flow around
labelControlPlaneNodes, createInternalLoadBalancerService, and the backend
service/instance group inspection to poll repeatedly until the control-plane
instances are no longer present in any backend group. Use a retry/wait loop
around the existing gceCloud.ListRegionForwardingRules,
gceCloud.GetRegionBackendService, and gceCloud.ListInstancesInInstanceGroup
checks instead of a single immediate read, and only fail after the timeout if
the excluded nodes still appear.
In `@openshift-tests/main.go`:
- Around line 185-210: The zone handling in main.go is synthesizing a fake value
with region + "-a", which can point tests at the wrong GCP zone. Update the
setup around gcecloud.CreateGCECloud and the testContext.CloudConfig assignment
to derive Zone and ManagedZones from authoritative cluster topology or infra
data (such as a node zone label), and use that real zone consistently instead of
hardcoding the suffix-based guess.
- Around line 133-139: The root Cobra command in main currently uses the default
stdout/stderr streams, so help and usage output can leak plain text to stdout.
Update the root command setup to direct Cobra’s output and error streams away
from stdout, using stderr or the appropriate logger before calling Execute on
root. Keep the fix in the root command initialization path so all help/usage
paths from cobra.Command are covered.
- Around line 107-120: The skip handling in the spec setup is only overriding
Run, so skipped parallel-suite specs still execute through RunParallel. Update
the logic in the spec registration block around decoratedName, spec.RunParallel,
and the skippedSpecs check so that skipped entries also replace or disable
RunParallel and return an ExtensionTestResult with ResultSkipped, ensuring
skippedSpecs.Name never runs via SpawnProcessToRunTest.
---
Nitpick comments:
In `@openshift-tests/Makefile`:
- Around line 14-18: Add a new test target in the Makefile so `make test` is
available for the documented workflow, and make `check` cover the entire Go
module instead of only the root package. Update the existing `check` recipe and
the new `test` target to use the module-wide package pattern from the
`openshift-tests` Makefile context, keeping `GOWORK=$(GOWORK)` on each Go
command. Use the `check` and `test` target names as the entry points to ensure
future subpackages are included.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
🧹 Nitpick comments (3)
openshift-hack/sync-openshift-tests-k8s-deps.py (2)
62-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCaptured stderr is lost on failure, hurting debuggability.
run()usescheck=Truewithcapture_output=Trueforload_go_mod_json. Ifgo mod edit -jsonfails (e.g. malformed go.mod), the raisedCalledProcessError's default traceback omits the captured stderr text, leaving only the exit code — making CI/build failures harder to diagnose.♻️ Proposed fix to surface captured stderr on failure
def run(command, cwd, env=None, capture_output=False): - return subprocess.run( - command, - check=True, - cwd=cwd, - env=env, - text=True, - capture_output=capture_output, - ) + try: + return subprocess.run( + command, + check=True, + cwd=cwd, + env=env, + text=True, + capture_output=capture_output, + ) + except subprocess.CalledProcessError as exc: + if exc.stderr: + print(exc.stderr, file=sys.stderr) + raise🤖 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/sync-openshift-tests-k8s-deps.py` around lines 62 - 80, The `run()` helper is hiding useful stderr when `load_go_mod_json()` calls `go mod edit -json` and the command fails. Update `run()` so failures from `subprocess.run` surface captured stderr (for example by wrapping or enriching the raised error), and make sure the `load_go_mod_json` call path still uses `run()` so malformed go.mod parsing reports the actual stderr output instead of only an exit code.
109-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for the core diff logic.
target_replace_pairsandbuild_edit_flagsare pure functions over parsed go.mod JSON and encode the entire sync policy (root-replace > root-require > cloud-provider fallback, plus minimal-diff edit flags). They're straightforward to unit test with syntheticRequire/Replacedicts, without needing to mockgo/subprocess. Given this repo's contribution norms around testing changes, worth covering this logic directly.Based on learnings: "Add tests relevant to the fixed bug or new feature" / "Write tests for your changes."
🤖 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/sync-openshift-tests-k8s-deps.py` around lines 109 - 162, Add unit tests for the pure sync-policy logic in target_replace_pairs and build_edit_flags using synthetic go.mod JSON inputs. Cover the key precedence rules (root Replace over root Require over fallback) and verify build_edit_flags produces minimal diffs, including no-op output when target_pairs already matches openshift_tests_pairs and only dropreplace/replace entries for changed paths.Source: Learnings
openshift-tests/Makefile (1)
15-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
checktarget silently reformats instead of failing on drift.
go fmt .rewrites files and exits 0 even when formatting changes were needed, so this target won't catch unformatted code in CI — it just masks it. Usegofmt -l(non-mutating) to fail when output is non-empty, consistent with the repo'stools/verify-gofmt.shconvention.🔧 Proposed fix
.PHONY: check check: - GOWORK=$(GOWORK) go fmt . + `@if` [ -n "$$(GOWORK=$(GOWORK) gofmt -l .)" ]; then \ + echo "The following files are not gofmt'd:"; \ + GOWORK=$(GOWORK) gofmt -l .; \ + exit 1; \ + fi GOWORK=$(GOWORK) go vet .🤖 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/Makefile` around lines 15 - 19, The check target in Makefile is mutating files with go fmt instead of failing on formatting drift. Update the check recipe to use a non-mutating format verification approach, matching the repository’s gofmt verification convention (for example, the same behavior as tools/verify-gofmt.sh), so it exits non-zero when formatting changes are needed. Keep the existing GOWORK handling and leave the go vet step unchanged.
🤖 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.
Nitpick comments:
In `@openshift-hack/sync-openshift-tests-k8s-deps.py`:
- Around line 62-80: The `run()` helper is hiding useful stderr when
`load_go_mod_json()` calls `go mod edit -json` and the command fails. Update
`run()` so failures from `subprocess.run` surface captured stderr (for example
by wrapping or enriching the raised error), and make sure the `load_go_mod_json`
call path still uses `run()` so malformed go.mod parsing reports the actual
stderr output instead of only an exit code.
- Around line 109-162: Add unit tests for the pure sync-policy logic in
target_replace_pairs and build_edit_flags using synthetic go.mod JSON inputs.
Cover the key precedence rules (root Replace over root Require over fallback)
and verify build_edit_flags produces minimal diffs, including no-op output when
target_pairs already matches openshift_tests_pairs and only dropreplace/replace
entries for changed paths.
In `@openshift-tests/Makefile`:
- Around line 15-19: The check target in Makefile is mutating files with go fmt
instead of failing on formatting drift. Update the check recipe to use a
non-mutating format verification approach, matching the repository’s gofmt
verification convention (for example, the same behavior as
tools/verify-gofmt.sh), so it exits non-zero when formatting changes are needed.
Keep the existing GOWORK handling and leave the go vet step unchanged.
| require.NoError(t, err) | ||
| assert.True(t, shouldReuse, "should reuse master IG even with bootstrap node present") | ||
|
|
||
| for name, tc := range map[string]struct { |
There was a problem hiding this comment.
Rewriting this to table driven tests doesn't make sense here. It makes understanding your change / fix pair complex.
If you want to rewrite to table driven / rework these tests, please do as part of another PR, and we can evaluate it separately from this bug fix.
There was a problem hiding this comment.
It's not worth the effort to resequence these. The previous tests were missing coverage both of pre-existing and added functionality. This was the simplest way to add coverage without exploding the size of the tests.
I don't plan to make this change.
There was a problem hiding this comment.
It makes the diff ten times harder to follow, especially for anyone not familiar with this code - which is likely given how we're currently operating in PIXAA.
Adding 2-3 test cases to the existing structure would have been fewer lines changed than rewriting the whole file. And "not worth the effort to resequence" , it's a rebase.
Again, I'm not against rewriting it to table driven tests, just as a separate PR please.
There was a problem hiding this comment.
It's not worth the effort to resequence these.
You're currently reinventing behaviour we have a pattern for on the 3CMO. With added python scripts and an entirely separate vendor module.
There was a problem hiding this comment.
It's not worth the effort to resequence these.
You're currently reinventing behaviour we have a pattern for on the 3CMO. With added python scripts and an entirely separate vendor module.
Is this related to table-driven tests?
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/go.mod`:
- Line 108: The OTEL SDK dependency is still pinned to an affected release in
openshift-tests/go.mod. Update the go.opentelemetry.io/otel/sdk module, and any
related OTEL core modules it must stay in sync with, to a patched version at or
above 1.43.0 so the dependency set no longer includes the vulnerable release.
In `@openshift-tests/main.go`:
- Around line 153-164: The provider initialization path in main should not use
an unbounded context for credential discovery and early cluster API setup.
Update the initialization in the block around google.FindDefaultCredentials (and
the related startup path mentioned by the review) to use a timed context with
cancellation instead of context.Background(), and ensure the context is properly
canceled after the initialization completes.
- Around line 155-160: The credential setup in the main initialization path
ignores the error returned by os.Setenv, which can hide failures when setting
GOOGLE_APPLICATION_CREDENTIALS. Update the logic around the os.Setenv call in
main so the error is checked and propagated/returned instead of being discarded,
and keep the behavior gated by the existing GOOGLE_APPLICATION_CREDENTIALS and
CLUSTER_PROFILE_DIR checks. Use the existing main setup code and the os.Setenv
call as the anchor for the fix.
In `@openshift-tests/sync-openshift-tests-k8s-deps.py`:
- Around line 17-20: The usage example is pointing to the wrong script path, so
update the docstring in sync-openshift-tests-k8s-deps.py to reference the actual
location under openshift-tests or remove the path-specific direct-invocation
example. Keep the make sync-openshift-tests-k8s-deps invocation, and ensure the
repository-root guidance matches the real script name and location.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.35.0 // indirect | ||
| go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.35.0 // indirect | ||
| go.opentelemetry.io/otel/metric v1.41.0 // indirect | ||
| go.opentelemetry.io/otel/sdk v1.41.0 // indirect |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== openshift-tests/go.mod around OTEL entries =="
nl -ba openshift-tests/go.mod | sed -n '90,130p'
echo
echo "== all OTEL references in openshift-tests/go.mod and openshift-tests/go.sum =="
rg -n "go.opentelemetry.io/otel|otel/sdk" openshift-tests/go.mod openshift-tests/go.sum || trueRepository: openshift/cloud-provider-gcp
Length of output: 255
🌐 Web query:
go.opentelemetry.io/otel/sdk v1.41.0 GHSA-hfvc-g4fc-pqhx patched version
💡 Result:
The vulnerability GHSA-hfvc-g4fc-pqhx, which affects go.opentelemetry.io/otel/sdk, is patched in version 1.43.0 [1][2]. This vulnerability involves a PATH hijacking risk on BSD and Solaris platforms due to the use of a bare command name (kenv) instead of an absolute path [1][3]. Version 1.41.0 is considered an affected version, as the vulnerability affects versions 1.15.0 through 1.42.0 [1][4][2]. Users are advised to upgrade to 1.43.0 or later to resolve this security issue [1][5][2].
Citations:
- 1: GHSA-hfvc-g4fc-pqhx
- 2: https://nvd.nist.gov/vuln/detail/CVE-2026-39883
- 3: https://osv.dev/vulnerability/CVE-2026-39883
- 4: https://osv.dev/vulnerability/GHSA-hfvc-g4fc-pqhx
- 5: Update module go.opentelemetry.io/otel/sdk to v1.43.0 [SECURITY] equinix-labs/otel-init-go#43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== openshift-tests module file size =="
wc -l openshift-tests/go.mod
echo
echo "== direct requires in openshift-tests/go.mod =="
sed -n '/^require (/,/^)/p' openshift-tests/go.mod | sed -n '1,200p'Repository: openshift/cloud-provider-gcp
Length of output: 7156
Bump the OTEL SDK off the affected release. In openshift-tests/go.mod, go.opentelemetry.io/otel/sdk v1.41.0 is in the affected range fixed in 1.43.0; update the OTEL core modules together to a patched release.
🤖 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 OTEL SDK dependency is still pinned
to an affected release in openshift-tests/go.mod. Update the
go.opentelemetry.io/otel/sdk module, and any related OTEL core modules it must
stay in sync with, to a patched version at or above 1.43.0 so the dependency set
no longer includes the vulnerable release.
Source: Linters/SAST tools
| 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") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound provider initialization with a timeout.
context.Background() gives credential discovery and cluster API calls no deadline, so a stalled dependency can hang the extension startup. Use a timed context for this initialization path.
Proposed fix
- ctx := context.Background()
+ ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
+ defer cancel()As per path instructions, "**/*.go: Go security (prodsec-skills): - context.Context for cancellation and timeouts".
Also applies to: 183-196
🤖 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 153 - 164, The provider initialization
path in main should not use an unbounded context for credential discovery and
early cluster API setup. Update the initialization in the block around
google.FindDefaultCredentials (and the related startup path mentioned by the
review) to use a timed context with cancellation instead of
context.Background(), and ensure the context is properly canceled after the
initialization completes.
Source: Path instructions
| // 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")) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Check the os.Setenv error.
Line 159 ignores the error return from os.Setenv; please return it so credential setup failures are visible.
Proposed fix
- os.Setenv("GOOGLE_APPLICATION_CREDENTIALS", filepath.Join(clusterProfileDir, "gce.json"))
+ if err := os.Setenv("GOOGLE_APPLICATION_CREDENTIALS", filepath.Join(clusterProfileDir, "gce.json")); err != nil {
+ return nil, fmt.Errorf("failed to set GOOGLE_APPLICATION_CREDENTIALS: %w", err)
+ }As per path instructions, "**/*.go: Go security (prodsec-skills): - Never ignore error returns".
📝 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.
| // 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")) | |
| } | |
| // 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 != "" { | |
| if err := os.Setenv("GOOGLE_APPLICATION_CREDENTIALS", filepath.Join(clusterProfileDir, "gce.json")); err != nil { | |
| return nil, fmt.Errorf("failed to set GOOGLE_APPLICATION_CREDENTIALS: %w", err) | |
| } | |
| } |
🤖 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 155 - 160, The credential setup in the
main initialization path ignores the error returned by os.Setenv, which can hide
failures when setting GOOGLE_APPLICATION_CREDENTIALS. Update the logic around
the os.Setenv call in main so the error is checked and propagated/returned
instead of being discarded, and keep the behavior gated by the existing
GOOGLE_APPLICATION_CREDENTIALS and CLUSTER_PROFILE_DIR checks. Use the existing
main setup code and the os.Setenv call as the anchor for the fix.
Source: Path instructions
| version without putting `openshift-tests` into `go.work`. | ||
|
|
||
| Invoke from the repository root with either: | ||
| ./openshift-hack/sync-openshift-tests-k8s-deps.py |
There was a problem hiding this comment.
So when/how often do I need to call this script?
There was a problem hiding this comment.
I meant to add that 🤦 I'll add it.
But to avoid suspense: you don't. Rebasebot does it.
| }, | ||
| // The LB IG name matches an existing external IG name. filterNodesWithExistingExternalInstanceGroups | ||
| // skips the IG to avoid treating the LB's own IG as an external group to reuse. | ||
| "external IG with same name as LB IG is skipped": { |
There was a problem hiding this comment.
I think we're missing a case for this bug here, where we have prefiltered the 'nodes' (that's the vendored cloud provider code doing it's job) list we want a test something like "masters excluded by label are not in nodes so master IGs should not be reused" where we have GCEInstances with masters, but no nodes matching
I think we want:
- master GCE instances exist
- master IGs exist containing them
- but nodes only has workers.
Then assert wantIGLinks is empty and only workers are in wantIncludedNodes.
There was a problem hiding this comment.
I flip-flopped on this, because in general my preference is also for integrated tests at a level where the desired outcome is more obvious. I think this would duplicate "IG contains instance not in node list and not matching prefix so IG should not be reused" in the other unit. In the end I went with that because they're shorter.
However, I could be persuaded to fold all the TestEvaluateExternalInstanceGroup tests in here. What do you think?
|
Have we spoken with any other teams about carrying OTE on upstream repositories and their challenges with it? |
There was a big slack thread about it. What I took away from that was that we generally thought it was a good idea, but the separate go module which can't be part of the workspace was a PITA. I've tried to address that here, but the path to retreat to CCCMO remains open. |
|
Infra flake: |
I have reviewed that thread, I should have been more specific. I wanted to know if there were any teams who had been successful in doing something like this. My read of that particular thread was that SPLAT and previous cluster infra deemed it too complex. Perhaps storage tried this |
My take is that this is what SPLAT originally intended before moving the tests to CCCMO. The issue they hit was with automated rebases. The advantage of moving it to CCCMO is that you don't have automated rebases any more, so if a problem comes up it's straightforward to fix because you were doing it manually anyway. That's also a disadvantage, though, because now:
And in fact the moved AWS tests don't use our shipped code at all. The other advantage we lose by moving it to CCCMO is one of the primary design advantages of OTE: testable single PR changes. So I thought it was worth another go before giving up. The situation with the downstream-only forked ginkgo (why do we keep doing this?) makes it complicated because we obviously don't want to pollute the parent repo with it. The solution to this (in CCCMO and elsewhere) is to use an isolated go module, so I've copied that. This seemed to be the source of the rebase issues: if upstream bumps k8s incompatibly and the test module is pulling in upstream e2es, it won't compile any more. My idea is a one-way sync for k8s dependencies in the test module, executed automatically after rebasebot updates the parent repo. If it works in practise, perhaps we could move AWS back. If it doesn't... 🤷 We move it to CCCMO and maintain the rebases manually in the future. Anyway, all of that is really incidental to the purpose of this PR which is:
There was nowhere to put the e2e, so I had to create that anyway. The ability to put all of those things in a single PR is just an attractive benefit of using OTE this way. |
|
/test unit |
|
/jira cherrypick OCPBUGS-84569 4.19 |
|
@mdbooth: This pull request references Jira Issue OCPBUGS-84569, which is valid. 3 validation(s) were run on this bug
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. |
|
@theobarberbany This version no longer removes your tests, just adds a new test, so we can use them as a regression indicator. The new test is a combination of the previous 2 new tests. I think it's easier to understand the complete test coverage this way, and the additional complexity in the setup code to support customisation of one of the fake instance groups was minimal. I have verified that the new test's coverage is strictly a superset of the old tests both in terms of coverage profile and branching logic. I'd recommend removing them, but we can do that in another PR. |
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@mdbooth: 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. |
|
@mdbooth sorry for not getting back sooner, I think we're missing a unit case for the bug scenario: TestFilterNodesWithExistingExternalInstanceGroups needs something to test the scenario where we have masters that would be included, but they've been pre filtered by the cloud provider library (by the label) So you have an IG containing testInfraName-master-0, and gceHostNamesInZone only has workers. HasAll fails, old code would've fallen through to allHaveNodePrefix -> true -> reuse. New code -> no reuse. The test case needs nodes with only workers/infra, so gceHostNamesInZone won't contain masters, but the IGs still will. I think the e2es that you have PRs up for cover it, but it's worth unit testing too? |
…balancers on masters We previously used allHaveNodePrefix() to ensure that if a master instance group contained the bootstrap machine this would not cause it to be excluded from re-use. This worked because the bootstrap machine's name has the same prefix as all other machines in the cluster. However, this logic unconditionally always included the master instance group. If the masters are labelled with node.kubernetes.io/exclude-from-external-load-balancers the CCM framework code will have filtered the Nodes before passing them to provider-GCP. But this logic meant we added them anyway, sending unintended traffic to the control plane machines.
openshift-ci-robot
removed
the
verified
Added. |
|
@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. |
|
/lgtm |
openshift-ci-robot
added
the
verified
|
@theobarberbany: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: theobarberbany The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@mdbooth: Jira Issue Verification Checks: Jira Issue OCPBUGS-84569 Jira Issue OCPBUGS-84569 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
@mdbooth: new pull request created: #129 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 kubernetes-sigs/prow repository. |
|
@mdbooth: new pull request created: #130 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 kubernetes-sigs/prow repository. |
|
Fix included in release 5.0.0-0.nightly-2026-07-21-023721 |
6 participants
We previously used allHaveNodePrefix() to ensure that if a master instance group contained the bootstrap machine this would not cause it to be excluded from re-use. This worked because the bootstrap machine's name has the same prefix as all other machines in the cluster.
However, this logic unconditionally always included the master instance group. If the masters are labelled with node.kubernetes.io/exclude-from-external-load-balancers the CCM framework code will have filtered the Nodes before passing them to provider-GCP. But this logic meant we added them anyway, sending unintended traffic to the control plane machines.
Summary by CodeRabbit