Skip to content
This repository has been archived by the owner on Jan 19, 2022. It is now read-only.

Make 403 an allowable Pub/Sub UP status #2385

Merged
merged 2 commits into from
May 21, 2020

Conversation

elefeint
Copy link
Contributor

Starting this week, Cloud Pub/Sub accounts without pubsub.subscriptions.list permission (normally granted through Pub/Sub Viewer role) receive 403 Permission Denied instead of 404 Resource Not Found.

This PR makes 403 a valid healthcheck UP status and updates documentation to make actuator use more prominent.

Fixes #2374.

Copy link
Contributor

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

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

Looks good, but note the checkstyle failure.

@@ -25,6 +25,27 @@ dependencies {

This starter is also available from https://start.spring.io[Spring Initializr] through the `GCP Messaging` entry.

=== Spring Boot Actuator Support
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you move this section up?
Our convention is to have configuration after deps in refdoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll move it back. My thought was that configuration is really long for pubsub, and this needed to be more prominent, but maybe not.

@sonarcloud
Copy link

sonarcloud bot commented May 21, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

Merging #2385 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2385   +/-   ##
=========================================
  Coverage     73.91%   73.92%           
- Complexity     2096     2097    +1     
=========================================
  Files           260      260           
  Lines          7576     7577    +1     
  Branches        785      785           
=========================================
+ Hits           5600     5601    +1     
  Misses         1614     1614           
  Partials        362      362           
Flag Coverage Δ Complexity Δ
#integration ? ?
#unittests 73.92% <100.00%> (+<0.01%) 2097.00 <0.00> (+1.00)
Impacted Files Coverage Δ Complexity Δ
...configure/pubsub/health/PubSubHealthIndicator.java 92.85% <100.00%> (+0.54%) 4.00 <0.00> (+1.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ae7d64...a942094. Read the comment docs.

@elefeint elefeint merged commit ec91246 into master May 21, 2020
@elefeint elefeint deleted the pubsub-actuator-403-means-success branch May 21, 2020 19:16
@elefeint elefeint restored the pubsub-actuator-403-means-success branch May 27, 2020 20:51
@meltsufin meltsufin deleted the pubsub-actuator-403-means-success branch September 11, 2020 16:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

PubSubHealthIndicator reported false alarm when pubsub was actually UP
2 participants