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

ServiceRouter support #332

Merged
merged 2 commits into from
Sep 29, 2020
Merged

ServiceRouter support #332

merged 2 commits into from
Sep 29, 2020

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Sep 24, 2020 •
edited
Loading

Changes proposed in this PR:

  • Add support for service-router
  • adds missing tests to servicedefaults and serviceresolver config entries
  • fixes nil pointer exception in SyncedCondition() and SyncedConditionStatus() methods
  • fixes controller bug added during refactoring where we wouldn't update the condition to synced if the object existed in Consul and hadn't changed but its condition was set to unknown (which could occur if the write to consul succeeded but we got an error for some reason)
  • added some logging and fields to controller

How I've tested this PR:

How I expect reviewers to test this PR:

  • A code review should be sufficient given there are integration tests

Checklist:

  • Tests added

@lkysow lkysow force-pushed the crd-controller-router branch 5 times, most recently from 3bc8f60 to 2eb9fa3 Compare September 24, 2020 16:37
lkysow
lkysow commented Sep 24, 2020
View reviewed changes
controllers/configentry_controller.go
return r.syncSuccessful(ctx, crdCtrl, configEntry)
} else if configEntry.SyncedConditionStatus() == corev1.ConditionTrue {
} else if configEntry.SyncedConditionStatus() != corev1.ConditionTrue {
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a bug that was introduced during refactoring. I've fixed it here and added a test for it.

@lkysow lkysow marked this pull request as ready for review September 24, 2020 16:56
@lkysow lkysow mentioned this pull request Sep 24, 2020
Merged
@lkysow lkysow force-pushed the crd-controller-router branch 2 times, most recently from 10a84f0 to d50ab58 Compare September 24, 2020 18:10
thisisnotashwin
thisisnotashwin reviewed Sep 25, 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.

This looks great. Rebasing with master might require a little bit of work to fold in some of these renames into ProxyDefaults as well unfortunately

controllers/configentry_controller.go Show resolved Hide resolved
api/v1alpha1/servicerouter_types.go Outdated Show resolved Hide resolved
controllers/configentry_controller.go Show resolved Hide resolved
controllers/configentry_controller_test.go
},
Spec: v1alpha1.ServiceDefaultsSpec{
Protocol: "http",
},
Copy link
Contributor

Choose a reason for hiding this comment

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

We could explicitly set the Synced status here to unknown? It makes the test case feel more explicit. Also, you might need to add a case for proxydefaults

subcommand/controller/command.go Outdated
if err = (&controllers.ServiceRouterController{
ConfigEntryController: configEntryReconciler,
Client: mgr.GetClient(),
Log: ctrl.Log.WithName("controller").WithName("servicerouter"),
Copy link
Contributor

Choose a reason for hiding this comment

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

you can pull servicerouter into the common package which should be available once you rebase crd-controller-base
ProxyDefaults potentially still says "controllers" as well

@lkysow lkysow force-pushed the crd-controller-router branch from d50ab58 to 822f15e Compare September 25, 2020 20:08
lkysow
lkysow commented Sep 25, 2020
View reviewed changes
api/v1alpha1/servicedefaults_types.go
@@ -94,11 +92,18 @@ func (in *ServiceDefaults) SetSyncedCondition(status corev1.ConditionStatus, rea

func (in *ServiceDefaults) SyncedCondition() (status corev1.ConditionStatus, reason string, message string) {
cond := in.Status.GetCondition(ConditionSynced)
if cond == nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing nil pointer exception

lkysow
lkysow commented Sep 25, 2020
View reviewed changes
config/crd/bases/consul.hashicorp.com_proxydefaults.yaml
@@ -27,85 +27,66 @@ spec:
description: ProxyDefaults is the Schema for the proxydefaults API
Copy link
Member Author

Choose a reason for hiding this comment

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

The yaml files are autogenerated so no need to review. The descriptions come from the struct tags so they can be reviewed there.

lkysow
lkysow commented Sep 25, 2020
View reviewed changes
connect-inject/handler.go
@@ -371,7 +371,7 @@ func (h *Handler) Mutate(req *v1beta1.AdmissionRequest) *v1beta1.AdmissionRespon
// all patches are created to guarantee no errors were encountered in
// that process before modifying the Consul cluster.
if h.EnableNamespaces {
if err := namespaces.EnsureExists(h.ConsulClient, h.consulNamespace(req.Namespace), h.CrossNamespaceACLPolicy); err != nil {
if _, err := namespaces.EnsureExists(h.ConsulClient, h.consulNamespace(req.Namespace), h.CrossNamespaceACLPolicy); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

EnsureExists now returns (bool, err) so we can log when a namespace gets created (vs already existed)

lkysow
lkysow commented Sep 25, 2020
View reviewed changes
controllers/configentry_controller_test.go
@@ -140,6 +181,11 @@ func TestConfigEntryControllers_createsConfigEntry(t *testing.T) {
Address: consul.HTTPAddr,
})
req.NoError(err)
if c.consulPrereq != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

Needed to add consulPrereq to all the tests because service-router (and service-splitter) require the protocol of the service to be set (which requires service-defaults)

thisisnotashwin
thisisnotashwin approved these changes Sep 25, 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.

It looks like we are inconsistent in the usage of "controllers" and "controller" in controller/command.go. A quick cmd+f cmd+r should fix that. Looks beautiful otherwise.

@lkysow
Copy link
Member Author

lkysow commented Sep 25, 2020

It looks like we are inconsistent in the usage of "controllers" and "controller" in controller/command.go. A quick cmd+f cmd+r should fix that. Looks beautiful otherwise.

Nice catch, I switched to controller to match the internal logging (I think, I don't remember but I had a good reason)

@lkysow lkysow requested review from a team and kschoche and removed request for a team September 28, 2020 17:10
@ishustava ishustava mentioned this pull request Sep 28, 2020
Merged
1 task
kschoche
kschoche approved these changes Sep 29, 2020
View reviewed changes
Copy link
Contributor

@kschoche kschoche 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!!

@lkysow lkysow merged commit 90b9c80 into crd-controller-base Sep 29, 2020
@lkysow lkysow deleted the crd-controller-router branch September 29, 2020 16:35
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

@kschoche kschoche kschoche approved these changes

Assignees
No one assigned
Labels
theme/crds
Projects
None yet
Milestone
No milestone
Development

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