-
Notifications
You must be signed in to change notification settings - Fork 22
Deny production runs of example.org trust domains #229
Conversation
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.
Do we really want to enforce people to override? If they are just doing a quick install for testing, shouldn't that be possible with the default values?
If we want people to explicitly set those it would be better to remove the defaults.
This feels a bit like a unnecessary additional config value.
That's the point of the pr. If you just helm install the chart, it just works with example.org for a quick install for testing as it does today. If you say explicitly, your trying to deploy for production by adding examples/production/values.yaml, then it forces you to specify the things you should be specifying. No one should be deploying a production spire as example.org? The pr helps you find all the things you need to override to get a valid production site by enforcing you set them.
That would make the, "just kick the tires" install much harder. But I could get behind that if there is agreement that is easier. It would be less complicated in the chart, just harder to get an example going. We could put the example values in examples/testing-deployment/values.yaml or something and then have people need to use that for a test deployment?
Necessary or unnecessary depends on the usability path question above. |
Side bar commentary by Marco with Kevin indicate that other maintainers are ok with the idea that the initial installation of SPIRE using these Helm charts not be production ready.
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.
Update to include new global.
Also, set default back to false until we determine that tests should be rewritten
ed879a2
to
f6bfe6f
Compare
Something like productionChecks for the value instead. |
That sounds good to me. |
Signed-off-by: Kevin Fox <[email protected]>
9c2f54b
to
459a382
Compare
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: kfox1111 <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Co-authored-by: Faisal Memon <[email protected]> Signed-off-by: kfox1111 <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
charts/spire/values.yaml
Outdated
@@ -24,6 +24,9 @@ global: | |||
## @param global.spire.image.registry Override all Spire image registries at once | |||
registry: "" | |||
|
|||
## @param global.spire.productionChecks Check values, such as trustDomain, are suitable for production deployment | |||
productionChecks: false |
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.
charts/spire/charts/spiffe-oidc-discovery-provider/templates/configmap.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Marco Franssen <[email protected]> Signed-off-by: kfox1111 <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Co-authored-by: Marco Franssen <[email protected]> Signed-off-by: kfox1111 <[email protected]>
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.
LGTM 🚀
This pr adds some tests to ensure that if the user is deploying for production that they set appropriate settings.
fixes: #195