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.

Sign up for GitHub

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

Distinguish between different unnamed node ports when preserving #4026

Merged
merged 1 commit into from
Sep 3, 2021
Merged

Distinguish between different unnamed node ports when preserving #4026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelogs/unreleased/4026-sseago
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Distinguish between different unnamed node ports when preserving
13 changes: 10 additions & 3 deletions pkg/restore/service_action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ func deleteNodePorts(service *corev1api.Service) error {
// to the last-applied-config annotation. We'll retain these values, and
// clear out any other (presumably auto-assigned) NodePort values.
explicitNodePorts := sets.NewString()
unnamedPortInts := sets.NewInt()
lastAppliedConfig, ok := service.Annotations[annotationLastAppliedConfig]
if ok {
appliedServiceUnstructured := new(map[string]interface{})
Expand Down Expand Up @@ -123,7 +124,7 @@ func deleteNodePorts(service *corev1api.Service) error {
portName, ok := p["name"]
if !ok {
// unnamed port
explicitNodePorts.Insert("")
unnamedPortInts.Insert(nodePortInt)
} else {
explicitNodePorts.Insert(portName.(string))
}
Expand All @@ -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 != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

@sseago
Could you please explain why this check is necessary, and could you please add a test case to justify this condition?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

if !explicitNodePorts.Has(port.Name) {
service.Spec.Ports[i].NodePort = 0
}
} else {
if !unnamedPortInts.Has(int(port.NodePort)) {
service.Spec.Ports[i].NodePort = 0
}
}
}

Expand Down
4 changes: 4 additions & 0 deletions pkg/restore/service_action_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,9 @@ func TestServiceActionExecute(t *testing.T) {
{
NodePort: 8080,
},
{
NodePort: 9090,
},
},
},
},
Expand All @@ -212,6 +215,7 @@ func TestServiceActionExecute(t *testing.T) {
{
NodePort: 8080,
},
{},
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

},
},
},
Expand Down