ConsulCatalog StrictChecks

[djenriquez]

What does this PR do?

This PR allows users to define a StrictChecks config for the consulcatalog provider. StrictChecks lets a user define which healthcheck statuses should be allowed when determining which upstreams to include in the upstream targets.

StrictChecks defaults to "passing", "warning" to preserve the existing behavior, allowing this config to be backwards compatible.

Fix #10074

Motivation

Our organization relies on "warning" health statuses to determine upstreams that are "starting up", which is a different state than "critical", but not yet ready for traffic. The existing build sends traffic immediately to "warning" upstreams, which causes 5XX errors for our use case.

With this config, users can choose what health status to allow based on the available statuses that Consul supports as their organization requires.

More

• Added/updated tests
• Added/updated documentation

Additional Notes

Question: will I need to add additional logic to properly handle the different configuration input methods or will the configuration handler automatically unmarshal the values into the provider?

[djenriquez]

Reference Issue: #10074

[jspdown]

Thanks a lot for opening this PR 😃

I have some feedback, nothing critical.

will I need to add additional logic to properly handle the different configuration input methods or will the configuration handler automatically unmarshal the values into the provider?

The configuration will be automatically unmarshaled, there's nothing else to add in the code. But for updating the reference part of the documentation you will have to run make generate.

[djenriquez]

The only thing I'm unsure of is whether the default values are applied when a user does not define strictChecks in their config, I might be missing a mapping somewhere for this new property. This will be critical to backwards compat, any ideas of the best way to test that or verify that it should work?

[djenriquez]

Hi all, checking in if there are other items or actions needed? Thanks!

[jspdown]

@djenriquez No, sorry for the delay. I couldn't allocate time to review your changes recently. I will have a look at it later today.

[youkoulayley]

Tested and works perfectly, just a few minor comments

[djenriquez]

Tested and works perfectly, just a few minor comments

Thanks for the review! Feedback applied 👍 .

[nmengin]

Thank you for your contribution 👍

LGTM

[mmatur]

LGTM

[jspdown]

Thanks a lot for this PR, LGTM 🚢

[mmatur]

@djenriquez could you please rebase your PR.

Our bot is not able to do it because the PR has been created from an GitHub Organisation fork.

[djenriquez]

@djenriquez could you please rebase your PR.

Our bot is not able to do it because the PR has been created from an GitHub Organisation fork.

Definitely, sorry for the delay. Thanks!