[go: nahoru, domu]
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

Handle AWS encryption details #12495

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Handle AWS encryption details #12495

wants to merge 6 commits into from

Conversation

artjen
Copy link
Collaborator
@artjen artjen commented Jun 26, 2024 •
edited by github-actions bot
Loading
🛠 DevTools 🛠

Open in GitHub Codespaces

Install mlflow from this PR

pip install git+https://github.com/mlflow/mlflow.git@refs/pull/12495/merge

Checkout with GitHub CLI

gh pr checkout 12495

Related Issues/PRs

#xxx

What changes are proposed in this pull request?

Enable MLflow to use AWS server-side encryption details when uploading files.

How is this PR tested?

  • Existing unit/integration tests
  • New unit/integration tests
  • Manual tests

Does this PR require documentation update?

  • No. You can skip the rest of this section.
  • Yes. I've updated:
    • Examples
    • API references
    • Instructions

Release Notes

Is this a user-facing change?

  • No. You can skip the rest of this section.
  • Yes. Give a description of this change to be included in the release notes for MLflow users.

What component(s), interfaces, languages, and integrations does this PR affect?

Components

  • area/artifacts: Artifact stores and artifact logging
  • area/build: Build and test infrastructure for MLflow
  • area/deployments: MLflow Deployments client APIs, server, and third-party Deployments integrations
  • area/docs: MLflow documentation pages
  • area/examples: Example code
  • area/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registry
  • area/models: MLmodel format, model serialization/deserialization, flavors
  • area/recipes: Recipes, Recipe APIs, Recipe configs, Recipe Templates
  • area/projects: MLproject format, project running backends
  • area/scoring: MLflow Model server, model deployment tools, Spark UDFs
  • area/server-infra: MLflow Tracking server backend
  • area/tracking: Tracking Service, tracking client APIs, autologging

Interface

  • area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev server
  • area/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
  • area/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registry
  • area/windows: Windows support

Language

  • language/r: R APIs and clients
  • language/java: Java APIs and clients
  • language/new: Proposals for new client languages

Integrations

  • integrations/azure: Azure and Azure ML integrations
  • integrations/sagemaker: SageMaker integrations
  • integrations/databricks: Databricks integrations

How should the PR be classified in the release notes? Choose one:

  • rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
  • rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
  • rn/feature - A new user-facing feature worth mentioning in the release notes
  • rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
  • rn/documentation - A user-facing documentation change worth mentioning in the release notes

Should this PR be included in the next patch release?

Yes should be selected for bug fixes, documentation updates, and other small changes. No should be selected for new features and larger changes. If you're unsure about the release classification of this PR, leave this unchecked to let the maintainers decide.

What is a minor/patch release?
  • Minor release: a release that increments the second part of the version number (e.g., 1.2.0 -> 1.3.0).
    Bug fixes, doc updates and new features usually go into minor releases.
  • Patch release: a release that increments the third part of the version number (e.g., 1.2.0 -> 1.2.1).
    Bug fixes and doc updates usually go into patch releases.
  • Yes (this PR will be cherry-picked and included in the next patch release)
  • No (this PR will be included in the next minor release)

@artjen
handle aws encryption details
f1f2482 
Signed-off-by: Arthur Jenoudet <arthur.jenoudet@databricks.com>
@github-actions github-actions bot added patch-2.14.2 area/model-registry Model registry, model registry APIs, and the fluent client calls for model registry rn/feature Mention under Features in Changelogs. labels Jun 26, 2024
@artjen artjen requested a review from kriscon-db June 26, 2024 23:52
@artjen
Copy link
Collaborator Author
artjen commented Jun 26, 2024

@mlflow-automation autoformat

@artjen
fix lint
eee7f7c 
Signed-off-by: Arthur Jenoudet <arthur.jenoudet@databricks.com>
@harupy harupy mentioned this pull request Jun 27, 2024
Merged
39 tasks
@harupy
Copy link
Member
harupy commented Jun 27, 2024

@mlflow-automation autoformat

mlflow-automation and others added 3 commits June 27, 2024 02:10
@mlflow-automation
Merge remote-tracking branch 'base/master' into handle-aws-encryption…
d09c97c 
…-details
@mlflow-automation
Autoformat: https://github.com/mlflow/mlflow/actions/runs/9689533222
c307295 
Signed-off-by: mlflow-automation <mlflow-automation@users.noreply.github.com>
@artjen
retrigger tests
e332c67 
Signed-off-by: Arthur Jenoudet <arthur.jenoudet@databricks.com>
@github-actions GitHub Actions
Copy link
github-actions bot commented Jun 27, 2024 •
edited
Loading

Documentation preview for 32f1594 will be available when this CircleCI job
completes successfully.

More info

@artjen
fix tests
32f1594 
Signed-off-by: Arthur Jenoudet <arthur.jenoudet@databricks.com>
arpitjasa-db
arpitjasa-db reviewed Jun 27, 2024
View reviewed changes
Copy link
Collaborator
@arpitjasa-db arpitjasa-db left a comment

Choose a reason for hiding this comment

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

I know we synced offline about manually testing this, maybe @BenWilson2 knows someone from the field side who has access to an AWS instance for encryption testing. Otherwise, probably file an IT ticket to try this out

mlflow/protos/databricks_uc_registry_messages.proto
@@ -190,6 +192,26 @@ enum StorageMode {
DEFAULT_STORAGE = 2;
}

message EncryptionDetails {
oneof encryption_details_type {
// Details for CLOUD_MANAGED_SSE_KEYS:
Copy link
Collaborator

Choose a reason for hiding this comment

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

oneof just one option? I assume we're trying to match some passed in format exactly, so if this is necessary, let's add a comment explaining why

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is to match the UCMR proto, derived from the UC proto. We're only exposing the information we need.

mlflow/protos/databricks_uc_registry_messages.proto

enum SseEncryptionAlgorithm {
SSE_ENCRYPTION_ALGORITHM_UNSPECIFIED = 0;
AWS_SSE_KMS = 2; // "aws:kms" in x-amz-server-side-encryption' header
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
AWS_SSE_KMS = 2; // "aws:kms" in x-amz-server-side-encryption' header
AWS_SSE_KMS = 1; // "aws:kms" in x-amz-server-side-encryption' header

Unless there's a reason why this needs to start at 2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is to match the UCMR proto, derived from the UC proto. We're only exposing the information we need.

@artjen artjen requested a review from arpitjasa-db June 28, 2024 17:08
@BenWilson2
Copy link
Member

I know we synced offline about manually testing this, maybe @BenWilson2 knows someone from the field side who has access to an AWS instance for encryption testing. Otherwise, probably file an IT ticket to try this out

@michael-berk would you have access to an AWS account that would make testing this simple? It's a bit challenging for us.

@artjen
Copy link
Collaborator Author
artjen commented Jul 2, 2024

@BenWilson2 @michael-berk Any guidance on how to test this manually?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kriscon-db kriscon-db Awaiting requested review from kriscon-db

@arpitjasa-db arpitjasa-db Awaiting requested review from arpitjasa-db

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area/model-registry Model registry, model registry APIs, and the fluent client calls for model registry patch-2.14.2 rn/feature Mention under Features in Changelogs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@artjen @harupy @BenWilson2 @arpitjasa-db @mlflow-automation