Merge https://github.com/kubernetes/cloud-provider-gcp:master (70f494f) into main

[cloud-team-rebase-bot]

This is an automated rebase PR generated by RebaseBot.

Summary

• Source: https://github.com/kubernetes/cloud-provider-gcp:master
• Destination: https://github.com/openshift/cloud-provider-gcp:main
• 13 new upstream commits

Dropped downstream commits

• c126af0 UPSTREAM: : Updating and vendoring go modules after an upstream rebase (dropped by tag policy)
• f51ba21 UPSTREAM: 1250: Support Universe Domain client options (Universe Domain Support kubernetes/cloud-provider-gcp#1250) (dropped by tag policy)

Logs

View job log

Summary by CodeRabbit

• New Features

  • Added optional strict validation for cached Google Cloud access tokens.
  • Added administrative commands to inspect CIDR blocks and IP address allocations in table or JSON format.
  • Improved IP address allocation handling, including IPv4/IPv6 support, retries, capacity management, and request queuing.
  • Expanded Google Cloud firewall rule support, including deny rules, IPv6, priorities, directions, and disabled rules.
  • Improved state-store setup reliability with retries and readiness checks.
  • Added IPv6-only cluster handling.

• Bug Fixes

  • Improved fallback behavior when cloud configuration cannot be read.

[coderabbitai]

Walkthrough

This pull request adds strict gcloud cache invalidation, extracts Metis IPAM logic into an engine, adds Metis administration APIs and CLI commands, expands GCE firewall mapping, improves kops retries, adds IPv6-only cluster handling, and updates workflow and module versions.

Changes

Strict gcloud cache invalidation

Layer / File(s)	Summary
Configuration-aware cache validation cmd/gke-gcloud-auth-plugin/cred.go, cmd/gke-gcloud-auth-plugin/cred_test.go	Strict mode records gcloud configuration metadata and refreshes cached tokens when the configuration hash changes. Tests cover hash changes, read failures, and active configuration selection.

Metis IPAM and administration

Layer / File(s)	Summary
IPAM engine extraction and wiring metis/pkg/daemon/engine.go, metis/pkg/daemon/server.go, metis/pkg/daemon/daemon.go, metis/pkg/daemon/server_test.go	IPAM allocation, release, checking, retry, dynamic capacity, and pending requests now use IPAMEngine.
Administrative inspection API and CLI metis/api/admin/v1/admin.proto, metis/pkg/store/admin.go, metis/pkg/daemon/admin.go, metis/pkg/grpc.go, metis/pkg/cni/plugin.go, metis/cmd/admin.go, metis/cmd/main.go	Hidden admin commands query filtered CIDR and IP tables through local gRPC and render table or JSON output.
Released IP results and diagnostics metis/pkg/store/store.go, metis/pkg/store/store_test.go, metis/pkg/daemon/monitor.go, metis/pkg/daemon/watcher.go	ReleaseIPByOwner returns released IP addresses. Selected diagnostic logs use verbosity level 4.

GCE, cluster, and kops behavior

Layer / File(s)	Summary
Firewall argument mapping providers/gce/gce_util.go, providers/gce/gce_util_test.go	Firewall commands support allow and deny rules, ranges, tags, IPv6, priority, direction, disabled state, and portless protocols.
State-store retries tools/kops/pkg/kops/gcp.go, tools/kops/pkg/kops/kops_test.go	State-store setup retries bucket operations, checks write readiness, and falls back to user-account IAM grants.
IPv6-only cluster handling cmd/cloud-controller-manager/gketenantcontrollermanager.go	IPv6-only clusters use a discard IPv6 CIDR for node IPAM. Other clusters retain ProviderConfig-based CIDR selection.

Workflow and module maintenance

Layer / File(s)	Summary
Pinned checkout revisions .github/workflows/*	Five workflows now use newer pinned actions/checkout revisions.
Go module updates go.mod, metis/go.mod, providers/go.mod, test/e2e/go.mod	Google API, Kubernetes, logging, networking, gRPC, and related dependency versions are updated.

Estimated code review effort: 5 (Critical) | ~120 minutes

Suggested labels: rebase/manual

Suggested reviewers: nrb, racheljpg

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning)

Check name	Status	Explanation	Resolution
No-Sensitive-Data-In-Logs	❌ Error	The new IPAM engine logs pod names, namespaces, container IDs, full IP configs, CIDRs, and released IP addresses at Info/V4 levels.	Remove sensitive request fields from logs. Log only operation status and sanitized counts or opaque identifiers; do not log IPs, CIDRs, container IDs, or full configs.
Docstring Coverage	⚠️ Warning	Docstring coverage is 17.54% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (13 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately identifies the pull request as an upstream merge from kubernetes/cloud-provider-gcp into main.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	The PR changes no Ginkgo test declarations; the repository's Ginkgo Describe and It titles are static string literals with no dynamic values.
Test Structure And Quality	✅ Passed	The PR changes no Ginkgo test code or Ginkgo lifecycle/wait calls; changed tests use Go's standard testing package, so this check is not applicable.
Microshift Test Compatibility	✅ Passed	No Ginkgo e2e Go files changed; only test/e2e/go.mod and go.sum changed, and all existing Ginkgo files are byte-identical to the base.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	The PR changes only test/e2e module metadata; it adds no e2e Go files or Ginkgo declarations, so no SNO compatibility issue applies.
Topology-Aware Scheduling Compatibility	✅ Passed	The diff adds no deployment manifests or scheduling constraints; the only modified controller changes IPv6-only CIDR handling and does not alter replicas, affinity, selectors, tolerations, topology...
Ote Binary Stdout Contract	✅ Passed	No OTE or openshift-tests integration exists; the e2e binary runs through kubetest2, and suite init/RunSpecs setup has no direct stdout writes.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	The PR adds no Ginkgo e2e tests: test/e2e changes only go.mod/go.sum, and changed *_test.go files contain standard Go tests with no Ginkgo markers or external connectivity.
No-Weak-Crypto	✅ Passed	The PR adds only standard SHA-256 config hashing; the exact additions contain no MD5, SHA-1, DES, RC4, Blowfish, or ECB usage and no non-constant-time token comparisons.
Container-Privileges	✅ Passed	PR changes no container/Kubernetes privilege settings; the only non-vendored hostNetwork: true entry is pre-existing and absent from the PR diff.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions

---

_{Comment @coderabbitai help to get the list of available commands.}

[openshift-ci]

Hi @cloud-team-rebase-bot[bot]. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign damdo for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

Actionable comments posted: 6

🧹 Nitpick comments (9)

cmd/gke-gcloud-auth-plugin/cred_test.go (5)

40-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Nit: extra_args line is tab-indented while the rest of the template uses 4 spaces.

-	"extra_args": "%s",
+    "extra_args": "%s",

🤖 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 `@cmd/gke-gcloud-auth-plugin/cred_test.go` around lines 40 - 46, Align the
extra_args line in the credential template with the surrounding JSON fields by
using the same 4-space indentation, without changing its formatting or content.

---

857-868: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Test builds the expected path with path.Join while production uses filepath.Join.

Equivalent on Linux/macOS but divergent on Windows — and gcloudConfigDir explicitly added a Windows branch, so these tests will fail there. Same applies to the fakeReadFile map keys.

♻️ Suggested change

-			if filename != path.Join(fakeCloudSDKConfig, "configurations", "config_overridden") {
+			if filename != filepath.Join(fakeCloudSDKConfig, "configurations", "config_overridden") {

🤖 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 `@cmd/gke-gcloud-auth-plugin/cred_test.go` around lines 857 - 868, Update
TestCurrentGcloudConfigHonorsActiveConfigEnvironmentVariable and the related
fakeReadFile map keys to construct expected configuration paths with
filepath.Join instead of path.Join, matching the platform-aware behavior in
gcloudConfigDir and keeping the tests portable on Windows.

---

168-214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Nit: interpolate fakeCloudSDKConfig / fakeConfigName instead of re-hardcoding them.

config_hash already uses interpolation; the dir and active-config values are duplicated literals that will silently drift if the constants change.

🤖 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 `@cmd/gke-gcloud-auth-plugin/cred_test.go` around lines 168 - 214, Update the
expected cache JSON strings in the test fixtures wantCacheFile,
wantCacheFileWithExtraArgs, wantCacheFileImpersonateServiceAccount, and
wantCacheFileWithAuthzToken to interpolate fakeCloudSDKConfig and fakeConfigName
for config_dir and active_config instead of hard-coded values, while preserving
the existing config_hash interpolation and other expected fields.

---

168-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

gcloud config dir and active-config name are re-hardcoded as literals instead of reusing the fixture constants. Both sites already have fakeCloudSDKConfig and fakeConfigName available, and config_hash is interpolated in the same blocks — so the schema is half-parameterized and will silently drift if either constant changes.

• cmd/gke-gcloud-auth-plugin/cred_test.go#L168-L214: replace the "/Users/username/.config/gcloud" and "default" literals in each wantCacheFile* with fakeCloudSDKConfig / fakeConfigName concatenation.
• cmd/gke-gcloud-auth-plugin/cred_test.go#L975-L975: build the map key as path.Join(fakeCloudSDKConfig, "configurations", "config_"+fakeConfigName) to match the neighboring activeConfig entry.

🤖 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 `@cmd/gke-gcloud-auth-plugin/cred_test.go` around lines 168 - 214, Replace the
hardcoded gcloud config directory and active configuration values in all
wantCacheFile* fixtures at cmd/gke-gcloud-auth-plugin/cred_test.go lines 168-214
with fakeCloudSDKConfig and fakeConfigName concatenation. Also update the map
key at cmd/gke-gcloud-auth-plugin/cred_test.go line 975 to use
path.Join(fakeCloudSDKConfig, "configurations", "config_"+fakeConfigName),
matching the neighboring activeConfig entry.

---

246-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Strict mode is now on for the entire TestExecCredential table, so the legacy (flag-off) cache path is no longer covered here.

Since GKE_AUTH_PLUGIN_STRICT_CACHE_INVALIDATION is the opt-in gate, the default-off behavior — cache written and accepted without gcloud_config — is the one most users get. Consider moving the t.Setenv into the strict subtests, or adding one flag-off case that asserts the legacy schema round-trips.

🤖 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 `@cmd/gke-gcloud-auth-plugin/cred_test.go` around lines 246 - 247, The
TestExecCredential table currently enables strict cache invalidation for every
case, leaving the default-off legacy cache behavior untested. Move the
strictCacheInvalidationEnvVar setting into only the strict-mode subtests, or add
a flag-off case that verifies cache entries without gcloud_config are written
and accepted.

cmd/gke-gcloud-auth-plugin/cred.go (2)

304-312: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Nit: local variable shadows the gcloudConfig type name.

Compiles, but makes the block harder to read and would break if the type is needed here later. Consider cfg.

♻️ Suggested rename

 	if strictCacheInvalidationEnabled() {
-		if gcloudConfig, err := p.currentGcloudConfig(); err != nil {
+		if cfg, err := p.currentGcloudConfig(); err != nil {
 			// Cache writes are best effort. Do not make authentication depend on
 			// reading gcloud's configuration files.
 			klog.V(4).Infof("Strict cache invalidation: unable to read gcloud configuration; using legacy cache behavior: %v", err)
-		} else if gcloudConfig != nil {
-			c.GcloudConfig = gcloudConfig
+		} else if cfg != nil {
+			c.GcloudConfig = cfg
 		}
 	}

🤖 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 `@cmd/gke-gcloud-auth-plugin/cred.go` around lines 304 - 312, Rename the local
variable declared in the strict cache invalidation block around
currentGcloudConfig from gcloudConfig to cfg, and update its references in the
nil check and c.GcloudConfig assignment; leave the gcloudConfig type and
surrounding behavior unchanged.

---

388-403: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Nit: hoist the configurations / config_ literals into constants.

activeConfig is already a named constant; keeping these siblings inline splits the gcloud layout knowledge across the file. Also consider strings.TrimSpace on the env-var value for symmetry with the file-read path.

🤖 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 `@cmd/gke-gcloud-auth-plugin/cred.go` around lines 388 - 403, Update the
configuration-loading flow around activeConfigName to introduce named constants
for the "configurations" directory and "config_" filename prefix, then use them
when building the active configuration path. Also trim whitespace from the
cloudsdkActiveConfigEnvVar value before validating or using it, matching the
existing file-read path.

metis/pkg/daemon/engine.go (1)

52-65: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

e.monitor is read without holding requestsMu.

SetMonitor (Lines 80-84) writes e.monitor under requestsMu.Lock(), but handleDynamicAllocation (Lines 221, 231) reads e.monitor without acquiring any lock. There's no live race today since daemon.go calls SetMonitor before the serving goroutine starts, but the asymmetric locking is fragile against future refactors (e.g., a future hot-reload of the monitor).

♻️ Proposed fix

 func (e *IPAMEngine) handleDynamicAllocation(ctx context.Context, req *adaptiveipam.AllocatePodIPRequest) error {
 	clientKey := cniClient{...}
 
-	if e.monitor == nil {
+	e.requestsMu.RLock()
+	monitor := e.monitor
+	e.requestsMu.RUnlock()
+	if monitor == nil {
 		e.logger.V(2).Info("No monitor available, failing fast on exhaustion", "network", req.Network)
 		return fmt.Errorf("failed to allocate ipv4 for pod %s/%s: %w", req.PodNamespace, req.PodName, store.ErrNoAvailableIPs)
 	}
 	...
-		e.monitor.enqueue()
+		monitor.enqueue()

Also applies to: 79-84, 213-245

🤖 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 `@metis/pkg/daemon/engine.go` around lines 52 - 65, Synchronize all reads of
e.monitor in handleDynamicAllocation with requestsMu, matching the lock used by
SetMonitor. Ensure the monitor is read while holding the mutex, or copied under
the mutex before use, so future monitor updates cannot race with allocation
handling.

providers/gce/gce_test.go (1)

503-510: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reflection-based test on unexported option.ClientOption type names is brittle.

optionTypeName inspects the unexported struct type names (withTokenSource, withUniverseDomain, withAuthCredentialsJSON) internal to google.golang.org/api/option. These names are implementation details of the vendored library, not part of its public contract; a future library upgrade could rename these types (or change how WithAuthCredentialsJSON/WithUniverseDomain are implemented internally) and silently break this test without any actual regression in clientOptions' behavior.

Also applies to: 532-573

🤖 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 `@providers/gce/gce_test.go` around lines 503 - 510, Remove the
reflection-based optionTypeName assertions and update the affected clientOptions
tests to validate observable option behavior instead of unexported google option
type names. Preserve coverage for token source, universe domain, and auth
credentials JSON configuration using public effects or supported option APIs,
without depending on internal type names.

🤖 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 `@metis/pkg/daemon/engine.go`:
- Around line 333-361: Update onCIDRAdded so it returns without closing or
deleting any waiting request when availableIPs is zero or less, while preserving
the existing wake-up limit and cleanup behavior for positive capacity.
- Around line 247-263: Update maybeDynamicAllocation to explicitly handle a
non-nil undrainErr returned by UndrainOneCIDRBlock: surface it through the
function’s error result instead of proceeding silently to
handleDynamicAllocation. Preserve the existing retry path when undraining
succeeds and the dynamic-allocation fallback when no block is undrained without
error.
- Around line 87-138: Update AllocatePodIP so that when IPv6 allocation fails
after a successful IPv4 allocation, it releases the IPv4 reservation before
returning the error. Reuse the existing release/deallocation mechanism and
preserve the current error return behavior; leave allocations unchanged when
both paths succeed.

In `@providers/gce/gce_util.go`:
- Around line 223-236: In the firewall argument-building logic, avoid mutating
the caller-owned slices on fw. For SourceRanges, DestinationRanges, and
TargetTags, copy each non-empty slice to a local value, sort the copy, and use
it for the corresponding CLI argument while leaving the original fw fields
unchanged.
- Around line 238-240: Update the priority handling in the firewall-rule
argument construction around fw.Priority so an explicitly configured value of 0
is preserved and emitted as --priority 0. Use the existing ForceSendFields or
another upstream explicit-set indicator to distinguish unset priority from
explicit zero, while retaining omission for values that were not configured.
- Around line 200-221: Update firewallToGcloudArgs to emit deny entries using
the gcloud-compatible --action DENY and --rules flags instead of --deny, while
preserving the existing sorted deny specification generation and handling of
allowed entries.

---

Nitpick comments:
In `@cmd/gke-gcloud-auth-plugin/cred_test.go`:
- Around line 40-46: Align the extra_args line in the credential template with
the surrounding JSON fields by using the same 4-space indentation, without
changing its formatting or content.
- Around line 857-868: Update
TestCurrentGcloudConfigHonorsActiveConfigEnvironmentVariable and the related
fakeReadFile map keys to construct expected configuration paths with
filepath.Join instead of path.Join, matching the platform-aware behavior in
gcloudConfigDir and keeping the tests portable on Windows.
- Around line 168-214: Update the expected cache JSON strings in the test
fixtures wantCacheFile, wantCacheFileWithExtraArgs,
wantCacheFileImpersonateServiceAccount, and wantCacheFileWithAuthzToken to
interpolate fakeCloudSDKConfig and fakeConfigName for config_dir and
active_config instead of hard-coded values, while preserving the existing
config_hash interpolation and other expected fields.
- Around line 168-214: Replace the hardcoded gcloud config directory and active
configuration values in all wantCacheFile* fixtures at
cmd/gke-gcloud-auth-plugin/cred_test.go lines 168-214 with fakeCloudSDKConfig
and fakeConfigName concatenation. Also update the map key at
cmd/gke-gcloud-auth-plugin/cred_test.go line 975 to use
path.Join(fakeCloudSDKConfig, "configurations", "config_"+fakeConfigName),
matching the neighboring activeConfig entry.
- Around line 246-247: The TestExecCredential table currently enables strict
cache invalidation for every case, leaving the default-off legacy cache behavior
untested. Move the strictCacheInvalidationEnvVar setting into only the
strict-mode subtests, or add a flag-off case that verifies cache entries without
gcloud_config are written and accepted.

In `@cmd/gke-gcloud-auth-plugin/cred.go`:
- Around line 304-312: Rename the local variable declared in the strict cache
invalidation block around currentGcloudConfig from gcloudConfig to cfg, and
update its references in the nil check and c.GcloudConfig assignment; leave the
gcloudConfig type and surrounding behavior unchanged.
- Around line 388-403: Update the configuration-loading flow around
activeConfigName to introduce named constants for the "configurations" directory
and "config_" filename prefix, then use them when building the active
configuration path. Also trim whitespace from the cloudsdkActiveConfigEnvVar
value before validating or using it, matching the existing file-read path.

In `@metis/pkg/daemon/engine.go`:
- Around line 52-65: Synchronize all reads of e.monitor in
handleDynamicAllocation with requestsMu, matching the lock used by SetMonitor.
Ensure the monitor is read while holding the mutex, or copied under the mutex
before use, so future monitor updates cannot race with allocation handling.

In `@providers/gce/gce_test.go`:
- Around line 503-510: Remove the reflection-based optionTypeName assertions and
update the affected clientOptions tests to validate observable option behavior
instead of unexported google option type names. Preserve coverage for token
source, universe domain, and auth credentials JSON configuration using public
effects or supported option APIs, without depending on internal type names.

🪄 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: f290b13c-196c-44a5-b281-ad8a417c3e09

📥 Commits

Reviewing files that changed from the base of the PR and between bcf8f50 and 27f6695.

⛔ Files ignored due to path filters (18)

• go.sum is excluded by !**/*.sum
• metis/go.sum is excluded by !**/*.sum
• providers/go.sum is excluded by !**/*.sum
• test/e2e/go.sum is excluded by !**/*.sum
• vendor/github.com/go-logr/logr/context_noslog.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/go-logr/logr/context_slog.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/go-logr/logr/funcr/funcr.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/go-logr/logr/funcr/slogsink.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/go-logr/logr/sloghandler.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/go-logr/logr/slogr.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/go-logr/logr/slogsink.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/compute/v0.beta/compute-api.json is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/compute/v0.beta/compute-gen.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/internal/version.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/networkservices/v1beta1/networkservices-api.json is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/networkservices/v1beta1/networkservices-gen.go is excluded by !**/vendor/**, !vendor/**
• vendor/modules.txt is excluded by !**/vendor/**, !vendor/**
• vendor/sigs.k8s.io/structured-merge-diff/v6/typed/remove.go is excluded by !**/vendor/**, !vendor/**

📒 Files selected for processing (22)

• .github/workflows/dependabot-sync.yml
• .github/workflows/release.yml
• .github/workflows/tag-auth-provider-gcp.yml
• .github/workflows/tag-ccm.yml
• .github/workflows/tag-gke-gcloud-auth-plugin.yml
• cmd/gke-gcloud-auth-plugin/cred.go
• cmd/gke-gcloud-auth-plugin/cred_test.go
• go.mod
• metis/go.mod
• metis/pkg/daemon/daemon.go
• metis/pkg/daemon/engine.go
• metis/pkg/daemon/monitor.go
• metis/pkg/daemon/server.go
• metis/pkg/daemon/server_test.go
• metis/pkg/store/store.go
• metis/pkg/store/store_test.go
• providers/gce/gce.go
• providers/gce/gce_test.go
• providers/gce/gce_util.go
• providers/gce/gce_util_test.go
• providers/go.mod
• test/e2e/go.mod

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf 'Repo root: '; pwd

# Locate relevant files
git ls-files 'metis/pkg/daemon/*' 'metis/pkg/**/server_test.go' 'metis/pkg/**/*store*' | sed -n '1,200p'

echo '--- engine.go outline ---'
ast-grep outline metis/pkg/daemon/engine.go --view expanded || true

echo '--- search for allocateIP / ReleaseIPByOwner / AllocatePodIP tests ---'
rg -n "func \(e \*IPAMEngine\) allocateIP|ReleaseIPByOwner|AllocatePodIP|idempotency" metis/pkg -S || true

Repository: openshift/cloud-provider-gcp

Length of output: 15284

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '1,260p' metis/pkg/daemon/engine.go
echo '---'
sed -n '1,260p' metis/pkg/daemon/server_test.go

Repository: openshift/cloud-provider-gcp

Length of output: 17130

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '=== engine.go (lines 1-260) ==='
nl -ba metis/pkg/daemon/engine.go | sed -n '1,260p'

echo '=== server_test.go (lines 1-260) ==='
nl -ba metis/pkg/daemon/server_test.go | sed -n '1,260p'

Repository: openshift/cloud-provider-gcp

Length of output: 238

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Try to locate the store implementation and relevant tests in a compact way.
git ls-files 'metis/pkg/**' | rg 'store|engine|server_test|daemon' || true

Repository: openshift/cloud-provider-gcp

Length of output: 599

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
from pathlib import Path
import re

engine = Path('metis/pkg/daemon/engine.go').read_text()
m = re.search(r'func \(e \*IPAMEngine\) AllocatePodIP[\s\S]*?\n}\n', engine)
print('=== AllocatePodIP block ===')
print(m.group(0) if m else 'not found')

print('=== ReleaseIPByOwner references in metis/pkg/daemon ===')
for p in Path('metis/pkg/daemon').glob('**/*.go'):
    text = p.read_text()
    if 'ReleaseIPByOwner' in text:
        for i, line in enumerate(text.splitlines(), 1):
            if 'ReleaseIPByOwner' in line:
                print(f'{p}:{i}:{line}')
PY

Repository: openshift/cloud-provider-gcp

Length of output: 2649

---

Roll back the IPv4 allocation on IPv6 failure

If IPv4 allocation succeeds and the IPv6 path fails, AllocatePodIP returns without releasing the IPv4 reservation. That leaves pool capacity tied up until a later DEL/retry, so partial dual-stack failures can leak addresses.

🤖 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 `@metis/pkg/daemon/engine.go` around lines 87 - 138, Update AllocatePodIP so
that when IPv6 allocation fails after a successful IPv4 allocation, it releases
the IPv4 reservation before returning the error. Reuse the existing
release/deallocation mechanism and preserve the current error return behavior;
leave allocations unchanged when both paths succeed.

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

undrainErr from UndrainOneCIDRBlock is silently discarded.

If undrainErr != nil, the code falls straight into handleDynamicAllocation without logging or surfacing the error at all — a real store failure (e.g., DB error) looks identical to "no undrainable block found." This hides diagnosable failures behind the scale-up fallback path.

As per path instructions, **/*.go files must "Never ignore error returns."

🐛 Proposed fix

 	undrained, undrainErr := e.store.UndrainOneCIDRBlock(ctx, req.Network, store.IPv4)
+	if undrainErr != nil {
+		e.logger.Error(undrainErr, "failed to undrain CIDR block", "network", req.Network, "podName", req.PodName, "podNamespace", req.PodNamespace)
+	}
 	if undrainErr == nil && undrained {

📝 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 (e *IPAMEngine) maybeDynamicAllocation(ctx context.Context, req *adaptiveipam.AllocatePodIPRequest, params store.AllocateIPParams, err error) (bool, error) {
	if params.IPFamily != store.IPv4 || !errors.Is(err, store.ErrNoAvailableIPs) {
	return false, nil
	}
	undrained, undrainErr := e.store.UndrainOneCIDRBlock(ctx, req.Network, store.IPv4)
	if undrainErr == nil && undrained {
	e.logger.Info("Successfully undrained one CIDR block, retrying local allocation", "network", req.Network, "podName", req.PodName, "podNamespace", req.PodNamespace)
	return true, nil
	}
	if err := e.handleDynamicAllocation(ctx, req); err != nil {
	return false, status.Errorf(codes.ResourceExhausted, "failed to allocate ipv4 for pod %s/%s: %v", req.PodNamespace, req.PodName, err)
	}
	return true, nil
	}
	func (e *IPAMEngine) maybeDynamicAllocation(ctx context.Context, req *adaptiveipam.AllocatePodIPRequest, params store.AllocateIPParams, err error) (bool, error) {
	if params.IPFamily != store.IPv4 || !errors.Is(err, store.ErrNoAvailableIPs) {
	return false, nil
	}
	undrained, undrainErr := e.store.UndrainOneCIDRBlock(ctx, req.Network, store.IPv4)
	if undrainErr != nil {
	e.logger.Error(undrainErr, "failed to undrain CIDR block", "network", req.Network, "podName", req.PodName, "podNamespace", req.PodNamespace)
	}
	if undrainErr == nil && undrained {
	e.logger.Info("Successfully undrained one CIDR block, retrying local allocation", "network", req.Network, "podName", req.PodName, "podNamespace", req.PodNamespace)
	return true, nil
	}
	if err := e.handleDynamicAllocation(ctx, req); err != nil {
	return false, status.Errorf(codes.ResourceExhausted, "failed to allocate ipv4 for pod %s/%s: %v", req.PodNamespace, req.PodName, err)
	}
	return true, 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 `@metis/pkg/daemon/engine.go` around lines 247 - 263, Update
maybeDynamicAllocation to explicitly handle a non-nil undrainErr returned by
UndrainOneCIDRBlock: surface it through the function’s error result instead of
proceeding silently to handleDynamicAllocation. Preserve the existing retry path
when undraining succeeds and the dynamic-allocation fallback when no block is
undrained without error.

Source: Path instructions

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

onCIDRAdded wakes a client even when availableIPs is 0.

The loop closes/deletes an entry and increments count before checking count >= availableIPs, so with availableIPs == 0 it still wakes exactly one waiter (retry cycle for capacity that doesn't exist).

🐛 Proposed fix

 	count := 0
+	if availableIPs <= 0 {
+		return
+	}
 	for client, ch := range netMap {
 		close(ch)
 		delete(netMap, client)
 		count++
 		if count >= availableIPs {
 			break
 		}
 	}

📝 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 (e *IPAMEngine) onCIDRAdded(network string, availableIPs int) {
	e.requestsMu.Lock()
	defer e.requestsMu.Unlock()
	e.logger.Info("CIDR added, checking for waiting CNI requests to wake up", "network", network, "availableIPs", availableIPs)
	netMap := e.requestsMap[network]
	if len(netMap) == 0 {
	return
	}
	count := 0
	for client, ch := range netMap {
	close(ch)
	delete(netMap, client)
	count++
	if count >= availableIPs {
	break
	}
	}
	if len(netMap) == 0 {
	delete(e.requestsMap, network)
	}
	if count > 0 {
	e.logger.Info("Successfully woke up waiting CNI requests", "network", network, "count", count)
	}
	}
	func (e *IPAMEngine) onCIDRAdded(network string, availableIPs int) {
	e.requestsMu.Lock()
	defer e.requestsMu.Unlock()
	e.logger.Info("CIDR added, checking for waiting CNI requests to wake up", "network", network, "availableIPs", availableIPs)
	netMap := e.requestsMap[network]
	if len(netMap) == 0 {
	return
	}
	count := 0
	if availableIPs <= 0 {
	return
	}
	for client, ch := range netMap {
	close(ch)
	delete(netMap, client)
	count++
	if count >= availableIPs {
	break
	}
	}
	if len(netMap) == 0 {
	delete(e.requestsMap, network)
	}
	if count > 0 {
	e.logger.Info("Successfully woke up waiting CNI requests", "network", network, "count", count)
	}
	}

🤖 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 `@metis/pkg/daemon/engine.go` around lines 333 - 361, Update onCIDRAdded so it
returns without closing or deleting any waiting request when availableIPs is
zero or less, while preserving the existing wake-up limit and cleanup behavior
for positive capacity.

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

Does gcloud compute firewall-rules createsupport a--denyflag, or only--action=DENY --rules=...?

💡 Result:

The gcloud compute firewall-rules create command does not support a standalone --deny flag [1][2]. Instead, to create a rule that denies traffic, you must use the --action flag set to deny in combination with the --rules flag [1][2]. The command structure requires you to specify the action (either allow or deny) and the associated protocols and ports [1][2]. For example: gcloud compute firewall-rules create MY-RULE --action deny --rules tcp:80,icmp [1] In this usage, the --action flag explicitly defines the behavior, and --rules defines the traffic to which that action applies [1][2]. Attempting to use a non-existent --deny flag will result in an error [1].

Citations:

• 1: https://cloud.google.com/sdk/gcloud/reference/compute/firewall-rules/create
• 2: https://docs.cloud.google.com/firewall/docs/using-firewalls

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant helper and nearby command construction.
ast-grep outline providers/gce/gce_util.go --view expanded || true
echo '--- FILE SNIPPET ---'
sed -n '1,320p' providers/gce/gce_util.go

echo '--- TEST SEARCH ---'
rg -n --context 2 'firewallToGcloudArgs|--deny|--action|--rules|compute firewall-rules' providers -g '!**/vendor/**'

Repository: openshift/cloud-provider-gcp

Length of output: 22752

---

🌐 Web query:

gcloud compute firewall-rules create update --deny flag documentation --action DENY --rules

💡 Result:

The gcloud compute firewall-rules create command requires either the --action flag or the specific --allow flag to define the rule's behavior [1][2]. When using the --action flag to create a DENY rule, you must also specify the --rules flag [1][2][3]. The correct syntax for a DENY rule is: gcloud compute firewall-rules create [RULE_NAME] --action DENY --rules [PROTOCOL:PORTS] In this command: - [RULE_NAME]: The name you choose for the firewall rule [1]. - --action DENY: Specifies that matching traffic should be denied [1][3]. - --rules [PROTOCOL:PORTS]: Defines the protocols and optional ports to which the rule applies (e.g., tcp:80, udp:53, or all) [1][3]. Example usage: gcloud compute firewall-rules create MY-DENY-RULE --action DENY --rules tcp:80,icmp [1][2] Key Considerations: - You must specify either --action (with --rules) or --allow (which implies an ALLOW action) [1][2]. - DENY rules take precedence over ALLOW rules that have the same priority [1][4]. - The default priority for firewall rules is 1000 [1][4]. You can adjust this using the --priority flag [3][4].

Citations:

• 1: https://cloud.google.com/sdk/gcloud/reference/compute/firewall-rules/create
• 2: https://docs.cloud.google.com/sdk/gcloud/reference/compute/firewall-rules/create
• 3: https://docs.cloud.google.com/firewall/docs/using-firewalls
• 4: https://cloud.google.com/compute/docs/reference/rest/v1/firewalls/insert

---

Emit --action DENY --rules for deny firewalls
gcloud compute firewall-rules doesn’t accept --deny; deny rules use --action DENY --rules ..., so any firewall with Denied entries will generate an invalid command here.

🤖 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 `@providers/gce/gce_util.go` around lines 200 - 221, Update
firewallToGcloudArgs to emit deny entries using the gcloud-compatible --action
DENY and --rules flags instead of --deny, while preserving the existing sorted
deny specification generation and handling of allowed entries.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

sort.Strings mutates the caller's *compute.Firewall slices in place.

sort.Strings(fw.SourceRanges), sort.Strings(fw.DestinationRanges), and sort.Strings(fw.TargetTags) reorder the backing arrays of the fields on the passed-in fw pointer as a side effect of merely generating a display/CLI string. If fw is reused afterward (e.g. passed to the actual GCE API call, logged elsewhere, or read concurrently), callers get a silently reordered slice they didn't ask for.

🔧 Proposed fix: sort local copies instead of the caller's slices

 	if len(fw.SourceRanges) > 0 {
-		sort.Strings(fw.SourceRanges)
-		args = append(args, fmt.Sprintf("--source-ranges %v", strings.Join(fw.SourceRanges, ",")))
+		sourceRanges := append([]string(nil), fw.SourceRanges...)
+		sort.Strings(sourceRanges)
+		args = append(args, fmt.Sprintf("--source-ranges %v", strings.Join(sourceRanges, ",")))
 	}
 
 	if len(fw.DestinationRanges) > 0 {
-		sort.Strings(fw.DestinationRanges)
-		args = append(args, fmt.Sprintf("--destination-ranges %v", strings.Join(fw.DestinationRanges, ",")))
+		destinationRanges := append([]string(nil), fw.DestinationRanges...)
+		sort.Strings(destinationRanges)
+		args = append(args, fmt.Sprintf("--destination-ranges %v", strings.Join(destinationRanges, ",")))
 	}
 
 	if len(fw.TargetTags) > 0 {
-		sort.Strings(fw.TargetTags)
-		args = append(args, fmt.Sprintf("--target-tags %v", strings.Join(fw.TargetTags, ",")))
+		targetTags := append([]string(nil), fw.TargetTags...)
+		sort.Strings(targetTags)
+		args = append(args, fmt.Sprintf("--target-tags %v", strings.Join(targetTags, ",")))
 	}

📝 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

	if len(fw.SourceRanges) > 0 {
	sort.Strings(fw.SourceRanges)
	args = append(args, fmt.Sprintf("--source-ranges %v", strings.Join(fw.SourceRanges, ",")))
	}
	// Sort all slices to prevent the event from being duped
	sort.Strings(allPorts)
	allow := strings.Join(allPorts, ",")
	sort.Strings(fw.SourceRanges)
	srcRngs := strings.Join(fw.SourceRanges, ",")
	sort.Strings(fw.TargetTags)
	targets := strings.Join(fw.TargetTags, ",")
	return fmt.Sprintf("--description %q --allow %v --source-ranges %v --target-tags %v --project %v", fw.Description, allow, srcRngs, targets, projectID)
	if len(fw.DestinationRanges) > 0 {
	sort.Strings(fw.DestinationRanges)
	args = append(args, fmt.Sprintf("--destination-ranges %v", strings.Join(fw.DestinationRanges, ",")))
	}
	if len(fw.TargetTags) > 0 {
	sort.Strings(fw.TargetTags)
	args = append(args, fmt.Sprintf("--target-tags %v", strings.Join(fw.TargetTags, ",")))
	}
	if len(fw.SourceRanges) > 0 {
	sourceRanges := append([]string(nil), fw.SourceRanges...)
	sort.Strings(sourceRanges)
	args = append(args, fmt.Sprintf("--source-ranges %v", strings.Join(sourceRanges, ",")))
	}
	if len(fw.DestinationRanges) > 0 {
	destinationRanges := append([]string(nil), fw.DestinationRanges...)
	sort.Strings(destinationRanges)
	args = append(args, fmt.Sprintf("--destination-ranges %v", strings.Join(destinationRanges, ",")))
	}
	if len(fw.TargetTags) > 0 {
	targetTags := append([]string(nil), fw.TargetTags...)
	sort.Strings(targetTags)
	args = append(args, fmt.Sprintf("--target-tags %v", strings.Join(targetTags, ",")))
	}

🤖 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 `@providers/gce/gce_util.go` around lines 223 - 236, In the firewall
argument-building logic, avoid mutating the caller-owned slices on fw. For
SourceRanges, DestinationRanges, and TargetTags, copy each non-empty slice to a
local value, sort the copy, and use it for the corresponding CLI argument while
leaving the original fw fields unchanged.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

fw.Priority != 0 treats a legitimate priority of 0 as "unset".

GCP firewall rule priority is a valid 0–65535 range where 0 is the highest priority (default is 1000). A caller that explicitly wants priority 0 will have the --priority flag silently omitted here, so gcloud falls back to its own default (1000) instead of the intended value.

🔧 Proposed fix: use ForceSendFields (or a pointer/explicit-set flag upstream) to distinguish "unset" from "explicitly zero"

-	if fw.Priority != 0 {
+	if fw.Priority != 0 || slices.Contains(fw.ForceSendFields, "Priority") {
 		args = append(args, fmt.Sprintf("--priority %v", fw.Priority))
 	}

📝 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

	if fw.Priority != 0 {
	args = append(args, fmt.Sprintf("--priority %v", fw.Priority))
	}
	if fw.Priority != 0 || slices.Contains(fw.ForceSendFields, "Priority") {
	args = append(args, fmt.Sprintf("--priority %v", fw.Priority))
	}

🤖 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 `@providers/gce/gce_util.go` around lines 238 - 240, Update the priority
handling in the firewall-rule argument construction around fw.Priority so an
explicitly configured value of 0 is preserved and emitted as --priority 0. Use
the existing ForceSendFields or another upstream explicit-set indicator to
distinguish unset priority from explicit zero, while retaining omission for
values that were not configured.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Actionable comments posted: 7

🧹 Nitpick comments (2)

metis/pkg/daemon/admin.go (1)

10-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Wrap store errors in a gRPC status.

Both handlers return the raw store error. gRPC maps it to codes.Unknown, and the message forwards the SQLite error text, which exposes schema and query details to the client. The engine handlers in metis/pkg/daemon/engine.go wrap store failures with status.Errorf. Follow the same pattern here.

♻️ Proposed change

 func (s *adaptiveIpamServer) ListCIDRBlocks(ctx context.Context, req *adminv1.ListCIDRBlocksRequest) (*adminv1.AdminTableDumpResponse, error) {
 	headers, results, err := s.store.AdminListCIDRBlocks(ctx, req.Filter)
 	if err != nil {
-		return nil, err
+		s.logger.Error(err, "failed to list cidr blocks")
+		return nil, status.Errorf(codes.Internal, "failed to list cidr blocks")
 	}
 	return buildAdminTableDumpResponse(headers, results), nil
 }

Apply the same change to ListIPAddresses and add the google.golang.org/grpc/codes and google.golang.org/grpc/status imports.

🤖 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 `@metis/pkg/daemon/admin.go` around lines 10 - 25, Wrap store failures in both
adaptiveIpamServer.ListCIDRBlocks and ListIPAddresses with a gRPC status error
using the appropriate non-Unknown code and a safe client-facing message,
matching the status.Errorf pattern used by engine handlers. Add the required
grpc codes and status imports, while preserving successful response
construction.

metis/pkg/store/admin.go (1)

40-81: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Add a row limit to the admin table dump.

adminQueryTable accumulates every matching row into results, and metis/pkg/daemon/admin.go then copies all of them into a single gRPC response message. The ip_addresses table holds one record per address in every CIDR block, so an unfiltered dump on a node with several /24 blocks produces thousands of rows in one message. Large results can exceed the default 4 MiB gRPC receive limit on the client and cause a failed call instead of a truncated table.

Add a bounded LIMIT with an optional page or limit parameter.

🤖 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 `@metis/pkg/store/admin.go` around lines 40 - 81, Add bounded pagination to
adminQueryTable by introducing an optional limit/page parameter, applying the
limit in the table query before accumulating results, and preserving the
existing row conversion and error handling. Ensure callers such as the admin
gRPC handler pass or enforce the bound so large tables return a
truncated/page-sized response instead of exceeding message limits.

🤖 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 `@metis/api/admin/v1/admin.proto`:
- Around line 3-5: Add a Buf module configuration rooted at metis/api so the
admin.v1 package is recognized under that module when Buf runs from the
repository root. Ensure the module declaration covers metis/api/admin/v1 and
uses the repository’s existing Buf configuration conventions.

In `@metis/cmd/admin.go`:
- Around line 97-119: Update printDumpResponse to return an error, validate
outputFormat so only "table" and the supported JSON format are accepted, and
return an error for unknown formats. Capture and propagate json.MarshalIndent
errors instead of ignoring them, then update executeAdminListCommand to handle
the returned error by reporting it to stderr and exiting nonzero.
- Around line 82-95: Update executeAdminListCommand to create a cancellable
timeout context using the adminRPCTimeout constant (30 seconds) instead of
context.Background(), defer its cancellation, and pass it to queryFunc so the
administrative RPC cannot block indefinitely.

In `@metis/pkg/daemon/engine.go`:
- Around line 80-84: Protect all reads of IPAMEngine.monitor with requestsMu by
adding a small getMonitor accessor alongside SetMonitor, then update both
monitor accesses in handleDynamicAllocation to use the accessor and retain the
existing nil-check and enqueue behavior.

In `@metis/pkg/store/admin.go`:
- Around line 23-33: Replace the raw SQL filter flow in metis/pkg/store/admin.go
lines 23-33 within Store.adminQueryTable with validated allow-listed column
handling and database/sql bound placeholders; reject unknown columns and remove
fmt.Sprintf-based query construction. Update metis/api/admin/v1/admin.proto
lines 15-25 for ListCIDRBlocksRequest and ListIPAddressesRequest to replace
string filter with structured column/value criteria and revise comments so they
describe structured filters rather than SQL text.

In `@tools/kops/pkg/kops/gcp.go`:
- Around line 117-121: Handle the error returned by rmCmd.Run() in the probe
cleanup path before reporting readiness; if deletion fails, return a wrapped
cleanup error or otherwise explicitly record and handle it, and do not claim the
state store is ready while the probe remains. Keep the successful cleanup path
returning nil in the surrounding readiness check.
- Around line 89-100: Update runWithRetry and its callers to accept and
propagate context.Context, execute each command with exec.CommandContext, and
enforce a timeout for every retry attempt. Apply equivalent per-command
deadlines to the initial gsutil ls operation and readiness probes used by
EnsureStateStore, preserving retry behavior while ensuring hung processes are
cancelled.

---

Nitpick comments:
In `@metis/pkg/daemon/admin.go`:
- Around line 10-25: Wrap store failures in both
adaptiveIpamServer.ListCIDRBlocks and ListIPAddresses with a gRPC status error
using the appropriate non-Unknown code and a safe client-facing message,
matching the status.Errorf pattern used by engine handlers. Add the required
grpc codes and status imports, while preserving successful response
construction.

In `@metis/pkg/store/admin.go`:
- Around line 40-81: Add bounded pagination to adminQueryTable by introducing an
optional limit/page parameter, applying the limit in the table query before
accumulating results, and preserving the existing row conversion and error
handling. Ensure callers such as the admin gRPC handler pass or enforce the
bound so large tables return a truncated/page-sized response instead of
exceeding message limits.

🪄 Autofix

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: dd5baafb-ec7d-4ecd-bf8c-ec95cecdbf94

📥 Commits

Reviewing files that changed from the base of the PR and between 51c3264 and 6261bcb.

⛔ Files ignored due to path filters (20)

• go.sum is excluded by !**/*.sum
• metis/api/admin/v1/admin.pb.go is excluded by !**/*.pb.go
• metis/api/admin/v1/admin_grpc.pb.go is excluded by !**/*.pb.go
• metis/go.sum is excluded by !**/*.sum
• providers/go.sum is excluded by !**/*.sum
• test/e2e/go.sum is excluded by !**/*.sum
• vendor/github.com/go-logr/logr/context_noslog.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/go-logr/logr/context_slog.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/go-logr/logr/funcr/funcr.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/go-logr/logr/funcr/slogsink.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/go-logr/logr/sloghandler.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/go-logr/logr/slogr.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/go-logr/logr/slogsink.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/compute/v0.beta/compute-api.json is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/compute/v0.beta/compute-gen.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/internal/version.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/networkservices/v1beta1/networkservices-api.json is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/networkservices/v1beta1/networkservices-gen.go is excluded by !**/vendor/**, !vendor/**
• vendor/modules.txt is excluded by !**/vendor/**, !vendor/**
• vendor/sigs.k8s.io/structured-merge-diff/v6/typed/remove.go is excluded by !**/vendor/**, !vendor/**

📒 Files selected for processing (29)

• .github/workflows/dependabot-sync.yml
• .github/workflows/release.yml
• .github/workflows/tag-auth-provider-gcp.yml
• .github/workflows/tag-ccm.yml
• .github/workflows/tag-gke-gcloud-auth-plugin.yml
• cmd/gke-gcloud-auth-plugin/cred.go
• cmd/gke-gcloud-auth-plugin/cred_test.go
• go.mod
• metis/api/admin/v1/admin.proto
• metis/cmd/admin.go
• metis/cmd/main.go
• metis/go.mod
• metis/pkg/cni/plugin.go
• metis/pkg/daemon/admin.go
• metis/pkg/daemon/daemon.go
• metis/pkg/daemon/engine.go
• metis/pkg/daemon/monitor.go
• metis/pkg/daemon/server.go
• metis/pkg/daemon/server_test.go
• metis/pkg/grpc.go
• metis/pkg/store/admin.go
• metis/pkg/store/store.go
• metis/pkg/store/store_test.go
• providers/gce/gce_util.go
• providers/gce/gce_util_test.go
• providers/go.mod
• test/e2e/go.mod
• tools/kops/pkg/kops/gcp.go
• tools/kops/pkg/kops/kops_test.go

🚧 Files skipped from review as they are similar to previous changes (16)

• .github/workflows/dependabot-sync.yml
• .github/workflows/release.yml
• metis/pkg/store/store_test.go
• .github/workflows/tag-auth-provider-gcp.yml
• metis/pkg/daemon/daemon.go
• .github/workflows/tag-gke-gcloud-auth-plugin.yml
• providers/go.mod
• metis/go.mod
• test/e2e/go.mod
• metis/pkg/daemon/monitor.go
• .github/workflows/tag-ccm.yml
• go.mod
• providers/gce/gce_util.go
• metis/pkg/daemon/server_test.go
• cmd/gke-gcloud-auth-plugin/cred.go
• metis/pkg/store/store.go

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Locate buf configuration files and inspect module roots.
fd -H -t f 'buf.yaml|buf.work.yaml|buf.gen.yaml|buf.work.yml|buf.yml' --exec sh -c 'echo "=== $1 ==="; cat "$1"' _ {}

# Check how the sibling adaptiveipam proto declares its package.
fd -t f -e proto . metis/api --exec sh -c 'echo "=== $1 ==="; rg -n "^package|go_package" "$1"' _ {}

Repository: openshift/cloud-provider-gcp

Length of output: 693

---

🏁 Script executed:

#!/bin/bash
set -eu

echo '=== candidate Buf files ==='
git ls-files | grep -E '(^|/)(buf(\.work)?\.(yaml|yml)|buf\.gen\.(yaml|yml))$' || true

echo '=== Buf references ==='
rg -n --hidden -S 'buf (lint|build|generate|format)|buf\.yaml|buf\.work|PACKAGE_DIRECTORY_MATCH' \
  -g '!vendor/**' -g '!node_modules/**' . || true

echo '=== metis/api tree ==='
git ls-files metis/api

echo '=== repository root entries ==='
git ls-files | awk -F/ 'NF == 1 {print}' | sort

Repository: openshift/cloud-provider-gcp

Length of output: 906

---

Add a Buf module rooted at metis/api.
No Buf module is declared in this repository. If CI runs Buf from the repository root, PACKAGE_DIRECTORY_MATCH will reject admin.v1 in metis/api/admin/v1.

🧰 Tools

🪛 Buf (1.72.0)

[error] 3-3: Files with package "admin.v1" must be within a directory "admin/v1" relative to root but were in directory "metis/api/admin/v1".

(PACKAGE_DIRECTORY_MATCH)

🤖 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 `@metis/api/admin/v1/admin.proto` around lines 3 - 5, Add a Buf module
configuration rooted at metis/api so the admin.v1 package is recognized under
that module when Buf runs from the repository root. Ensure the module
declaration covers metis/api/admin/v1 and uses the repository’s existing Buf
configuration conventions.

Source: Linters/SAST tools

[coderabbitai]

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Add a timeout to the admin RPC context.

Line 89 passes context.Background() to queryFunc. The call has no deadline. If the daemon accepts the connection but never answers, the CLI blocks forever with no way to recover except a signal.

As per path instructions, **/*.go files must use "context.Context for cancellation and timeouts".

🐛 Proposed fix

 	defer conn.Close()
-	res, err := queryFunc(context.Background(), client)
+	ctx, cancel := context.WithTimeout(context.Background(), adminRPCTimeout)
+	defer cancel()
+	res, err := queryFunc(ctx, client)
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "failed to query: %v\n", err)
 		os.Exit(1)
 	}

Declare the constant near the top of the file:

// adminRPCTimeout bounds a single administrative query against the daemon.
const adminRPCTimeout = 30 * time.Second

📝 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 executeAdminListCommand(outputFormat string, queryFunc func(context.Context, adminv1.AdminClient) (*adminv1.AdminTableDumpResponse, error)) {
	client, conn, err := getAdminClient()
	if err != nil {
	fmt.Fprintf(os.Stderr, "failed to connect: %v\n", err)
	os.Exit(1)
	}
	defer conn.Close()
	res, err := queryFunc(context.Background(), client)
	if err != nil {
	fmt.Fprintf(os.Stderr, "failed to query: %v\n", err)
	os.Exit(1)
	}
	printDumpResponse(res, outputFormat)
	}
	// adminRPCTimeout bounds a single administrative query against the daemon.
	const adminRPCTimeout = 30 * time.Second
	func executeAdminListCommand(outputFormat string, queryFunc func(context.Context, adminv1.AdminClient) (*adminv1.AdminTableDumpResponse, error)) {
	client, conn, err := getAdminClient()
	if err != nil {
	fmt.Fprintf(os.Stderr, "failed to connect: %v\n", err)
	os.Exit(1)
	}
	defer conn.Close()
	ctx, cancel := context.WithTimeout(context.Background(), adminRPCTimeout)
	defer cancel()
	res, err := queryFunc(ctx, client)
	if err != nil {
	fmt.Fprintf(os.Stderr, "failed to query: %v\n", err)
	os.Exit(1)
	}
	printDumpResponse(res, outputFormat)
	}

🤖 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 `@metis/cmd/admin.go` around lines 82 - 95, Update executeAdminListCommand to
create a cancellable timeout context using the adminRPCTimeout constant (30
seconds) instead of context.Background(), defer its cancellation, and pass it to
queryFunc so the administrative RPC cannot block indefinitely.

Source: Path instructions

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Handle the marshal error and reject unknown output formats.

Line 116 discards the json.MarshalIndent error. If marshalling fails, the command prints null and exits 0, so a script cannot detect the failure.

Line 98 also treats every value other than table as JSON. A typed value such as -o yaml silently produces JSON instead of an error.

As per path instructions, **/*.go files must "Never ignore error returns".

🐛 Proposed fix

-func printDumpResponse(res *adminv1.AdminTableDumpResponse, outputFormat string) {
-	if outputFormat == "table" {
+func printDumpResponse(res *adminv1.AdminTableDumpResponse, outputFormat string) error {
+	switch outputFormat {
+	case "table":
 		w := tabwriter.NewWriter(os.Stdout, 0, 0, 2, ' ', 0)
 		// Print Headers
 		fmt.Fprintln(w, strings.ToUpper(strings.Join(res.Headers, "\t")))
 		// Print Rows
 		for _, row := range res.Rows {
 			fmt.Fprintln(w, strings.Join(row.Values, "\t"))
 		}
-		w.Flush()
-	} else {
+		return w.Flush()
+	case "json":
 		var jsonPayload []map[string]interface{}
 		for _, row := range res.Rows {
 			rowMap := make(map[string]interface{})
 			for i, header := range res.Headers {
 				rowMap[header] = row.Values[i]
 			}
 			jsonPayload = append(jsonPayload, rowMap)
 		}
-		b, _ := json.MarshalIndent(jsonPayload, "", "  ")
+		b, err := json.MarshalIndent(jsonPayload, "", "  ")
+		if err != nil {
+			return fmt.Errorf("failed to marshal output: %w", err)
+		}
 		fmt.Println(string(b))
+		return nil
+	default:
+		return fmt.Errorf("unsupported output format %q, want \"table\" or \"json\"", outputFormat)
 	}
 }

Then surface the error in executeAdminListCommand:

if err := printDumpResponse(res, outputFormat); err != nil {
	fmt.Fprintf(os.Stderr, "failed to print output: %v\n", err)
	os.Exit(1)
}

📝 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 printDumpResponse(res *adminv1.AdminTableDumpResponse, outputFormat string) {
	if outputFormat == "table" {
	w := tabwriter.NewWriter(os.Stdout, 0, 0, 2, ' ', 0)
	// Print Headers
	fmt.Fprintln(w, strings.ToUpper(strings.Join(res.Headers, "\t")))
	// Print Rows
	for _, row := range res.Rows {
	fmt.Fprintln(w, strings.Join(row.Values, "\t"))
	}
	w.Flush()
	} else {
	var jsonPayload []map[string]interface{}
	for _, row := range res.Rows {
	rowMap := make(map[string]interface{})
	for i, header := range res.Headers {
	rowMap[header] = row.Values[i]
	}
	jsonPayload = append(jsonPayload, rowMap)
	}
	b, _ := json.MarshalIndent(jsonPayload, "", " ")
	fmt.Println(string(b))
	}
	}
	func printDumpResponse(res *adminv1.AdminTableDumpResponse, outputFormat string) error {
	switch outputFormat {
	case "table":
	w := tabwriter.NewWriter(os.Stdout, 0, 0, 2, ' ', 0)
	// Print Headers
	fmt.Fprintln(w, strings.ToUpper(strings.Join(res.Headers, "\t")))
	// Print Rows
	for _, row := range res.Rows {
	fmt.Fprintln(w, strings.Join(row.Values, "\t"))
	}
	return w.Flush()
	case "json":
	var jsonPayload []map[string]interface{}
	for _, row := range res.Rows {
	rowMap := make(map[string]interface{})
	for i, header := range res.Headers {
	rowMap[header] = row.Values[i]
	}
	jsonPayload = append(jsonPayload, rowMap)
	}
	b, err := json.MarshalIndent(jsonPayload, "", " ")
	if err != nil {
	return fmt.Errorf("failed to marshal output: %w", err)
	}
	fmt.Println(string(b))
	return nil
	default:
	return fmt.Errorf("unsupported output format %q, want \"table\" or \"json\"", outputFormat)
	}
	}

🤖 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 `@metis/cmd/admin.go` around lines 97 - 119, Update printDumpResponse to return
an error, validate outputFormat so only "table" and the supported JSON format
are accepted, and return an error for unknown formats. Capture and propagate
json.MarshalIndent errors instead of ignoring them, then update
executeAdminListCommand to handle the returned error by reporting it to stderr
and exiting nonzero.

Source: Path instructions

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Read e.monitor under the same lock that SetMonitor uses.

SetMonitor writes e.monitor while holding requestsMu. handleDynamicAllocation reads e.monitor at Line 221 and Line 231 without holding any lock. In metis/pkg/daemon/daemon.go, server.engine.SetMonitor(monitorInstance) runs on the daemon goroutine, and the gRPC server serves requests concurrently after server.start(). That makes the write and the read a data race.

Add a small accessor and use it in handleDynamicAllocation.

🔒️ Proposed fix

 // SetMonitor updates the monitor associated with the IPAMEngine.
 func (e *IPAMEngine) SetMonitor(m *Monitor) {
 	e.requestsMu.Lock()
 	defer e.requestsMu.Unlock()
 	e.monitor = m
 }
+
+// getMonitor returns the monitor associated with the IPAMEngine.
+func (e *IPAMEngine) getMonitor() *Monitor {
+	e.requestsMu.RLock()
+	defer e.requestsMu.RUnlock()
+	return e.monitor
+}

Then use the accessor in handleDynamicAllocation:

monitor := e.getMonitor()
if monitor == nil {
	e.logger.V(2).Info("No monitor available, failing fast on exhaustion", "network", req.Network)
	return fmt.Errorf("failed to allocate ipv4 for pod %s/%s: %w", req.PodNamespace, req.PodName, store.ErrNoAvailableIPs)
}
...
monitor.enqueue()

📝 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 (e *IPAMEngine) SetMonitor(m *Monitor) {
	e.requestsMu.Lock()
	defer e.requestsMu.Unlock()
	e.monitor = m
	}
	// SetMonitor updates the monitor associated with the IPAMEngine.
	func (e *IPAMEngine) SetMonitor(m *Monitor) {
	e.requestsMu.Lock()
	defer e.requestsMu.Unlock()
	e.monitor = m
	}
	// getMonitor returns the monitor associated with the IPAMEngine.
	func (e *IPAMEngine) getMonitor() *Monitor {
	e.requestsMu.RLock()
	defer e.requestsMu.RUnlock()
	return e.monitor
	}

🤖 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 `@metis/pkg/daemon/engine.go` around lines 80 - 84, Protect all reads of
IPAMEngine.monitor with requestsMu by adding a small getMonitor accessor
alongside SetMonitor, then update both monitor accesses in
handleDynamicAllocation to use the accessor and retain the existing nil-check
and enqueue behavior.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

The admin filter is modeled as a raw SQLite WHERE clause. The proto defines filter as native SQL text, and the store concatenates that text straight into the query. One root cause produces the injection surface: the API contract carries executable SQL instead of structured criteria. Changing only the store cannot fix it, because the wire type would still be a clause.

• metis/pkg/store/admin.go#L23-L33: replace the concatenated WHERE %s with an allow-listed column name and a bound placeholder value, and reject unknown columns.
• metis/api/admin/v1/admin.proto#L15-L25: replace the string filter field in ListCIDRBlocksRequest and ListIPAddressesRequest with structured fields, for example a repeated message that holds a column name and a value, and update the field comments so they no longer describe SQL.

As per path instructions, **/*.go files must use "database/sql with placeholders; no fmt.Sprintf in queries".

📍 Affects 2 files

• metis/pkg/store/admin.go#L23-L33 (this comment)
• metis/api/admin/v1/admin.proto#L15-L25

🤖 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 `@metis/pkg/store/admin.go` around lines 23 - 33, Replace the raw SQL filter
flow in metis/pkg/store/admin.go lines 23-33 within Store.adminQueryTable with
validated allow-listed column handling and database/sql bound placeholders;
reject unknown columns and remove fmt.Sprintf-based query construction. Update
metis/api/admin/v1/admin.proto lines 15-25 for ListCIDRBlocksRequest and
ListIPAddressesRequest to replace string filter with structured column/value
criteria and revise comments so they describe structured filters rather than SQL
text.

Source: Path instructions

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

ast-grep outline tools/kops/pkg/kops/gcp.go --items all --type function
rg -n -C 3 'func EnsureStateStore|func runWithRetry|runWithRetry\(|type Config struct' tools/kops --glob '*.go'

Repository: openshift/cloud-provider-gcp

Length of output: 4778

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- gcp.go ---'
cat -n tools/kops/pkg/kops/gcp.go | sed -n '1,180p'

printf '%s\n' '--- config and callers ---'
cat -n tools/kops/pkg/kops/config.go | sed -n '1,100p'
rg -n -C 4 'EnsureStateStore\(|EnsureSSHKey\(|CleanSSHKey\(|context\.|CommandContext|exec\.Command' tools/kops --glob '*.go'

printf '%s\n' '--- retry tests ---'
cat -n tools/kops/pkg/kops/kops_test.go | sed -n '90,180p'

Repository: openshift/cloud-provider-gcp

Length of output: 23159

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- command lifecycle and context propagation ---'
cat -n tools/kops/main.go | sed -n '1,125p'
cat -n tools/kops/pkg/kops/lifecycle.go | sed -n '1,55p'
rg -n 'ExecuteContext|SetContext|cmd\.Context\(|context\.Background|context\.TODO|func Up|func Down' tools/kops --glob '*.go'

printf '%s\n' '--- static behavioral verifier ---'
python3 - <<'PY'
from pathlib import Path
import re

p = Path("tools/kops/pkg/kops/gcp.go")
s = p.read_text()

m = re.search(r'func runWithRetry\(.*?\n\}', s, re.S)
if not m:
    raise SystemExit("runWithRetry not found")
body = m.group(0)

checks = {
    "retry executes synchronously": "cmd.Run()" in body,
    "retry delay is also synchronous": "time.Sleep(delay)" in body,
    "helper has no context parameter": "context.Context" not in body,
    "helper does not create context-bound commands": "exec.CommandContext" not in body,
    "EnsureStateStore uses unbounded command construction": bool(re.search(r'exec\.Command\("gsutil"', s)),
}
for name, result in checks.items():
    print(f"{name}: {result}")
if not all(checks.values()):
    raise SystemExit("unexpected source shape")
PY

Repository: openshift/cloud-provider-gcp

Length of output: 7941

---

Add cancellation and per-attempt deadlines to state-store commands.

runWithRetry calls cmd.Run() synchronously, so a hung gsutil process blocks EnsureStateStore indefinitely. Thread context.Context through the call chain, use exec.CommandContext with a per-attempt timeout, and apply equivalent bounds to the initial gsutil ls and readiness probes.

🤖 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 `@tools/kops/pkg/kops/gcp.go` around lines 89 - 100, Update runWithRetry and
its callers to accept and propagate context.Context, execute each command with
exec.CommandContext, and enforce a timeout for every retry attempt. Apply
equivalent per-command deadlines to the initial gsutil ls operation and
readiness probes used by EnsureStateStore, preserving retry behavior while
ensuring hung processes are cancelled.

Source: Path instructions

[coderabbitai]

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Handle the probe deletion error.

Line 119 discards a failed gsutil rm command. The function then reports readiness while leaving probe objects in the state-store bucket. Return a wrapped cleanup error or record and handle the failure explicitly.

Proposed fix

 		if err := cpCmd.Run(); err == nil {
 			// Probe succeeded; clean up the probe file
 			rmCmd := exec.Command("gsutil", "rm", probeFile)
-			_ = rmCmd.Run()
+			if err := rmCmd.Run(); err != nil {
+				return fmt.Errorf("remove readiness probe %q: %w", probeFile, err)
+			}
 			fmt.Printf("KOPS_STATE_STORE %s is ready and writable.\n", stateStore)
 			return nil

As per path instructions, “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.

Suggested change

	// Probe succeeded; clean up the probe file
	rmCmd := exec.Command("gsutil", "rm", probeFile)
	_ = rmCmd.Run()
	fmt.Printf("KOPS_STATE_STORE %s is ready and writable.\n", stateStore)
	return nil
	// Probe succeeded; clean up the probe file
	rmCmd := exec.Command("gsutil", "rm", probeFile)
	if err := rmCmd.Run(); err != nil {
	return fmt.Errorf("remove readiness probe %q: %w", probeFile, err)
	}
	fmt.Printf("KOPS_STATE_STORE %s is ready and writable.\n", stateStore)
	return 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 `@tools/kops/pkg/kops/gcp.go` around lines 117 - 121, Handle the error returned
by rmCmd.Run() in the probe cleanup path before reporting readiness; if deletion
fails, return a wrapped cleanup error or otherwise explicitly record and handle
it, and do not claim the state store is ready while the probe remains. Keep the
successful cleanup path returning nil in the surrounding readiness check.

Source: Path instructions

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (4)

metis/pkg/daemon/engine.go (4)

80-84: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Read e.monitor under requestsMu.

SetMonitor writes e.monitor while holding requestsMu. handleDynamicAllocation reads e.monitor at Line 221 and Line 231 without the lock. SetMonitor runs from the daemon goroutine while the gRPC server serves requests, so the write and the reads race.

🔒️ Proposed fix

 // SetMonitor updates the monitor associated with the IPAMEngine.
 func (e *IPAMEngine) SetMonitor(m *Monitor) {
 	e.requestsMu.Lock()
 	defer e.requestsMu.Unlock()
 	e.monitor = m
 }
+
+// getMonitor returns the monitor associated with the IPAMEngine.
+func (e *IPAMEngine) getMonitor() *Monitor {
+	e.requestsMu.RLock()
+	defer e.requestsMu.RUnlock()
+	return e.monitor
+}

Then use monitor := e.getMonitor() in handleDynamicAllocation and replace both e.monitor accesses.

🤖 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 `@metis/pkg/daemon/engine.go` around lines 80 - 84, Add a `getMonitor` helper
on `IPAMEngine` that reads `e.monitor` while holding `requestsMu`, then update
`handleDynamicAllocation` to obtain `monitor := e.getMonitor()` and use that
local value for both monitor accesses. Keep `SetMonitor`’s existing locked write
unchanged.

---

344-358: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

onCIDRAdded wakes one waiter when availableIPs is 0.

The loop closes the channel and increments count before it compares count >= availableIPs. If availableIPs is 0, one waiter still wakes and retries an allocation for capacity that does not exist.

🐛 Proposed fix

 	var awakenedClients []string
 	count := 0
+	if availableIPs <= 0 {
+		return
+	}
 	for client, ch := range netMap {

🤖 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 `@metis/pkg/daemon/engine.go` around lines 344 - 358, Update the
waiter-awakening loop in onCIDRAdded to return without closing or removing any
client channels when availableIPs is zero or less. Perform the capacity check
before processing the first waiter, while preserving the existing limit and
awakened-client tracking for positive capacity.

---

252-256: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Do not discard undrainErr.

A store failure from UndrainOneCIDRBlock produces the same control flow as "no block was undrained". The code proceeds to handleDynamicAllocation without logging or returning the error, so real store failures stay invisible.

🐛 Proposed fix

 	undrained, undrainErr := e.store.UndrainOneCIDRBlock(ctx, req.Network, store.IPv4)
+	if undrainErr != nil {
+		e.logger.Error(undrainErr, "failed to undrain CIDR block", "network", req.Network, "podName", req.PodName, "podNamespace", req.PodNamespace)
+	}
 	if undrainErr == nil && undrained {

As per path instructions, **/*.go files must "Never ignore error returns."

🤖 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 `@metis/pkg/daemon/engine.go` around lines 252 - 256, Handle a non-nil
undrainErr immediately after calling UndrainOneCIDRBlock in the allocation flow:
log or propagate the store error according to the surrounding error-handling
convention, and do not continue to handleDynamicAllocation. Preserve the
existing retry behavior when undrainErr is nil and undrained is true, while
allowing the no-block-undrained case to continue normally.

Source: Path instructions

---

111-132: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Release the IPv4 allocation when the IPv6 allocation fails.

If allocateIP succeeds for IPv4 and then fails for IPv6, AllocatePodIP returns the error without releasing the IPv4 reservation. The address stays reserved until a later DEL or retry. Repeated partial dual-stack failures consume pool capacity.

Call e.store.ReleaseIPByOwner for the IPv4 container and interface before returning the IPv6 error.

🤖 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 `@metis/pkg/daemon/engine.go` around lines 111 - 132, Update AllocatePodIP’s
IPv6 allocation error path to call e.store.ReleaseIPByOwner for the previously
successful IPv4 container and interface before returning the IPv6 error.
Preserve the existing error propagation and only release the IPv4 reservation
when IPv6 allocateIP fails.

🤖 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 `@cmd/cloud-controller-manager/gketenantcontrollermanager.go`:
- Around line 130-137: Update the IPv6-only branch in StartNodeIpamController to
require ipam.CloudAllocatorType before assigning the 100::/64 fallback CIDR. If
another allocator is configured, fail fast instead of selecting this fallback;
preserve the existing provider-config CIDR path for non-IPv6-only clusters.

In `@go.mod`:
- Around line 17-24: Update the Kubernetes replace directives in the go.mod
replacement block to use v0.36.3 consistently with the declared k8s.io
dependencies, then regenerate go.mod and go.sum. If any replacement must remain
at v0.36.0, document the justification instead.

---

Duplicate comments:
In `@metis/pkg/daemon/engine.go`:
- Around line 80-84: Add a `getMonitor` helper on `IPAMEngine` that reads
`e.monitor` while holding `requestsMu`, then update `handleDynamicAllocation` to
obtain `monitor := e.getMonitor()` and use that local value for both monitor
accesses. Keep `SetMonitor`’s existing locked write unchanged.
- Around line 344-358: Update the waiter-awakening loop in onCIDRAdded to return
without closing or removing any client channels when availableIPs is zero or
less. Perform the capacity check before processing the first waiter, while
preserving the existing limit and awakened-client tracking for positive
capacity.
- Around line 252-256: Handle a non-nil undrainErr immediately after calling
UndrainOneCIDRBlock in the allocation flow: log or propagate the store error
according to the surrounding error-handling convention, and do not continue to
handleDynamicAllocation. Preserve the existing retry behavior when undrainErr is
nil and undrained is true, while allowing the no-block-undrained case to
continue normally.
- Around line 111-132: Update AllocatePodIP’s IPv6 allocation error path to call
e.store.ReleaseIPByOwner for the previously successful IPv4 container and
interface before returning the IPv6 error. Preserve the existing error
propagation and only release the IPv4 reservation when IPv6 allocateIP fails.

🪄 Autofix

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: 3391588e-e625-417c-8016-6490d5a6faa1

📥 Commits

Reviewing files that changed from the base of the PR and between 6261bcb and 99911d0.

⛔ Files ignored due to path filters (43)

• go.sum is excluded by !**/*.sum
• metis/go.sum is excluded by !**/*.sum
• providers/go.sum is excluded by !**/*.sum
• test/e2e/go.sum is excluded by !**/*.sum
• vendor/cloud.google.com/go/auth/CHANGES.md is excluded by !**/vendor/**, !vendor/**
• vendor/cloud.google.com/go/auth/credentials/detect.go is excluded by !**/vendor/**, !vendor/**
• vendor/cloud.google.com/go/auth/credentials/filetypes.go is excluded by !**/vendor/**, !vendor/**
• vendor/cloud.google.com/go/auth/httptransport/transport.go is excluded by !**/vendor/**, !vendor/**
• vendor/cloud.google.com/go/auth/internal/internal.go is excluded by !**/vendor/**, !vendor/**
• vendor/cloud.google.com/go/auth/internal/regionalaccessboundary/external_accounts_config_providers.go is excluded by !**/vendor/**, !vendor/**
• vendor/cloud.google.com/go/auth/internal/regionalaccessboundary/regional_access_boundary.go is excluded by !**/vendor/**, !vendor/**
• vendor/cloud.google.com/go/auth/internal/retry/retry.go is excluded by !**/vendor/**, !vendor/**
• vendor/cloud.google.com/go/auth/internal/retry/retry_linux.go is excluded by !**/vendor/**, !vendor/**
• vendor/cloud.google.com/go/auth/internal/transport/headers/headers.go is excluded by !**/vendor/**, !vendor/**
• vendor/cloud.google.com/go/auth/internal/trustboundary/trust_boundary.go is excluded by !**/vendor/**, !vendor/**
• vendor/cloud.google.com/go/auth/internal/version.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/mattn/go-sqlite3/callback.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/mattn/go-sqlite3/sqlite3-binding.c is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/mattn/go-sqlite3/sqlite3-binding.h is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/mattn/go-sqlite3/sqlite3_opt_vtable.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/compute/v0.alpha/compute-api.json is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/compute/v0.alpha/compute-gen.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/compute/v0.alpha/compute2-gen.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/compute/v1/compute-api.json is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/compute/v1/compute-gen.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/compute/v1/compute2-gen.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/compute/v1/compute3-gen.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/internal/version.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/networkservices/v1/networkservices-api.json is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/networkservices/v1/networkservices-gen.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/api/transport/http/dial.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/grpc/clientconn.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/grpc/clientconn_disconnect_reason_noplan9.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/grpc/clientconn_disconnect_reason_plan9.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/grpc/internal/envconfig/xds.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/grpc/internal/grpcsync/callback_serializer.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/grpc/internal/resolver/config_selector.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/grpc/internal/transport/client_stream.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/grpc/internal/transport/http2_client.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/grpc/internal/transport/transport.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/grpc/stream.go is excluded by !**/vendor/**, !vendor/**
• vendor/google.golang.org/grpc/version.go is excluded by !**/vendor/**, !vendor/**
• vendor/modules.txt is excluded by !**/vendor/**, !vendor/**

📒 Files selected for processing (9)

• cmd/cloud-controller-manager/gketenantcontrollermanager.go
• go.mod
• metis/go.mod
• metis/pkg/daemon/engine.go
• metis/pkg/daemon/monitor.go
• metis/pkg/daemon/watcher.go
• metis/pkg/store/store.go
• providers/go.mod
• test/e2e/go.mod

🚧 Files skipped from review as they are similar to previous changes (3)

• metis/go.mod
• metis/pkg/store/store.go
• metis/pkg/daemon/monitor.go

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C 5 \
  'CIDRAllocatorType|CloudAllocatorType|RangeAllocatorType|IPAMFrom(Cluster|Cloud)AllocatorType|cidr-allocator-type|gke-tenant-controller' \
  --glob '*.{go,yaml,yml}' || true

Repository: openshift/cloud-provider-gcp

Length of output: 166

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

target='cmd/cloud-controller-manager/gketenantcontrollermanager.go'

printf '%s\n' '--- target outline ---'
if command -v ast-grep >/dev/null 2>&1; then
  ast-grep outline "$target" || true
fi

printf '%s\n' '--- target sections ---'
sed -n '1,190p' "$target"
sed -n '220,270p' "$target"

printf '%s\n' '--- local call sites and symbols ---'
rg -n -C 8 \
  'StartNodeIpamController|CloudAllocatorType|RangeAllocatorType|IPAMFrom|CIDRAllocatorType|cidr-allocator-type|100::/64|isIPV6OnlyCluster|getCIDRsFromProviderConfig' \
  --glob '*.go' --glob 'go.mod' --glob 'go.sum' . || true

printf '%s\n' '--- tracked dependency/module files ---'
git ls-files '*go.mod' '*go.sum' '*vendor*' | head -200

Repository: openshift/cloud-provider-gcp

Length of output: 50386

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- allocator configuration references ---'
rg -n \
  'CIDRAllocatorType|cidrAllocatorType|cidr-allocator-type|CIDRAllocator|CloudAllocator|RangeAllocator|IPAMFromCluster|IPAMFromCloud' \
  --glob '!*.sum' \
  --glob '!vendor/**' \
  --glob '!third_party/**' \
  --glob '*.{go,yaml,yml,json,toml,md,txt}' . \
  | head -300 || true

printf '%s\n' '--- focused StartNodeIpamController implementation ---'
sed -n '31,165p' pkg/controller/nodeipam/starter.go

printf '%s\n' '--- focused allocator implementation ---'
sed -n '39,132p' pkg/controller/nodeipam/ipam/cidr_allocator.go
sed -n '100,155p' pkg/controller/nodeipam/node_ipam_controller.go

printf '%s\n' '--- relevant tracked config/deployment filenames ---'
git ls-files | rg '(^|/)(deploy|config|charts|manifests|examples|test|cmd)/|(\.yaml|\.yml)$' | head -300 || true

Repository: openshift/cloud-provider-gcp

Length of output: 46802

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- legacy IPAM path ---'
sed -n '1,120p' pkg/controller/nodeipam/legacyprovider.go
rg -n -C 6 \
  'SyncFromCluster|SyncFromCloud|func NewProvider|clusterCIDR|Mode' \
  pkg/controller/nodeipam pkg/controller/nodeipam/ipam \
  | head -260 || true

printf '%s\n' '--- allocator options and defaults ---'
sed -n '1,240p' cmd/cloud-controller-manager/options/nodeipamcontroller.go
rg -n -C 6 \
  'CIDRAllocatorType|ClusterCIDR|AllocateNodeCIDRs|CloudAllocator|RangeAllocator' \
  vendor/k8s.io/cloud-provider vendor/k8s.io/kube-controller-manager cmd pkg \
  --glob '*.go' \
  | head -260 || true

printf '%s\n' '--- read-only source-flow verifier ---'
python3 - <<'PY'
from pathlib import Path

target = Path("cmd/cloud-controller-manager/gketenantcontrollermanager.go").read_text()
starter = Path("pkg/controller/nodeipam/starter.go").read_text()
controller = Path("pkg/controller/nodeipam/node_ipam_controller.go").read_text()
allocator = Path("pkg/controller/nodeipam/ipam/cidr_allocator.go").read_text()
legacy = Path("pkg/controller/nodeipam/legacyprovider.go").read_text()

checks = {
    "fallback_literal": 'cidrs = "100::/64"' in target,
    "shared_allocator_forwarded": 'ipam.CIDRAllocatorType(mgrCfg.completedConfig.ComponentConfig.KubeCloudShared.CIDRAllocatorType)' in target,
    "fallback_passed_to_starter": 'nodeipam.StartNodeIpamController(' in target and 'cidrs,' in target,
    "starter_processes_cluster_cidr": 'cidrs, dualStack, err := ProcessCIDRs(clusterCIDR)' in starter,
    "range_allocator_uses_cluster_cidrs": 'case RangeAllocatorType:' in allocator and 'NewCIDRRangeAllocator' in allocator,
    "non_cloud_validates_cluster_cidrs": 'if allocatorType != ipam.CloudAllocatorType {' in controller,
    "legacy_cluster_mode_exists": 'case ipam.IPAMFromClusterAllocatorType:' in legacy,
    "legacy_cloud_mode_exists": 'case ipam.IPAMFromCloudAllocatorType:' in legacy,
}
for name, ok in checks.items():
    print(f"{name}={'PASS' if ok else 'FAIL'}")
if not all(checks.values()):
    raise SystemExit(1)
PY

Repository: openshift/cloud-provider-gcp

Length of output: 49411

---

Reject non-cloud allocators for the IPv6-only fallback.

StartNodeIpamController passes 100::/64 as clusterCIDR, and RangeAllocatorType can allocate it as Node.Spec.PodCIDRs. Enforce ipam.CloudAllocatorType before selecting this fallback, or fail fast for other allocator types.

🤖 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 `@cmd/cloud-controller-manager/gketenantcontrollermanager.go` around lines 130
- 137, Update the IPv6-only branch in StartNodeIpamController to require
ipam.CloudAllocatorType before assigning the 100::/64 fallback CIDR. If another
allocator is configured, fail fast instead of selecting this fallback; preserve
the existing provider-config CIDR path for non-IPv6-only clusters.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

go mod edit -json go.mod |
  jq -r '.Replace[]? | "\(.Old.Path)@\(.Old.Version // "*") => \(.New.Path)@\(.New.Version // "local")"'

go list -m -f '{{.Path}} {{.Version}}{{with .Replace}} => {{.Path}} {{.Version}}{{end}}' all |
  rg '^k8s.io/(api|apimachinery|apiserver|client-go|cloud-provider|component-base|component-helpers|controller-manager) '

Repository: openshift/cloud-provider-gcp

Length of output: 1727

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- go.mod replacement directives ---'
go mod edit -json go.mod | jq -r '.Replace[]? | "\(.Old.Path)@\(.Old.Version // "*") => \(.New.Path)@\(.New.Version // "local")"'

printf '%s\n' '--- relevant go.mod declarations ---'
nl -ba go.mod | sed -n '1,90p'

printf '%s\n' '--- vendor metadata for Kubernetes modules ---'
if [ -f vendor/modules.txt ]; then
  rg -n -A2 -B1 '^## explicit|^# k8s.io/(api|apimachinery|apiserver|client-go|code-generator|component-base|component-helpers|controller-manager)(@|$)' vendor/modules.txt
else
  printf '%s\n' 'vendor/modules.txt not present'
fi

printf '%s\n' '--- module files ---'
git ls-files '*go.mod' '*go.sum' | sort

Repository: openshift/cloud-provider-gcp

Length of output: 1747

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- go.mod declarations and replacements ---'
awk 'NR <= 90 { printf "%6d  %s\n", NR, $0 }' go.mod

printf '%s\n' '--- vendor metadata for selected modules ---'
if [ -f vendor/modules.txt ]; then
  rg -n -A2 -B1 '^# k8s.io/(api|apimachinery|apiserver|client-go|code-generator|component-base|component-helpers|controller-manager)(@|$)' vendor/modules.txt
else
  printf '%s\n' 'vendor/modules.txt not present'
fi

printf '%s\n' '--- effective module versions with module mode ---'
GOFLAGS=-mod=mod go list -m -f '{{.Path}} {{.Version}}{{with .Replace}} => {{.Path}} {{.Version}}{{end}}' all 2>&1 |
  rg '^k8s.io/(api|apimachinery|apiserver|client-go|code-generator|component-base|component-helpers|controller-manager)( |$)|^go:'

Repository: openshift/cloud-provider-gcp

Length of output: 4637

---

🏁 Script executed:

#!/bin/bash
set -u

printf '%s\n' '--- replacement directives in the remainder of go.mod ---'
awk 'NR >= 300 { printf "%6d  %s\n", NR, $0 }' go.mod | rg 'replace|k8s.io/'

printf '%s\n' '--- effective versions for directly declared Kubernetes modules ---'
GOFLAGS=-mod=mod go list -m -f '{{.Path}} {{.Version}}{{with .Replace}} => {{.Path}} {{.Version}}{{end}}' \
  k8s.io/api k8s.io/apimachinery k8s.io/apiserver k8s.io/client-go \
  k8s.io/code-generator k8s.io/component-base k8s.io/component-helpers \
  k8s.io/controller-manager k8s.io/cloud-provider k8s.io/cloud-provider-gcp/providers \
  k8s.io/kubernetes 2>&1

printf '%s\n' '--- working tree changes ---'
git status --short

Repository: openshift/cloud-provider-gcp

Length of output: 519

---

🏁 Script executed:

#!/bin/bash
set -u

printf '%s\n' '--- effective versions outside workspace mode ---'
GOWORK=off GOFLAGS=-mod=mod go list -m -f '{{.Path}} {{.Version}}{{with .Replace}} => {{.Path}} {{.Version}}{{end}}' \
  k8s.io/api k8s.io/apimachinery k8s.io/apiserver k8s.io/client-go \
  k8s.io/code-generator k8s.io/component-base k8s.io/component-helpers \
  k8s.io/controller-manager k8s.io/cloud-provider k8s.io/cloud-provider-gcp/providers \
  k8s.io/kubernetes 2>&1

printf '%s\n' '--- all replacement directives with locations ---'
rg -n '^replace|=>|k8s.io/(api|apimachinery|apiserver|client-go|code-generator|component-base|component-helpers|controller-manager)' go.mod

Repository: openshift/cloud-provider-gcp

Length of output: 2858

---

Update the Kubernetes replacement block to match the declared versions. go.mod:153-180 forces the v0.36.3 modules to v0.36.0, so the root build does not use the declared patch updates. If v0.36.0 is intentional, document the reason; otherwise update the replacements and regenerate go.mod and go.sum.

🤖 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 `@go.mod` around lines 17 - 24, Update the Kubernetes replace directives in the
go.mod replacement block to use v0.36.3 consistently with the declared k8s.io
dependencies, then regenerate go.mod and go.sum. If any replacement must remain
at v0.36.0, document the justification instead.

Source: MCP tools