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 ACL support for health checks controller #368

Merged
merged 13 commits into from
Nov 4, 2020

Conversation

kschoche
Copy link
Contributor

@kschoche kschoche commented Oct 27, 2020

Changes proposed in this PR:

  • server-acl-init will now add additional rules for the connect-inject-webhook when ACLs and health checks are enabled.
  • deprecate -create-inject-auth-method
  • deprecate -create-inject-namespace-token. This flag was always passed when -enable-namespaces was true so now we just look at-enable-namespaces and -create-inject-token
  • undeprecate -create-inject-token. This will always be passed by helm if connect inject is enabled.
  • move all namespaced server-acl-init/command_test.go tests that used -create-inject-namespace-token from oss to ent as these are now invalid for oss (they always were but they got away with being in oss because they weren't passing -enable-namespaces
  • tests to validate backwards compatibility are added and coverage for new flags.

How I've tested this PR:
unit tests + some manual testing of the command flags

How I expect reviewers to test this PR:
run the unit tests and try out the full stack by grabbing
hashicorp/consul-helm#665

This table summarizes what helm would pass with versions <= 0.25.0 (old) and > 0.25.0 (new):

Enable Namespaces Enable Healthchecks Flags Passed Auth Method Token Global Token
Old FALSE N/A -create-inject-auth-method Yes No N/A
TRUE N/A -create-inject-auth-method, -enable-namespaces, -create-inject-namespace-token Yes Yes Yes
New FALSE FALSE -create-inject-token Yes No N/A
TRUE FALSE -create-inject-token, -enable-namespaces Yes Yes Yes
TRUE TRUE -create-inject-token, -enable-namespaces, -enable-healthchecks Yes Yes Yes
FALSE TRUE -create-inject-token, -enable-healthchecks Yes Yes No

Checklist:

  • Tests added
  • CHANGELOG entry added - this will be part of the full health checks changelog entry

@kschoche kschoche added type/enhancement New feature or request area/acls Related to ACLs theme/health-checks About Consul health checking labels Oct 27, 2020
@kschoche kschoche requested a review from a team October 27, 2020 14:09
@kschoche kschoche self-assigned this Oct 27, 2020
@kschoche kschoche requested review from lkysow and ishustava and removed request for a team October 27, 2020 14:09
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 great! I've added some comments and suggestions, but I don't think they are blocking the merge.

subcommand/server-acl-init/command.go Outdated Show resolved Hide resolved
subcommand/server-acl-init/command_ent_test.go Outdated Show resolved Hide resolved
subcommand/server-acl-init/command_ent_test.go Outdated Show resolved Hide resolved
subcommand/server-acl-init/command_test.go Outdated Show resolved Hide resolved
subcommand/server-acl-init/rules.go Show resolved Hide resolved
subcommand/server-acl-init/command.go Outdated Show resolved Hide resolved
subcommand/server-acl-init/command.go Outdated Show resolved Hide resolved
subcommand/server-acl-init/command.go Outdated Show resolved Hide resolved
kschoche and others added 2 commits November 2, 2020 13:29
Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
@lkysow lkysow mentioned this pull request Nov 2, 2020
2 tasks
@kschoche kschoche merged commit 66c2678 into master Nov 4, 2020
@kschoche kschoche deleted the health_checks_acl_support branch November 4, 2020 23:19
@kschoche kschoche mentioned this pull request Nov 11, 2020
1 task
ndhanushkodi pushed a commit to ndhanushkodi/consul-k8s that referenced this pull request Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/acls Related to ACLs 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