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.

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

Implement Healthcheck status for keys loading #738

Merged
merged 24 commits into from
Mar 24, 2023

Conversation

usmansaleem
Copy link
Contributor

@usmansaleem usmansaleem commented Mar 14, 2023

PR Description

  • Implement Healthcheck status for keys loading.
  • Add AWS endpoint override cli option --aws-endpoint-override (useful for testing against localstack).

Sample healthcheck:

{
	"status": "UP",
	"checks": [
		{
			"id": "default-check",
			"status": "UP"
		},
		{
			"id": "keys-check",
			"status": "UP",
			"checks": [
				{
					"id": "aws-bulk-loading",
					"status": "UP",
					"data": {
						"keys-loaded": 2,
						"error-count": 0
					}
				},
				{
					"id": "config-files-loading",
					"status": "UP",
					"data": {
						"keys-loaded": 0,
						"error-count": 0
					}
				}
			]
		}
	],
	"outcome": "UP"
}

Fixed Issue(s)

#715
#730

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@usmansaleem usmansaleem marked this pull request as ready for review March 16, 2023 23:11
build.gradle Outdated Show resolved Hide resolved
promise -> {
final JsonObject statusJson =
new JsonObject()
.put("keys-loaded", results.getValues().size())
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these results updated when the keys are reloaded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, these will be re-evaluated during reload.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can a test be added for the reload case, I think have a test for reloading config so shouldn't be too tricky hopefully 🤞

@usmansaleem usmansaleem added TeamCerberus Under active development by TeamCerberus @Consensys doc-change-required Indicates an issue or PR that requires doc to be updated labels Mar 23, 2023
@usmansaleem usmansaleem requested a review from jframe March 23, 2023 06:41
.then()
.statusCode(200)
.contentType(ContentType.JSON)
.body("status", equalTo("UP"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, be good to have a test showing that checks have the appropriate status and error count values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it in AWS Secrets Manager AT.

final String publicKey = KEY_PAIR.getPublicKey().toString();

final AwsSecretsManagerUtil awsSecretsManagerUtil =
new AwsSecretsManagerUtil(region, rwAwsAccessKeyId, rwAwsSecretAccessKey);
new AwsSecretsManagerUtil(
region, rwAwsAccessKeyId, rwAwsSecretAccessKey, awsEndpointOverride);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this appropriate test or not but would be good to have a test case for config-files-loading check as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added healthcheck AT related to config-files-loading.

promise -> {
final JsonObject statusJson =
new JsonObject()
.put("keys-loaded", results.getValues().size())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can a test be added for the reload case, I think have a test for reloading config so shouldn't be too tricky hopefully 🤞

@usmansaleem usmansaleem requested a review from jframe March 24, 2023 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TeamCerberus Under active development by TeamCerberus @Consensys
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants