feat: Add address validation for gRPC connections and corresponding t… - #7117
Open
Anjali-Chauhan1 wants to merge 1 commit into
Open
feat: Add address validation for gRPC connections and corresponding t…#7117Anjali-Chauhan1 wants to merge 1 commit into
Anjali-Chauhan1 wants to merge 1 commit into
Conversation
…ests Signed-off-by: Anjali-Chauhan1 <anjalichauhan1036@gmail.com>
Contributor
|
👋 Hi @Anjali-Chauhan1, welcome to PipeCD and thanks for opening your first pull request! We’re really happy to have you here Before your PR gets merged, please check a few important things below. Helpful resources
DCO Sign-offAll commits must include a In case you forget to sign-off your commit(s), follow these steps: For the last commit: git commit --amend --signoff
git push --force-with-leaseFor multiple commits: git rebase --signoff origin/master
git push --force-with-leaseRun checks locallyBefore pushing updates, please run: make checkThis runs the same checks as CI and helps catch issues early. 💬 Need help?If anything is unclear, feel free to ask in this PR or join us on the CNCF Slack in the #pipecd channel. Thanks for contributing to PipeCD! ❤️ |
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
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.
What this PR does:
Adds
--addressformat validation toOptions.Validate()inpkg/app/pipectl/client/client.go.A new
validateAddresshelper mirrors grpc-go's own target parsing instead of inventing its own rules:resolver.Get), the address is accepted and left to that resolver — endpoint syntax is resolver-specific, sodns:///localhost(defaults to:443) andunix:///var/run/pipecd.sock(no port at all) stay valid.passthroughresolver and must be a dialablehost:port: non-empty host, no whitespace, and a port resolving vianet.LookupPortto 1–65535.Using the resolver registry rather than a hardcoded scheme allowlist keeps this correct automatically — it rejects
xds://, for example, which is a real gRPC scheme but is not linked into the pipectl binary.Also adds
pkg/app/pipectl/client/client_test.go. This package previously had no tests; the new file coversvalidateAddress,Options.Validate(), and aNewClientcase asserting a bad address fails fast rather than burning the 5s dial timeout (37 subtests).Why we need it:
Options.Validate()only checked that--addresswas non-empty. Any non-empty malformed value passed through togrpc.DialContext()and surfaced after the dial timeout as:That is hard to act on, and
Optionsis shared by every pipectl subcommand (application, deployment, encrypt, event, planpreview, transfer, plugin), so it affects all pipectl users.Behaviour was verified empirically against grpc v1.79.3 rather than from the docs. Inputs now rejected up front:
localhostmust be in host:port form (e.g. localhost:9000) or use a gRPC scheme such as dns:///http://localhost:9000scheme "http" is not a supported gRPC resolvertcp://localhost:9000scheme "tcp" is not a supported gRPC resolverlocalhost:9000/path"9000/path" is not a valid portlocalhost:abc"abc" is not a valid portlocalhost:andlocalhost:0port must be between 1 and 65535" localhost:9000"host must not contain whitespace:9000host must not be emptyThe last three matter because the stdlib fails silently on them:
net.SplitHostPort("localhost:")returns an empty port with no error, andnet.LookupPort("tcp", "")returns0, nil— so gRPC would happily dial port 0.Which issue(s) this PR fixes:
Fixes #7089
This deviates from the approach proposed in the issue. That algorithm (
SplitHostPort, falling back tourl.Parse, then checking for a non-empty host or any scheme) scored 9/13 against real gRPC behaviour — it acceptshttp://…,tcp://…,localhost:9000/path, andlocalhost:abc, becauseSplitHostPortdoes not validate that the port is numeric and "has a scheme" is too weak a test. Its instinct to exempt resolver URIs from the port requirement was correct and is preserved here.Does this PR introduce a user-facing change?:
Yes.
--addressnow fails immediately with a specific message naming the problem and echoing the offending value, instead of a multi-line gRPC transport error after the dial timeout. Every previously valid form still works —host:port, IPv4, bracketed IPv6, FQDN, named ports (localhost:http), anddns:///,unix:///,unix:,passthrough:///resolver URIs.