Skip to content

Commit

Permalink
liveness information should also be available at /health (besides /he…
Browse files Browse the repository at this point in the history
…alth/liveness)

to avoid breaking changes
  • Loading branch information
asalan316 committed May 24, 2023
1 parent f09bc70 commit f853498
Show file tree
Hide file tree
Showing 14 changed files with 98 additions and 33 deletions.
4 changes: 3 additions & 1 deletion jobs/golangapiserver/spec
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,9 @@ properties:
Hash-Value of the password used for basic access authentication to connect to the protected health-endpoints.
More secure alternative of setting the password. Set to `""` if you don't want to use it.
autoscaler.apiserver.health.unprotected_endpoints:
description: "List of all health-endpoints, that run _without_ basic access authentication.Valid endpoints are /debug/pprof, /health/liveness, /health/prometheus, /health/readiness"
description: |
List of all health-endpoints, that run _without_ basic access authentication.
Valid endpoints are /debug/pprof, /health, /health/liveness, /health/prometheus, /health/readiness
default: [] # protect everything
autoscaler.apiserver.health.readiness_enabled:
default: false
Expand Down
4 changes: 3 additions & 1 deletion jobs/metricsforwarder/spec
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,9 @@ properties:
description: "the password of health endpoint"
default: ''
autoscaler.metricsforwarder.health.unprotected_endpoints:
description: "List of all health-endpoints, that run _without_ basic access authentication.Valid endpoints are /debug/pprof, /health/liveness, /health/prometheus, /health/readiness"
description: |
List of all health-endpoints, that run _without_ basic access authentication.
Valid endpoints are /debug/pprof, /health, /health/liveness, /health/prometheus, /health/readiness
default: [] # protect everything
autoscaler.metricsforwarder.health.readiness_enabled:
default: false
Expand Down
4 changes: 3 additions & 1 deletion jobs/metricsgateway/spec
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,9 @@ properties:
description: "the password of health endpoint"
default: ''
autoscaler.metricsgateway.health.unprotected_endpoints:
description: "List of all health-endpoints, that run _without_ basic access authentication.Valid endpoints are /debug/pprof, /health/liveness, /health/prometheus, /health/readiness"
description: |
List of all health-endpoints, that run _without_ basic access authentication.
Valid endpoints are /debug/pprof, /health, /health/liveness, /health/prometheus, /health/readiness
default: [] # protect everything
autoscaler.metricsgateway.health.readiness_enabled:
default: false
Expand Down
4 changes: 3 additions & 1 deletion jobs/metricsserver/spec
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,9 @@ properties:
description: "the password of health endpoint"
default: ''
autoscaler.metricsserver.health.unprotected_endpoints:
description: "List of all health-endpoints, that run _without_ basic access authentication.Valid endpoints are /debug/pprof, /health/liveness, /health/prometheus, /health/readiness"
description: |
List of all health-endpoints, that run _without_ basic access authentication.
Valid endpoints are /debug/pprof, /health, /health/liveness, /health/prometheus, /health/readiness
default: [] # protect everything
autoscaler.metricsserver.health.readiness_enabled:
default: false
Expand Down
4 changes: 3 additions & 1 deletion jobs/operator/spec
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,9 @@ properties:
description: "the password of health endpoint"
default: ''
autoscaler.operator.health.unprotected_endpoints:
description: "List of all health-endpoints, that run _without_ basic access authentication.Valid endpoints are /debug/pprof, /health/liveness, /health/prometheus, /health/readiness"
description: |
List of all health-endpoints, that run _without_ basic access authentication.
Valid endpoints are /debug/pprof, /health, /health/liveness, /health/prometheus, /health/readiness
default: [] # protect everything
autoscaler.operator.health.readiness_enabled:
default: false
Expand Down
4 changes: 3 additions & 1 deletion jobs/scalingengine/spec
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,9 @@ properties:
description: "the password of health endpoint"
default: ''
autoscaler.scalingengine.health.unprotected_endpoints:
description: "List of all health-endpoints, that run _without_ basic access authentication.Valid endpoints are /debug/pprof, /health/liveness, /health/prometheus, /health/readiness"
description: |
List of all health-endpoints, that run _without_ basic access authentication.
Valid endpoints are /debug/pprof, /health, /health/liveness, /health/prometheus, /health/readiness
default: [] # protect everything
autoscaler.scalingengine.health.readiness_enabled:
default: false
Expand Down
2 changes: 1 addition & 1 deletion jobs/scheduler/spec
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ properties:
autoscaler.scheduler.health.unprotected_endpoints:
description: |
List of all health-endpoints, that run _without_ basic access authentication.
Valid endpoints are /health/liveness, /health/prometheus
Valid endpoints are /health, /health/liveness, /health/prometheus
default: [] # protect everything
autoscaler.changeloglock_timeout_seconds:
default: 180
Expand Down
44 changes: 31 additions & 13 deletions src/acceptance/api/basic_auth_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package api_test

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

Expand All @@ -10,8 +11,11 @@ import (

var _ = Describe("AutoScaler Health Endpoints with Basic Auth", func() {

urlfor := func(name string) func() string {
urlfor := func(name string, path string) func() string {
return func() string {
if path != "" {
healthURL = fmt.Sprintf("%s%s", cfg.ASApiEndpoint, path)
}
healthURL := strings.Replace(healthURL, cfg.ServiceName, cfg.ServiceName+"-"+name, 1)
return healthURL
}
Expand All @@ -20,12 +24,12 @@ var _ = Describe("AutoScaler Health Endpoints with Basic Auth", func() {
func(url func() string, statusCode func() int) {
Expect(Get(url())).To(Equal(statusCode()), "to get status code %d when getting %s", statusCode(), url())
},
Entry("API Server", urlfor("apiserver"), getStatus),
Entry("Eventgenerator", urlfor("eventgenerator"), getStatus),
Entry("Scaling Engine", urlfor("scalingengine"), getStatus),
Entry("Operator", urlfor("operator"), getStatus),
Entry("Metrics Forwarder", urlfor("metricsforwarder"), getStatus),
Entry("Scheduler", urlfor("scheduler"), getStatus),
Entry("API Server", urlfor("apiserver", ""), getStatus),
Entry("Eventgenerator", urlfor("eventgenerator", ""), getStatus),
Entry("Scaling Engine", urlfor("scalingengine", ""), getStatus),
Entry("Operator", urlfor("operator", ""), getStatus),
Entry("Metrics Forwarder", urlfor("metricsforwarder", ""), getStatus),
Entry("Scheduler", urlfor("scheduler", ""), getStatus),
)

DescribeTable("Basic Auth Credentials Provided",
Expand All @@ -34,12 +38,26 @@ var _ = Describe("AutoScaler Health Endpoints with Basic Auth", func() {
cfg.HealthEndpointsBasicAuthEnabled = true
Expect(Get(url())).To(Equal(statusCode()), "to get status code %d when getting %s", statusCode(), url())
},
Entry("API Server", urlfor("apiserver"), getStatus),
Entry("Eventgenerator", urlfor("eventgenerator"), getStatus),
Entry("Scaling Engine", urlfor("scalingengine"), getStatus),
Entry("Operator", urlfor("operator"), getStatus),
Entry("Metrics Forwarder", urlfor("metricsforwarder"), getStatus),
Entry("Scheduler", urlfor("scheduler"), getStatus),
Entry("API Server", urlfor("apiserver", ""), getStatus),
Entry("Eventgenerator", urlfor("eventgenerator", ""), getStatus),
Entry("Scaling Engine", urlfor("scalingengine", ""), getStatus),
Entry("Operator", urlfor("operator", ""), getStatus),
Entry("Metrics Forwarder", urlfor("metricsforwarder", ""), getStatus),
Entry("Scheduler", urlfor("scheduler", ""), getStatus),
)

DescribeTable("Liveness with Basic Auth Credentials Provided",

func(url func() string, statusCode func() int) {
cfg.HealthEndpointsBasicAuthEnabled = true
Expect(Get(url())).To(Equal(statusCode()), "to get status code %d when getting %s", statusCode(), url())
},
Entry("API Server", urlfor("apiserver", "/health"), getStatus),
Entry("Eventgenerator", urlfor("eventgenerator", "/health"), getStatus),
Entry("Scaling Engine", urlfor("scalingengine", "/health"), getStatus),
Entry("Operator", urlfor("operator", "/health"), getStatus),
Entry("Metrics Forwarder", urlfor("metricsforwarder", "/health"), getStatus),
Entry("Scheduler", urlfor("scheduler", "/health"), getStatus),
)
})

Expand Down
29 changes: 23 additions & 6 deletions src/autoscaler/healthendpoint/health_readiness_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,6 @@ var _ = Describe("Health Readiness", func() {
prometheus.NewRegistry(), func() time.Time { return *timesetter })
Expect(err).ShouldNot(HaveOccurred())
})

// TODO: Check that `NewHealthRouterWithBasicAuth` returns an error if and only if
// the configuration is bad, i.e.: protection needed but parameters for basic auth insufficient

Context("Authentication parameter checks", func() {
When("username and password are defined", func() {
BeforeEach(func() {
Expand Down Expand Up @@ -149,6 +145,18 @@ var _ = Describe("Health Readiness", func() {
End()
})
})
When("/health endpoint is called", func() {
It("should response with liveness", func() {
apitest.New().Debug().
Handler(healthRoute).
Get(routes.LivenessBasePath).
Expect(t).
Status(http.StatusOK).
Header("Content-Type", "application/json").
Body(`{"overall_status" : "UP", "checks" : [] }`).
End()
})
})
When("/health/readiness endpoint is called", func() {
It("should response OK", func() {
apitest.New().Debug().
Expand Down Expand Up @@ -362,6 +370,17 @@ var _ = Describe("Health Readiness", func() {
Body(`{"overall_status" : "UP", "checks" : [] }`).
End()
})
It("should have json response", func() {
apitest.New().
Handler(healthRoute).
Get(routes.LivenessBasePath).
BasicAuth("test-user-name", "test-user-password").
Expect(t).
Status(http.StatusOK).
Header("Content-Type", "application/json").
Body(`{"overall_status" : "UP", "checks" : [] }`).
End()
})
})

})
Expand All @@ -385,8 +404,6 @@ var _ = Describe("Health Readiness", func() {
End()
})
})
// TODO Q. The same endpoint, we would like to offer as with and without basic auth.
//look's complex
When("Default / endpoint is called", func() {
It("should require basic auth", func() {
apitest.New().
Expand Down
3 changes: 2 additions & 1 deletion src/autoscaler/healthendpoint/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ func NewHealthRouterWithBasicAuth(conf models.HealthConfig, healthCheckers []Che
func addLivelinessHandlers(conf models.HealthConfig, mainRouter *mux.Router, time func() time.Time,
authMiddleware *basicAuthenticationMiddleware) error {
livenessHandler := common.VarsFunc(readiness([]Checker{}, time))
livenessRouter := mainRouter.PathPrefix(routes.LivenessPath).Subrouter()
livenessRouter := mainRouter.PathPrefix(routes.LivenessBasePath).Subrouter()

if endpointsNeedsProtection(routes.LivenessPath, conf) {
if !conf.BasicAuthPossible() {
Expand All @@ -118,6 +118,7 @@ func addLivelinessHandlers(conf models.HealthConfig, mainRouter *mux.Router, tim
livenessRouter.Use(authMiddleware.middleware)
}
livenessRouter.Handle("", livenessHandler)
livenessRouter.Handle("/liveness", livenessHandler)

return nil
}
Expand Down
3 changes: 1 addition & 2 deletions src/autoscaler/routes/routes.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ const (
SyncActiveSchedulesPath = "/v1/syncSchedules"
SyncActiveSchedulesRouteName = "SyncActiveSchedules"

BrokerHealthPath = "/health"

EnvelopePath = "/v1/envelopes"
EnvelopeReportRouteName = "ReportEnvelope"
CustomMetricsPath = "/v1/apps/{appid}/metrics"
Expand All @@ -56,6 +54,7 @@ const (
PublicApiInfoPath = "/v1/info"
PublicApiInfoRouteName = "GetPublicApiInfo"
LivenessPath = "/health/liveness"
LivenessBasePath = "/health"
ReadinessPath = "/health/readiness"
PrometheusPath = "/health/prometheus"
PprofPath = "/debug/pprof"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public ResponseEntity<String> getPrometheusMetrics() throws IOException {
return new ResponseEntity<>(stream.toString(), HttpStatus.OK);
}

@GetMapping(value = "/liveness")
@GetMapping(value = {"", "/liveness"})
public ResponseEntity<Map<String, Object>> getLiveness() {
Map<String, Object> body = new HashMap<>();
body.put("status", "Up");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package org.cloudfoundry.autoscaler.scheduler.health;
package org.cloudfoundry.autoscaler.scheduler.rest.healthControllerTest;

import static org.assertj.core.api.Assertions.assertThat;

Expand Down Expand Up @@ -105,6 +105,20 @@ public void givenCorrectCredentialsLivenessShouldBeAvailable() {
assertThat(response.getBody()).isEqualTo("{\"status\":\"Up\"}");
}

@Test
public void givenCorrectCredentialsLivenessBaseUrlShouldBeAvailable() {

ResponseEntity<String> response =
this.restTemplate
.withBasicAuth("prometheus", "someHash")
.getForEntity(baseLivenessUrl(), String.class);
assertThat(response.getStatusCode().value())
.describedAs("Http status code should be OK")
.isEqualTo(200);
assertThat(response.getHeaders().getContentType()).isEqualTo(MediaType.APPLICATION_JSON);
assertThat(response.getBody()).isEqualTo("{\"status\":\"Up\"}");
}

@Test
public void givenNoCredentialsLivenessShouldReturn401() {

Expand All @@ -128,6 +142,10 @@ private String prometheusMetricsUrl() {
}

private String livenessUrl() {
return "http://localhost:" + healthServerConfig.getPort() + "/health/liveness";
return baseLivenessUrl() + "/liveness";
}

private String baseLivenessUrl() {
return "http://localhost:" + healthServerConfig.getPort() + "/health";
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package org.cloudfoundry.autoscaler.scheduler.health;
package org.cloudfoundry.autoscaler.scheduler.rest.healthControllerTest;

import static org.assertj.core.api.Assertions.assertThat;

Expand Down

0 comments on commit f853498

Please sign in to comment.