feat(cli,sdk,manifests): credential name resolution + platform ingress NetworkPolicy#1566
Open
markturansky wants to merge 5 commits into
Open
feat(cli,sdk,manifests): credential name resolution + platform ingress NetworkPolicy#1566markturansky wants to merge 5 commits into
markturansky wants to merge 5 commits into
Conversation
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNetworkPolicy renamed to ChangesCredential CLI & SDK
Manifests & misc
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
markturansky
force-pushed
the
fix/platform-ingress-netpol-base
branch
2 times, most recently
from
514e8d6 to
baa4ad7
Compare
…ress rules The allow-from-runner-namespaces NP (#1553) uses podSelector: {} (all pods) but only permits ingress from runner pods, blocking OpenShift router traffic to the frontend and all other services. This caused outages on both Stage and UAT clusters. Replace with allow-platform-ingress that permits: - OpenShift router ingress (policy-group.network.openshift.io/ingress label) - Intra-namespace pod-to-pod traffic - Runner pod ingress from any namespace (original intent of #1553) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
markturansky
force-pushed
the
fix/platform-ingress-netpol-base
branch
from
beb2298 to
f416ffa
Compare
markturansky
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/ambient-cli/cmd/acpctl/apply/cmd.go`:
- Around line 246-248: The code treats any error from
client.Credentials().FindByName(ctx, doc.Name) as a "not found" and falls
through to create; change this so only a genuine not-found error triggers
creation and all other errors are returned/handled. Specifically, in the block
around existing, err := client.Credentials().FindByName(ctx, doc.Name) (and the
subsequent token := os.ExpandEnv(doc.Token) / create path), detect the
provider/API "not found" sentinel (or inspect the returned error for a
NotFound/IsNotFound condition) and only execute the create logic when that
condition is true; for any other err, propagate or log and return the error
instead of proceeding to create. Ensure you reference
client.Credentials().FindByName, doc.Name and the create branch when making the
conditional change.
In `@components/ambient-cli/cmd/acpctl/credential/cmd.go`:
- Around line 471-479: The resolveCredentialID function currently falls back to
client.Credentials().FindByName on any error from client.Credentials().Get,
which can hide transport/auth/server errors; change it so that after calling
client.Credentials().Get(ctx, nameOrID) you only call FindByName when the Get
error indicates a not-found/404 condition (use the SDK's NotFound sentinel or
inspect the HTTP/status code), and for any other error return that error wrapped
with context (e.g., "failed to get credential %q") instead of falling back; keep
the final FindByName call and its existing not-found error message unchanged.
In `@components/ambient-cli/cmd/acpctl/delete/cmd.go`:
- Around line 136-139: The code currently ignores all errors from
client.Credentials().FindByName and proceeds to call Delete using deleteID;
change this so that if FindByName returns an error you only continue when the
error explicitly means "not found" and otherwise return/propagate the error.
Concretely, after calling client.Credentials().FindByName(ctx, name) check if
findErr != nil and then if !errors.Is(findErr, <package>.ErrNotFound) (or use
the repository/client IsNotFound helper if available) return or wrap findErr;
only when the error is the NotFound sentinel keep using deleteID (or set
deleteID from cred when found) before calling client.Credentials().Delete(ctx,
deleteID). Ensure you import errors and use the package-specific
ErrNotFound/IsNotFound symbol to distinguish not-found from other failures.
In `@components/ambient-cli/cmd/acpctl/get/cmd.go`:
- Around line 473-478: The current logic calls client.Credentials().FindByName
whenever client.Credentials().Get returns any error; change this so only a
not-found error triggers the fallback. In the block around
client.Credentials().Get(ctx, name) and client.Credentials().FindByName(ctx,
name), detect a not-found condition (e.g., errors.Is(err, <notFoundSentinel>) or
checking gRPC status.Code(err) == codes.NotFound) and only then call FindByName;
for any other error return fmt.Errorf("get credential %q: %w", name, err)
immediately. Keep the existing error wrap/message and the cred variable usage
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c05fe9fb-2da2-4577-80ec-1f5038d28dfa
📒 Files selected for processing (9)
.gitignorecomponents/ambient-cli/cmd/acpctl/apply/cmd.gocomponents/ambient-cli/cmd/acpctl/credential/cmd.gocomponents/ambient-cli/cmd/acpctl/delete/cmd.gocomponents/ambient-cli/cmd/acpctl/describe/cmd.gocomponents/ambient-cli/cmd/acpctl/get/cmd.gocomponents/ambient-sdk/go-sdk/client/credential_api.gocomponents/ambient-sdk/go-sdk/client/credential_extensions.gocomponents/manifests/base/runner-networkpolicy.yaml
✅ Files skipped from review due to trivial changes (1)
- .gitignore
Comment on lines
+246
to
248
| existing, err := client.Credentials().FindByName(ctx, doc.Name) | ||
| if err != nil { | ||
| token := os.ExpandEnv(doc.Token) |
Contributor
There was a problem hiding this comment.
Don’t treat every lookup error as create-miss.
Line 247 routes to create on any FindByName error. This should only happen on not-found; otherwise transient/API failures are misclassified and can produce incorrect create behavior.
As per coding guidelines, “Handle errors and edge cases explicitly rather than ignoring them.”
🤖 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 `@components/ambient-cli/cmd/acpctl/apply/cmd.go` around lines 246 - 248, The
code treats any error from client.Credentials().FindByName(ctx, doc.Name) as a
"not found" and falls through to create; change this so only a genuine not-found
error triggers creation and all other errors are returned/handled. Specifically,
in the block around existing, err := client.Credentials().FindByName(ctx,
doc.Name) (and the subsequent token := os.ExpandEnv(doc.Token) / create path),
detect the provider/API "not found" sentinel (or inspect the returned error for
a NotFound/IsNotFound condition) and only execute the create logic when that
condition is true; for any other err, propagate or log and return the error
instead of proceeding to create. Ensure you reference
client.Credentials().FindByName, doc.Name and the create branch when making the
conditional change.
Comment on lines
+471
to
+479
| func resolveCredentialID(ctx context.Context, client *sdkclient.Client, nameOrID string) (string, error) { | ||
| cred, err := client.Credentials().Get(ctx, nameOrID) | ||
| if err == nil { | ||
| return cred.ID, nil | ||
| } | ||
| cred, err = client.Credentials().FindByName(ctx, nameOrID) | ||
| if err != nil { | ||
| return "", fmt.Errorf("credential %q not found by ID or name", nameOrID) | ||
| } |
Contributor
There was a problem hiding this comment.
Only fallback to name lookup on NotFound errors.
Line 472 currently falls back to FindByName for any Get failure, which can mask transport/auth/server errors as “not found.” Gate fallback strictly to 404/not-found and propagate other errors with context.
As per coding guidelines, “Handle errors and edge cases explicitly rather than ignoring them.”
🤖 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 `@components/ambient-cli/cmd/acpctl/credential/cmd.go` around lines 471 - 479,
The resolveCredentialID function currently falls back to
client.Credentials().FindByName on any error from client.Credentials().Get,
which can hide transport/auth/server errors; change it so that after calling
client.Credentials().Get(ctx, nameOrID) you only call FindByName when the Get
error indicates a not-found/404 condition (use the SDK's NotFound sentinel or
inspect the HTTP/status code), and for any other error return that error wrapped
with context (e.g., "failed to get credential %q") instead of falling back; keep
the final FindByName call and its existing not-found error message unchanged.
Comment on lines
+136
to
+139
| if cred, findErr := client.Credentials().FindByName(ctx, name); findErr == nil { | ||
| deleteID = cred.ID | ||
| } | ||
| if err := client.Credentials().Delete(ctx, deleteID); err != nil { |
Contributor
There was a problem hiding this comment.
Avoid silent fallback when name resolution fails.
Line 136 ignores FindByName errors and proceeds with Delete anyway. Restrict fallback to explicit not-found and return other errors; otherwise API/auth failures are hidden.
As per coding guidelines, “Handle errors and edge cases explicitly rather than ignoring them.”
🤖 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 `@components/ambient-cli/cmd/acpctl/delete/cmd.go` around lines 136 - 139, The
code currently ignores all errors from client.Credentials().FindByName and
proceeds to call Delete using deleteID; change this so that if FindByName
returns an error you only continue when the error explicitly means "not found"
and otherwise return/propagate the error. Concretely, after calling
client.Credentials().FindByName(ctx, name) check if findErr != nil and then if
!errors.Is(findErr, <package>.ErrNotFound) (or use the repository/client
IsNotFound helper if available) return or wrap findErr; only when the error is
the NotFound sentinel keep using deleteID (or set deleteID from cred when found)
before calling client.Credentials().Delete(ctx, deleteID). Ensure you import
errors and use the package-specific ErrNotFound/IsNotFound symbol to distinguish
not-found from other failures.
Comment on lines
473
to
+478
| cred, err := client.Credentials().Get(ctx, name) | ||
| if err != nil { | ||
| return fmt.Errorf("get credential %q: %w", name, err) | ||
| cred, err = client.Credentials().FindByName(ctx, name) | ||
| if err != nil { | ||
| return fmt.Errorf("get credential %q: %w", name, err) | ||
| } |
Contributor
There was a problem hiding this comment.
Use not-found-aware fallback in get credentials.
Line 474 should not automatically switch to FindByName for every Get error. Keep fallback for not-found only, and return non-404 failures directly.
As per coding guidelines, “Handle errors and edge cases explicitly rather than ignoring them.”
🤖 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 `@components/ambient-cli/cmd/acpctl/get/cmd.go` around lines 473 - 478, The
current logic calls client.Credentials().FindByName whenever
client.Credentials().Get returns any error; change this so only a not-found
error triggers the fallback. In the block around client.Credentials().Get(ctx,
name) and client.Credentials().FindByName(ctx, name), detect a not-found
condition (e.g., errors.Is(err, <notFoundSentinel>) or checking gRPC
status.Code(err) == codes.NotFound) and only then call FindByName; for any other
error return fmt.Errorf("get credential %q: %w", name, err) immediately. Keep
the existing error wrap/message and the cred variable usage unchanged.
…commands Add resolveCredentialID helper to credential subcommand (get, update, delete, token) enabling name-based lookups with ID fallback. Add FindByName to SDK credential_extensions.go using TSL search. Fix apply to use FindByName for upsert. Add platform-managed pod ingress rule to runner NetworkPolicy. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
markturansky
force-pushed
the
fix/platform-ingress-netpol-base
branch
from
9110752 to
a5ef660
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
acpctl credentialsubcommands (get,update,delete,token) now accept credential names in addition to IDs, with automatic name→ID resolution viaFindByNameFindByName()toCredentialAPIin the Go SDK using TSL search (name = '<name>')acpctl applycredential upsert now usesFindByNameinstead ofGet(which only accepts IDs)acpctl delete credential <name>resolves name→ID before deletionacpctl describe credentialandacpctl get credentialfall back toFindByNamewhenGetby ID failsambient-code.io/managed: "true"label across namespacesCreateretainsproject_idinjection for backward compat with deployed server (TODO to remove after migration202505120001is deployed)Tested against live cluster
< /dev/null | Operation | Command | Result |
|---|---|---|
| List |
acpctl credential list| ✅ || Create |
acpctl credential create --name ... --provider ...| ✅ || Get by name |
acpctl credential get <name>| ✅ || Get by ID |
acpctl credential get <id>| ✅ || Describe by name |
acpctl describe credential <name>| ✅ || Update by name |
acpctl credential update <name> --description ...| ✅ || Token by name |
acpctl credential token <name>| ✅ || Delete by name |
acpctl credential delete <name> --confirm| ✅ || Bind |
acpctl credential bind <name> --project <project>| ✅ || Apply YAML |
acpctl apply -f -(Credential) | ✅ |Test plan
credential bindcreates RoleBinding withscope=credentialgo buildsucceeds with no errors🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
.secrets/to .gitignore.New Features