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

acl: acl replication routine to report the last error message #10612

Merged
merged 3 commits into from
Aug 6, 2021

Conversation

gmichelo
Copy link
Contributor

This PR adds the new field LastErrorMessage to the ACLReplicationStatus endpoint. Whenever the Goroutine executing the runACLReplicator encounters an error, both LastError and LastErrorMessage field in the endpoint are updated.

Test TestACLReplication_TokensRedacted is updated to check also the LastErrorMessage field.

Fixes #10521.

@hashicorp-cla
Copy link

hashicorp-cla commented Jul 14, 2021

CLA assistant check
All committers have signed the CLA.

@vercel vercel bot temporarily deployed to Preview – consul July 14, 2021 10:03 Inactive
acl_endpoint_test.go:507:
        	Error Trace:	acl_endpoint_test.go:507
        	            				retry.go:148
        	            				retry.go:149
        	            				retry.go:103
        	            				acl_endpoint_test.go:504
        	Error:      	Received unexpected error:
        	            	codec.decoder: decodeValue: Cannot decode non-nil codec value into nil error (1 methods)
        	Test:       	TestACLEndpoint_ReplicationStatus
@vercel vercel bot temporarily deployed to Preview – consul July 15, 2021 09:33 Inactive
@dnephin dnephin added the theme/acls ACL and token generation label Jul 20, 2021
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! This looks really good. I left a couple comments below for a couple small improvements.

It would also be great to include a changelog entry. A changelog entry can be added in a file named .changelog/10612.txt (named using the PR number). You can see some other examples in that directory for the exact syntax to use.

@@ -484,11 +484,12 @@ func (s *Server) IsACLReplicationEnabled() bool {
s.config.ACLTokenReplication
}

func (s *Server) updateACLReplicationStatusError() {
func (s *Server) updateACLReplicationStatusError(errorMsg error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are already doing a err != nil check in the caller, we were thinking it would be safer to accept a string here instead of an error. That way no new callers can accidentally pass a nil error.

Suggested change
func (s *Server) updateACLReplicationStatusError(errorMsg error) {
func (s *Server) updateACLReplicationStatusError(errorMsg string) {

which I guess will requires a change to the line below that uses errorMsg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

agent/structs/acl.go Show resolved Hide resolved
@dnephin dnephin added the waiting-reply Waiting on response from Original Poster or another individual in the thread label Aug 3, 2021
@vercel vercel bot temporarily deployed to Preview – consul August 6, 2021 21:36 Inactive
@dnephin dnephin removed the waiting-reply Waiting on response from Original Poster or another individual in the thread label Aug 6, 2021
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@dnephin dnephin merged commit d3325b0 into hashicorp:main Aug 6, 2021
@hc-github-team-consul-core
Copy link
Collaborator

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/423688.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/acls ACL and token generation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ensure replication routines can report what the last error message was
4 participants