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

Add a field to disable following redirects on http checks #12685

Merged
merged 1 commit into from Apr 7, 2022

Conversation

kyhavlov
Copy link
Contributor

@kyhavlov kyhavlov commented Apr 1, 2022

This PR adds a new option for HTTP checks, disable_redirects, that allows for disabling the following of redirects for HTTP checks. The intention is to default this to true in a future release so that redirects must explicitly be enabled.

@kyhavlov kyhavlov requested a review from a team April 1, 2022 22:19
@kyhavlov kyhavlov requested a review from a team as a code owner April 1, 2022 22:19
@github-actions github-actions bot added theme/config Relating to Consul Agent configuration, including reloading type/docs Documentation needs to be created/updated/clarified labels Apr 1, 2022
@hashicorp-ci
Copy link
Contributor

🤔 This PR has changes in the website/ directory but does not have a type/docs-cherrypick label. If the changes are for the next version, this can be ignored. If they are updates to current docs, attach the label to auto cherrypick to the stable-website branch after merging.

Copy link
Contributor

@markan markan left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -206,6 +206,9 @@ The table below shows this endpoint's support for

- `Body` `(string: "")` - Specifies a body that should be sent with `HTTP` checks.

- `DisableRedirects` `(bool: false)` - Specifies whether to disable following HTTP
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want caveat about the future change in the default here as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want caveat about the future change in the default here as well?

Friends, no. As a general technical communication principle, we describe current functionality and never future functionality. It potentially causes confusion for users and maintenance overhead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If our intent is to change the default later (which would be a breaking change for health checks that relied on redirects) shouldn't we document that beforehand? Or is a changelog note enough?

Copy link
Member

Choose a reason for hiding this comment

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

It might be better to state this in the 1.12 release notes. When the default value is changed in a later release, it should be called out as a breaking change in the changelog, and also mentioned in the upgrade guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok - it's in the changelog but I removed both occurrences of the note from the docs

Copy link
Contributor

@trujillo-adam trujillo-adam left a comment

Choose a reason for hiding this comment

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

From a docs perspective, we should stop referring to changes that will come later.

@@ -206,6 +206,9 @@ The table below shows this endpoint's support for

- `Body` `(string: "")` - Specifies a body that should be sent with `HTTP` checks.

- `DisableRedirects` `(bool: false)` - Specifies whether to disable following HTTP
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want caveat about the future change in the default here as well?

Friends, no. As a general technical communication principle, we describe current functionality and never future functionality. It potentially causes confusion for users and maintenance overhead.

Comment on lines 70 to 71
By default, Consul will follow HTTP redirects. This can be disabled by setting the
`disable_redirects` field to `true`. In a future release, this will default to true.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
By default, Consul will follow HTTP redirects. This can be disabled by setting the
`disable_redirects` field to `true`. In a future release, this will default to true.
Consul follows HTTP redirects by default. Set the
`disable_redirects` field to `true` to disable redirects.

@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 5, 2022 23:12 Inactive
@kyhavlov kyhavlov added this to the 1.12.0 milestone Apr 6, 2022
@kyhavlov kyhavlov added the backport/1.12 Changes are backported to 1.12 label Apr 7, 2022
Copy link
Contributor

@trujillo-adam trujillo-adam left a comment

Choose a reason for hiding this comment

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

👍

@kyhavlov kyhavlov merged commit 9780b67 into main Apr 7, 2022
@kyhavlov kyhavlov deleted the http-check-redirect-option branch April 7, 2022 18:29
@hc-github-team-consul-core
Copy link
Collaborator

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/630473.

@hc-github-team-consul-core
Copy link
Collaborator

🍒✅ Cherry pick of commit 9780b67 onto release/1.12.x succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request Apr 7, 2022
Add a field to disable following redirects on http checks
@hc-github-team-consul-core
Copy link
Collaborator

🍒❌ Cherry pick of commit 9780b67 onto release/1.11.x failed! Build Log

@hc-github-team-consul-core
Copy link
Collaborator

🍒❌ Cherry pick of commit 9780b67 onto release/1.10.x failed! Build Log

@hc-github-team-consul-core
Copy link
Collaborator

🍒❌ Cherry pick of commit 9780b67 onto release/1.9.x failed! Build Log

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.12 Changes are backported to 1.12 pr/no-metrics-test theme/config Relating to Consul Agent configuration, including reloading type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants