-
Notifications
You must be signed in to change notification settings - Fork 326
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
Feature/name k8s node #312
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!! Had a few suggested changes, but LGTM with those addressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-approving for posterity 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mostly looks good and works in a non-secure installation; thanks for making these changes!
The one major issue I've found is that it doesn't work with ACLs enabled. That is because the ACL rules for the sync catalog process hard-code the node name to k8s-sync. Once that is fixed, we will also need to make sure that the token is updated when the node name changes. Currently, the behavior of the acl-init job is to not update the token if the Kubernetes secret for a component already exists.
Other than that, I had a question about adding ConsulNodeName
to the Syncer
interface. I don't think this is the pattern we've used here in the past, so I'm curious why you decided to implement it this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on the ACLs! I've added support in the ACL system for configuring and updating the sync node name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ACL changes look good! Everything works as expected from a functional perspective with ACLs enabled. Thank you for adding those validations checking that node names are valid DNS names too!
I have a couple of comments about the code. I'd like to address them before we merge if possible:
- About the
Syncer
interface, I'd prefer that we used our existing pattern of passing the node name directly to the resource struct rather than changing theSyncer
interface. - We don't have any tests in
resource_test.go
orsyncer_test.go
that check that changing the node name works. I left some comments about that in my first review.
I've also left a couple of suggestions.
a4a73d8
to
50e2ccf
Compare
Update of PR #103.
Changes proposed in this PR:
How I've tested this PR:
Using consul-helm #580, I deployed with catalog sync enabled by default.
How I expect reviewers to test this PR:
Similarly.
Checklist:
☑️ Tests added
☑️ CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)
-> to be added when the PR exists to link to