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

feat: add group ID option #319

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

nico151999
Copy link
Contributor

The intention is to make group IDs configurable. I opened an issue in order to target this. Before working on the respective changes in the linkerd2 repo I thought it would make sense to get this feature implemented here first.

This pull request adds the option to set the group ID via command line and via annotation. The tests available via just ran successfully in the dev container and some manual tests also ran well. I had to tweak my setup a little bit in order to make it work in WSL2 behind an HTTP proxy, so my setup might differ from the reference setup. Still, I assume that my tweaks did not have an impact on the outcome of the tests. It is also worth mentioning that tweaking IP tables manually is not part of my daily business. I just mention it for the reviewers to pay special attention. Thanks in advance for the review. Looking forward to this feature getting merged.

@mateiidavid
Copy link
Member

@nico151999, I'm sorry it took me so long to review this. This looks good! I tested it out in the control plane and there are no side effects (that I've noticed).

I'll ack & merge it if you can do a rebase. We've changed some proxy-init code (including making the package public, so I anticipate it won't be a super clean merge). If CI's green and happy, we're good to go.

@nico151999
Copy link
Contributor Author

@mateiidavid I probably won't get round to it for the next few days, but yes, I will do a rebase, probably some time next week. Nice to see some feedback. Once this is merged I would like to ask you to have a look at my PR in the main linkerd2 repo, too (as linked in the issue). It will probably need a rebase, too, but I would like to have the attention of someone who can approve the changes so that I won't refactor it redundantly and need to do yet another rebase then. Anyway, let's first get the changes to proxy-init in. Thanks!

@mateiidavid mateiidavid self-assigned this Apr 12, 2024
@mateiidavid mateiidavid self-requested a review April 12, 2024 16:15
This adds the option to set the group ID via command line and via annotation

Signed-off-by: Nico Feulner <[email protected]>
@nico151999 nico151999 force-pushed the nico151999/add-group-id-option branch from fa692a2 to 42e82d5 Compare April 15, 2024 07:55
@nico151999
Copy link
Contributor Author

@mateiidavid rebased 😄

Copy link
Member

@mateiidavid mateiidavid left a comment

Choose a reason for hiding this comment

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

Thanks @nico151999!

@mateiidavid mateiidavid merged commit 386fc62 into linkerd:main Apr 15, 2024
13 checks passed
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.

3 participants