Support downstream entries/spiffeIds (closes #43) #44

keeganwitt · 2023-01-09T18:39:10Z

I looked at all the places using Admin and used that to determine what I probably needed for adding Downstream. I haven't tested that this actually works though.

keeganwitt · 2023-01-09T19:44:44Z

This code seems to only support a k8s_psat parent, but if you are using another method (for example x509pop to have secondary SPIRE servers in different K8s clusters), then there's no way to specify the parent in the CRD.

parentID, err := spiffeid.FromPathf(trustDomain, "/spire/agent/k8s_psat/%s/%s", clusterName, node.UID)

I think this would need to be changed in this PR also.

faisal-memon · 2023-01-10T18:14:15Z

Do we need to bump the version of the crd for this change?

faisal-memon · 2023-01-10T18:20:16Z

This code seems to only support a k8s_psat parent, but if you are using another method (for example x509pop to have secondary SPIRE servers in different K8s clusters), then there's no way to specify the parent in the CRD.
parentID, err := spiffeid.FromPathf(trustDomain, "/spire/agent/k8s_psat/%s/%s", clusterName, node.UID)
I think this would need to be changed in this PR also.

Were you able to specify the parent with k8s-workload-registrar?

keeganwitt · 2023-01-11T04:29:33Z

This code seems to only support a k8s_psat parent, but if you are using another method (for example x509pop to have secondary SPIRE servers in different K8s clusters), then there's no way to specify the parent in the CRD.
parentID, err := spiffeid.FromPathf(trustDomain, "/spire/agent/k8s_psat/%s/%s", clusterName, node.UID)
I think this would need to be changed in this PR also.
Were you able to specify the parent with k8s-workload-registrar?

Yes, this is what we're doing today with the Registrar.

keeganwitt · 2023-01-11T04:35:30Z

Do we need to bump the version of the crd for this change?

Yea, I suppose so. Bumped.

pkg/spireentry/entries.go

faisal-memon · 2023-01-12T22:54:20Z

Do we need to bump the version of the crd for this change?

Yea, I suppose so. Bumped.

I think I was mistaken, we don't need to bump the version for just adding a new field. https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#adding-a-field

api/v1alpha1/clusterspiffeid_types.go

keeganwitt · 2023-01-13T15:16:14Z

Created #54 for the parentID part.

keeganwitt · 2023-01-13T15:17:55Z

Do we need to bump the version of the crd for this change?

Yea, I suppose so. Bumped.

I think I was mistaken, we don't need to bump the version for just adding a new field. https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#adding-a-field

OK. Dropped that commit.

faisal-memon · 2023-01-13T18:51:47Z

Thanks @keeganwitt for the updates. Changes look good to me.

azdagron

Thanks for opening this contribution, @keeganwitt!

Looks like a spot was missed in api/v1alpha1/clusterspiffeid_webhook.go where the spec is parsed into the ParsedClusterSPIFFEID struct. Downstream is not being set on the parsed spec, which ultimately results in the downstream bit not being set on the registration entry.

I'd encourage you to test out your changes before submitting. It would have found the issue.

azdagron · 2023-01-17T13:41:11Z

config/crd/bases/spire.spiffe.io_clusterspiffeids.yaml

@@ -48,6 +48,9 @@ spec:
                items:
                  type: string
                type: array
+              downstream:
+                description: Downstream indicates that the entry describes a downstream SPIRE server.


Was make manifests run after adding the field to the CRD? The formatting seems different here than what was generated for me locally...

No. I didn't know about that script. I did this by hand. We should document this in a CONTRIBUTING.md.

azdagron · 2023-01-17T13:42:06Z

demo/config/cluster1/crd/spire.spiffe.io_clusterspiffeids.yaml

@@ -48,6 +48,9 @@ spec:
                items:
                  type: string
                type: array
+              downstream:


This might need to be updated based the answer to my question about make manifests...

azdagron · 2023-01-17T13:42:22Z

demo/config/cluster2/crd/spire.spiffe.io_clusterspiffeids.yaml

@@ -48,6 +48,9 @@ spec:
                items:
                  type: string
                type: array
+              downstream:


This might need to be updated based the answer to my question about make manifests...

keeganwitt · 2023-01-17T14:47:28Z

Thanks for opening this contribution, @keeganwitt!

Looks like a spot was missed in api/v1alpha1/clusterspiffeid_webhook.go where the spec is parsed into the ParsedClusterSPIFFEID struct. Downstream is not being set on the parsed spec, which ultimately results in the downstream bit not being set on the registration entry.

I'd encourage you to test out your changes before submitting. It would have found the issue.

No testing, as noted in the description, this was my best guess what changes were needed. Do you have a recommendation for how to test? I couldn't test with my deployment because we're still using the registrar. Sounds like we could use a CONTRIBUTING.md?

azdagron · 2023-01-17T15:05:29Z

We definitely need a CONTRIBUTING.md :)

To test, I normally run make docker-build and then make some changes to the integration test scripts to exercise things. See demo/test.sh.

Signed-off-by: Keegan Witt <[email protected]>

azdagron

Thanks, @keeganwitt. Looks good.

keeganwitt requested review from azdagron and MarcosDY as code owners January 9, 2023 18:39

keeganwitt force-pushed the support_downstream_entries branch from 81f9a0b to fe2e29d Compare January 9, 2023 18:43

keeganwitt force-pushed the support_downstream_entries branch 2 times, most recently from 000292a to 7f9644d Compare January 10, 2023 05:46

faisal-memon mentioned this pull request Jan 10, 2023

Migration guide from k8s-workload-registrar #40

Merged

keeganwitt commented Jan 11, 2023

View reviewed changes

pkg/spireentry/entries.go Outdated Show resolved Hide resolved

keeganwitt force-pushed the support_downstream_entries branch from 78b5009 to aad2dbe Compare January 12, 2023 01:00

faisal-memon reviewed Jan 12, 2023

View reviewed changes

api/v1alpha1/clusterspiffeid_types.go Outdated Show resolved Hide resolved

keeganwitt force-pushed the support_downstream_entries branch from aad2dbe to 3d5c98b Compare January 13, 2023 15:17

azdagron reviewed Jan 17, 2023

View reviewed changes

Support downstream entries/spiffeIds (closes spiffe#43)

ef4cfeb

Signed-off-by: Keegan Witt <[email protected]>

keeganwitt force-pushed the support_downstream_entries branch from 3d5c98b to ef4cfeb Compare January 17, 2023 16:25

azdagron approved these changes Jan 17, 2023

View reviewed changes

Merge branch 'main' into support_downstream_entries

eca2dcc

azdagron merged commit 426e7b6 into spiffe:main Jan 17, 2023

keeganwitt deleted the support_downstream_entries branch January 17, 2023 19:45

Support downstream entries/spiffeIds (closes #43) #44

Support downstream entries/spiffeIds (closes #43) #44

Uh oh!

Conversation

keeganwitt commented Jan 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keeganwitt commented Jan 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

faisal-memon commented Jan 10, 2023

Uh oh!

faisal-memon commented Jan 10, 2023

Uh oh!

keeganwitt commented Jan 11, 2023

Uh oh!

keeganwitt commented Jan 11, 2023

Uh oh!

Uh oh!

faisal-memon commented Jan 12, 2023

Uh oh!

Uh oh!

keeganwitt commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keeganwitt commented Jan 13, 2023

Uh oh!

faisal-memon commented Jan 13, 2023

Uh oh!

azdagron left a comment

Choose a reason for hiding this comment

Uh oh!

azdagron Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

keeganwitt Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

azdagron Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

azdagron Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

keeganwitt commented Jan 17, 2023

Uh oh!

azdagron commented Jan 17, 2023

Uh oh!

azdagron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keeganwitt commented Jan 9, 2023 •

edited

Loading

keeganwitt commented Jan 9, 2023 •

edited

Loading

keeganwitt commented Jan 13, 2023 •

edited

Loading