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

Expose method and expected status code on http checks #122

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Expose method and expected status code on http checks #122

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2cdd622
Expose method and expected status code on http checks
gkze Dec 22, 2023
dec74e8
Address (some) PR comments
gkze Jan 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/health/health_checks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func New(name string, probe Probe, onCheckEnd func(bool, bool, string)) (*Prober
}
return p, err
}
if probe.HttpGet != nil {
if probe.Http != nil {
err := p.addProber(p.getHttpChecker)
if err != nil {
return nil, err
Expand Down Expand Up @@ -102,13 +102,15 @@ func (p *Prober) addProber(factory func() (health.ICheckable, error)) error {
}

func (p *Prober) getHttpChecker() (health.ICheckable, error) {
url, err := p.probe.HttpGet.getUrl()
url, err := p.probe.Http.getUrl()
if err != nil {
return nil, err
}
checker, err := checkers.NewHTTP(&checkers.HTTPConfig{
URL: url,
Timeout: time.Duration(p.probe.TimeoutSeconds) * time.Second,
URL: url,
Timeout: time.Duration(p.probe.TimeoutSeconds) * time.Second,
Method: p.probe.Http.Method,
StatusCode: p.probe.Http.StatusCode,
})
if err != nil {
return nil, err
Expand Down
37 changes: 23 additions & 14 deletions src/health/probe.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@ package health

import (
"fmt"
"net/http"
"net/url"
"strings"
)

type Probe struct {
Exec *ExecProbe `yaml:"exec,omitempty"`
HttpGet *HttpProbe `yaml:"http_get,omitempty"`
Http *HttpProbe `yaml:"http,omitempty"`
Copy link
Owner

Choose a reason for hiding this comment

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

This breaks backward compatibility.
Do you think we can have both http and http_get fields at the same time?
Even nicer would be to use the deprecation warning:
https://github.com/F1bonacc1/process-compose/blob/main/src/types/deprecation.go

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, will do. Can you give an example of how this is done?

Copy link
Owner

Choose a reason for hiding this comment

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

Certainly.
Check out this commit as an example:
45fceb3

I mistakenly used on-failure instead of on_failure, once discovered I kept both of them:

  1. I issued a warning during the first month after deprecation.
  2. Error + 5s sleep during the second month
  3. Exit 1 after that.

I would go ahead and add the deprecationCheck to validators.go and assign the http_get values (if exist) to http (if not defined) in mutators.go.

If that's too many steps, let me know and I will add this content.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for the delay just getting back to this. I now realize that leaving both http_get and http we would need to make them mutually exclusive right? What should the behavior be in that case? I'm thinking failing parsing / validation if both of them are set, so that would mean changing the signature of Probe.validateAndSetDefaults(). Let me know your thoughts.

Copy link
Owner

Choose a reason for hiding this comment

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

IMO in case both of them are defined you can fail the loading in loader.validators.go.
Please let me know if you need any help with this feature.

InitialDelay int `yaml:"initial_delay_seconds,omitempty"`
PeriodSeconds int `yaml:"period_seconds,omitempty"`
TimeoutSeconds int `yaml:"timeout_seconds,omitempty"`
Expand All @@ -22,10 +23,12 @@ type ExecProbe struct {
}

type HttpProbe struct {
Host string `yaml:"host,omitempty"`
Path string `yaml:"path,omitempty"`
Scheme string `yaml:"scheme,omitempty"`
Port int `yaml:"port,omitempty"`
Host string `yaml:"host,omitempty"`
Path string `yaml:"path,omitempty"`
Scheme string `yaml:"scheme,omitempty"`
Port int `yaml:"port,omitempty"`
Method string `yaml:"method,omitempty"`
StatusCode int `yaml:"status_code,omitempty"`
}

func (h HttpProbe) getUrl() (*url.URL, error) {
Expand Down Expand Up @@ -60,20 +63,26 @@ func (p *Probe) validateAndSetDefaults() {
}

func (p *Probe) validateAndSetHttpDefaults() {
if p.HttpGet == nil {
if p.Http == nil {
return
}
if len(strings.TrimSpace(p.HttpGet.Host)) == 0 {
p.HttpGet.Host = "127.0.0.1"
if len(strings.TrimSpace(p.Http.Host)) == 0 {
p.Http.Host = "127.0.0.1"
}
if len(strings.TrimSpace(p.HttpGet.Scheme)) == 0 {
p.HttpGet.Scheme = "http"
if len(strings.TrimSpace(p.Http.Scheme)) == 0 {
p.Http.Scheme = "http"
}
if len(strings.TrimSpace(p.HttpGet.Path)) == 0 {
p.HttpGet.Path = "/"
if len(strings.TrimSpace(p.Http.Path)) == 0 {
p.Http.Path = "/"
}
if p.HttpGet.Port < 1 || p.HttpGet.Port > 65535 {
if p.Http.Port < 1 || p.Http.Port > 65535 {
// if undefined or wrong value - will be treated as undefined
p.HttpGet.Port = 0
p.Http.Port = 0
}
if len(strings.TrimSpace(p.Http.Method)) == 0 {
p.Http.Method = http.MethodGet
}
if p.Http.StatusCode == 0 {
p.Http.StatusCode = http.StatusOK
}
}
4 changes: 2 additions & 2 deletions src/health/probe_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
func TestProbe_validateAndSetDefaults(t *testing.T) {
type fields struct {
Exec *ExecProbe
HttpGet *HttpProbe
Http *HttpProbe
InitialDelay int
PeriodSeconds int
TimeoutSeconds int
Expand Down Expand Up @@ -71,7 +71,7 @@ func TestProbe_validateAndSetDefaults(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
p := &Probe{
Exec: tt.fields.Exec,
HttpGet: tt.fields.HttpGet,
Http: tt.fields.Http,
F1bonacc1 marked this conversation as resolved.
Show resolved Hide resolved
InitialDelay: tt.fields.InitialDelay,
PeriodSeconds: tt.fields.PeriodSeconds,
TimeoutSeconds: tt.fields.TimeoutSeconds,
Expand Down
10 changes: 5 additions & 5 deletions src/loader/merger_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func getBaseProcess() *types.ProcessConfig {
},
LivenessProbe: nil,
ReadinessProbe: &health.Probe{
HttpGet: &health.HttpProbe{
Http: &health.HttpProbe{
Host: "127.0.0.1",
Path: "/is",
Scheme: "http",
Expand Down Expand Up @@ -83,7 +83,7 @@ func getOverrideProcess() *types.ProcessConfig {
},
},
LivenessProbe: &health.Probe{
HttpGet: &health.HttpProbe{
Http: &health.HttpProbe{
Host: "google.com",
Path: "/isAlive",
Scheme: "https",
Expand All @@ -96,7 +96,7 @@ func getOverrideProcess() *types.ProcessConfig {
FailureThreshold: 5,
},
ReadinessProbe: &health.Probe{
HttpGet: &health.HttpProbe{
Http: &health.HttpProbe{
Host: "google.com",
Path: "/isAlive",
Scheme: "https",
Expand Down Expand Up @@ -150,7 +150,7 @@ func getMergedProcess() *types.ProcessConfig {
},
},
LivenessProbe: &health.Probe{
HttpGet: &health.HttpProbe{
Http: &health.HttpProbe{
Host: "google.com",
Path: "/isAlive",
Scheme: "https",
Expand All @@ -163,7 +163,7 @@ func getMergedProcess() *types.ProcessConfig {
FailureThreshold: 5,
},
ReadinessProbe: &health.Probe{
HttpGet: &health.HttpProbe{
Http: &health.HttpProbe{
Host: "google.com",
Path: "/isAlive",
Scheme: "https",
Expand Down
8 changes: 4 additions & 4 deletions src/loader/mutators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,9 +139,9 @@ func renderProbe(probe *health.Probe, tpl *templater.Templater, vars types.Vars)

if probe.Exec != nil {
probe.Exec.Command = tpl.RenderWithExtraVars(probe.Exec.Command, vars)
} else if probe.HttpGet != nil {
probe.HttpGet.Path = tpl.RenderWithExtraVars(probe.HttpGet.Path, vars)
probe.HttpGet.Host = tpl.RenderWithExtraVars(probe.HttpGet.Host, vars)
probe.HttpGet.Scheme = tpl.RenderWithExtraVars(probe.HttpGet.Scheme, vars)
} else if probe.Http != nil {
probe.Http.Path = tpl.RenderWithExtraVars(probe.Http.Path, vars)
probe.Http.Host = tpl.RenderWithExtraVars(probe.Http.Host, vars)
probe.Http.Scheme = tpl.RenderWithExtraVars(probe.Http.Scheme, vars)
F1bonacc1 marked this conversation as resolved.
Show resolved Hide resolved
}
}
16 changes: 8 additions & 8 deletions src/loader/mutators_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,14 +404,14 @@ func Test_renderTemplates(t *testing.T) {
Name: procNoWorkingDir,
Command: "echo {{ .TEST }}",
LivenessProbe: &health.Probe{
HttpGet: &health.HttpProbe{
Http: &health.HttpProbe{
Path: "/{{.PROJ_PATH}}/{{.PROC_PATH}}",
Scheme: "{{.SCHEME}}",
Host: "{{.HOST}}",
},
},
ReadinessProbe: &health.Probe{
HttpGet: &health.HttpProbe{
Http: &health.HttpProbe{
Path: "/{{.PROJ_PATH}}/{{.PROC_PATH}}",
Scheme: "{{.SCHEME}}",
Host: "{{.HOST}}",
Expand Down Expand Up @@ -466,12 +466,12 @@ func Test_renderTemplates(t *testing.T) {
compareStrings(t, "echo proc test", p.LivenessProbe.Exec.Command, "liveness probe command")
compareStrings(t, "echo <no value>", p.Command, "process command")
case "with http":
compareStrings(t, "/prj_path/proc_path", p.ReadinessProbe.HttpGet.Path, "readiness probe path")
compareStrings(t, "host", p.ReadinessProbe.HttpGet.Host, "readiness probe host")
compareStrings(t, "https", p.ReadinessProbe.HttpGet.Scheme, "readiness probe scheme")
compareStrings(t, "https", p.LivenessProbe.HttpGet.Scheme, "liveness probe scheme")
compareStrings(t, "host", p.LivenessProbe.HttpGet.Host, "liveness probe host")
compareStrings(t, "/prj_path/proc_path", p.LivenessProbe.HttpGet.Path, "liveness probe path")
compareStrings(t, "/prj_path/proc_path", p.ReadinessProbe.Http.Path, "readiness probe path")
compareStrings(t, "host", p.ReadinessProbe.Http.Host, "readiness probe host")
compareStrings(t, "https", p.ReadinessProbe.Http.Scheme, "readiness probe scheme")
compareStrings(t, "https", p.LivenessProbe.Http.Scheme, "liveness probe scheme")
compareStrings(t, "host", p.LivenessProbe.Http.Host, "liveness probe host")
compareStrings(t, "/prj_path/proc_path", p.LivenessProbe.Http.Path, "liveness probe path")
compareStrings(t, "echo test", p.Command, "process command")
}
}
Expand Down