Distinguish between different unnamed node ports when preserving #4026
Conversation
Signed-off-by: Scott Seago <sseago@redhat.com>
sseago
force-pushed
the
service-action-unnamed-nodeport
branch
from
fe8782e
to
8d714d3
Compare
| @@ -212,6 +215,7 @@ func TestServiceActionExecute(t *testing.T) { | |||
| { | |||
| NodePort: 8080, | |||
| }, | |||
| {}, | |||
There was a problem hiding this comment.
We preserve the NodePort: 8080 on restore because the last applied config has an entry for it, but the NodePort: 9090 should be removed, since there is no corresponding config entry. The nodeport removal code doesn't remove entire port entries, just the nodeport element. In a real cluster, the nodeport element is just one attribute of the port. Look at a service yaml output in a real cluster for more context. Also, note the other tests in this file to see examples where there are multiple attributes and just nodeport is removed.
There was a problem hiding this comment.
I added this to the test because the bug in the code was removing or preserving all nodeports based on the presence or absence of any unnamed node ports, rather than just preserving the specific unnamed ports found in the config.
|
Is this PR able to fix this #2308? |
|
@jenting I think that issue is more complicated -- but also, some of the existing nodePort-related changes since that issue was created may have fixed it anyway. This PR is actually just a minor update to the logic around the last-applied-configuration annotation. The logic applied was faulty in the case of unnamed ports, and this fixed that. It's possible that the overall nodeport-deleting code does resolve the referenced issue, but if so it's not a result of this PR. It's worth revisiting #2308 and seeing whether it's still an issue. But this PR isn't intended to fix that issue. |
| @@ -135,8 +136,14 @@ func deleteNodePorts(service *corev1api.Service) error { | |||
| } | |||
|
|
|||
| for i, port := range service.Spec.Ports { | |||
| if !explicitNodePorts.Has(port.Name) { | |||
| service.Spec.Ports[i].NodePort = 0 | |||
| if port.Name != "" { | |||
There was a problem hiding this comment.
@sseago
Could you please explain why this check is necessary, and could you please add a test case to justify this condition?
There was a problem hiding this comment.
The intent of the preserve node ports functionality is if a named port is found in the last applied configuration that matches a named port in the Service, its nodePort field is retained, and if a named port in the service does not have a match in that annotation, the nodePort field is removed. For unnamed node ports, we need to perform the same matching on the nodePort value.
The prior version of this inserted an empty entry into explicitNodePorts if there was no name. The effect was that if any unnamed node ports were listed in the configuration annotation, then all unnamed node ports were preserved, which does not match the intended goal. The fix is to track named nodeports by name and unnamed nodeports by port number. So if this port has a name, we compare it to the node port name list from the annotation, and if this port has no name, then we compare its port number to the unnamed nodeport number list from the annotation.
There was a problem hiding this comment.
For a test of this, see the modified service_action_test below. I've added a second NodePort value that doesn't match the annotation, so we see one is preserved and one is not for the port.Name == "" case, and the previously existing named node port test case handles testing this for the port.Name != "" case.
There was a problem hiding this comment.
Without this fix, the below test case modification resulted in a test failure since it preserved both unnamed ports rather than just the one that matched.
|
@reasonerjt Did you have any further questions for @sseago regarding this change or are you okay for it to be merged? Thanks :) |
Signed-off-by: Scott Seago sseago@redhat.com
Thank you for contributing to Velero!
Please add a summary of your change
Service action should delete nodeports unless specified in last-applied-configuration. The current code attempts to do this as follows:
if the port is named, then preserve it if the named port is in the configuration. Otherwise, if the port is unnamed, then preserve it if the unnamed port is in the configuration.
The previous code lumps all unnamed ports together -- it preserves all if any exist in the configuration. The updated match check compares port number when name is missing. unit test update exposes the prior bug.
Does your change fix a particular issue?
Fixes #4025
Please indicate you've done the following:
/kind changelog-not-required.site/content/docs/main.