Skip to content
This repository has been archived by the owner on Mar 22, 2024. It is now read-only.

Add support for spire-server ingress #68

Merged
merged 45 commits into from
May 23, 2023
Merged

Conversation

kfox1111
Copy link
Contributor

@kfox1111 kfox1111 commented Feb 28, 2023

This patch enables the spire-server to be exposed outside the cluster utilizing an ingress controller that supports ssl passthrough such as ingress-nginx. It also adds tests to ensure that an agent can reach it through an ingress.

Resolves #61

@kfox1111
Copy link
Contributor Author

Ingress object comes right from "helm create"

@marcofranssen marcofranssen changed the title Ingress support for spire-server Add support for spire-server ingress Mar 1, 2023
@marcofranssen
Copy link
Contributor

My main concern is how we can guarantee we don't introduce vulnerabilities from allowing to expose insecure ingress configurations. Maybe we should also consider a opiniated out of the box secure setup. I'm fine with both, but I think we need to discuss this and agree collectively on the solution design.

@kfox1111
Copy link
Contributor Author

kfox1111 commented Mar 1, 2023

My main concern is how we can guarantee we don't introduce vulnerabilities from allowing to expose insecure ingress configurations. Maybe we should also consider a opiniated out of the box secure setup. I'm fine with both, but I think we need to discuss this and agree collectively on the solution design.

Hmm..... Yeah, I understand trying to save the user from themselves.... how to do it though, since that part of the k8s api was never standardized....

We could force the annotations in unless they disable them, but its still not guaranteed another ingress controller would honor them. in fact, aws seems to do it quite differently: https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.2/guide/tasks/ssl_redirect/#example-ingress-manifest

It would be a pretty ugly game of wack a mole...

One option might be to need an extra value when enabling ingress that states: IHaveReadTheInstructions:true before it allows the chart to be installed with ingress.enabled true? Then they have the opportunity to review the instructions, and if they set it without looking, its on them?

@kfox1111
Copy link
Contributor Author

kfox1111 commented Mar 1, 2023

Actually, I thought we were talking about the other pr...

In this one, the only way to pass it through ingress is with sni. So it can't be insecurely passed through.

@kfox1111
Copy link
Contributor Author

kfox1111 commented Mar 1, 2023

@marcofranssen
Copy link
Contributor

My main concern is how we can guarantee we don't introduce vulnerabilities from allowing to expose insecure ingress configurations. Maybe we should also consider a opiniated out of the box secure setup. I'm fine with both, but I think we need to discuss this and agree collectively on the solution design.

Hmm..... Yeah, I understand trying to save the user from themselves.... how to do it though, since that part of the k8s api was never standardized....

We could force the annotations in unless they disable them, but its still not guaranteed another ingress controller would honor them. in fact, aws seems to do it quite differently: kubernetes-sigs.github.io/aws-load-balancer-controller/v2.2/guide/tasks/ssl_redirect/#example-ingress-manifest

It would be a pretty ugly game of wack a mole...

One option might be to need an extra value when enabling ingress that states: IHaveReadTheInstructions:true before it allows the chart to be installed with ingress.enabled true? Then they have the opportunity to review the instructions, and if they set it without looking, its on them?

Yeah, difficult topic, I think for this PR due to the SNI part it is less of an issue. I think we can still add the annotations I suggested here and then merge it if other maintainers agree.

@faisal-memon what is your thoughts on this? Or should we check with the SSC?

kfox1111 and others added 12 commits March 2, 2023 04:24
This patch enables the spire-server to be exposed outside the cluster
utilizing an ingress controller that supports ssl passthrough such
as ingress-nginx. It also adds tests to ensure that an agent can
reach it through an ingress.

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Co-authored-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
See https://github.com/sigstore/cosign-installer\#usage on proper usage

Resolves #66

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Co-authored-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@faisal-memon
Copy link
Contributor

faisal-memon commented May 14, 2023

Thinking about this one and how it relates to the [same thing done with the oidc](https://github.com/spiffe/helm-ch
/arts/commit/cc7121e021cf8f569ced4a6dd767f9fdf5c32da9). If both are deployed will we be then deploying 2 ingress controllers?

Im wondering if a better way to do this would be to do this would be for people to use the [official ingress nginx chart]~~ (https://artifacthub.io/packages/helm/ingress-nginx/ingress-nginx) and provide the config to link with that. Similar to how we do with external databases. Then you can just have a single nginx to expose whatever ingress you want.

Im also concerned about maintaining an embedded nginx subchart and having to maintain versions, testing, api changes, etc

@faisal-memon
Copy link
Contributor

faisal-memon commented May 15, 2023

Nm, disregard the above. We are just deploying the ingress resource, not the controller.

@faisal-memon
Copy link
Contributor

Overall this looks good to me. There is some duplication with the 2 ingress templates? Is there any ways to condense that down with a helper?

Copy link
Contributor

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

Few suggestions.

examples/production/README.md Outdated Show resolved Hide resolved
examples/production/README.md Show resolved Hide resolved
examples/production/README.md Show resolved Hide resolved
charts/spire/charts/spire-server/values.yaml Show resolved Hide resolved
kfox1111 and others added 4 commits May 17, 2023 12:28
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Co-authored-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@mrsabath
Copy link
Contributor

Once the ingress support is merged, we should work on instruction documentation on helping setup ingress in major cloud providers in context of the SPIRE deployment. See the issue #293

Co-authored-by: Marco Franssen <marco.franssen@gmail.com>
Copy link
Contributor

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

LGTM

@marcofranssen marcofranssen enabled auto-merge (squash) May 23, 2023 13:54
@marcofranssen marcofranssen merged commit 0ba0388 into main May 23, 2023
135 of 137 checks passed
@marcofranssen marcofranssen deleted the spire-server-ingress branch May 23, 2023 13:59
marcofranssen added a commit that referenced this pull request May 25, 2023
* c1c5b11 Merge pull request #306 from spiffe/remove-1.21
* 0df45e3 Fix up docs
* ed038fe Upgrade to spire 1.6.4 (#308)
* dc5d9cf Fix root README.md
* e4447fd Upgrade Tornjak to new image v1.2.1 (#299)
* 69f402e Update docs
* 38d51d5 Apply suggestions from code review
* a1ba235 Update docs
* 1922085 Fix hooks for K3s (#305)
* 4fb549e Remove 1.21.x testing
* 88efc77 Allow to use spire-server as an upstream authority (#304)
* 0ba0388 Add support for spire-server ingress (#68)
* 4777a30 Bump test chart dependencies (#301)
* 00c2c1a Fix the generated pr so that it runs jobs too (#303)
* dd1ad49 Update images for cve's found by the cronjob (#290)
* 1c69470 Updated Tornjak documenation with Not-for-production labels (#297)
* 7809637 Merge pull request #296 from spiffe/dependabot/github_actions/helm/kind-action-1.7.0
* e61ed17 Merge pull request #295 from spiffe/dependabot/github_actions/sigstore/cosign-installer-3.0.5
* 9975e58 Merge pull request #245 from spiffe/tags
* 7bb7ece Bump helm/kind-action from 1.6.0 to 1.7.0
* f1623a5 Bump sigstore/cosign-installer from 3.0.4 to 3.0.5
* f8db5a3 Fix Tornjak persistence issue (#294)
* b30b412 Tornjak reuse spire-lib.cluster-domain macro (#292)
* 90c9eb5 Fix kubectl-image macro to handle version deprecation
* 300d1cc Apply deprecation of image.version to Tornjak
* d850486 Instead of removing version, first deprecate version
* 59e422b Add documentation for all image.tag values
* d1f3cdb Switch image.version to image.tag
* 31ce704 Cleanup maintainer handbook (#287)
* a2da943 Remove manual dispatch from dummy workflow (#288)
* 807558b Bump helm/kind-action from 1.5.0 to 1.6.0 (#285)
* 3df67db Bump sigstore/cosign-installer from 3.0.3 to 3.0.4 (#286)
* 5505d41 Merge pull request #283 from spiffe/additional-k8s-native-feature-tornjak-frontend
* 391f093 Allow to configure topologySpreadConstraints for tornjak-frontend
* 5cc26d3 Allow to configure tolerations for tornjak-frontend
* 3537161 Allow to configure affinity for tornjak-frontend
* aed6fdf Use the correct kubectl for the cluster (#248)
* ee43c5e Add nodeSelector for tornjak
* fc13cbd Merge pull request #234 from spiffe/tornjak
* ed472aa Update documentation
* a11cfc9 Allow to define the resources for tornjak backend
* 382e0d4 Upgrade Tornjak image to version v1.2.0  (#259)
* 657c460 Update charts/spire/charts/tornjak-frontend/templates/service.yaml
* 7521caf Update charts/spire/charts/spire-server/templates/tornjak-config.yaml
* b64c352 Update charts/spire/charts/spire-server/templates/tests/test-tornjak-connection.yaml
* 6ddf6ab Improve tornjak docs (#276)
* 80d34f0 Use common post-install scripts for testing
* f5efa0c Remove dead macros
* bd86518 Fixing shellcheck
* 91bdea2 Provide minimal resources to prevent accidental crashes due to resource exhaustion
* 1675997 Tornjak global image fix (#228)
* 5e827ee Add Tornjak Tests (#220)
* bdba97b Add empty directory to Tornjak to support npm cache (#224)
* da186c5 Split Tornjak Frontend into separate subchart (#179)
* 6d22126 Add Tornjak
* 2669d8b Add maintainer's handbook. (#265)
* 72596ae Skip tests for docs folders (#281)
* 7c71738 Bump test chart dependencies (#279)
* 05addae Add json to test path (#280)
* 8d9b734 Switch the spire tests to always run (#250)

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
marcofranssen added a commit that referenced this pull request May 25, 2023
* c1c5b11 Merge pull request #306 from spiffe/remove-1.21
* 0df45e3 Fix up docs
* ed038fe Upgrade to spire 1.6.4 (#308)
* dc5d9cf Fix root README.md
* e4447fd Upgrade Tornjak to new image v1.2.1 (#299)
* 69f402e Update docs
* 38d51d5 Apply suggestions from code review
* a1ba235 Update docs
* 1922085 Fix hooks for K3s (#305)
* 4fb549e Remove 1.21.x testing
* 88efc77 Allow to use spire-server as an upstream authority (#304)
* 0ba0388 Add support for spire-server ingress (#68)
* 4777a30 Bump test chart dependencies (#301)
* 00c2c1a Fix the generated pr so that it runs jobs too (#303)
* dd1ad49 Update images for cve's found by the cronjob (#290)
* 1c69470 Updated Tornjak documenation with Not-for-production labels (#297)
* 7809637 Merge pull request #296 from spiffe/dependabot/github_actions/helm/kind-action-1.7.0
* e61ed17 Merge pull request #295 from spiffe/dependabot/github_actions/sigstore/cosign-installer-3.0.5
* 9975e58 Merge pull request #245 from spiffe/tags
* 7bb7ece Bump helm/kind-action from 1.6.0 to 1.7.0
* f1623a5 Bump sigstore/cosign-installer from 3.0.4 to 3.0.5
* f8db5a3 Fix Tornjak persistence issue (#294)
* b30b412 Tornjak reuse spire-lib.cluster-domain macro (#292)
* 90c9eb5 Fix kubectl-image macro to handle version deprecation
* 300d1cc Apply deprecation of image.version to Tornjak
* d850486 Instead of removing version, first deprecate version
* 59e422b Add documentation for all image.tag values
* d1f3cdb Switch image.version to image.tag
* 31ce704 Cleanup maintainer handbook (#287)
* a2da943 Remove manual dispatch from dummy workflow (#288)
* 807558b Bump helm/kind-action from 1.5.0 to 1.6.0 (#285)
* 3df67db Bump sigstore/cosign-installer from 3.0.3 to 3.0.4 (#286)
* 5505d41 Merge pull request #283 from spiffe/additional-k8s-native-feature-tornjak-frontend
* 391f093 Allow to configure topologySpreadConstraints for tornjak-frontend
* 5cc26d3 Allow to configure tolerations for tornjak-frontend
* 3537161 Allow to configure affinity for tornjak-frontend
* aed6fdf Use the correct kubectl for the cluster (#248)
* ee43c5e Add nodeSelector for tornjak
* fc13cbd Merge pull request #234 from spiffe/tornjak
* ed472aa Update documentation
* a11cfc9 Allow to define the resources for tornjak backend
* 382e0d4 Upgrade Tornjak image to version v1.2.0  (#259)
* 657c460 Update charts/spire/charts/tornjak-frontend/templates/service.yaml
* 7521caf Update charts/spire/charts/spire-server/templates/tornjak-config.yaml
* b64c352 Update charts/spire/charts/spire-server/templates/tests/test-tornjak-connection.yaml
* 6ddf6ab Improve tornjak docs (#276)
* 80d34f0 Use common post-install scripts for testing
* f5efa0c Remove dead macros
* bd86518 Fixing shellcheck
* 91bdea2 Provide minimal resources to prevent accidental crashes due to resource exhaustion
* 1675997 Tornjak global image fix (#228)
* 5e827ee Add Tornjak Tests (#220)
* bdba97b Add empty directory to Tornjak to support npm cache (#224)
* da186c5 Split Tornjak Frontend into separate subchart (#179)
* 6d22126 Add Tornjak
* 2669d8b Add maintainer's handbook. (#265)
* 72596ae Skip tests for docs folders (#281)
* 7c71738 Bump test chart dependencies (#279)
* 05addae Add json to test path (#280)
* 8d9b734 Switch the spire tests to always run (#250)

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ingress support for spire-server
4 participants