-
Notifications
You must be signed in to change notification settings - Fork 905
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
When services share IP, modifying any service should fail instead of allocating new IP #1230
Conversation
4a18f16
to
3437e4e
Compare
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.
On a cursory look, this LGTM. Quite what we discussed on slack, thanks! :)
controller/controller_test.go
Outdated
Status: statusAssigned([]string{"4.5.6.0"}), | ||
} | ||
if c.SetBalancer(l, "test2", svc2, k8s.EpsOrSlices{}) != k8s.SyncStateError { | ||
t.Fatalf("SetBalancer failed") |
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.
nit: setBalancer did fail, we just expected it to fail
if errors.Is(err, allocator.ErrCannotShareKey) { | ||
c.client.Errorf(svc, "svcCannotShareKey", "current IP not allowed by config:%s", err) | ||
return false |
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.
Can we add a comment explaining why we want to return here? Either link to an issue (there is not, right? We just talked on slack, I guess?) or explain the situation briefly
…allocating newIP Users prefer seeing errors when modify services that are sharing the sameIP using "allow-shared-ip" annotation than getting newly allocated serviceIP Signed-off-by: Mohamed Mahmoud <mmahmoud@redhat.com>
3437e4e
to
588065b
Compare
LGTM |
Users prefer seeing errors when modify services that are sharing the sameIP
using "allow-shared-ip" annotation than getting newly allocated serviceIP
Unit-test (using new test case in controller component to repro the issue)
Signed-off-by: Mohamed Mahmoud mmahmoud@redhat.com