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: use cluster name in label selector of service controller #117

Merged
merged 5 commits into from
Jan 4, 2024

Conversation

NEwa-05
Copy link
Collaborator

@NEwa-05 NEwa-05 commented Jan 3, 2024

What does this PR do?

Patch controller service creation to add cluster name as label (already done for Registry service and Proxy service)

Motivation

When deploying 2 separate instances of Traefikee, the controller is messing up its configuration.

More

  • Yes, I updated the tests accordingly
  • Yes, I ran make test and all the tests passed

@mloiseleur
Copy link
Contributor

@NEwa-05 🤔 Why the " Yes, I updated the tests accordingly" is checked and there is no test. I'm wondering 😅. Anyway, I suggest to add a test on this in order to ensure it won't disappear with time.

@NEwa-05
Copy link
Collaborator Author

NEwa-05 commented Jan 4, 2024

Oups, I did work on another test/PR and checked the update box for this, my bad.

If we do that for this service we need to do it as well for the registry and proxy as well then.
Why not, it makes sense, I'll update the PR with the test for both services.

@NEwa-05
Copy link
Collaborator Author

NEwa-05 commented Jan 4, 2024

Tests has been added for both services.

traefikee/Chart.yaml Outdated Show resolved Hide resolved
@mloiseleur
Copy link
Contributor

See my comments. Otherwise, it looks good !

@NEwa-05 NEwa-05 requested a review from mloiseleur January 4, 2024 10:54
@mloiseleur mloiseleur changed the title [patch] controller service with cluster name label selector feat!: 💥 use cluster name in label selector Jan 4, 2024
@mloiseleur mloiseleur changed the title feat!: 💥 use cluster name in label selector feat!: 💥 use cluster name in label selector of controller Jan 4, 2024
@mloiseleur
Copy link
Contributor

mloiseleur commented Jan 4, 2024

@NEwa-05 I tried to improve the title. Feel free to update it if I missed something. Also, you should test the upgrade, on a Deployment it's a breaking change to update label selector : it's an immutable field. On Service, maybe not.

@NEwa-05
Copy link
Collaborator Author

NEwa-05 commented Jan 4, 2024

I have installed a new deployment and upgraded a new deployment without issues.

I do see the point of the breaking change, but unless the controller pod where not labeled with the cluster name in older helm deployment there will be no change for the service and pod mapping.

For me, it's more a fix than a change since both proxies and registry were already using cluster name within the service label selector, WDYT

@mloiseleur mloiseleur changed the title feat!: 💥 use cluster name in label selector of controller feat: use cluster name in label selector of controller Jan 4, 2024
Copy link
Contributor

@darkweaver87 darkweaver87 left a comment

Choose a reason for hiding this comment

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

LGTM

@mloiseleur mloiseleur changed the title feat: use cluster name in label selector of controller fix: use cluster name in label selector of service controller Jan 4, 2024
@mloiseleur mloiseleur added kind/bug/fix a bug fix and removed kind/enhancement a new or improved feature. labels Jan 4, 2024
@traefiker traefiker merged commit cf17114 into traefik:master Jan 4, 2024
1 check passed
@NEwa-05 NEwa-05 deleted the ctrl_svc branch January 4, 2024 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug/fix a bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants