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.

Sign up for GitHub

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

Jump to bottom

OPIK-547 Store and retrieve LLM provider api key #845

Merged
merged 3 commits into from
Dec 11, 2024
Merged

OPIK-547 Store and retrieve LLM provider api key #845

merged 3 commits into from
Dec 11, 2024

Conversation

BorisTkachenko
Copy link
Contributor

Details

Store and retrieve LLM provider api key

Testing

Added integration tests

Documentation

https://www.notion.so/cometml/Online-Scoring-HLD-14d7124010a380508f32fdbd541e5705

@BorisTkachenko BorisTkachenko self-assigned this Dec 10, 2024
@BorisTkachenko BorisTkachenko force-pushed the boryst/OPIK-547-api-keys-store-and-retrieve branch from b2bbb65 to c46f4ef Compare December 10, 2024 12:31
@BorisTkachenko BorisTkachenko marked this pull request as ready for review December 10, 2024 12:31
@BorisTkachenko BorisTkachenko requested a review from a team as a code owner December 10, 2024 12:31
thiagohora
thiagohora reviewed Dec 10, 2024
View reviewed changes
apps/opik-backend/config.yml Outdated Show resolved Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/ProviderApiKeyUpdate.java Outdated Show resolved Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/infrastructure/OpikConfiguration.java Show resolved Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/infrastructure/EncryptionService.java Outdated Show resolved Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/infrastructure/EncryptionService.java Outdated Show resolved Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/infrastructure/EncryptionConfig.java Show resolved Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/ProxyService.java Outdated Show resolved Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/ProxyService.java Outdated
log.info("Got provider api key with id '{}', workspaceId '{}'", id, workspaceId);

return providerApiKey.toBuilder()
.apiKey(encryptionService.decrypt(providerApiKey.apiKey()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for internal usage. Api key will not be serialized, and should never be exposed outside of application. So there is no sense in additional Serializer configurations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Related to the other thread. Ideally, api key deserialization should be triggered on the last minute, just right before inserting it as auth header in the client call towards the provider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This field is excluded from serialization when returned to UI as I mentioned above. I removed decryption, since it will be used in some other service which is not implemented yet.

andrescrz
andrescrz reviewed Dec 10, 2024
View reviewed changes
Copy link
Collaborator

@andrescrz andrescrz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Let's polish this a bit more and it should be good to go.

Most of comments are minor, we can discuss the biggest ones:

  • Early encryption, late decryption.
  • Listing endpoint: fine to be in a separated PR.

apps/opik-backend/src/main/java/com/comet/opik/api/ProviderApiKey.java Outdated Show resolved Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/ProxyResource.java Outdated Show resolved Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/ProxyResource.java Outdated Show resolved Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/ProxyResource.java Outdated Show resolved Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/ProviderApiKeyDAO.java Outdated Show resolved Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/infrastructure/EncryptionService.java Outdated Show resolved Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/infrastructure/EncryptionService.java Outdated Show resolved Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/infrastructure/EncryptionService.java Outdated Show resolved Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/infrastructure/EncryptionService.java Outdated Show resolved Hide resolved
...d/src/main/resources/liquibase/db-app-state/migrations/000006_add_provider_api_key_table.sql Outdated Show resolved Hide resolved
thiagohora
thiagohora approved these changes Dec 11, 2024
View reviewed changes
Copy link
Contributor

@thiagohora thiagohora left a comment

Choose a reason for hiding this comment

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

A few more comments, but feel free to address them in a following PR

apps/opik-backend/src/main/java/com/comet/opik/OpikApplication.java
@@ -69,7 +70,7 @@ public void initialize(Bootstrap<OpikConfiguration> bootstrap) {
.withPlugins(new SqlObjectPlugin(), new Jackson2Plugin()))
.modules(new DatabaseAnalyticsModule(), new IdGeneratorModule(), new AuthModule(), new RedisModule(),
new RateLimitModule(), new NameGeneratorModule(), new HttpModule(), new EventModule(),
new ConfigurationModule(), new BiModule())
new ConfigurationModule(), new BiModule(), new EncryptionUtilsModule())
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need a module since this will be static. We can just set the configuration using the run method below.

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, it could be set in run. Since we are using DI this approach seems more consistent. Can be changed, don't have any preferences here.

apps/opik-backend/src/main/java/com/comet/opik/api/LlmProvider.java Show resolved Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/LlmProviderApiKeyService.java

@Override
public ProviderApiKey get(UUID id, String workspaceId) {
log.info("Getting provider api key with id '{}', workspaceId '{}'", id, workspaceId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's be consistent with logs. We can use them either in all public methods or just as controllers.

apps/opik-backend/src/main/java/com/comet/opik/infrastructure/EncryptionUtils.java
}

public static String encrypt(String data) {
init();
Copy link
Contributor

Choose a reason for hiding this comment

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

We can init this only once when setting the config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, will make code more readable.

...ackend/src/test/java/com/comet/opik/api/resources/v1/priv/LlmProviderApiKeyResourceTest.java
updateProviderApiKey(UUID.randomUUID(), providerApiKey, apiKey, workspaceName, 404);
}

private UUID createProviderApiKey(LlmProvider provider, String providerApiKey, String apiKey, String workspaceName,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use Podam to generate the beans. Also, we can extract these cans to a ResourceClient class like SpanResourceClient, for reusability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree about reusability. Regarding Podam, LlmProvider can be generated within the method, but providerApiKey should be passed as param, since it's used lated in the outer scope.

@BorisTkachenko BorisTkachenko merged commit 0f71bad into main Dec 11, 2024
7 checks passed
@BorisTkachenko BorisTkachenko deleted the boryst/OPIK-547-api-keys-store-and-retrieve branch December 11, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thiagohora thiagohora thiagohora approved these changes

@andrescrz andrescrz Awaiting requested review from andrescrz

Assignees

@BorisTkachenko BorisTkachenko

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BorisTkachenko @andrescrz @thiagohora