-
Notifications
You must be signed in to change notification settings - Fork 15
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
Dm 900 Fix deployment to prevent override of origin url #193
Conversation
@thegentlemanphysicist have you tested these changes with a deployment? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you tested these changes with a deployment?
Heads up that if we followed the OWASP Docker Security Cheat Sheet RULE #9 - Use static analysis tools we'd have a guardrail that would be helping us detect misconfigurations in Kubernetes & Docker when we update a template file. If we're continuously deploying the branch that this PR will merge into we will end up testing these changes with a deployment by the action of merging, so detecting potential misconfigurations early is how we might shift left & raise confidence that by merging this type of PR we'd have shared confidence that it would deploy correctly with the newfound flexibility to override ORIGIN
while still retaining a good default value.
displayName: The vanity url for the deployment | ||
value: https://${NAME}-${APP_GROUP}-${TAG_NAME}.apps.silver.devops.gov.bc.ca | ||
required: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why make the new template variable required when it has a good default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thegentlemanphysicist have you tested these changes with a deployment?
I redeployed dev and test, though it's probably best someone else tries the deployment to make sure the modified readme still work @BryanGK I'll document the two cases on the PR's description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wenzowski I do not believe you can pass in params to a default param in these yaml config files (i tried). Meaning the ${NAME}, ${APP_GROUP}, and ${TAG_NAME} are treated as strings when the template is processed. Despite the fact that it's not a 'good' or 'valid' default value the form of the ORIGIN param, having it's for defined will allow us to easily fix the deployment config if the form of these urls ever change or if our pipeline structure changes (i.e. we no longer use a reverse proxy in test, or we have multiple named deployments in dev, etc)
All that said I agree it is a bit hacky I'm not certain the templates are a flexible enough framework to allow something equivalent to a conditional statements to be used.
ORIGIN: ${ORIGIN ? ORIGIN : https://${NAME}-${APP_GROUP}-${TAG_NAME}.apps.silver.devops.gov.bc.ca }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, so the default value parameter isn’t interpolated. That’s a reason to perhaps consider migrating to Helm from the default OpenShift templating system. I agree that an expressive default value like this helps understand what string to pass in to the template.
displayName: The vanity url for the deployment | ||
value: https://${NAME}-${APP_GROUP}-${TAG_NAME}.apps.silver.devops.gov.bc.ca | ||
required: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, so the default value parameter isn’t interpolated. That’s a reason to perhaps consider migrating to Helm from the default OpenShift templating system. I agree that an expressive default value like this helps understand what string to pass in to the template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Previous deployments were overriding the setting of the origin url (a vanity url that allows keycloak to redirect the app users to the correct url.) Documentation has been updated,
Test cases