Skip to content

Allow 307 return code#161

Closed
irivera007 wants to merge 1 commit into1Password:masterfrom
irivera007:master
Closed

Allow 307 return code#161
irivera007 wants to merge 1 commit into1Password:masterfrom
irivera007:master

Conversation

@irivera007
Copy link

When deployed the Redis container keeps crashing given that the services cannot get a healthy check, allowing 307 fixes this.

When deployed the Redis container keeps crashing given that the services cannot get a healthy check, allowing 307 fixes this.
@petr0sec
Copy link

petr0sec commented Dec 8, 2021

I was about to open a PR for this exact thing. Wish I would have found this before spending an hour troubleshooting why my containers were crash looping. Thanks for opening this @irivera007

@petr0sec
Copy link

petr0sec commented Dec 8, 2021

FWIW, 307 is really not a proper response code for a health check. I am guessing they changed the healthcheck endpoint (possibly to /app ?) and that's probably what needs to be updated. I'll let 1password confirm though. FWIW, I've changed the healthcheck endpoint to /app on my end rather than the status code and it seems to be stable now.

@irivera007
Copy link
Author

@petr0sec thank you!
your approach makes more sense, thank you for sharing. I agree lets see what 1pwd says about the final solution for this. Im sure this benefits the community as you saw it could be real struggle troubleshooting

@devilleweppenaar
Copy link
Contributor

Thank you for creating this PR and providing details. We're looking into this and will provide feedback shortly.

In the meantime, since release 2.3.0 of the SCIM bridge you can now enable an optional ping server that can be used for health check purposes. Some more details are available in the linked changelog or the help output of the SCIM bridge (./op-scim --help).

@chasdl
Copy link
Collaborator

chasdl commented Dec 10, 2021

Thanks for all the feedback and help here. This was an oversight in our latest release.

Prior to release v2.3.0 of the SCIM bridge, hitting GET / would return a 200 if everything was healthy, and therefore was a proper path for health checks. Now with v2.3.0 we are redirecting / to /app in order to serve the new React app, so defining /app as the health check path by default gets us to a working state.

As @devillexio mentioned, since release v2.3.0, you can enable an optional ping server on /ping for health check purposes. This will be the way forward as the default for our helm charts, but as this requires the OP_PING_SERVER option to be enabled and is disabled by default, we can stick to /app for this config.

Changes made in #162

@chasdl chasdl closed this Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants