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

Read emulatorEnabled value for storage emulators from mounted secret instead of environment variables #819

Merged
merged 3 commits into from
Dec 30, 2024
Merged

Read emulatorEnabled value for storage emulators from mounted secret instead of environment variables #819

merged 3 commits into from
Dec 30, 2024

Conversation

shreyas-s-rao
Copy link
Collaborator

@shreyas-s-rao shreyas-s-rao commented Dec 24, 2024 •
edited
Loading

/area compliance security
/kind task

What this PR does / why we need it:
Read emulatorEnabled value for storage emulators from mounted secret instead of environment variables, to avoid mounting and exposing data from secrets as environment variables, even if they are non-credential fields.
Changes in this PR:

  • Read emulatorEnabled fields for providers Azure ABS and Google GCS from mounted secret files/JSON, instead of from environment variables
  • Refactor provider GCS snapstore construction code
  • Remove usage of env var GOOGLE_EMULATOR_ENABLED
  • Remove usage of env var AZURE_EMULATOR_ENABLED
  • Add IsEmulatorEnabled to SnapstoreConfig struct, to allow programatic downstream users of etcdbr code to set emulator enablement when creating SnapstoreConfig. For instance, etcd-druid can set this value to true when running local e2e tests against Azurite emulator.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
/invite @anveshreddy18 @renormalize

Release note:

Remove usage of environment variables to determine whether storage emulators are enabled.

@shreyas-s-rao shreyas-s-rao added this to the v0.33.0 milestone Dec 24, 2024
@shreyas-s-rao shreyas-s-rao requested a review from a team as a code owner December 24, 2024 09:18
@gardener-robot gardener-robot added the needs/review Needs review label Dec 24, 2024
@gardener-robot gardener-robot added area/compliance Compliance related area/security Security related kind/task General task size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Dec 24, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 24, 2024
@shreyas-s-rao shreyas-s-rao added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 24, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 24, 2024
anveshreddy18
anveshreddy18 reviewed Dec 24, 2024
View reviewed changes
Copy link
Contributor

@anveshreddy18 anveshreddy18 left a comment

Choose a reason for hiding this comment

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

Thanks @shreyas-s-rao for the PR.

I've tested with fakegcs and it works as expected there. But when testing with real GCS, it's not working, checking that I found the below issue. Can you PTAL.

pkg/snapstore/gcs_snapstore.go Outdated Show resolved Hide resolved
@shreyas-s-rao shreyas-s-rao added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 25, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 25, 2024
@shreyas-s-rao
Copy link
Collaborator Author

New addition as part of 3d32db9:

  • Added field IsEmulatorEnabled to SnapstoreConfig struct, to allow programatic downstream users of etcdbr code to set emulator enablement when creating SnapstoreConfig. For instance, etcd-druid can set this value to true when running local e2e tests against Azurite emulator, since druid needs to construct its own snapstore client to access the object storage bucket when running e2e tests, to clean up any existing objects before starting the test run.

@shreyas-s-rao shreyas-s-rao force-pushed the task/remove-emulator-enabled-env-vars branch from 3d32db9 to be75e89 Compare December 25, 2024 15:46
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 25, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 25, 2024
@shreyas-s-rao shreyas-s-rao added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 25, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 25, 2024
shreyas-s-rao added a commit to shreyas-s-rao/etcd-druid that referenced this pull request Dec 25, 2024
@shreyas-s-rao
Use gardener/etcd-backup-restore#819 image and dependency version
b88bc2a
@shreyas-s-rao shreyas-s-rao force-pushed the task/remove-emulator-enabled-env-vars branch from be75e89 to e9ab312 Compare December 25, 2024 17:04
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 25, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 25, 2024
@shreyas-s-rao shreyas-s-rao added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 25, 2024
@shreyas-s-rao shreyas-s-rao mentioned this pull request Dec 25, 2024
Merged
4 tasks
@shreyas-s-rao shreyas-s-rao added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 25, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 25, 2024
renormalize
renormalize requested changes Dec 26, 2024
View reviewed changes
Copy link
Member

@renormalize renormalize left a comment

Choose a reason for hiding this comment

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

Thanks @shreyas-s-rao for this PR!
Everything looks good, just one tiny nit from me for the sake of correctness.

pkg/snapstore/abs_snapstore.go Outdated Show resolved Hide resolved
@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Dec 26, 2024
renormalize
renormalize requested changes Dec 26, 2024
View reviewed changes
pkg/snapstore/abs_snapstore.go Outdated Show resolved Hide resolved
@gardener-robot gardener-robot added the needs/rebase Needs git rebase label Dec 26, 2024
@gardener-robot
Copy link

@shreyas-s-rao You need rebase this pull request with latest master branch. Please check.

@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 26, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 26, 2024
@shreyas-s-rao
Read emulatorEnabled value for storage emulators from mounted secre…
83f790c 
…t instead of environment variables
@shreyas-s-rao
Address review comments from @anveshreddy18; introduce `SnapstoreConf…
9cd6564 
…ig.IsEmulatorEnabled`
@shreyas-s-rao
Address review comments from @anveshreddy18 and @renormalize - thrwo …
ace57cc 
…error if emulator is enabled but `storageAPIEndpoint`/`domain` is not provided
@shreyas-s-rao shreyas-s-rao force-pushed the task/remove-emulator-enabled-env-vars branch from 4205657 to ace57cc Compare December 26, 2024 13:12
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 26, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 26, 2024
renormalize
renormalize approved these changes Dec 26, 2024
View reviewed changes
Copy link
Member

@renormalize renormalize left a comment

Choose a reason for hiding this comment

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

Thanks!

anveshreddy18
anveshreddy18 approved these changes Dec 26, 2024
View reviewed changes
Copy link
Contributor

@anveshreddy18 anveshreddy18 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 @shreyas-s-rao.

@shreyas-s-rao shreyas-s-rao added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 27, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 27, 2024
@shreyas-s-rao shreyas-s-rao merged commit 2d7dab8 into gardener:master Dec 30, 2024
9 checks passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Dec 30, 2024
@shreyas-s-rao shreyas-s-rao deleted the task/remove-emulator-enabled-env-vars branch December 30, 2024 12:46
This was referenced Dec 30, 2024
Closed
Merged
Merged
@anveshreddy18 anveshreddy18 mentioned this pull request Jan 9, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anveshreddy18 anveshreddy18 anveshreddy18 approved these changes

@renormalize renormalize renormalize approved these changes

Assignees

@renormalize renormalize

@anveshreddy18 anveshreddy18

Labels
area/compliance Compliance related area/security Security related kind/task General task needs/changes Needs (more) changes needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/rebase Needs git rebase needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Milestone
v0.33.0
Development

Successfully merging this pull request may close these issues.
7 participants
@shreyas-s-rao @gardener-robot @renormalize @anveshreddy18 @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3