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

PLONEOPS-18362 : configure timeout property for keywhiz server #10

Merged
merged 5 commits into from Jan 28, 2020

Conversation

@vn0ytch
Copy link
Contributor

vn0ytch commented Jan 27, 2020

Description

Write here

Fixes

Changes proposed in this pull request.

Check List

  • All test passed
  • Added test to ensure to fix/ensure properly.
public KeywhizClient keywhizClient(
OneOpsConfig config, @Qualifier("keywhizKeyStore") KeywhizKeyStore keywhizKeyStore)
throws GeneralSecurityException {
return new KeywhizClient(config.getKeywhiz().getBaseUrl(), keywhizKeyStore);
OneOpsConfig.Keywhiz keywhiz = config.getKeywhiz();
return new KeywhizClient(/*config.getKeywhiz().getBaseUrl(),*/ keywhizKeyStore, keywhiz);

This comment has been minimized.

Copy link
@sureshg

sureshg Jan 27, 2020

Member

Keywhiz config has the keywhiz store. So why do we need to inject both configs?

This comment has been minimized.

Copy link
@vn0ytch

vn0ytch Jan 27, 2020

Author Contributor

@sureshg : config.getKeywhiz().getKeyStore() returns KeyStore not KeywhizKeyStore.

This comment has been minimized.

Copy link
@sureshg
.connectTimeout(5, SECONDS)
.readTimeout(5, SECONDS)
.writeTimeout(5, SECONDS)
.connectTimeout(keywhiz != null ? keywhiz.getConnectTimeout() : 5, SECONDS)

This comment has been minimized.

Copy link
@sureshg

sureshg Jan 27, 2020

Member

keywhiz can't be null as we are using the baseUrl above. Also, the default value shouldn't be in the code. It should be part of the application yaml config.

This comment has been minimized.

Copy link
@vn0ytch

vn0ytch Jan 27, 2020

Author Contributor

Yeah, i will update that now.

.readTimeout(5, SECONDS)
.writeTimeout(5, SECONDS)
.connectTimeout(keywhiz != null ? keywhiz.getConnectTimeout() : 5, SECONDS)
.readTimeout(keywhiz != null ? keywhiz.getReadTimeout() : 5, SECONDS)

This comment has been minimized.

Copy link
@sureshg

sureshg Jan 27, 2020

Member

Remove the !=null checks

This comment has been minimized.

Copy link
@vn0ytch

vn0ytch Jan 27, 2020

Author Contributor

yes

@@ -64,6 +64,9 @@ oneops:
svc-user: ${KEYWHIZ_USER}
svc-password: ${KEYWHIZ_PASS}
secret-max-size: ${KEYWHIZ_SECRET_MAX_SIZE:350000}
connect-timeout: ${KEYWHIZ_CONNECT_TIMEOUT}

This comment has been minimized.

Copy link
@sureshg

sureshg Jan 27, 2020

Member

Add the default value here like in max size.

This comment has been minimized.

Copy link
@vn0ytch

vn0ytch Jan 27, 2020

Author Contributor

ok

This comment has been minimized.

Copy link
@vn0ytch

vn0ytch Jan 27, 2020

Author Contributor

${KEYWHIZ_CONNECT_TIMEOUT:5}
Like this: ^

This comment has been minimized.

Copy link
@sureshg

sureshg Jan 27, 2020

Member

yes, but the default value should be 10 seconds.

@sureshg sureshg merged commit c1a0a46 into oneops:master Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.