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

Handle warning only responses and log warnings #100

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

kdubb
Copy link
Contributor

@kdubb kdubb commented Dec 22, 2022

Warning only responses

When a request expecting a 204 returns warnings it does so by returning a standaard 200 response with the warnings attached. Previously this would result in an exception becaues of the mismatched response status code.

Logging warnings

Any Vault response can attach warnings. Those warnings are now logged for all responses.

### Warning only responses
When a request expecting a 204 returns warnings it does so by returning a standaard 200 response with the warnings attached. Previously this would result in an exception becaues of the mismatched response status code.

### Logging warnings
Any Vault response can attach warnings. Those warnings are now logged for all responses.
@kdubb
Copy link
Contributor Author

kdubb commented Dec 22, 2022

@vsevel We need this ASAP, but we need it back on 2.15. How do we go about back-porting this after merge?

@kdubb
Copy link
Contributor Author

kdubb commented Dec 22, 2022

FYI, the reason this is an issue is because we are trying to upgrade to Vault 1.12 and it generates warnings on one of the PKI calls (which we aren't prepared to fix at this point) which turns into an exception because of the 204/200 mismatch.

@kdubb
Copy link
Contributor Author

kdubb commented Dec 22, 2022

I pushed a 2.1.x branch to release a back port from.

@kdubb kdubb merged commit b97237b into quarkiverse:main Feb 8, 2023
@kdubb kdubb deleted the fix/warnings branch February 8, 2023 14:23
@vsevel
Copy link
Contributor

vsevel commented Feb 8, 2023

which we aren't prepared to fix at this point

you mean the situation in your infrastructure, which causes the warning?


in this code:

    @Override
    public String toString() {
        return "VaultClientException{" +
                "operationName='" + operationName + '\'' +
                ", requestPath='" + requestPath + '\'' +
                ", status=" + status +
                ", body='" + body + '\'' +
                '}';
    }

may the body contain confidential information? if not we do not want to include it in a toString - I see that even in the previous code it was already part of the toString :(


so here we are not sure what we are going to get?

var result = response.bodyAsJson(AbstractVaultDTO.class);
...
        if (!(result instanceof AbstractVaultDTO<?, ?>)) {
            return;
        }

what does happen if json is not a AbstractVaultDTO? it builds another object (I am surprised it would)? or it fails?
another defensive way to code it, is to use bodyAsJsonObject()


I pushed a 2.1.x branch to release a back port from.

ok

@kdubb
Copy link
Contributor Author

kdubb commented Feb 8, 2023

you mean the situation in your infrastructure, which causes the warning?

Yes when using 1.12 warnings are being generated that are not affecting us. I am not sure exactly what they are at the moment.

may the body contain confidential information?

No. As far as I've ever seen, Vault warnings/errors are not be leaking confidential information. This would be a pretty serious Vault issue and we need the information to be able to be logged.

so here we are not sure what we are going to get?

Correct but the method logResultWarnings is generalized to be called from two cases. The first (you've highlighted) always returns an AbstractorVaultDTO<>, the second case may be an AbstractVaultDTO<>.

what does happen if json is not a AbstractVaultDTO?

Nothing. This is to log warnings which we can only get from an AbstractVaultDTO<>, if it's not one we don't log anything.

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.

2 participants