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

Fix: only one of trust_bundle_path, trust_bundle_url, or insecure_bootstrap can be set #4532

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

SilvaMatteus
Copy link
Contributor

@mnp reported in issue #4530 that it was possible to set trust_bundle_url and insecure_bootstrap in the Agent configuration. There was a test for this case. However, the test was just checking if there was an error. There was an error but not the expected one. This commit also adds expectErrorContains to the test case struct so tests can check the expected error message. Also, more tests added.

Pull Request check list

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

Affected functionality

SPIRE Agent config validation.

Description of change
Now, the validation code checks when the insecure bootstrap option is used with trust bundle config options and returns proper error messages. Tests were updated.

Which issue this PR fixes

#4530

Copy link
Collaborator

@MarcosDY MarcosDY left a comment

Choose a reason for hiding this comment

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

Thanks!! it looks great just some minor nits

cmd/spire-agent/cli/run/run.go Outdated Show resolved Hide resolved
cmd/spire-agent/cli/run/run.go Outdated Show resolved Hide resolved
cmd/spire-agent/cli/run/run_test.go Outdated Show resolved Hide resolved
@SilvaMatteus SilvaMatteus force-pushed the matteus/address-issue-4530 branch from 59fc39c to 66897f7 Compare October 12, 2023 10:36
@SilvaMatteus SilvaMatteus requested a review from MarcosDY October 12, 2023 10:38
@SilvaMatteus
Copy link
Contributor Author

Hey @MarcosDY, thank you for the review. I addressed your comments. Could you take a second look at it?

@SilvaMatteus SilvaMatteus force-pushed the matteus/address-issue-4530 branch from 66897f7 to d8be6af Compare October 13, 2023 16:16
…tstrap can be set

@mnp reported in issue 4530 that it was possible to set trust_bundle_url
and insecure_bootstrap in the Agent configuration. There was a test for
this case. However, the test was just checking if there was an error.
There was an error but not the expected one. This commit also adds
expectErrorContains to the test case struct so tests can check the
expected error message. Also, more tests added.

Signed-off-by: Matteus Silva <silvamatteus@lsd.ufcg.edu.br>
@SilvaMatteus SilvaMatteus force-pushed the matteus/address-issue-4530 branch from d8be6af to fbf4c14 Compare October 15, 2023 20:29
Copy link
Collaborator

@MarcosDY MarcosDY left a comment

Choose a reason for hiding this comment

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

Thanks!!!

@MarcosDY MarcosDY merged commit 71f2414 into spiffe:main Oct 16, 2023
31 checks passed
@amartinezfayo amartinezfayo added this to the 1.8.4 milestone Oct 18, 2023
@rturner3 rturner3 modified the milestones: 1.8.4, 1.8.3 Oct 19, 2023
faisal-memon pushed a commit to faisal-memon/spire that referenced this pull request Dec 2, 2023
…tstrap can be set (spiffe#4532)

@mnp reported in issue 4530 that it was possible to set trust_bundle_url
and insecure_bootstrap in the Agent configuration. There was a test for
this case. However, the test was just checking if there was an error.
There was an error but not the expected one. This commit also adds
expectErrorContains to the test case struct so tests can check the
expected error message. Also, more tests added.

Signed-off-by: Matteus Silva <silvamatteus@lsd.ufcg.edu.br>
Signed-off-by: Faisal Memon <fymemon@yahoo.com>
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.

4 participants