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

fix: missing UDP field in checkType #14885

Merged
merged 3 commits into from Oct 5, 2022
+26 −2
Merged

fix: missing UDP field in checkType #14885

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5140958
fix: missing UDP field in checkType
huikang Oct 5, 2022
9e95656
Add changelog
huikang Oct 5, 2022
209aafe
Update doc
huikang Oct 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions .changelog/14885.txt
View file
Open in desktop
@@ -0,0 +1,4 @@
```release-note:bug
checks: Fixed a bug that prevented registration of UDP health checks from agent configuration files, such as service definition files with embedded health check definitions.
```

1 change: 1 addition & 0 deletions agent/config/builder.go
View file
Open in desktop
Expand Up @@ -1562,6 +1562,7 @@ func (b *builder) checkVal(v *CheckDefinition) *structs.CheckDefinition {
Body: stringVal(v.Body),
DisableRedirects: boolVal(v.DisableRedirects),
TCP: stringVal(v.TCP),
UDP: stringVal(v.UDP),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see agent/config/builder.go mentioned in the developer docs that type to capture all the places that need to be checked for health check changes. Can you mention the config builder file? Maybe it can just be included in lines 4 and 5 (which both reference agent/config/config.go, which I think rely on this builder.go file).

Copy link
Contributor

Choose a reason for hiding this comment

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

I realize I didn't link to the dev docs I was referencing 🤦

https://github.com/hashicorp/consul/blob/main/docs/service-discovery/health-checks.md#check-registration-flows

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jkirschner-hashicorp , thanks. The doc is updated; please take a look.

Interval: b.durationVal(fmt.Sprintf("check[%s].interval", id), v.Interval),
DockerContainerID: stringVal(v.DockerContainerID),
Shell: stringVal(v.Shell),
Expand Down
18 changes: 18 additions & 0 deletions agent/config/builder_test.go
View file
Open in desktop
Expand Up @@ -326,6 +326,24 @@ func TestBuilder_ServiceVal_MultiError(t *testing.T) {
require.Contains(t, b.err.Error(), "cannot have both socket path")
}

func TestBuilder_ServiceVal_with_Check(t *testing.T) {
b := builder{}
svc := b.serviceVal(&ServiceDefinition{
Name: strPtr("unbound"),
ID: strPtr("unbound"),
Port: intPtr(12345),
Checks: []CheckDefinition{
{
Interval: strPtr("5s"),
UDP: strPtr("localhost:53"),
},
},
})
require.NoError(t, b.err)
require.Equal(t, 1, len(svc.Checks))
require.Equal(t, "localhost:53", svc.Checks[0].UDP)
}

func intPtr(v int) *int {
return &v
}
Expand Down