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

feat(balancer) upstream healthcheck headers #8255

Merged
merged 3 commits into from
Feb 18, 2022

Conversation

mayocream
Copy link
Contributor

@mayocream mayocream commented Jan 2, 2022

Summary

Add option to set upstream active healthcheck HTTP headers.

see:

Full changelog

  • Add an option to set healthcheck HTTP headers
  • Add related tests

Issues resolved

ref:

FT-2195

@@ -73,6 +73,14 @@ local check_verify_certificate = Schema.define {
required = true,
}

local check_headers = Schema.define {
type = "array",
Copy link
Member

Choose a reason for hiding this comment

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

I saw the unique=true in array, so was just thinking should this rather be a set then? Is the order a requirement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, didn't notice that.
I'd like to replace this custom type with typedef.headers.

@mayocream mayocream force-pushed the feat/upstream_healthcheck_headers branch from bd64c8b to fe2beba Compare January 19, 2022 08:56
@mayocream mayocream force-pushed the feat/upstream_healthcheck_headers branch from fe2beba to 2bdcc6b Compare January 19, 2022 08:57
@mayocream
Copy link
Contributor Author

Thanks @flrgh, I've created following PR to lua-resty-heathcheck

@mayocream
Copy link
Contributor Author

The PR to upstream lib is merged, we will be waiting for lua-resty-heathcheck 1.5.x release.

@mayocream
Copy link
Contributor Author

The lua-resty-healthcheck version is bumped to 1.5.0, 4ceda40

It's ready to merge.

Copy link
Contributor

@flrgh flrgh left a comment

Choose a reason for hiding this comment

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

👍 lgtm!

I think it would be nice to see an integration test case using an upstream with healthcheck headers (even if we're just doubling up on testing the lua-resty-healthcheck API at that point), but IMO this is perfectly good-to-go as-is.

(Actually... @Hayk-S I wonder if the upstream healthcheck headers case would be a good candidate for E2E testing?)

@Hayk-S
Copy link

Hayk-S commented Feb 15, 2022

@flrgh I think there will be no harm from additional tests. I will probably get back to you for more details on this.

@mayocream mayocream merged commit 81f71fb into master Feb 18, 2022
@mayocream mayocream deleted the feat/upstream_healthcheck_headers branch February 18, 2022 02:47
mayocream added a commit that referenced this pull request Aug 15, 2022
aboudreault pushed a commit that referenced this pull request Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants