Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tproxy: transparent_proxy reference docs #20241

Merged
merged 3 commits into from
Apr 3, 2024
Merged

tproxy: transparent_proxy reference docs #20241

merged 3 commits into from
Apr 3, 2024

Conversation

tgross
Copy link
Member

@tgross tgross commented Mar 27, 2024

@tgross tgross mentioned this pull request Mar 27, 2024
10 tasks
@tgross tgross force-pushed the f-tproxy-docs branch 2 times, most recently from ed3a075 to 9355ec6 Compare March 27, 2024 19:36
@tgross tgross changed the title tproxy: transparent_proxy docs tproxy: transparent_proxy reference docs Mar 27, 2024
@tgross tgross added this to the 1.8.0 milestone Mar 27, 2024
@tgross tgross added theme/consul/connect Consul Connect integration theme/docs Documentation issues and enhancements labels Mar 27, 2024
@tgross tgross marked this pull request as ready for review March 27, 2024 19:44
tgross added a commit that referenced this pull request Mar 28, 2024
Update the service mesh integration docs to explain how Consul needs to be
configured for transparent proxy. Update the walkthrough to assume that
`transparent_proxy` mode is the best approach, and move the manually-configured
`upstreams` to a separate section for users who don't want to use Consul DNS.

Ref: #20175
Ref: #20241
Copy link
Member

@gulducat gulducat left a comment

Choose a reason for hiding this comment

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

I love how you include the preview links in the PR description, very convenient!

I have a few comments and found a couple broken links. also the changeset includes a lot of actual code, which I don't think was intended for this docs PR?

aside from those things, LGTM!

api/consul.go Outdated
Comment on lines 170 to 171
TransparentProxy *ConsulTransparentProxy `mapstructure:"transparent_proxy" hcl:"transparent_proxy,block"`
Config map[string]interface{} `hcl:"config,block"`
Copy link
Member

Choose a reason for hiding this comment

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

just curious why Config made its way down here, away from its fellow uncommented fields above? is there a logic behind the field order here that I'm not seeing?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's no longer in this PR, but the Config block on all these Consul fields is a mushy "escape hatch", kind of like the task.config block. So by moving it to the bottom I'm trying to say "this is leftover". But I see how having a comment on only the field above it makes it look like these are paired now. I'll go back and add some docstrings to the rest.

website/content/docs/job-specification/proxy.mdx Outdated Show resolved Hide resolved
@tgross
Copy link
Member Author

tgross commented Apr 3, 2024

also the changeset includes a lot of actual code, which I don't think was intended for this docs PR?

Oops, I needed to rebase on the tproxy branch. Lemme fix that and then I'll address your remaining comments.

tgross added a commit that referenced this pull request Apr 3, 2024
Update the service mesh integration docs to explain how Consul needs to be
configured for transparent proxy. Update the walkthrough to assume that
`transparent_proxy` mode is the best approach, and move the manually-configured
`upstreams` to a separate section for users who don't want to use Consul DNS.

Ref: #20175
Ref: #20241
tgross added a commit that referenced this pull request Apr 3, 2024
)

Update the service mesh integration docs to explain how Consul needs to be
configured for transparent proxy. Update the walkthrough to assume that
`transparent_proxy` mode is the best approach, and move the manually-configured
`upstreams` to a separate section for users who don't want to use Consul DNS.

Ref: #20175
Ref: #20241
@tgross
Copy link
Member Author

tgross commented Apr 3, 2024

I've just merged the other docs PR, so I'll rebase on that and just take one more pass to make sure there are no broken links.

@tgross tgross merged commit ced955d into f-tproxy Apr 3, 2024
6 checks passed
@tgross tgross deleted the f-tproxy-docs branch April 3, 2024 20:31
tgross added a commit that referenced this pull request Apr 4, 2024
)

Update the service mesh integration docs to explain how Consul needs to be
configured for transparent proxy. Update the walkthrough to assume that
`transparent_proxy` mode is the best approach, and move the manually-configured
`upstreams` to a separate section for users who don't want to use Consul DNS.

Ref: #20175
Ref: #20241
tgross added a commit that referenced this pull request Apr 4, 2024
tgross added a commit that referenced this pull request Apr 4, 2024
)

Update the service mesh integration docs to explain how Consul needs to be
configured for transparent proxy. Update the walkthrough to assume that
`transparent_proxy` mode is the best approach, and move the manually-configured
`upstreams` to a separate section for users who don't want to use Consul DNS.

Ref: #20175
Ref: #20241
tgross added a commit that referenced this pull request Apr 4, 2024
philrenaud pushed a commit that referenced this pull request Apr 18, 2024
)

Update the service mesh integration docs to explain how Consul needs to be
configured for transparent proxy. Update the walkthrough to assume that
`transparent_proxy` mode is the best approach, and move the manually-configured
`upstreams` to a separate section for users who don't want to use Consul DNS.

Ref: #20175
Ref: #20241
philrenaud pushed a commit that referenced this pull request Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/consul/connect Consul Connect integration theme/docs Documentation issues and enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants