Skip to content

fix(egress): allow credential vault bindings with defaultAction allow#1066

Merged
jwx0925 merged 4 commits into
mainfrom
fix/credential-vault-default-allow-policy
Jun 16, 2026
Merged

fix(egress): allow credential vault bindings with defaultAction allow#1066
jwx0925 merged 4 commits into
mainfrom
fix/credential-vault-default-allow-policy

Conversation

@Pangjiping

Copy link
Copy Markdown
Collaborator

Summary

  • When network policy uses defaultAction: allow, credential vault bindings no longer require explicit egress allow rules for each binding host.
  • Hosts are still validated via pol.Evaluate() — explicit deny rules remain enforced.
  • Adds tests for both defaultAction: allow pass-through and deny rule enforcement.

Test plan

  • TestCredentialVaultAllowsDefaultAllowWithoutExplicitRules — vault create succeeds with defaultAction: allow + empty egress
  • TestCredentialVaultDefaultAllowRespectsExplicitDenyRule — vault create rejected when host hit by explicit deny rule
  • All egress tests pass (go test ./...)

🤖 Generated with Claude Code

When the network policy uses defaultAction: allow, credential vault
bindings no longer require explicit egress allow rules for each host.
The host is still validated against pol.Evaluate() so explicit deny
rules remain enforced.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e only

pol.Evaluate already combines defaultAction with explicit rules, so
the separate explicitAllowRuleMatches check was redundant. Remove it
along with the now-unused hostMatchesPattern helper.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Pangjiping Pangjiping added bug Something isn't working component/egress labels Jun 15, 2026
…idate

The per-binding validateBindingPolicy → explicitAllowCoversHost path
already rejects nil policy and uncovered hosts. The early len(Egress)
guard was redundant.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2895e27f95

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread components/egress/pkg/credentialvault/vault.go Outdated
…date error message

The blank name check in validateCandidate was redundant — all code
paths already go through normalizeCredential which rejects blank
names. Also updated the binding host error message to reflect that
the check is now policy-based, not explicit-rule-based.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 735992fe97

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread components/egress/pkg/credentialvault/vault.go

@jwx0925 jwx0925 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the change set. Ignoring the wildcard sampling concern discussed separately, the core behavior looks correct: credential vault bindings now follow the effective egress policy evaluation, allowing defaultAction=allow while still respecting explicit denies.

@jwx0925 jwx0925 merged commit 67cdad6 into main Jun 16, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working component/egress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants