Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding downstream support to CRD #2885

Merged
merged 2 commits into from
Mar 30, 2022
Merged

Adding downstream support to CRD #2885

merged 2 commits into from
Mar 30, 2022

Conversation

noxora
Copy link
Contributor

@noxora noxora commented Mar 24, 2022

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

Affected functionality

This PR impacts the Spiffe ID CRD resource and the controller in charge of reconciling the Spiffe ID resource

Description of change

This PR adds a field to the CRD which allows the user to mark resource as a downstream entry.

Which issue this PR fixes

fixes #2886

@faisal-memon
Copy link
Contributor

faisal-memon commented Mar 25, 2022

@noxora Can you update the README and add a section on using the new field? Or just a simple update to the SPIFFE ID Custom Resource Example section? https://github.com/spiffe/spire/blob/main/support/k8s/k8s-workload-registrar/mode-crd/README.md

@noxora
Copy link
Contributor Author

noxora commented Mar 25, 2022

@noxora Can you update the README and add a section on using the new field? Or just a simple update to the SPIFFE ID Custom Resource Example section? https://github.com/spiffe/spire/blob/main/support/k8s/k8s-workload-registrar/mode-crd/README.md

@faisal-memon I've gone ahead and added to the example, I don't think it's complicated enough to warrant its own section, since it's just adding a boolean field to the CRD, but it's a good idea to clarify that it's both optional and should default to false

Outside of that, I wanted to test my changes in an environment and make sure that there isn't a nil pointer exception because I'm worried about support/k8s/k8s-workload-registrar/mode-crd/api/spiffeid/v1beta1/spiffeid_types.go being nil and then attempting to set nil when creating the spiffeID

@faisal-memon
Copy link
Contributor

Outside of that, I wanted to test my changes in an environment and make sure that there isn't a nil pointer exception because I'm worried about support/k8s/k8s-workload-registrar/mode-crd/api/spiffeid/v1beta1/spiffeid_types.go being nil and then attempting to set nil when creating the spiffeID

Are you worried the new field will be nil?

@faisal-memon
Copy link
Contributor

The changes look good for manually created entries. Do we need a mechanism for auto-generated entries to have downstream set?

@noxora
Copy link
Contributor Author

noxora commented Mar 25, 2022

Yes, I was worried it would be nil. I don't think this change would impact auto-registration unless that needed to set downstream in the yaml. My other concerns were that there might be other behavior that needs to be updated that I hadn't addressed, since I don't know the entire system very well, just what I've read.

By manually created entries, do you mean those created with yaml files or those entered through a terminal on the server?

@noxora
Copy link
Contributor Author

noxora commented Mar 29, 2022

I wanted to followup and comment that I have had a chance to manually test this and it seems to work. I have linked my terminal's output for the test in slack. I can post it here as well as needed, but I am very confident this doesn't break existing functionality and adds the feature.

Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

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

\o/, thanks @noxora

@azdagron azdagron merged commit 08ee497 into spiffe:main Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for downstream entries in the Spiffe ID CRD
4 participants