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

Fix broken script use for non-master deployments #62

Closed
wants to merge 3 commits into from

Conversation

sgnn7
Copy link
Contributor

@sgnn7 sgnn7 commented May 2, 2019

Old code would error out deploying the database since templates weren't
properly made. We now ensure that we create the templates in both cases
though obviously the follower-deploy policy loading is left to the user
to properly do with the right policies.

Connected to #57

@sgnn7 sgnn7 requested review from doodlesbykumbi and izgeri May 2, 2019 18:26
doodlesbykumbi
doodlesbykumbi previously approved these changes May 3, 2019
Copy link
Contributor

@doodlesbykumbi doodlesbykumbi left a comment

Choose a reason for hiding this comment

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

LGTM!

@izgeri
Copy link
Contributor

izgeri commented May 7, 2019

To test this, someone would need to deploy the Conjur master outside of Kubernetes and verify that the end-to-end flow works. I am not comfortable merging until this flow is tested.

start Outdated
@@ -9,10 +9,16 @@ set -xeuo pipefail

./1_create_test_app_namespace.sh

# This script is run for both follower-based and master-based installs
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow this comment at all.

There are two flows that this repo can do - it can deploy the master cluster to k8s/oc as well as the followers, or it can deploy just the followers to k8s/oc and assume you have configured a master cluster elsewhere.

when you have a master cluster elsewhere (eg DEPLOY_MASTER_CLUSTER is false) and you run start, the start script has no way of knowing how to connect to your master cluster to load the policies.

the current flow is described in usage in the main project readme. when you are not deploying a master cluster to kubernetes, you have to manually initialize the authenticator CA, manually load the policies (and script 2 makes this easier), and then you run ./start.

I don't think this workflow has been tested with this change, and if for some reason it has and it works well, the README needs to be updated before this PR could go in.

@doodlesbykumbi doodlesbykumbi dismissed their stale review May 7, 2019 17:41

Geri raises good points

@sgnn7 sgnn7 force-pushed the fix-non-master-deployment branch from 8a98d9c to 73ec892 Compare May 13, 2019 16:53
sgnn7 added 2 commits May 14, 2019 11:53
Since deployment depends on some details from master deployment
regarding the database, these files are mandatory and if the user has
not yet created them, the scripts break in latter parts of the
start script. With this check, we have at least a basic check that stage
2 (load_policy) has been run to generate these files which are used in
stage 6 (deploy).
It's a little bit easier to read now tih some extra spacing and using an
`if` vs the `||` syntax.
@sgnn7 sgnn7 force-pushed the fix-non-master-deployment branch 2 times, most recently from fd6bada to dec7f67 Compare May 14, 2019 18:43
Stop script was switching namespaces when namespace in question might
not have been available so now we ensure that this does not happen.
@sgnn7 sgnn7 force-pushed the fix-non-master-deployment branch from dec7f67 to 7a98ead Compare May 14, 2019 18:58
@sgnn7
Copy link
Contributor Author

sgnn7 commented Aug 12, 2019

Closing this stale PR for now until we can get more time dedicated for it.

@sgnn7 sgnn7 closed this Aug 12, 2019
@izgeri izgeri deleted the fix-non-master-deployment branch December 6, 2019 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants