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

Simplify dev/test environment deployment #509

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

gl-johnson
Copy link
Contributor

@gl-johnson gl-johnson commented Mar 21, 2023

Desired Outcome

Clean up some redundancies in the deployment scripts, and make it simpler to specify which mode to run Secrets Provider in.

Implemented Changes

  • Consolidated deploy_init_env, deploy_k8s_rotation_env, and deploy_push_to_file into one function
  • Update test cases and dev bootstrap.env to use SECRETS_MODE to set the mode for deployment

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

Changelog

  • The CHANGELOG has been updated, or
  • This PR does not include user-facing changes and doesn't require a
    CHANGELOG update

Test coverage

  • This PR includes new unit and integration tests to go with the code
    changes, or
  • The changes in this PR do not require tests

Documentation

  • Docs (e.g. READMEs) were updated in this PR
  • A follow-up issue to update official docs has been filed here: [insert issue ID]
  • This PR does not require updating any documentation

Behavior

  • This PR changes product behavior and has been reviewed by a PO, or
  • These changes are part of a larger initiative that will be reviewed later, or
  • No behavior was changed with this PR

Security

  • Security architect has reviewed the changes in this PR,
  • These changes are part of a larger initiative with a separate security review, or
  • There are no security aspects to these changes

@gl-johnson gl-johnson force-pushed the cleanup-deployment-scripts branch from 2449ad1 to d7593dc Compare March 21, 2023 20:08
@gl-johnson gl-johnson changed the title Cleanup deployment scripts Simplify dev/test environment deployment Mar 21, 2023
@codeclimate
Copy link

codeclimate bot commented Mar 21, 2023

Code Climate has analyzed commit d7593dc and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 89.2% (0.0% change).

View more on Code Climate.

@gl-johnson gl-johnson marked this pull request as ready for review March 21, 2023 21:30
@gl-johnson gl-johnson requested a review from a team as a code owner March 21, 2023 21:30
name: test-app
command: ["sleep"]
args: ["infinity"]
volumeMounts:
- mountPath: /opt/secrets/conjur
name: conjur-secrets
readOnly: true
- image: '${PULL_DOCKER_REGISTRY_PATH}/${APP_NAMESPACE_NAME}/secrets-provider:latest'
Copy link
Contributor

Choose a reason for hiding this comment

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

With this change where are we pulling the images from? Is is the same for GKE and Openshift?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These manifests are only used when running ./bin/start with the --dev flag, so I think we can assume that the image already exists locally per the docs to deploy a local dev environment, the first step of which is to run ./bin/build.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, if it's only the dev image that's fine.

- image: '${PULL_DOCKER_REGISTRY_PATH}/${APP_NAMESPACE_NAME}/secrets-provider:latest'
imagePullPolicy: Always
- image: 'secrets-provider-for-k8s:latest'
imagePullPolicy: Never
Copy link
Contributor

Choose a reason for hiding this comment

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

With the imagePullPolicy to Never is it possible that a cached image will be used and not the build image?

Copy link
Contributor Author

@gl-johnson gl-johnson Mar 22, 2023

Choose a reason for hiding this comment

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

Hmm I'm not sure on this. This change is largely based on the existing dev manifests for K8s secrets mode, both of which omitted the registry path variables and use an imagePullPolicy of Never.

I think the assumption once again is that ./bin/build was run prior to the start script, which tags the locally built secrets-provider image as latest and dev

Edit: ./bin/start --dev actually fails unless you have run the build script first even with a cached secrets-provider:latest image, since it is expecting the dev tag here

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess my concern is if I rebuild a dev image with a small change, and redeploy the pod will the latest build be picked up?

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 should still be picked up as far as I can tell. Each time you run ./bin/build it overwrites dev/latest tags for the image locally, which is what will be used in the deployment when the imagePullPolicy is 'Never'.

I tested it with ./bin/start --dev and ./bin/start --dev --reload and it picks up code changes

Copy link
Contributor

@rpothier rpothier left a comment

Choose a reason for hiding this comment

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

LGTMM!

@gl-johnson gl-johnson merged commit a7750d9 into main Mar 23, 2023
@gl-johnson gl-johnson deleted the cleanup-deployment-scripts branch March 23, 2023 14:01
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.

2 participants