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.

Sign up for GitHub

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

Jump to bottom

Delete ACL token only for services that are deregistered. #601

Merged
merged 2 commits into from
Aug 12, 2021
Merged

Delete ACL token only for services that are deregistered. #601

merged 2 commits into from
Aug 12, 2021

Conversation

ishustava
Copy link
Contributor

@ishustava ishustava commented Aug 11, 2021 •
edited
Loading

Fixes #599

Changes proposed in this PR:

  • Instead of deleting any ACLs tokens for pods that the endpoints object doesn't point to, we now only delete tokens whenever services are deregistered. This is to fix an issue when endpoints controller has not yet processed the pod change for an endpoints object.

How I've tested this PR:
unit tests

How I expect reviewers to test this PR:
code review

Checklist:

  • Tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

ndhanushkodi
ndhanushkodi approved these changes Aug 11, 2021
View reviewed changes
Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

Looks great! The comments are helpful and clear! just had a non-blocking thought on the changelog

CHANGELOG.md Outdated Show resolved Hide resolved
sadjamz
sadjamz approved these changes Aug 11, 2021
View reviewed changes
Copy link
Contributor

@sadjamz sadjamz left a comment

Choose a reason for hiding this comment

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

Couldn't have written it better myself :))

@ishustava ishustava merged commit 1c7a8e6 into master Aug 12, 2021
@ishustava ishustava deleted the aclRaceCondition branch August 12, 2021 04:09
lawliet89 pushed a commit to lawliet89/consul-k8s that referenced this pull request Sep 13, 2021
@lkysow
Consul ent namespaces support for controller (hashicorp#601)
1f3bf28 
* Consul ent namespaces support for controller
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndhanushkodi ndhanushkodi ndhanushkodi approved these changes

@sadjamz sadjamz sadjamz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

endpoints-controller: race condition between ACL tokens and pods in endpoint resource
3 participants
@ishustava @ndhanushkodi @sadjamz