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 support of consul namespaces to health checks #376

Merged
merged 15 commits into from
Nov 5, 2020

Conversation

kschoche
Copy link
Contributor

@kschoche kschoche commented Nov 2, 2020

[Consul-ENT only]

Changes proposed in this PR:

  • connect-inject now adds a new annotation when Mutate is called "consul.hashicorp.com/consul-destination-namespace" when namespaces and mirroring are enabled.
  • health checks are created in namespaces if they are supported+used
  • existing unit tests are modified a bit to be able to re-use code between oss and consul-ent

How I've tested this PR:
unit tests have been added for the patch functionality as well as health checks resource.

How I expect reviewers to test this PR:
Unit tests.
Alternatively running and end to end test manually by:

  1. deploying with consul-helm consul-ent, along with
global:
  imageK8S: kschoche/consul-k8s-dev
  enableConsulNamespaces: true
  name: consul
  image: hashicorp/consul-enterprise:1.9.0-ent-beta1
  tls:
    enabled: true
server:
  enterpriseLicense:
    secretName: 'ent-license'
    secretKey: 'key'
connectInject:
  healthChecks:
    enabled: true
  enabled: true
  consulNamespaces:
    mirroringK8S: true
  1. Deploy a connect-injected application with k8s readiness probes into a k8s namespace

Checklist:

  • Tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

@kschoche kschoche added type/enhancement New feature or request theme/health-checks About Consul health checking theme/consul-namespaces labels Nov 2, 2020
@kschoche kschoche requested a review from a team November 2, 2020 16:51
@kschoche kschoche self-assigned this Nov 2, 2020
@kschoche kschoche requested review from lkysow and ishustava and removed request for a team November 2, 2020 16:51
Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

I tested this on my cluster with mirroring and it worked 💯 👌 .

Some suggestions around the tests and also the logic of when to add the annotation needs to be tweaked I believe.

connect-inject/handler.go Outdated Show resolved Hide resolved
connect-inject/handler.go Outdated Show resolved Hide resolved
connect-inject/handler.go Outdated Show resolved Hide resolved
connect-inject/health_check_resource.go Outdated Show resolved Hide resolved
connect-inject/handler_ent_test.go Outdated Show resolved Hide resolved
connect-inject/health_check_resource_ent_test.go Outdated Show resolved Hide resolved
connect-inject/health_check_resource_ent_test.go Outdated Show resolved Hide resolved
connect-inject/health_check_resource_test.go Outdated Show resolved Hide resolved
connect-inject/health_check_resource_test.go Outdated Show resolved Hide resolved
connect-inject/health_check_resource_ent_test.go Outdated Show resolved Hide resolved
kschoche and others added 3 commits November 4, 2020 09:43
Co-authored-by: Luke Kysow <1034429+lkysow@users.noreply.github.com>
Co-authored-by: Luke Kysow <1034429+lkysow@users.noreply.github.com>
Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

Changes look good. Note that I didn't rebuild the image.

connect-inject/handler.go Outdated Show resolved Hide resolved
connect-inject/health_check_resource_ent_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Looks good! I left a couple of comments, but they shouldn't block the merge.

@kschoche kschoche merged commit 0564d57 into master Nov 5, 2020
@kschoche kschoche deleted the health_checks_consul_ent branch November 5, 2020 20:51
ndhanushkodi pushed a commit to ndhanushkodi/consul-k8s that referenced this pull request Jul 9, 2021
…ations

Add custom annotation for service
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/consul-namespaces theme/health-checks About Consul health checking type/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants