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

Remove system.exit from the health indicator and enhance the exception handler #3585

Open
taban03 opened this issue Jun 5, 2024 · 4 comments

Comments

@taban03
Copy link
Contributor

taban03 commented Jun 5, 2024

Describe the bug
The health indicator class contains system.exit, that shouldn't be used, in favour of unhealthy response

protected void doHealthCheck(Health.Builder builder) {

        boolean authUp = true;
        if (loginProviders.isZosfmUsed()) {
            try {
                authUp = loginProviders.isZosmfAvailableAndOnline();
            } catch (AuthenticationServiceException ex) {
                System.exit(-1);
            }
        }

        builder.status(toStatus(authUp))
            .withDetail(CoreService.AUTH.getServiceId(), toStatus(authUp).getCode());

    }

An API ML (ZWE*) message should rather be logged, with information about what actions the user should take:

i.e.
Authentication service is not registered. Please verify the configuration and whether it is available
The status response 503 indicating that the service is DOWN should also returned.

https://github.com/zowe/api-layer/wiki/Issue-management

@taban03 taban03 added bug Verified defect in functionality new New issue that has not been worked on yet labels Jun 5, 2024
@taban03 taban03 changed the title Remove system.exit from the health indicator Remove system.exit from the health indicator and enhance the exception handler Jun 5, 2024
@taban03 taban03 added enhancement New feature or request and removed bug Verified defect in functionality labels Jun 5, 2024
@balhar-jakub
Copy link
Member

The addition of the log message with the correct message ID seems to be the correct direction.

As for the removal of System.exit - The authentication service doesn't exist or isn't working and as such the API Mediation Layer wouldn't work, stopping it seems like a reasonable choice. Can we use other way to stop it?

@pablocarle
Copy link
Contributor

The addition of the log message with the correct message ID seems to be the correct direction.

As for the removal of System.exit - The authentication service doesn't exist or isn't working and as such the API Mediation Layer wouldn't work, stopping it seems like a reasonable choice. Can we use other way to stop it?

I think the problem is that stopping it triggers a restart of the service by launcher. We've discussed this in the past, we should have a way to stop it without triggering a restart.
In this particular case it may require a user action to fix connection with z/OSMF.
I was thinking about the scenario with SAF provider, in this case the z/OSMF part of the condition will also not trigger.

@taban03
Copy link
Contributor Author

taban03 commented Jun 6, 2024

And another possible scenario would be multi-tenancy where I think the central GW wouldn't require any authentication

@balhar-jakub balhar-jakub added Priority: Medium size/S and removed new New issue that has not been worked on yet labels Jun 12, 2024
Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

3 participants