Skip to content

Error out consul connect envoy if agent explicitly disabled grpc#15794

Merged
kisunji merged 5 commits into
mainfrom
kisunji/NET-1767-envoy-grpc-disabled
Dec 19, 2022
Merged

Error out consul connect envoy if agent explicitly disabled grpc#15794
kisunji merged 5 commits into
mainfrom
kisunji/NET-1767-envoy-grpc-disabled

Conversation

@kisunji
Copy link
Copy Markdown
Contributor

@kisunji kisunji commented Dec 14, 2022

Description

Error out if no grpc ports are returned by /v1/agent/self instead of defaulting to 8502.

Update tests to reflect Consul's new default behavior to enable grpc TLS (#15302)

Testing & Reproduction steps

  • Added testcase for explicit disabling of both plaintext and TLS gRPC ports

PR Checklist

  • updated test coverage
  • external facing docs updated
  • not a security concern

@kisunji kisunji force-pushed the kisunji/NET-1767-envoy-grpc-disabled branch from 6765016 to 1a8accb Compare December 14, 2022 16:28
@github-actions github-actions Bot added the theme/cli Flags and documentation for the CLI interface label Dec 14, 2022
Comment thread command/connect/envoy/envoy.go Outdated
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure if the return was missed here or we intentionally went on to default to 8502. It looks safe to error here since if the call to Agent.Self doesn't yield any port, we probably don't want the command to continue.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It can't error out because typically agent:read is not granted to sidecars, which is required for /v1/agent/self. The existing code did the agent self lookup as an UX optimization.

Instead you could pick whichever port we already calculated and do a super simple net.Dial with a short timeout and see if the port is even open instead, which wouldn't even need ACL tokens.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, I'm not sure why it never errored-out before. I assume it was to enable some backwards compatibility with old APIs that didn't include the port. I don't honestly know the history on this particular area of code, aside from the recent changes I made. I'll have to think about it quite a bit, because there might be some situation where the API is not available, but gRPC is.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nevermind. RB already answered the question.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rboyer I ended up logging and defaulting only if it's an ACL perm denied and otherwise erroring out.

Comment thread command/connect/envoy/envoy_test.go Outdated
@kisunji kisunji force-pushed the kisunji/NET-1767-envoy-grpc-disabled branch from 1a8accb to 1a5a2fb Compare December 14, 2022 16:41
@kisunji kisunji marked this pull request as ready for review December 14, 2022 16:50
@kisunji kisunji requested review from a team, cthain, hashi-derek and pglass and removed request for a team December 14, 2022 16:54
@kisunji kisunji force-pushed the kisunji/NET-1767-envoy-grpc-disabled branch 2 times, most recently from 1bd5e3e to fff2726 Compare December 15, 2022 21:40
Comment thread command/connect/envoy/envoy_test.go Outdated
Comment on lines 549 to 562
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

WantArgs is never evaluated if WantErr exists so I removed it

@kisunji kisunji force-pushed the kisunji/NET-1767-envoy-grpc-disabled branch 2 times, most recently from 962dd94 to 7957ace Compare December 15, 2022 21:46
@kisunji kisunji requested a review from rboyer December 15, 2022 21:47
@kisunji kisunji force-pushed the kisunji/NET-1767-envoy-grpc-disabled branch from 7957ace to 59ea55b Compare December 15, 2022 21:50
Copy link
Copy Markdown

@pglass pglass left a comment

Choose a reason for hiding this comment

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

👍

Comment thread command/connect/envoy/envoy.go Outdated
@kisunji kisunji force-pushed the kisunji/NET-1767-envoy-grpc-disabled branch from f77c705 to 6df7ad4 Compare December 16, 2022 21:34
@kisunji kisunji force-pushed the kisunji/NET-1767-envoy-grpc-disabled branch from 6df7ad4 to 1788be7 Compare December 19, 2022 16:59
@vercel
Copy link
Copy Markdown

vercel Bot commented Dec 19, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
consul-ui-staging 🔄 Building (Inspect) Dec 19, 2022 at 4:59PM (UTC)

@kisunji kisunji merged commit f7b7f5d into main Dec 19, 2022
@kisunji kisunji deleted the kisunji/NET-1767-envoy-grpc-disabled branch December 19, 2022 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

theme/cli Flags and documentation for the CLI interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants