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

Fix spire-server configmap UpstreamAuthority/aws_pca and KeyManager/a… #489

Merged
merged 5 commits into from
Sep 20, 2023
Merged

Fix spire-server configmap UpstreamAuthority/aws_pca and KeyManager/a… #489

merged 5 commits into from
Sep 20, 2023

Conversation

unufr33
Copy link
Contributor

@unufr33 unufr33 commented Sep 18, 2023

Current configmap template renders to a wrong KeyManager and UpstreamAuthority configurarion when aws_kms and aws_pca are enabled and container is crashing. The proposed changes will fix the issue.

@unufr33
Fix spire-server configmap UpstreamAuthority/aws_pca and KeyManager/a…
19854c4 
…ws_kms

Signed-off-by: unufree <geno.velkov@gmail.com>
@kfox1111
Copy link
Contributor

Thanks for the fixes.

Could you please add a quick section to tests/unit/spire_test.go that turns the features on to see if it renders ok? Not having any tests at all, even "does it render to valid yaml" makes it hard to not accidentally break.

@faisal-memon faisal-memon added the bug Something isn't working label Sep 18, 2023
@faisal-memon
Copy link
Contributor

@unufr33 Do you have time to write the test? No problem if not, we can merge without it. We understand people are busy.

@unufr33
Add unit test
2b40037 
Signed-off-by: unufree <geno.velkov@gmail.com>
@unufr33
Copy link
Contributor Author

unufr33 commented Sep 19, 2023

Hope this works

unufr33 and others added 2 commits September 19, 2023 22:50
@unufr33 @faisal-memon
Update tests/unit/spire_test.go
d6c9679 
Co-authored-by: Faisal Memon <fymemon@yahoo.com>
Signed-off-by: unufr33 <129618334+unufr33@users.noreply.github.com>
@unufr33 @faisal-memon
Update tests/unit/spire_test.go
e0faaee 
Co-authored-by: Faisal Memon <fymemon@yahoo.com>
Signed-off-by: unufr33 <129618334+unufr33@users.noreply.github.com>
@unufr33
Execute gofmt
929b207 
Signed-off-by: unufree <geno.velkov@gmail.com>
@faisal-memon
Copy link
Contributor

Is this test failure related to this pr?

INFO: Downloading bootstrap version 'v2.0.0' of cosign to verify version to be installed...
      https://storage.googleapis.com/cosign-releases/v2.0.0/cosign-linux-amd64
Error: Process completed with exit code 56.

@unufr33
Copy link
Contributor Author

unufr33 commented Sep 20, 2023

I don't think the failed tests are related to my changes. My changes are in the "checks" job and it passes. The failed jobs passed several times before that .Can you please rerun ?

@kfox1111
Copy link
Contributor

Is this test failure related to this pr?

INFO: Downloading bootstrap version 'v2.0.0' of cosign to verify version to be installed...
      https://storage.googleapis.com/cosign-releases/v2.0.0/cosign-linux-amd64
Error: Process completed with exit code 56.

Looks like a transient network issue. I just re-triggered the failed job to rerun. I'm expecting it to pass.

@kfox1111
Copy link
Contributor

I don't think the failed tests are related to my changes. My changes are in the "checks" job and it passes. The failed jobs passed several times before that .Can you please rerun ?

Yeah. The tests look great. Thanks for writing them. :)

kfox1111
kfox1111 approved these changes Sep 20, 2023
View reviewed changes
Copy link
Contributor

@kfox1111 kfox1111 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@faisal-memon faisal-memon merged commit d3091a8 into spiffe:main Sep 20, 2023
43 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kfox1111 kfox1111 kfox1111 approved these changes

@faisal-memon faisal-memon faisal-memon approved these changes

@marcofranssen marcofranssen Awaiting requested review from marcofranssen

@dfeldman dfeldman Awaiting requested review from dfeldman

@mrsabath mrsabath Awaiting requested review from mrsabath

@edwbuck edwbuck Awaiting requested review from edwbuck

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@unufr33 @kfox1111 @faisal-memon