Skip to content

Commit

Permalink
support timeout retention on livez and readyz probe rewrite (fixes is…
Browse files Browse the repository at this point in the history 
…tio#18242)

* support timeout retention on livez and readyz probe rewrite (fixes istio#18242)

* fix incorrect error formatting

* fix tests for pilot-agent

* use internal struct for prober configuration
  • Loading branch information
@yp28 @dgn
yp28 authored and dgn committed Mar 3, 2020
1 parent 19cfec5 commit 2826c82
Show file tree
Hide file tree
Showing 13 changed files with 363 additions and 117 deletions.
10 changes: 5 additions & 5 deletions pilot/cmd/pilot-agent/main.go
View file
Expand Up @@ -506,11 +506,11 @@ var (
}
prober := kubeAppProberNameVar.Get()
statusServer, err := status.NewServer(status.Config{
LocalHostAddr: localHostAddr,
AdminPort: proxyAdminPort,
StatusPort: statusPort,
KubeAppHTTPProbers: prober,
NodeType: role.Type,
LocalHostAddr: localHostAddr,
AdminPort: proxyAdminPort,
StatusPort: statusPort,
KubeAppProbers: prober,
NodeType: role.Type,
})
if err != nil {
cancel()
Expand Down
46 changes: 27 additions & 19 deletions pilot/cmd/pilot-agent/status/server.go
View file
Expand Up @@ -45,7 +45,7 @@ const (
quitPath = "/quitquitquit"
// KubeAppProberEnvName is the name of the command line flag for pilot agent to pass app prober config.
// The json encoded string to pass app HTTP probe information from injector(istioctl or webhook).
// For example, ISTIO_KUBE_APP_PROBERS='{"/app-health/httpbin/livez":{"path": "/hello", "port": 8080}.
// For example, ISTIO_KUBE_APP_PROBERS='{"/app-health/httpbin/livez":{"httpGet":{"path": "/hello", "port": 8080}}.
// indicates that httpbin container liveness prober port is 8080 and probing path is /hello.
// This environment variable should never be set manually.
KubeAppProberEnvName = "ISTIO_KUBE_APP_PROBERS"
Expand All @@ -57,18 +57,24 @@ var (

// KubeAppProbers holds the information about a Kubernetes pod prober.
// It's a map from the prober URL path to the Kubernetes Prober config.
// For example, "/app-health/hello-world/livez" entry contains livenss prober config for
// For example, "/app-health/hello-world/livez" entry contains liveness prober config for
// container "hello-world".
type KubeAppProbers map[string]*corev1.HTTPGetAction
type KubeAppProbers map[string]*Prober

// Prober represents a single container prober
type Prober struct {
HTTPGet *corev1.HTTPGetAction `json:"httpGet"`
TimeoutSeconds int32 `json:"timeoutSeconds,omitempty"`
}

// Config for the status server.
type Config struct {
LocalHostAddr string
// KubeAppHTTPProbers is a json with Kubernetes application HTTP prober config encoded.
KubeAppHTTPProbers string
NodeType model.NodeType
StatusPort uint16
AdminPort uint16
// KubeAppProbers is a json with Kubernetes application prober config encoded.
KubeAppProbers string
NodeType model.NodeType
StatusPort uint16
AdminPort uint16
}

// Server provides an endpoint for handling status probes.
Expand All @@ -90,18 +96,21 @@ func NewServer(config Config) (*Server, error) {
NodeType: config.NodeType,
},
}
if config.KubeAppHTTPProbers == "" {
if config.KubeAppProbers == "" {
return s, nil
}
if err := json.Unmarshal([]byte(config.KubeAppHTTPProbers), &s.appKubeProbers); err != nil {
return nil, fmt.Errorf("failed to decode app http prober err = %v, json string = %v", err, config.KubeAppHTTPProbers)
if err := json.Unmarshal([]byte(config.KubeAppProbers), &s.appKubeProbers); err != nil {
return nil, fmt.Errorf("failed to decode app prober err = %v, json string = %v", err, config.KubeAppProbers)
}
// Validate the map key matching the regex pattern.
for path, prober := range s.appKubeProbers {
if !appProberPattern.Match([]byte(path)) {
return nil, fmt.Errorf(`invalid key, must be in form of regex pattern ^/app-health/[^\/]+/(livez|readyz)$`)
}
if prober.Port.Type != intstr.Int {
if prober.HTTPGet == nil {
return nil, fmt.Errorf(`invalid prober type, must be of type httpGet`)
}
if prober.HTTPGet.Port.Type != intstr.Int {
return nil, fmt.Errorf("invalid prober config for %v, the port must be int type", path)
}
}
Expand Down Expand Up @@ -216,19 +225,18 @@ func (s *Server) handleAppProbe(w http.ResponseWriter, req *http.Request) {

// Construct a request sent to the application.
httpClient := &http.Client{
// TODO: figure out the appropriate timeout?
Timeout: 10 * time.Second,
Timeout: time.Duration(prober.TimeoutSeconds) * time.Second,
// We skip the verification since kubelet skips the verification for HTTPS prober as well
// https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-probes/#configure-probes
Transport: &http.Transport{
TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
},
}
var url string
if prober.Scheme == corev1.URISchemeHTTPS {
url = fmt.Sprintf("https://localhost:%v%s", prober.Port.IntValue(), prober.Path)
if prober.HTTPGet.Scheme == corev1.URISchemeHTTPS {
url = fmt.Sprintf("https://localhost:%v%s", prober.HTTPGet.Port.IntValue(), prober.HTTPGet.Path)
} else {
url = fmt.Sprintf("http://localhost:%v%s", prober.Port.IntValue(), prober.Path)
url = fmt.Sprintf("http://localhost:%v%s", prober.HTTPGet.Port.IntValue(), prober.HTTPGet.Path)
}
appReq, err := http.NewRequest("GET", url, nil)
if err != nil {
Expand All @@ -244,7 +252,7 @@ func (s *Server) handleAppProbe(w http.ResponseWriter, req *http.Request) {
appReq.Header[name] = newValues
}

for _, h := range prober.HTTPHeaders {
for _, h := range prober.HTTPGet.HTTPHeaders {
if h.Name == "Host" || h.Name == ":authority" {
// Probe has specific host header override; honor it
appReq.Host = h.Value
Expand All @@ -255,7 +263,7 @@ func (s *Server) handleAppProbe(w http.ResponseWriter, req *http.Request) {
// Send the request.
response, err := httpClient.Do(appReq)
if err != nil {
log.Errorf("Request to probe app failed: %v, original URL path = %v\napp URL path = %v", err, path, prober.Path)
log.Errorf("Request to probe app failed: %v, original URL path = %v\napp URL path = %v", err, path, prober.HTTPGet.Path)
w.WriteHeader(http.StatusInternalServerError)
return
}
Expand Down
52 changes: 31 additions & 21 deletions pilot/cmd/pilot-agent/status/server_test.go
View file
Expand Up @@ -44,56 +44,66 @@ func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {

func TestNewServer(t *testing.T) {
testCases := []struct {
httpProbe string
err string
probe string
err string
}{
// Json can't be parsed.
{
httpProbe: "invalid-prober-json-encoding",
err: "failed to decode",
probe: "invalid-prober-json-encoding",
err: "failed to decode",
},
// map key is not well formed.
{
httpProbe: `{"abc": {"path": "/app-foo/health"}}`,
err: "invalid key",
probe: `{"abc": {"path": "/app-foo/health"}}`,
err: "invalid key",
},
// invalid probe type
{
probe: `{"/app-health/hello-world/readyz": {"tcpSocket": {"port": "8888"}}}`,
err: "invalid prober type",
},
// Port is not Int typed.
{
httpProbe: `{"/app-health/hello-world/readyz": {"path": "/hello/sunnyvale", "port": "container-port-dontknow"}}`,
err: "must be int type",
probe: `{"/app-health/hello-world/readyz": {"httpGet": {"path": "/hello/sunnyvale", "port": "container-port-dontknow"}}}`,
err: "must be int type",
},
// A valid input.
{
httpProbe: `{"/app-health/hello-world/readyz": {"path": "/hello/sunnyvale", "port": 8080},` +
`"/app-health/business/livez": {"path": "/buisiness/live", "port": 9090}}`,
probe: `{"/app-health/hello-world/readyz": {"httpGet": {"path": "/hello/sunnyvale", "port": 8080}},` +
`"/app-health/business/livez": {"httpGet": {"path": "/buisiness/live", "port": 9090}}}`,
},
// long request timeout
{
probe: `{"/app-health/hello-world/readyz": {"httpGet": {"path": "/hello/sunnyvale", "port": 8080},` +
`"initialDelaySeconds": 120,"timeoutSeconds": 10,"periodSeconds": 20}}`,
},
// A valid input with empty probing path, which happens when HTTPGetAction.Path is not specified.
{
httpProbe: `{"/app-health/hello-world/readyz": {"path": "/hello/sunnyvale", "port": 8080},
"/app-health/business/livez": {"port": 9090}}`,
probe: `{"/app-health/hello-world/readyz": {"httpGet": {"path": "/hello/sunnyvale", "port": 8080}},
"/app-health/business/livez": {"httpGet": {"port": 9090}}}`,
},
// A valid input without any prober info.
{
httpProbe: `{}`,
probe: `{}`,
},
}
for _, tc := range testCases {
_, err := NewServer(Config{
KubeAppHTTPProbers: tc.httpProbe,
KubeAppProbers: tc.probe,
})

if err == nil {
if tc.err != "" {
t.Errorf("test case failed [%v], expect error %v", tc.httpProbe, tc.err)
t.Errorf("test case failed [%v], expect error %v", tc.probe, tc.err)
}
continue
}
if tc.err == "" {
t.Errorf("test case failed [%v], expect no error, got %v", tc.httpProbe, err)
t.Errorf("test case failed [%v], expect no error, got %v", tc.probe, err)
}
// error case, error string should match.
if !strings.Contains(err.Error(), tc.err) {
t.Errorf("test case failed [%v], expect error %v, got %v", tc.httpProbe, tc.err, err)
t.Errorf("test case failed [%v], expect error %v, got %v", tc.probe, tc.err, err)
}
}
}
Expand All @@ -110,8 +120,8 @@ func TestAppProbe(t *testing.T) {
// Starts the pilot agent status server.
server, err := NewServer(Config{
StatusPort: 0,
KubeAppHTTPProbers: fmt.Sprintf(`{"/app-health/hello-world/readyz": {"path": "/hello/sunnyvale", "port": %v},
"/app-health/hello-world/livez": {"port": %v}}`, appPort, appPort),
KubeAppProbers: fmt.Sprintf(`{"/app-health/hello-world/readyz": {"httpGet": {"path": "/hello/sunnyvale", "port": %v}},
"/app-health/hello-world/livez": {"httpGet":{"port": %v}}}`, appPort, appPort),
})
if err != nil {
t.Errorf("failed to create status server %v", err)
Expand Down Expand Up @@ -175,8 +185,8 @@ func TestHttpsAppProbe(t *testing.T) {
// Starts the pilot agent status server.
server, err := NewServer(Config{
StatusPort: 0,
KubeAppHTTPProbers: fmt.Sprintf(`{"/app-health/hello-world/readyz": {"path": "/hello/sunnyvale", "port": %v, "scheme": "HTTPS"},
"/app-health/hello-world/livez": {"port": %v, "scheme": "HTTPS"}}`, appPort, appPort),
KubeAppProbers: fmt.Sprintf(`{"/app-health/hello-world/readyz": {"httpGet": {"path": "/hello/sunnyvale", "port": %v, "scheme": "HTTPS"}},
"/app-health/hello-world/livez": {"httpGet": {"port": %v, "scheme": "HTTPS"}}}`, appPort, appPort),
})
if err != nil {
t.Errorf("failed to create status server %v", err)
Expand Down
75 changes: 45 additions & 30 deletions pkg/kube/inject/app_probe.go
View file
Expand Up @@ -104,41 +104,40 @@ func extractStatusPort(sidecar *corev1.Container) int {
return -1
}

// convertAppProber returns a overwritten `HTTPGetAction` for pilot agent to take over.
func convertAppProber(probe *corev1.Probe, newURL string, statusPort int) *corev1.HTTPGetAction {
// convertAppProber returns an overwritten `Probe` for pilot agent to take over.
func convertAppProber(probe *corev1.Probe, newURL string, statusPort int) *corev1.Probe {
if probe == nil || probe.HTTPGet == nil {
return nil
}
c := probe.HTTPGet.DeepCopy()
p := probe.DeepCopy()
// Change the application container prober config.
c.Port = intstr.FromInt(statusPort)
c.Path = newURL
p.HTTPGet.Port = intstr.FromInt(statusPort)
p.HTTPGet.Path = newURL
// For HTTPS prober, we change to HTTP,
// and pilot agent uses https to request application prober endpoint.
// Kubelet -> HTTP -> Pilot Agent -> HTTPS -> Application
if c.Scheme == corev1.URISchemeHTTPS {
c.Scheme = corev1.URISchemeHTTP
if p.HTTPGet.Scheme == corev1.URISchemeHTTPS {
p.HTTPGet.Scheme = corev1.URISchemeHTTP
}
return c
return p
}

// DumpAppProbers returns a json encoded string as `status.KubeAppProbers`.
// Also update the probers so that all usages of named port will be resolved to integer.
func DumpAppProbers(podspec *corev1.PodSpec) string {
out := status.KubeAppProbers{}
updateNamedPort := func(p *corev1.Probe, portMap map[string]int32) *corev1.HTTPGetAction {
updateNamedPort := func(p *status.Prober, portMap map[string]int32) *status.Prober {
if p == nil || p.HTTPGet == nil {
return nil
}
h := p.HTTPGet
if h.Port.Type == intstr.String {
port, exists := portMap[h.Port.StrVal]
if p.HTTPGet.Port.Type == intstr.String {
port, exists := portMap[p.HTTPGet.Port.StrVal]
if !exists {
return nil
}
h.Port = intstr.FromInt(int(port))
p.HTTPGet.Port = intstr.FromInt(int(port))
}
return h
return p
}
for _, c := range podspec.Containers {
if c.Name == ProxyContainerName {
Expand All @@ -151,10 +150,10 @@ func DumpAppProbers(podspec *corev1.PodSpec) string {
portMap[p.Name] = p.ContainerPort
}
}
if h := updateNamedPort(c.ReadinessProbe, portMap); h != nil {
if h := updateNamedPort(kubeProbeToInternalProber(c.ReadinessProbe), portMap); h != nil {
out[readyz] = h
}
if h := updateNamedPort(c.LivenessProbe, portMap); h != nil {
if h := updateNamedPort(kubeProbeToInternalProber(c.LivenessProbe), portMap); h != nil {
out[livez] = h
}
}
Expand Down Expand Up @@ -192,11 +191,11 @@ func rewriteAppHTTPProbe(annotations map[string]string, podSpec *corev1.PodSpec,
continue
}
readyz, livez := status.FormatProberURL(c.Name)
if hg := convertAppProber(c.ReadinessProbe, readyz, statusPort); hg != nil {
*c.ReadinessProbe.HTTPGet = *hg
if rp := convertAppProber(c.ReadinessProbe, readyz, statusPort); rp != nil {
*c.ReadinessProbe = *rp
}
if hg := convertAppProber(c.LivenessProbe, livez, statusPort); hg != nil {
*c.LivenessProbe.HTTPGet = *hg
if lp := convertAppProber(c.LivenessProbe, livez, statusPort); lp != nil {
*c.LivenessProbe = *lp
}
}
}
Expand All @@ -206,7 +205,7 @@ func createProbeRewritePatch(annotations map[string]string, podSpec *corev1.PodS
if !ShouldRewriteAppHTTPProbers(annotations, spec) {
return []rfc6902PatchOperation{}
}
patch := []rfc6902PatchOperation{}
podPatches := []rfc6902PatchOperation{}
sidecar := FindSidecar(spec.Containers)
if sidecar == nil {
return nil
Expand All @@ -226,20 +225,36 @@ func createProbeRewritePatch(annotations map[string]string, podSpec *corev1.PodS
portMap[p.Name] = p.ContainerPort
}
readyz, livez := status.FormatProberURL(c.Name)
if after := convertAppProber(c.ReadinessProbe, readyz, statusPort); after != nil {
patch = append(patch, rfc6902PatchOperation{
if probePatch := convertAppProber(c.ReadinessProbe, readyz, statusPort); probePatch != nil {
podPatches = append(podPatches, rfc6902PatchOperation{
Op: "replace",
Path: fmt.Sprintf("/spec/containers/%v/readinessProbe/httpGet", i),
Value: *after,
Path: fmt.Sprintf("/spec/containers/%v/readinessProbe", i),
Value: *probePatch,
})
}
if after := convertAppProber(c.LivenessProbe, livez, statusPort); after != nil {
patch = append(patch, rfc6902PatchOperation{
if probePatch := convertAppProber(c.LivenessProbe, livez, statusPort); probePatch != nil {
podPatches = append(podPatches, rfc6902PatchOperation{
Op: "replace",
Path: fmt.Sprintf("/spec/containers/%v/livenessProbe/httpGet", i),
Value: *after,
Path: fmt.Sprintf("/spec/containers/%v/livenessProbe", i),
Value: *probePatch,
})
}
}
return patch
return podPatches
}

// kubeProbeToInternalProber converts a Kubernetes Probe to an Istio internal Prober
func kubeProbeToInternalProber(probe *corev1.Probe) *status.Prober {
if probe == nil {
return nil
}

if probe.HTTPGet == nil {
return nil
}

return &status.Prober{
HTTPGet: probe.HTTPGet,
TimeoutSeconds: probe.TimeoutSeconds,
}
}
2 changes: 1 addition & 1 deletion ...e/inject/testdata/inject/app_probe/hello-probes-with-flag-set-in-annotation.yaml.injected
View file
Expand Up @@ -147,7 +147,7 @@ spec:
- name: ISTIO_META_MESH_ID
value: cluster.local
- name: ISTIO_KUBE_APP_PROBERS
value: '{"/app-health/hello/livez":{"port":80},"/app-health/hello/readyz":{"port":3333},"/app-health/world/livez":{"port":90}}'
value: '{"/app-health/hello/livez":{"httpGet":{"port":80}},"/app-health/hello/readyz":{"httpGet":{"port":3333}},"/app-health/world/livez":{"httpGet":{"port":90}}}'
image: gcr.io/istio-testing/proxyv2:latest
imagePullPolicy: IfNotPresent
name: istio-proxy
Expand Down

0 comments on commit 2826c82

Please sign in to comment.