Skip to content

catalog: service endpoints inherits protocol from service when workload doesn't have one #18792

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

Merged
ishustava merged 2 commits into from
Sep 14, 2023
Merged

catalog: service endpoints inherits protocol from service when workload doesn't have one #18792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3155360
catalog: service endpoints inherits protocol from service when worklo…
ishustava Sep 13, 2023
e4a5121
PR comments
ishustava Sep 14, 2023
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
9 changes: 7 additions & 2 deletions internal/catalog/internal/controllers/endpoints/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,8 +311,13 @@ func workloadToEndpoint(svc *pbcatalog.Service, data *workloadData) *pbcatalog.E
continue
}

if workloadPort.Protocol != svcPort.Protocol {
// workload port mismatch - ignore it
// If workload protocol is not specified, we will default to service's protocol.
// This is because on some platforms (kubernetes), workload protocol is not always
// known, and so we need to inherit from the service instead.
if workloadPort.Protocol == pbcatalog.Protocol_PROTOCOL_UNSPECIFIED {
workloadPort.Protocol = svcPort.Protocol
Copy link
Member

@zalimeni zalimeni Sep 13, 2023

Choose a reason for hiding this comment

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

(Sorry for the ignorant question, and I think the answer is "no")

Could there be an unintended side-effect of modifying this in place? Thinking of the case where multiple services with different protocols select the same workload (in practice that's probably a bug, but having the protocol on the workload flap could create strange behavior). It looks like this is always a local service-specific value from a network call, but wanted to double check.

Copy link
Contributor Author

@ishustava ishustava Sep 14, 2023

Choose a reason for hiding this comment

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

Great q! I think we are safe here because even though it's a pointer to the workload port, we never write it back to the resource service(we only use it for reads), and so this value never gets re-used in a way that would cause the actual workload to change.

} else if workloadPort.Protocol != svcPort.Protocol {
// Otherwise, there's workload port mismatch - ignore it.
continue
}

Expand Down
45 changes: 45 additions & 0 deletions internal/catalog/internal/controllers/endpoints/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,51 @@ func TestWorkloadToEndpoint_AllAddressesFiltered(t *testing.T) {
require.Nil(t, workloadToEndpoint(service, data))
}

func TestWorkloadToEndpoint_MissingWorkloadProtocol(t *testing.T) {
// This test checks that when a workload is missing its protocol,
// we will default to service's protocol.

service := &pbcatalog.Service{
Ports: []*pbcatalog.ServicePort{
{TargetPort: "test-port", Protocol: pbcatalog.Protocol_PROTOCOL_HTTP},
},
}

workload := &pbcatalog.Workload{
Addresses: []*pbcatalog.WorkloadAddress{
{Host: "127.0.0.1"},
},
Ports: map[string]*pbcatalog.WorkloadPort{
"test-port": {Port: 8080},
},
}

data := &workloadData{
resource: rtest.Resource(types.WorkloadType, "foo").
WithData(t, workload).
Build(),
workload: workload,
}

expected := &pbcatalog.Endpoint{
TargetRef: data.resource.Id,
Addresses: []*pbcatalog.WorkloadAddress{
{Host: "127.0.0.1", Ports: []string{"test-port"}},
},
Ports: map[string]*pbcatalog.WorkloadPort{
"test-port": {Port: 8080, Protocol: pbcatalog.Protocol_PROTOCOL_HTTP},
},
// The health is critical because we are not setting the workload's
// health status. The tests for determineWorkloadHealth will ensure
// that we can properly determine the health status and the overall
// controller tests will prove that the integration works as expected.
HealthStatus: pbcatalog.Health_HEALTH_CRITICAL,
Identity: workload.Identity,
}

prototest.AssertDeepEqual(t, expected, workloadToEndpoint(service, data))
}

func TestServiceUnderManagement(t *testing.T) {
// This test ensures that we can properly detect when a service
// should have endpoints generated for it vs when those endpoints
Expand Down