Update Istio/SPIRE demo to use SPIRE Controller Manager #12680

jaellio · 2023-02-15T18:47:40Z

Please provide a description of what this PR is for.

Updates the Istio/SPIRE integration doc and demo to reference
the SPIRE Controller Manager instead of the k8s workload
registrar.

Notable Changes:

Updates csi driver volume to explicitly specify readOnly. Encountered errors when not setting readOnly on the volume and only specifying it on the volumeMount.
Clarifies that the SPIRE Controller Manager does not provide the same automatic workload registration workflow as the k8s workload registrar. A ClusterSPIFFEID must be created.
Adds instructions to create a ClusterSPIFFEID, label the ingress-gateway deployment, and register an example workload
Updates the command to check if the identities were created for the workloads (ingress-gateway and sleep)
- The previous command (kubectl exec -i -t $SPIRE_SERVER_POD -n spire -c spire-server -- /bin/sh -c "bin/spire-server entry show -socketPath /run/spire/sockets/server.sock") failed with the following error: Error: connection error: desc = "transport: error while dialing: dial unix /run/spire/sockets: connect: no such file or directory"
- The new command is a simplified version: kubectl exec -t $SPIRE_SERVER_POD -n spire -c spire-server -- ./bin/spire-server entry show
Updates example for creating federated registration entries using the ClusterSPIFFEID
Adds a docs test for the automated Istio/SPIRE demo

Resolves #12577

And to help us figure out who should review this PR, please
put an X in all the areas that this PR affects.

istio-testing · 2023-02-15T18:47:42Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

jaellio · 2023-02-15T18:48:02Z

/test all

frankbu · 2023-02-15T19:14:43Z

This doc looks like a good candidate for a doc test: https://github.com/istio/istio.io/tree/master/tests#test-authoring-overview

That way you can be sure it continues to work over time.

jaellio · 2023-02-15T21:50:17Z

This doc looks like a good candidate for a doc test: https://github.com/istio/istio.io/tree/master/tests#test-authoring-overview

That way you can be sure it continues to work over time.

Great idea, I'll add a test. Thanks for the info!

content/en/docs/ops/integrations/spire/index.md

bleggett

LGTM

content/en/docs/ops/integrations/spire/automatic_registration_test.sh

jaellio · 2023-02-24T20:29:05Z

This PR should be retested after istio/istio#43346 is merged to verify the doc.test.profile_none_istio.io succeeds.

ericvn · 2023-02-24T22:07:13Z

I hope that istio/istio#43346 will merge today, so the automation of the weekend should pull that commit into the test. Once that passes tests, is approved, and merged, we can retest this.

jaellio · 2023-02-24T23:56:47Z

/test doc.test.profile_none

kfaseela · 2023-02-25T07:06:48Z

/retest

jaellio · 2023-02-27T23:24:12Z

Retesting after the completion of #12788
/retest

linux-foundation-easycla · 2023-02-28T01:41:57Z

The committers listed above are authorized under a signed CLA.

✅ login: jaellio / name: Jackie Elliott (e354a89, 016b009, 15f914a, 96f4c7d, 97eae19)

kfaseela · 2023-02-28T07:44:58Z

As a part of moving Istio under the CNCF, we will be updating the CLA requirements for contributing Istio to use the CNCF CLA. This will be a multi-stage process, which has already started. If you have not already signed the CNCF CLA, you may find instructions to do so here. We expect to remove the requirement for signing the Google CLA in the upcoming weeks. Thank you for your patience, and please respond here if you run into difficulty with the new CLA process.

jaellio · 2023-03-01T00:08:50Z

content/en/docs/ops/integrations/spire/automatic_registration_test.sh

+# @setup profile=none
+set +u # Do not exit when value is unset. CHECK_FILE in the IstioOperator might be unset
+snip_define_istio_operator
+if ! istioctl install --set tag="$TAG" --set hub="$HUB" --skip-confirmation -f ./istio.yaml


Note: Since the istio-ingressgateway will not be ready, this command will take at least 5 minutes to execute before it returns an error. I can set the readiness timeout on install to be less than 5 minutes, but I don't want this to cause the other components to fail.

Would it be reasonable to set the readiness timeout to 30s or 1m?

I'm not sure I understand exactly why it is expected to time out, but would it be better if you use istioctl manifest generate | kubectl apply instead of istioctl install? https://preliminary.istio.io/latest/docs/setup/install/istioctl/#generate-a-manifest-before-installation

I was installing Istio before applying the ClusterSPIFFEID. Therefore the istio-ingressgateway wasn't registered with SPIRE and couldn't obtain its certificates. The install command would timeout after 5 min of waiting for the gateway to be ready. This was the expected behavior given how the demo was organized. To improve the customer experience and testability of the demo I have modified the demo's workflow so that the ClusterSPIFFEID is created prior to installing Istio. The Istio installation should complete successfully with this change.

Manager instead of k8s workload registration. Signed-off-by: jaellio <[email protected]>

controller manager. During cleanup, removes generated istio.yaml and chaim.pem files. Updates label to spiffe.io/spire-managed-identity. Signed-off-by: jaellio <[email protected]>

Signed-off-by: jaellio <[email protected]>

Created ClusterSPIFFEID before install istio. Previously install would fail because the ingress gateway wasn't registered/ Signed-off-by: jaellio <[email protected]>

frankbu

A couple of minor suggestions, but otherwise LGTM.

content/en/docs/ops/integrations/spire/index.md

frankbu · 2023-03-02T15:31:08Z

content/en/docs/ops/integrations/spire/index.md

+1. Apply the configuration:
+
+    {{< text syntax=bash snip_id=none >}}
+    $ istioctl install --skip-confirmation -f ./istio.yaml


IIUC, this call is expected to hang for 5 mins. If so, maybe it would be better to avoid that by using:

Suggested change

$ istioctl install --skip-confirmation -f ./istio.yaml

$ istioctl manifest generate --skip-confirmation -f ./istio.yaml | kubectl apply -f -

I was thinking of adding tests and updating the demo for the manual registration option in a separate PR. Should I still include this change here? You are correct that this call will hang. Currently, this PR is focussing on the automatic registration/SPIRE controller manager integration option.

I would, but it's up to you, esp. if you are planning to change it further in a followup PR soon.

I want to make sure using istioctl manifest generate doesn't break anything unexpectedly which I can confirm with a docs test for the manual registration option (and through manual testing). For example, if I updated istioctl install to istioctl manifest generate I would need to include the cmd to create the istio-system ns since it isn't created automatically.

content/en/docs/ops/integrations/spire/index.md

Signed-off-by: jaellio <[email protected]>

* Update Istio/SPIRE integration demo to use SPIRE Controller Manager instead of k8s workload registration. Signed-off-by: jaellio <[email protected]> * Adds test for automatic workload registration via the SPIRE controller manager. During cleanup, removes generated istio.yaml and chaim.pem files. Updates label to spiffe.io/spire-managed-identity. Signed-off-by: jaellio <[email protected]> * Adds missing newline Signed-off-by: jaellio <[email protected]> * Fix spelling error Signed-off-by: jaellio <[email protected]> * Add missing ns flag on role and rolebinding resource commands Signed-off-by: jaellio <[email protected]> * Delete sleep resources and uninstall before SPIRE Signed-off-by: jaellio <[email protected]> * Reconfigures demo so istio install is not expected to fail. Created ClusterSPIFFEID before install istio. Previously install would fail because the ingress gateway wasn't registered/ Signed-off-by: jaellio <[email protected]> * Remove references to v1.14 and update required version to 1.14+ Signed-off-by: jaellio <[email protected]> * Fix lint errors Signed-off-by: jaellio <[email protected]> --------- Signed-off-by: jaellio <[email protected]>

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Feb 15, 2023

istio-policy-bot added area/security kind/docs labels Feb 15, 2023

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 15, 2023

bleggett mentioned this pull request Feb 15, 2023

Use SPIRE Controller Manager for quickstart demo istio/istio#43346

Merged

bleggett reviewed Feb 15, 2023

View reviewed changes

content/en/docs/ops/integrations/spire/index.md Outdated Show resolved Hide resolved

jaellio force-pushed the updatespiredemo branch from 686ba83 to 016b009 Compare February 24, 2023 19:01

istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 24, 2023

jaellio marked this pull request as ready for review February 24, 2023 19:11

jaellio requested a review from a team as a code owner February 24, 2023 19:11

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Feb 24, 2023

bleggett approved these changes Feb 24, 2023

View reviewed changes

kfaseela reviewed Feb 24, 2023

View reviewed changes

content/en/docs/ops/integrations/spire/automatic_registration_test.sh Outdated Show resolved Hide resolved

jaellio added the do-not-merge Block automatic merging of a PR. label Feb 24, 2023

jaellio commented Mar 1, 2023

View reviewed changes

jaellio removed the do-not-merge Block automatic merging of a PR. label Mar 1, 2023

jaellio added 2 commits March 1, 2023 15:10

Update Istio/SPIRE integration demo to use SPIRE Controller

c64d811

Manager instead of k8s workload registration. Signed-off-by: jaellio <[email protected]>

Adds test for automatic workload registration via the SPIRE

d01a807

controller manager. During cleanup, removes generated istio.yaml and chaim.pem files. Updates label to spiffe.io/spire-managed-identity. Signed-off-by: jaellio <[email protected]>

jaellio added 4 commits March 1, 2023 15:10

Adds missing newline

45dafa1

Signed-off-by: jaellio <[email protected]>

Fix spelling error

812f0b1

Signed-off-by: jaellio <[email protected]>

Add missing ns flag on role and rolebinding resource commands

dab4acb

Signed-off-by: jaellio <[email protected]>

Delete sleep resources and uninstall before SPIRE

4da7716

Signed-off-by: jaellio <[email protected]>

jaellio force-pushed the updatespiredemo branch from 66659fe to ccb9c57 Compare March 1, 2023 23:11

Reconfigures demo so istio install is not expected to fail.

10e3341

Created ClusterSPIFFEID before install istio. Previously install would fail because the ingress gateway wasn't registered/ Signed-off-by: jaellio <[email protected]>

jaellio force-pushed the updatespiredemo branch from ccb9c57 to 10e3341 Compare March 1, 2023 23:22

frankbu reviewed Mar 2, 2023

View reviewed changes

jaellio added 2 commits March 2, 2023 10:35

Remove references to v1.14 and update required version to 1.14+

2c3e0ec

Signed-off-by: jaellio <[email protected]>

Fix lint errors

b84b686

Signed-off-by: jaellio <[email protected]>

frankbu approved these changes Mar 3, 2023

View reviewed changes

ericvn approved these changes Mar 3, 2023

View reviewed changes

istio-testing merged commit 127c4f2 into istio:master Mar 3, 2023

jaellio mentioned this pull request Mar 8, 2023

Update Istio/SPIRE demo manual registration workflow #12859

Open

jaellio mentioned this pull request May 11, 2024

Add jaellio as security maintainer istio/community#1399

Merged

6 tasks

	$ istioctl install --skip-confirmation -f ./istio.yaml
	$ istioctl manifest generate --skip-confirmation -f ./istio.yaml \| kubectl apply -f -

Update Istio/SPIRE demo to use SPIRE Controller Manager #12680

Update Istio/SPIRE demo to use SPIRE Controller Manager #12680

Uh oh!

Conversation

jaellio commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-testing commented Feb 15, 2023

Uh oh!

jaellio commented Feb 15, 2023

Uh oh!

frankbu commented Feb 15, 2023

Uh oh!

jaellio commented Feb 15, 2023

Uh oh!

Uh oh!

bleggett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaellio commented Feb 24, 2023

Uh oh!

ericvn commented Feb 24, 2023

Uh oh!

jaellio commented Feb 24, 2023

Uh oh!

kfaseela commented Feb 25, 2023

Uh oh!

jaellio commented Feb 27, 2023

Uh oh!

linux-foundation-easycla bot commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfaseela commented Feb 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frankbu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jaellio commented Feb 15, 2023 •

edited

Loading

linux-foundation-easycla bot commented Feb 28, 2023 •

edited

Loading