Skip to content

move ports metadata computation to the dedicated method #1231

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
merged 11 commits into from
Feb 21, 2023
Merged

move ports metadata computation to the dedicated method #1231

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0c3ec3e
fix
wind57 Feb 18, 2023
f143df6
no targeted tests
wind57 Feb 18, 2023
7fc0bb9
added tests
wind57 Feb 19, 2023
01596ee
started to add tests
wind57 Feb 20, 2023
5cad385
merge main
wind57 Feb 20, 2023
2d2152a
checkstyle
wind57 Feb 20, 2023
f09961d
Merge branch 'inline-selective-namespaces' into simplify-the-logic-a-bit
wind57 Feb 20, 2023
164c0a1
add some tests
wind57 Feb 20, 2023
436fd0c
format code
wind57 Feb 20, 2023
0f4d015
merged main
wind57 Feb 21, 2023
3ef6fa1
trigger build
wind57 Feb 21, 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
76 changes: 33 additions & 43 deletions ...ava/org/springframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,12 @@
package org.springframework.cloud.kubernetes.fabric8.discovery;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.function.Predicate;

import io.fabric8.kubernetes.api.model.EndpointAddress;
import io.fabric8.kubernetes.api.model.EndpointPort;
import io.fabric8.kubernetes.api.model.EndpointSubset;
import io.fabric8.kubernetes.api.model.Endpoints;
import io.fabric8.kubernetes.api.model.Service;
Expand All @@ -37,10 +35,7 @@
import org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryProperties;
import org.springframework.core.log.LogAccessor;
import org.springframework.util.CollectionUtils;
import org.springframework.util.StringUtils;

import static java.util.stream.Collectors.toMap;
import static org.springframework.cloud.kubernetes.commons.config.ConfigUtils.keysWithPrefix;
import static org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryConstants.NAMESPACE_METADATA_KEY;
import static org.springframework.cloud.kubernetes.fabric8.discovery.KubernetesDiscoveryClientUtils.endpoints;
import static org.springframework.cloud.kubernetes.fabric8.discovery.KubernetesDiscoveryClientUtils.endpointsPort;
Expand Down Expand Up @@ -135,52 +130,47 @@ else if (properties.namespaces().isEmpty()) {
}

private List<ServiceInstance> getNamespaceServiceInstances(EndpointSubsetNS es, String serviceId) {
String namespace = es.namespace();

List<EndpointSubset> subsets = es.endpointSubset();
if (subsets.isEmpty()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

slightly invert the logic here, if subsets is empty, return early.

LOG.debug(() -> "serviceId : " + serviceId + " does not have any subsets");
return List.of();
}

String namespace = es.namespace();
List<ServiceInstance> instances = new ArrayList<>();
if (!subsets.isEmpty()) {
Service service = client.services().inNamespace(namespace).withName(serviceId).get();
Map<String, String> serviceMetadata = serviceMetadata(serviceId, service, properties);
KubernetesDiscoveryProperties.Metadata metadataProps = properties.metadata();

for (EndpointSubset s : subsets) {
// Extend the service metadata map with per-endpoint port information (if
// requested)
Map<String, String> endpointMetadata = new HashMap<>(serviceMetadata);
Copy link
Contributor Author

@wind57 wind57 Feb 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

port metadata has moved to the method that does all the service's metadata computation. imho, this is a lot cleaner, as there is a single entry point where metadata is computed. The rest of the changes is just formatting, because of the dropped if statement (the one that I inverted).

if (metadataProps.addPorts()) {
Map<String, String> ports = s.getPorts().stream()
.filter(port -> StringUtils.hasText(port.getName()))
.collect(toMap(EndpointPort::getName, port -> Integer.toString(port.getPort())));
Map<String, String> portMetadata = keysWithPrefix(ports, metadataProps.portsPrefix());
LOG.debug(() -> "Adding port metadata: " + portMetadata);
endpointMetadata.putAll(portMetadata);
}

if (properties.allNamespaces()) {
endpointMetadata.put(NAMESPACE_METADATA_KEY, namespace);
}
Service service = client.services().inNamespace(namespace).withName(serviceId).get();
Map<String, String> serviceMetadata = serviceMetadata(serviceId, service, properties, subsets);

for (EndpointSubset endpointSubset : subsets) {

List<EndpointAddress> addresses = s.getAddresses();
if (properties.allNamespaces()) {
serviceMetadata.put(NAMESPACE_METADATA_KEY, namespace);
}

if (properties.includeNotReadyAddresses() && !CollectionUtils.isEmpty(s.getNotReadyAddresses())) {
if (addresses == null) {
addresses = new ArrayList<>();
}
addresses.addAll(s.getNotReadyAddresses());
List<EndpointAddress> addresses = endpointSubset.getAddresses();

if (properties.includeNotReadyAddresses()
&& !CollectionUtils.isEmpty(endpointSubset.getNotReadyAddresses())) {
if (addresses == null) {
addresses = new ArrayList<>();
}
addresses.addAll(endpointSubset.getNotReadyAddresses());
}

for (EndpointAddress endpointAddress : addresses) {
int endpointPort = endpointsPort(s, serviceId, properties, service);
String instanceId = null;
if (endpointAddress.getTargetRef() != null) {
instanceId = endpointAddress.getTargetRef().getUid();
}
instances.add(new DefaultKubernetesServiceInstance(instanceId, serviceId, endpointAddress.getIp(),
endpointPort, endpointMetadata,
servicePortSecureResolver.resolve(new ServicePortSecureResolver.Input(endpointPort,
service.getMetadata().getName(), service.getMetadata().getLabels(),
service.getMetadata().getAnnotations()))));
for (EndpointAddress endpointAddress : addresses) {
int endpointPort = endpointsPort(endpointSubset, serviceId, properties, service);
String instanceId = null;
if (endpointAddress.getTargetRef() != null) {
instanceId = endpointAddress.getTargetRef().getUid();
}
instances
.add(new DefaultKubernetesServiceInstance(instanceId, serviceId, endpointAddress.getIp(),
endpointPort, serviceMetadata,
servicePortSecureResolver.resolve(new ServicePortSecureResolver.Input(endpointPort,
service.getMetadata().getName(), service.getMetadata().getLabels(),
service.getMetadata().getAnnotations()))));
}
}

Expand Down
16 changes: 15 additions & 1 deletion ...rg/springframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClientUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.springframework.core.log.LogAccessor;
import org.springframework.util.StringUtils;

import static java.util.stream.Collectors.toMap;
import static org.springframework.cloud.kubernetes.commons.config.ConfigUtils.keysWithPrefix;
import static org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryConstants.HTTP;
import static org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryConstants.HTTPS;
Expand Down Expand Up @@ -121,8 +122,11 @@ static String primaryPortName(KubernetesDiscoveryProperties properties, Service
return primaryPortName;
}

/**
* labels, annotations and ports metadata.
*/
static Map<String, String> serviceMetadata(String serviceId, Service service,
KubernetesDiscoveryProperties properties) {
KubernetesDiscoveryProperties properties, List<EndpointSubset> endpointSubsets) {
Map<String, String> serviceMetadata = new HashMap<>();
KubernetesDiscoveryProperties.Metadata metadataProps = properties.metadata();
if (metadataProps.addLabels()) {
Expand All @@ -138,6 +142,16 @@ static Map<String, String> serviceMetadata(String serviceId, Service service,
serviceMetadata.putAll(annotationMetadata);
}

if (metadataProps.addPorts()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add port metadata to be computed here. We have a single method now responsible for all service metadata.

Map<String, String> ports = endpointSubsets.stream()
.flatMap(endpointSubset -> endpointSubset.getPorts().stream())
.filter(port -> StringUtils.hasText(port.getName()))
.collect(toMap(EndpointPort::getName, port -> Integer.toString(port.getPort())));
Map<String, String> portMetadata = keysWithPrefix(ports, properties.metadata().portsPrefix());
LOG.debug(() -> "Adding port metadata: " + portMetadata + " for serviceId : " + serviceId);
serviceMetadata.putAll(portMetadata);
}

return serviceMetadata;
}

Expand Down
120 changes: 107 additions & 13 deletions ...ringframework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClientUtilsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,13 +333,17 @@ void testServiceMetadataEmpty() {
String labelsPrefix = "";
boolean addAnnotations = false;
String annotationsPrefix = "";
boolean addPorts = false;
String portsPrefix = "";

KubernetesDiscoveryProperties.Metadata metadata = new KubernetesDiscoveryProperties.Metadata(addLabels,
labelsPrefix, addAnnotations, annotationsPrefix, false, "");
labelsPrefix, addAnnotations, annotationsPrefix, addPorts, portsPrefix);
KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L,
true, "", Set.of(), Map.of(), "", metadata, 0, false);
Service service = new ServiceBuilder().build();

Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties);
Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties,
List.of());
Assertions.assertEquals(result.size(), 0);
}

Expand All @@ -355,14 +359,18 @@ void testServiceMetadataAddLabelsNoPrefix(CapturedOutput output) {
String labelsPrefix = "";
boolean addAnnotations = false;
String annotationsPrefix = "";
boolean addPorts = false;
String portsPrefix = "";

KubernetesDiscoveryProperties.Metadata metadata = new KubernetesDiscoveryProperties.Metadata(addLabels,
labelsPrefix, addAnnotations, annotationsPrefix, false, "");
labelsPrefix, addAnnotations, annotationsPrefix, addPorts, portsPrefix);
KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L,
true, "", Set.of(), Map.of(), "", metadata, 0, false);
Service service = new ServiceBuilder()
.withMetadata(new ObjectMetaBuilder().withLabels(Map.of("a", "b")).build()).build();

Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties);
Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties,
List.of());
Assertions.assertEquals(result.size(), 1);
Assertions.assertEquals(result, Map.of("a", "b"));
Assertions.assertTrue(output.getOut().contains("Adding labels metadata: {a=b} for serviceId: my-service"));
Expand All @@ -380,14 +388,18 @@ void testServiceMetadataAddLabelsWithPrefix(CapturedOutput output) {
String labelsPrefix = "prefix-";
boolean addAnnotations = false;
String annotationsPrefix = "";
boolean addPorts = false;
String portsPrefix = "";

KubernetesDiscoveryProperties.Metadata metadata = new KubernetesDiscoveryProperties.Metadata(addLabels,
labelsPrefix, addAnnotations, annotationsPrefix, false, "");
labelsPrefix, addAnnotations, annotationsPrefix, addPorts, portsPrefix);
KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L,
true, "", Set.of(), Map.of(), "", metadata, 0, false);
Service service = new ServiceBuilder()
.withMetadata(new ObjectMetaBuilder().withLabels(Map.of("a", "b", "c", "d")).build()).build();

Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties);
Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties,
List.of());
Assertions.assertEquals(result.size(), 2);
Assertions.assertEquals(result, Map.of("prefix-a", "b", "prefix-c", "d"));
// so that result is deterministic in assertion
Expand All @@ -408,15 +420,19 @@ void testServiceMetadataAddAnnotationsNoPrefix(CapturedOutput output) {
String labelsPrefix = "";
boolean addAnnotations = true;
String annotationsPrefix = "";
boolean addPorts = false;
String portsPrefix = "";

KubernetesDiscoveryProperties.Metadata metadata = new KubernetesDiscoveryProperties.Metadata(addLabels,
labelsPrefix, addAnnotations, annotationsPrefix, false, "");
labelsPrefix, addAnnotations, annotationsPrefix, addPorts, portsPrefix);
KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L,
true, "", Set.of(), Map.of(), "", metadata, 0, false);
Service service = new ServiceBuilder().withMetadata(
new ObjectMetaBuilder().withAnnotations(Map.of("aa", "bb")).withLabels(Map.of("a", "b")).build())
.build();

Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties);
Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties,
List.of());
Assertions.assertEquals(result.size(), 1);
Assertions.assertEquals(result, Map.of("aa", "bb"));
Assertions
Expand All @@ -430,19 +446,23 @@ void testServiceMetadataAddAnnotationsNoPrefix(CapturedOutput output) {
* </pre>
*/
@Test
void testServiceMetadataAddAnnotationsWithPrefixPrefix(CapturedOutput output) {
void testServiceMetadataAddAnnotationsWithPrefix(CapturedOutput output) {
boolean addLabels = false;
String labelsPrefix = "";
boolean addAnnotations = true;
String annotationsPrefix = "prefix-";
boolean addPorts = false;
String portsPrefix = "";

KubernetesDiscoveryProperties.Metadata metadata = new KubernetesDiscoveryProperties.Metadata(addLabels,
labelsPrefix, addAnnotations, annotationsPrefix, false, "");
labelsPrefix, addAnnotations, annotationsPrefix, addPorts, portsPrefix);
KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L,
true, "", Set.of(), Map.of(), "", metadata, 0, false);
Service service = new ServiceBuilder().withMetadata(new ObjectMetaBuilder()
.withAnnotations(Map.of("aa", "bb", "cc", "dd")).withLabels(Map.of("a", "b")).build()).build();

Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties);
Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties,
List.of());
Assertions.assertEquals(result.size(), 2);
Assertions.assertEquals(result, Map.of("prefix-aa", "bb", "prefix-cc", "dd"));
// so that result is deterministic in assertion
Expand All @@ -463,15 +483,19 @@ void testServiceMetadataAddLabelsAndAnnotationsWithPrefix(CapturedOutput output)
String labelsPrefix = "label-";
boolean addAnnotations = true;
String annotationsPrefix = "annotation-";
boolean addPorts = false;
String portsPrefix = "";

KubernetesDiscoveryProperties.Metadata metadata = new KubernetesDiscoveryProperties.Metadata(addLabels,
labelsPrefix, addAnnotations, annotationsPrefix, false, "");
labelsPrefix, addAnnotations, annotationsPrefix, addPorts, portsPrefix);
KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L,
true, "", Set.of(), Map.of(), "", metadata, 0, false);
Service service = new ServiceBuilder().withMetadata(new ObjectMetaBuilder()
.withAnnotations(Map.of("aa", "bb", "cc", "dd")).withLabels(Map.of("a", "b", "c", "d")).build())
.build();

Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties);
Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties,
List.of());
Assertions.assertEquals(result.size(), 4);
Assertions.assertEquals(result,
Map.of("annotation-aa", "bb", "annotation-cc", "dd", "label-a", "b", "label-c", "d"));
Expand All @@ -486,4 +510,74 @@ void testServiceMetadataAddLabelsAndAnnotationsWithPrefix(CapturedOutput output)
output.getOut().contains("Adding annotations metadata: " + annotations + " for serviceId: my-service"));
}

/**
* <pre>
* - ports without prefix are added
* </pre>
*/
@Test
void testServiceMetadataAddPortsWithoutPrefix(CapturedOutput output) {
boolean addLabels = false;
String labelsPrefix = "";
boolean addAnnotations = false;
String annotationsPrefix = "prefix-";
boolean addPorts = true;
String portsPrefix = "";

KubernetesDiscoveryProperties.Metadata metadata = new KubernetesDiscoveryProperties.Metadata(addLabels,
labelsPrefix, addAnnotations, annotationsPrefix, addPorts, portsPrefix);
KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L,
true, "", Set.of(), Map.of(), "", metadata, 0, false);
Service service = new ServiceBuilder().withMetadata(new ObjectMetaBuilder()
.withAnnotations(Map.of("aa", "bb", "cc", "dd")).withLabels(Map.of("a", "b")).build()).build();

List<EndpointSubset> endpointSubsets = List.of(
new EndpointSubsetBuilder().withPorts(new EndpointPortBuilder().withPort(8081).withName("").build())
.build(),
new EndpointSubsetBuilder()
.withPorts(new EndpointPortBuilder().withPort(8080).withName("https").build()).build());

Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties,
endpointSubsets);
Assertions.assertEquals(result.size(), 1);
Assertions.assertEquals(result, Map.of("https", "8080"));
Assertions
.assertTrue(output.getOut().contains("Adding port metadata: {https=8080} for serviceId : my-service"));
}

/**
* <pre>
* - ports without prefix are added
* </pre>
*/
@Test
void testServiceMetadataAddPortsWithPrefix(CapturedOutput output) {
boolean addLabels = false;
String labelsPrefix = "";
boolean addAnnotations = false;
String annotationsPrefix = "prefix-";
boolean addPorts = true;
String portsPrefix = "prefix-";

KubernetesDiscoveryProperties.Metadata metadata = new KubernetesDiscoveryProperties.Metadata(addLabels,
labelsPrefix, addAnnotations, annotationsPrefix, addPorts, portsPrefix);
KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of(), true, 60L,
true, "", Set.of(), Map.of(), "", metadata, 0, false);
Service service = new ServiceBuilder().withMetadata(new ObjectMetaBuilder()
.withAnnotations(Map.of("aa", "bb", "cc", "dd")).withLabels(Map.of("a", "b")).build()).build();

List<EndpointSubset> endpointSubsets = List.of(
new EndpointSubsetBuilder().withPorts(new EndpointPortBuilder().withPort(8081).withName("http").build())
.build(),
new EndpointSubsetBuilder()
.withPorts(new EndpointPortBuilder().withPort(8080).withName("https").build()).build());

Map<String, String> result = KubernetesDiscoveryClientUtils.serviceMetadata("my-service", service, properties,
endpointSubsets);
Assertions.assertEquals(result.size(), 2);
Assertions.assertEquals(result, Map.of("prefix-https", "8080", "prefix-http", "8081"));
Assertions.assertTrue(output.getOut()
.contains("Adding port metadata: {prefix-http=8081, prefix-https=8080} for serviceId : my-service"));
}

}