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.

Sign up for GitHub

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

Jump to bottom

Support Consul Ent NS's for CRDs #323

Merged
merged 5 commits into from
Sep 14, 2020
Merged

Support Consul Ent NS's for CRDs #323

merged 5 commits into from
Sep 14, 2020

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Sep 10, 2020 •
edited
Loading

Changes proposed in this PR:

  • Support Consul Enterprise by registering resources into consul namespaces. Supports same settings as connect inject.
  • Refactors some namespace code copied across the codebase into a new package namespaces.
  • companion to Update ACLs for consul ent and controller #322

How I've tested this PR: Ran the acceptance tests from hashicorp/consul-helm#601 which tests with secure installation and a Docker image made with a merge of this branch and #322.

How I expect reviewers to test this PR: You could run the acceptance tests yourselves or review the ones I wrote and see that CI/CD passes for them.

Checklist:

  • Tests added

@lkysow lkysow force-pushed the crd-controller-consul-ent branch 2 times, most recently from db06113 to 88b33d1 Compare September 10, 2020 00:10
@lkysow lkysow force-pushed the crd-controller-consul-ent branch 2 times, most recently from 895fab2 to d411636 Compare September 10, 2020 00:25
@lkysow lkysow mentioned this pull request Sep 10, 2020
Merged
1 task
@lkysow lkysow requested review from thisisnotashwin, a team and ishustava and removed request for a team September 10, 2020 23:57
@lkysow lkysow marked this pull request as ready for review September 10, 2020 23:57
@lkysow lkysow mentioned this pull request Sep 10, 2020
Merged
thisisnotashwin
thisisnotashwin reviewed Sep 11, 2020
View reviewed changes
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

I can see the incoming merge conflicts but this looks great!!

controllers/servicedefaults_controller.go Show resolved Hide resolved
controllers/servicedefaults_controller.go Outdated Show resolved Hide resolved
controllers/servicedefaults_controller.go Show resolved Hide resolved
controllers/servicedefaults_controller.go Outdated Show resolved Hide resolved
controllers/servicedefaults_controller.go Show resolved Hide resolved
namespaces/namespaces.go Show resolved Hide resolved
ishustava
ishustava approved these changes Sep 12, 2020
View reviewed changes
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!! A couple of comments, but nothing blocking.

controllers/servicedefaults_controller.go Show resolved Hide resolved
namespaces/namespaces_test.go Outdated Show resolved Hide resolved
namespaces/namespaces_test.go Outdated Show resolved Hide resolved
subcommand/controller/command.go Outdated Show resolved Hide resolved
thisisnotashwin
thisisnotashwin approved these changes Sep 14, 2020
View reviewed changes
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

🦊

@lkysow lkysow merged commit 2092f5d into crd-controller-base Sep 14, 2020
@lkysow lkysow deleted the crd-controller-consul-ent branch September 14, 2020 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thisisnotashwin thisisnotashwin thisisnotashwin approved these changes

@ishustava ishustava ishustava approved these changes

Assignees
No one assigned
Labels
theme/consul-namespaces theme/crds type/enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lkysow @ishustava @thisisnotashwin