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

Add customPlugins and unsupportedBuiltInPlugins sections to spire-server #198

Merged
merged 21 commits into from
Aug 24, 2023

Conversation

kfox1111
Copy link
Contributor

@kfox1111 kfox1111 commented Apr 7, 2023

This patch enables end users to configure external plugins in the spire-server config. Unsupported internal plugins are not able to be set.

@marcofranssen marcofranssen force-pushed the spire-config-as-yaml-converted-json branch from 728e2f8 to 6fc5363 Compare April 13, 2023 19:31
@marcofranssen marcofranssen force-pushed the spire-config-as-yaml-converted-json branch 3 times, most recently from 42c2fbb to e846291 Compare April 14, 2023 07:53
@kfox1111 kfox1111 marked this pull request as ready for review April 14, 2023 18:41
@marcofranssen marcofranssen force-pushed the spire-config-as-yaml-converted-json branch from 474b5ac to eaca827 Compare April 17, 2023 09:53
@marcofranssen marcofranssen force-pushed the spire-config-as-yaml-converted-json branch from eaca827 to e3b0d00 Compare April 17, 2023 10:02
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.

I prefer to have a couple settings in here like we already have.

spire-server:
  nodeAttestor:
    xYZ:
      enabled: true

and same for other plugins.

spire-server:
  upstreamAuthority:
    xYZ:
      enabled: true

We could then loop the keys in those objects and people can still add their own plugins within there.

This probably adds some duplication in templates, but it also makes it more straight forward and less magic to merge things in the templates, so easier to understand and digest.

Futhermore the API will me better documented.

Base automatically changed from spire-config-as-yaml-converted-json to main April 17, 2023 15:50
@kfox1111
Copy link
Contributor Author

I prefer to have a couple settings in here like we already have.

spire-server:
  nodeAttestor:
    xYZ:
      enabled: true

and same for other plugins.

spire-server:
  upstreamAuthority:
    xYZ:
      enabled: true

We could then loop the keys in those objects and people can still add their own plugins within there.

This probably adds some duplication in templates, but it also makes it more straight forward and less magic to merge things in the templates, so easier to understand and digest.

Futhermore the API will me better documented.

There are... dragons there... But seems like there may be them here either way, maybe just less so with the plugin section?...

The issue we're going to hit, is in toYaml'ing the sections. If we're taking values, such as xYZ.enabled and jsoning it over to spire, its going to get confused by settings that are for helm, not for the actual server. We could remove them from the doc before toPrettyJson, but that can get messy too. With plugin being a different section, it allowed for the k8s config and the raw yaml things to be separate. Its all still able to be validated, as its proper yaml in helm values, so its not so bad.

I'll do a quick inventory though and see how bad removing the k8s stuff from the json at runtime would be....

@faisal-memon faisal-memon added this to the 0.8.x milestone Apr 18, 2023
@kfox1111 kfox1111 marked this pull request as ready for review August 8, 2023 17:34
Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@kfox1111 kfox1111 changed the title Add customPlugins and unsupportedBuiltInPluginsPlugins sections to spire-server Add customPlugins and unsupportedBuiltInPlugins sections to spire-server Aug 8, 2023
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
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
Copy link
Contributor

@edwbuck I think I incorporated all the feedback you gave via slack. Thanks again for the review.

Would be good to have the feedback in the PR. I see there is currently still a block by @edwbuck even though there is no change requested in this PR.

@faisal-memon faisal-memon modified the milestones: 0.12.0, 0.13.0 Aug 17, 2023
@edwbuck edwbuck dismissed their stale review August 22, 2023 16:04

code has changed a bit since the review, and it is not current

@edwbuck
Copy link
Contributor

edwbuck commented Aug 22, 2023

@edwbuck I think I incorporated all the feedback you gave via slack. Thanks again for the review.

Would be good to have the feedback in the PR. I see there is currently still a block by @edwbuck even though there is no change requested in this PR.

My concern was that the configuration section for the item was open ended. Basically it took a YAML object.

There are a number of subtle issues here. First, the config file output is not YAML, it is HCL. The syntax won't mesh up under many scenarios. Also, since we are simply passing a whole object, there isn't checking that the object is an array or an object, or something even more simple (a value).

That latter part can be fixed, but to have valid HCL output, you might need to force this into a multi-line string. Finally, the strings depend on the kind of the custom plugin / non-custom plugin, where some non-custom plugins are obviously without configuration, while others contain standard configurations (memory keyStore vs disk keyStore) that differ from partially required fields as in a custom keyStore.

@kfox1111
Copy link
Contributor Author

@edwbuck I think I incorporated all the feedback you gave via slack. Thanks again for the review.

Would be good to have the feedback in the PR. I see there is currently still a block by @edwbuck even though there is no change requested in this PR.

My concern was that the configuration section for the item was open ended. Basically it took a YAML object.

There are a number of subtle issues here. First, the config file output is not YAML, it is HCL. The syntax won't mesh up under many scenarios. Also, since we are simply passing a whole object, there isn't checking that the object is an array or an object, or something even more simple (a value).

That latter part can be fixed, but to have valid HCL output, you might need to force this into a multi-line string. Finally, the strings depend on the kind of the custom plugin / non-custom plugin, where some non-custom plugins are obviously without configuration, while others contain standard configurations (memory keyStore vs disk keyStore) that differ from partially required fields as in a custom keyStore.

We need some relief valve for users able to do unsupported stuff until we can support them. They have to sign up for it being unsupported though, which I think is a good tradeoff.

The config file output isn't YAML. We build up the config in YAML since thats easy to do in helm, then convert it to JSON (also easy to do in helm) and spire has the ability to take its config in either HCL or JSON. so we have a direct path via JSON. This avoids all the ugliness of generating HCL. This was done a while ago in #113

@faisal-memon faisal-memon enabled auto-merge (squash) August 24, 2023 20:59
@faisal-memon faisal-memon merged commit 51cba5b into main Aug 24, 2023
43 checks passed
@faisal-memon faisal-memon deleted the yaml-plugins branch August 24, 2023 21:13
marcofranssen added a commit that referenced this pull request Sep 15, 2023
* 38f0af4 Add support for Vault UpstreamAuthority plugin - K8s Auth (#415)
* 1aac2d4 Bump docker/login-action from 2 to 3
* 1f90867 Allow configuration of priorityClassName on spire-server statefulset (#480)
* 9ad2ed5 option to configure agent sds (#479)
* 693ce08 Remove ## values section from chart readms
* 65d5695 Migrate to readme-generator for helm maintained by bitnami (#431)
* dcc60a2 fix(charts/spire/spire-agent): podmonitor templating (#478)
* 48adb88 Bump actions/checkout from 3.6.0 to 4.0.0
* d1f52d6 Bump sigstore/cosign-installer from 3.1.1 to 3.1.2 (#473)
* 5273f4e Switch mysql and postgresql tests to HA Production configs (#471)
* e81a59a ingress-nginx production tests and spiffe-oidc-discovery-provider example (#136)
* b05175e Bump actions/checkout from 3.5.3 to 3.6.0
* 51cba5b Add customPlugins and unsupportedBuiltInPlugins sections to spire-server (#198)
* f4ee2c2 Bump github.com/onsi/ginkgo/v2 from 2.11.0 to 2.12.0 in /tests (#468)
* c817dd2 support datastore password secret created by external resources (#464)
* 71ac5af Split steps in check-versions wf for easier debugging (#467)
* d91403a Scan for updates to new images (#466)
* 7a5456e Bump helm.sh/helm/v3 from 3.11.3 to 3.12.3 in /tests (#462)
* cbe0001 Federation test (#423)

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
marcofranssen added a commit that referenced this pull request Sep 15, 2023
* 38f0af4 Add support for Vault UpstreamAuthority plugin - K8s Auth (#415)
* 1aac2d4 Bump docker/login-action from 2 to 3
* 1f90867 Allow configuration of priorityClassName on spire-server statefulset (#480)
* 9ad2ed5 option to configure agent sds (#479)
* 693ce08 Remove ## values section from chart readms
* 65d5695 Migrate to readme-generator for helm maintained by bitnami (#431)
* dcc60a2 fix(charts/spire/spire-agent): podmonitor templating (#478)
* 48adb88 Bump actions/checkout from 3.6.0 to 4.0.0
* d1f52d6 Bump sigstore/cosign-installer from 3.1.1 to 3.1.2 (#473)
* 5273f4e Switch mysql and postgresql tests to HA Production configs (#471)
* e81a59a ingress-nginx production tests and spiffe-oidc-discovery-provider example (#136)
* b05175e Bump actions/checkout from 3.5.3 to 3.6.0
* 51cba5b Add customPlugins and unsupportedBuiltInPlugins sections to spire-server (#198)
* f4ee2c2 Bump github.com/onsi/ginkgo/v2 from 2.11.0 to 2.12.0 in /tests (#468)
* c817dd2 support datastore password secret created by external resources (#464)
* 71ac5af Split steps in check-versions wf for easier debugging (#467)
* d91403a Scan for updates to new images (#466)
* 7a5456e Bump helm.sh/helm/v3 from 3.11.3 to 3.12.3 in /tests (#462)
* cbe0001 Federation test (#423)

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
marcofranssen added a commit that referenced this pull request Sep 15, 2023
* 38f0af4 Add support for Vault UpstreamAuthority plugin - K8s Auth (#415)
* 1aac2d4 Bump docker/login-action from 2 to 3
* 1f90867 Allow configuration of priorityClassName on spire-server statefulset (#480)
* 9ad2ed5 option to configure agent sds (#479)
* 693ce08 Remove ## values section from chart readms
* 65d5695 Migrate to readme-generator for helm maintained by bitnami (#431)
* dcc60a2 fix(charts/spire/spire-agent): podmonitor templating (#478)
* 48adb88 Bump actions/checkout from 3.6.0 to 4.0.0
* d1f52d6 Bump sigstore/cosign-installer from 3.1.1 to 3.1.2 (#473)
* 5273f4e Switch mysql and postgresql tests to HA Production configs (#471)
* e81a59a ingress-nginx production tests and spiffe-oidc-discovery-provider example (#136)
* b05175e Bump actions/checkout from 3.5.3 to 3.6.0
* 51cba5b Add customPlugins and unsupportedBuiltInPlugins sections to spire-server (#198)
* f4ee2c2 Bump github.com/onsi/ginkgo/v2 from 2.11.0 to 2.12.0 in /tests (#468)
* c817dd2 support datastore password secret created by external resources (#464)
* 71ac5af Split steps in check-versions wf for easier debugging (#467)
* d91403a Scan for updates to new images (#466)
* 7a5456e Bump helm.sh/helm/v3 from 3.11.3 to 3.12.3 in /tests (#462)
* cbe0001 Federation test (#423)

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
marcofranssen added a commit that referenced this pull request Sep 15, 2023
* 38f0af4 Add support for Vault UpstreamAuthority plugin - K8s Auth (#415)
* 1aac2d4 Bump docker/login-action from 2 to 3
* 1f90867 Allow configuration of priorityClassName on spire-server statefulset (#480)
* 9ad2ed5 option to configure agent sds (#479)
* 693ce08 Remove ## values section from chart readms
* 65d5695 Migrate to readme-generator for helm maintained by bitnami (#431)
* dcc60a2 fix(charts/spire/spire-agent): podmonitor templating (#478)
* 48adb88 Bump actions/checkout from 3.6.0 to 4.0.0
* d1f52d6 Bump sigstore/cosign-installer from 3.1.1 to 3.1.2 (#473)
* 5273f4e Switch mysql and postgresql tests to HA Production configs (#471)
* e81a59a ingress-nginx production tests and spiffe-oidc-discovery-provider example (#136)
* b05175e Bump actions/checkout from 3.5.3 to 3.6.0
* 51cba5b Add customPlugins and unsupportedBuiltInPlugins sections to spire-server (#198)
* f4ee2c2 Bump github.com/onsi/ginkgo/v2 from 2.11.0 to 2.12.0 in /tests (#468)
* c817dd2 support datastore password secret created by external resources (#464)
* 71ac5af Split steps in check-versions wf for easier debugging (#467)
* d91403a Scan for updates to new images (#466)
* 7a5456e Bump helm.sh/helm/v3 from 3.11.3 to 3.12.3 in /tests (#462)
* cbe0001 Federation test (#423)

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.

None yet

4 participants