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

Refactor decoding Base64 to byte[] #9912

Merged
merged 3 commits into from
Apr 4, 2024
Merged

Refactor decoding Base64 to byte[] #9912

merged 3 commits into from
Apr 4, 2024

Conversation

MichaelMorrisEst
Copy link
Contributor

Type of change

Refactoring

Description

Introduce new methods in operator-common Util class for decoding Base64 to byte[] and refactor code to use the new methods

Closes #9911

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Write tests
  • Make sure all tests pass
  • Update documentation
  • Check RBAC rights for Kubernetes / OpenShift roles
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Reference relevant issue(s) and close them after merging
  • Update CHANGELOG.md
  • Supply screenshots for visual changes, such as Grafana dashboards

…64 to byte[]

Signed-off-by: MichaelMorris <michael.morris@est.tech>
@scholzj scholzj added this to the 0.41.0 milestone Apr 3, 2024
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Thanks for the PR ... I left one comment that might apply to more places.

@@ -127,10 +128,10 @@ private static KeycloakInstance createKeycloakInstance(String namespaceName) {
Secret keycloakSecret = kubeClient().getSecret(namespaceName, KEYCLOAK_SECRET_NAME);

String usernameEncoded = keycloakSecret.getData().get("username");
String username = new String(Base64.getDecoder().decode(usernameEncoded.getBytes(StandardCharsets.UTF_8)), StandardCharsets.UTF_8);
String username = new String(Util.decodeBytesFromBase64(usernameEncoded.getBytes(StandardCharsets.UTF_8)), StandardCharsets.UTF_8);
Copy link
Member

Choose a reason for hiding this comment

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

Here, but also in the other cases of this ... should these go directly to String instead using the already existing Utils.decodeFromBase64(String data, Charset charset) method? I think when the byte[] is actually used, Util.decodeBytesFromBase64 makes sense. but where we just create a String from it right away, we should use the method to decode it directly to String, or?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thats a good point, I've updated the implementation

Signed-off-by: MichaelMorris <michael.morris@est.tech>
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Few more nits. But LGTM overall, thanks.

import java.util.HashMap;
import java.util.List;
import java.util.Map;


Copy link
Member

Choose a reason for hiding this comment

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

Not needed I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 131 to 134
String username = Util.decodeFromBase64(usernameEncoded);

String passwordEncoded = keycloakSecret.getData().get("password");
String password = new String(Base64.getDecoder().decode(passwordEncoded.getBytes(StandardCharsets.UTF_8)), StandardCharsets.UTF_8);
String password = Util.decodeFromBase64(passwordEncoded);
Copy link
Member

Choose a reason for hiding this comment

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

Should these keep the UTF_8 charset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Signed-off-by: MichaelMorris <michael.morris@est.tech>
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM. I will run the system tests, but I do not expect anything to break there.

@scholzj
Copy link
Member

scholzj commented Apr 3, 2024

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the PR!

Copy link
Contributor

@fvaleri fvaleri left a comment

Choose a reason for hiding this comment

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

Thanks.

@scholzj scholzj merged commit d9b5b33 into strimzi:main Apr 4, 2024
21 checks passed
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.

[Enhancement]: Introduce new methods in operator-common Util class for decoding Base64 to byte[]
4 participants