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 2 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
15 changes: 8 additions & 7 deletions ...8-autoconfig/src/main/java/org/springframework/cloud/kubernetes/fabric8/Fabric8Utils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package org.springframework.cloud.kubernetes.fabric8;

import io.fabric8.kubernetes.client.KubernetesClient;
import jakarta.annotation.Nullable;
import org.apache.commons.logging.LogFactory;

import org.springframework.cloud.kubernetes.commons.KubernetesNamespaceProvider;
Expand All @@ -39,14 +40,14 @@ private Fabric8Utils() {
private static final LogAccessor LOG = new LogAccessor(LogFactory.getLog(Fabric8Utils.class));

/**
* this method does the namespace resolution for both config map and secrets
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to update this documentation a while ago, as it is re-used in other places as well, and will be used in discovery also at some point in time, its on my TODO list

* implementations. It tries these places to find the namespace:
* this method does the namespace resolution. Namespace is being searched according to the
* order below.
*
* <pre>
* 1. from a normalized source (which can be null)
* 2. from a property 'spring.cloud.kubernetes.client.namespace', if such is present
* 1. from incoming namespace, which can be null.
* 2. from a property 'spring.cloud.kubernetes.client.namespace', if such is present.
* 3. from a String residing in a file denoted by `spring.cloud.kubernetes.client.serviceAccountNamespacePath`
* property, if such is present
* property, if such is present.
* 4. from a String residing in `/var/run/secrets/kubernetes.io/serviceaccount/namespace` file,
* if such is present (kubernetes default path)
* 5. from KubernetesClient::getNamespace, which is implementation specific.
Expand All @@ -60,8 +61,8 @@ private Fabric8Utils() {
* @return application namespace
* @throws NamespaceResolutionFailedException when namespace could not be resolved
*/
public static String getApplicationNamespace(KubernetesClient client, String namespace, String configurationTarget,
KubernetesNamespaceProvider provider) {
public static String getApplicationNamespace(KubernetesClient client, @Nullable String namespace,
String configurationTarget, KubernetesNamespaceProvider provider) {

if (StringUtils.hasText(namespace)) {
LOG.debug(configurationTarget + " namespace : " + namespace);
Expand Down
37 changes: 17 additions & 20 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 @@ -42,9 +42,9 @@
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;
import static org.springframework.cloud.kubernetes.fabric8.discovery.KubernetesDiscoveryClientUtils.serviceMetadata;
import static org.springframework.cloud.kubernetes.fabric8.discovery.KubernetesDiscoveryClientUtils.subsetsFromEndpoints;

/**
* Kubernetes implementation of {@link DiscoveryClient}.
Expand Down Expand Up @@ -104,37 +104,34 @@ public List<ServiceInstance> getInstances(String serviceId) {
Objects.requireNonNull(serviceId);

List<EndpointSubsetNS> subsetsNS = getEndPointsList(serviceId).stream()
.map(x -> subsetsFromEndpoints(x, () -> client.getNamespace())).toList();
.map(KubernetesDiscoveryClientUtils::subsetsFromEndpoints).toList();

List<ServiceInstance> instances = new ArrayList<>();
if (!subsetsNS.isEmpty()) {
for (EndpointSubsetNS es : subsetsNS) {
instances.addAll(getNamespaceServiceInstances(es, serviceId));
}
for (EndpointSubsetNS es : subsetsNS) {
instances.addAll(getNamespaceServiceInstances(es, serviceId));
}

return instances;
}

public List<Endpoints> getEndPointsList(String serviceId) {
if (properties.allNamespaces()) {
return client.endpoints().inAnyNamespace().withField("metadata.name", serviceId)
.withLabels(properties.serviceLabels()).list().getItems();
LOG.debug(() -> "searching for endpoints in all namespaces");
return endpoints(client.endpoints().inAnyNamespace().withNewFilter(), properties, serviceId);
}
if (properties.namespaces().isEmpty()) {
return client.endpoints().withField("metadata.name", serviceId).withLabels(properties.serviceLabels())
.list().getItems();
else if (properties.namespaces().isEmpty()) {
LOG.debug(() -> "searching for endpoints in namespace : " + client.getNamespace());
return endpoints(client.endpoints().withNewFilter(), properties, serviceId);
}
return findEndPointsFilteredByNamespaces(serviceId);
}

private List<Endpoints> findEndPointsFilteredByNamespaces(String serviceId) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

first, I inlined findEndPointsFilteredByNamespaces into getEndPointsList, so the code became a bit simpler to read:

	public List<Endpoints> getEndPointsList(String serviceId) {
		if (properties.allNamespaces()) {
			LOG.debug(() -> "searching for endpoints in all namespaces");
			return client.endpoints().inAnyNamespace().withField("metadata.name", serviceId)
					.withLabels(properties.serviceLabels()).list().getItems();
		}
		else if (properties.namespaces().isEmpty()) {
			LOG.debug(() -> "searching for endpoints in namespace : " + client.getNamespace());
			return client.endpoints().withField("metadata.name", serviceId).withLabels(properties.serviceLabels())
					.list().getItems();
		}
		else {
			LOG.debug(() -> "searching for endpoints in namespaces : " + properties.namespaces());
			List<Endpoints> endpoints = new ArrayList<>();
			for (String namespace : properties.namespaces()) {
				endpoints.addAll(client.endpoints().inNamespace(namespace).withField("metadata.name", serviceId)
					.withLabels(properties.serviceLabels()).list().getItems());
			}
			return endpoints;
		}
	}

Then I noticed that we re-use this withField("metadata.name", serviceId) .withLabels(properties.serviceLabels()).list().getItems() in all 3 branches, so I had to find a way to extract it. I re-used an addition in the fabric8 client called FilterNested, basically you can do:

FilterNested<FilterWatchListDeletable<Endpoints, EndpointsList, Resource<Endpoints>>> filterNested = client.endpoints().inAnyNamespace().withNewFilter();

and then chain the filters you want. So I created a static method:

	static List<Endpoints> endpoints(
			FilterNested<FilterWatchListDeletable<Endpoints, EndpointsList, Resource<Endpoints>>> filterNested,
			KubernetesDiscoveryProperties properties, String serviceId) {
		return filterNested.withField("metadata.name", serviceId).withLabels(properties.serviceLabels()).endFilter()
				.list().getItems();
	}

in KubernetesDiscoveryClientUtils and refactored the code to:

	public List<Endpoints> getEndPointsList(String serviceId) {
		if (properties.allNamespaces()) {
			LOG.debug(() -> "searching for endpoints in all namespaces");
			return endpoints(client.endpoints().inAnyNamespace().withNewFilter(), properties, serviceId);
		}
		else if (properties.namespaces().isEmpty()) {
			LOG.debug(() -> "searching for endpoints in namespace : " + client.getNamespace());
			return endpoints(client.endpoints().withNewFilter(), properties, serviceId);
		}
		else {
			LOG.debug(() -> "searching for endpoints in namespaces : " + properties.namespaces());
			List<Endpoints> endpoints = new ArrayList<>();
			for (String namespace : properties.namespaces()) {
				endpoints.addAll(
						endpoints(client.endpoints().inNamespace(namespace).withNewFilter(), properties, serviceId));
			}
			return endpoints;
		}
	}

I've also adjusted some failing tests and added some more.

List<Endpoints> endpoints = new ArrayList<>();
for (String ns : properties.namespaces()) {
endpoints.addAll(getClient().endpoints().inNamespace(ns).withField("metadata.name", serviceId)
.withLabels(properties.serviceLabels()).list().getItems());
else {
LOG.debug(() -> "searching for endpoints in namespaces : " + properties.namespaces());
List<Endpoints> endpoints = new ArrayList<>();
for (String namespace : properties.namespaces()) {
endpoints.addAll(
endpoints(client.endpoints().inNamespace(namespace).withNewFilter(), properties, serviceId));
}
return endpoints;
}
return endpoints;
}

private List<ServiceInstance> getNamespaceServiceInstances(EndpointSubsetNS es, String serviceId) {
Expand Down
19 changes: 13 additions & 6 deletions ...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 @@ -20,13 +20,16 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.function.Supplier;
import java.util.stream.Collectors;

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.EndpointsList;
import io.fabric8.kubernetes.api.model.Service;
import io.fabric8.kubernetes.client.dsl.FilterNested;
import io.fabric8.kubernetes.client.dsl.FilterWatchListDeletable;
import io.fabric8.kubernetes.client.dsl.Resource;
import org.apache.commons.logging.LogFactory;

import org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryProperties;
Expand All @@ -49,11 +52,8 @@ private KubernetesDiscoveryClientUtils() {

}

static EndpointSubsetNS subsetsFromEndpoints(Endpoints endpoints, Supplier<String> clientNamespace) {
if (endpoints != null && endpoints.getSubsets() != null) {
return new EndpointSubsetNS(endpoints.getMetadata().getNamespace(), endpoints.getSubsets());
}
return new EndpointSubsetNS(clientNamespace.get(), List.of());
static EndpointSubsetNS subsetsFromEndpoints(Endpoints endpoints) {
return new EndpointSubsetNS(endpoints.getMetadata().getNamespace(), endpoints.getSubsets());
}

static int endpointsPort(EndpointSubset endpointSubset, String serviceId, KubernetesDiscoveryProperties properties,
Expand Down Expand Up @@ -141,6 +141,13 @@ static Map<String, String> serviceMetadata(String serviceId, Service service,
return serviceMetadata;
}

static List<Endpoints> endpoints(
FilterNested<FilterWatchListDeletable<Endpoints, EndpointsList, Resource<Endpoints>>> filterNested,
KubernetesDiscoveryProperties properties, String serviceId) {
return filterNested.withField("metadata.name", serviceId).withLabels(properties.serviceLabels()).endFilter()
.list().getItems();
}

private static Optional<Integer> fromMap(Map<String, Integer> existingPorts, String key, String message) {
Integer fromPrimaryPortName = existingPorts.get(key);
if (fromPrimaryPortName == null) {
Expand Down
21 changes: 14 additions & 7 deletions ...ework/cloud/kubernetes/fabric8/discovery/KubernetesDiscoveryClientFilterMetadataTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import io.fabric8.kubernetes.api.model.ServicePort;
import io.fabric8.kubernetes.api.model.ServicePortBuilder;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.dsl.FilterNested;
import io.fabric8.kubernetes.client.dsl.FilterWatchListDeletable;
import io.fabric8.kubernetes.client.dsl.MixedOperation;
import io.fabric8.kubernetes.client.dsl.Resource;
Expand All @@ -53,6 +54,7 @@
import static org.mockito.Mockito.when;
import static org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryProperties.Metadata;

@SuppressWarnings("unchecked")
class KubernetesDiscoveryClientFilterMetadataTest {

private static final KubernetesClient CLIENT = Mockito.mock(KubernetesClient.class);
Expand All @@ -68,6 +70,9 @@ class KubernetesDiscoveryClientFilterMetadataTest {
private final FilterWatchListDeletable<Endpoints, EndpointsList, Resource<Endpoints>> filter = Mockito
.mock(FilterWatchListDeletable.class);

private final FilterNested<FilterWatchListDeletable<Endpoints, EndpointsList, Resource<Endpoints>>> filterNested = Mockito
.mock(FilterNested.class);

@Test
void testAllExtraMetadataDisabled() {
String serviceId = "s";
Expand Down Expand Up @@ -218,24 +223,26 @@ private void setupServiceWithLabelsAndAnnotationsAndPorts(String serviceId, Stri
Service service = new ServiceBuilder().withNewMetadata().withNamespace(namespace).withLabels(labels)
.withAnnotations(annotations).endMetadata().withNewSpec().withPorts(getServicePorts(ports)).endSpec()
.build();
when(this.serviceOperation.withName(serviceId)).thenReturn(this.serviceResource);
when(this.serviceResource.get()).thenReturn(service);
when(CLIENT.services()).thenReturn(this.serviceOperation);
when(CLIENT.services().inNamespace(anyString())).thenReturn(this.serviceOperation);
when(serviceOperation.withName(serviceId)).thenReturn(serviceResource);
when(serviceResource.get()).thenReturn(service);
when(CLIENT.services()).thenReturn(serviceOperation);
when(CLIENT.services().inNamespace(anyString())).thenReturn(serviceOperation);

ObjectMeta objectMeta = new ObjectMeta();
objectMeta.setNamespace(namespace);

Endpoints endpoints = new EndpointsBuilder().withMetadata(objectMeta).addNewSubset()
.addAllToPorts(getEndpointPorts(ports)).addNewAddress().endAddress().endSubset().build();

when(CLIENT.endpoints()).thenReturn(this.endpointsOperation);
when(CLIENT.endpoints()).thenReturn(endpointsOperation);
when(endpointsOperation.withNewFilter()).thenReturn(filterNested);

EndpointsList endpointsList = new EndpointsList(null, Collections.singletonList(endpoints), null, null);
when(filter.list()).thenReturn(endpointsList);
when(filter.withLabels(anyMap())).thenReturn(filter);
when(filterNested.withLabels(anyMap())).thenReturn(filterNested);

when(CLIENT.endpoints().withField(eq("metadata.name"), eq(serviceId))).thenReturn(filter);
when(filterNested.withField(eq("metadata.name"), eq(serviceId))).thenReturn(filterNested);
when(filterNested.endFilter()).thenReturn(filter);

}

Expand Down
22 changes: 9 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 @@ -46,31 +46,27 @@
@ExtendWith(OutputCaptureExtension.class)
class KubernetesDiscoveryClientUtilsTests {

@Test
void testSubsetsFromEndpointsNullEndpoints() {
EndpointSubsetNS result = KubernetesDiscoveryClientUtils.subsetsFromEndpoints(null, () -> "default");
Assertions.assertNotNull(result);
Assertions.assertEquals(result.endpointSubset(), List.of());
Assertions.assertEquals(result.namespace(), "default");
}

@Test
void testSubsetsFromEndpointsEmptySubsets() {
Endpoints endpoints = new EndpointsBuilder()
.withMetadata(new ObjectMetaBuilder().withNamespace("non-default").build()).build();
EndpointSubsetNS result = KubernetesDiscoveryClientUtils.subsetsFromEndpoints(endpoints, () -> "default");
EndpointSubsetNS result = KubernetesDiscoveryClientUtils.subsetsFromEndpoints(endpoints);
Assertions.assertNotNull(result);
Assertions.assertEquals(result.endpointSubset(), List.of());
Assertions.assertEquals(result.namespace(), "non-default");
}

@Test
void testSubsetsFromEndpointsNullSubsets() {
void testSubsetsFromEndpointsNonEmptySubsets() {
Endpoints endpoints = new EndpointsBuilder().withSubsets((List<EndpointSubset>) null)
.withMetadata(new ObjectMetaBuilder().withNamespace("non-default").build()).build();
EndpointSubsetNS result = KubernetesDiscoveryClientUtils.subsetsFromEndpoints(endpoints, () -> "default");
.withMetadata(new ObjectMetaBuilder().withNamespace("default").build())
.withSubsets(
new EndpointSubsetBuilder().withPorts(new EndpointPortBuilder().withPort(8080).build()).build())
.build();
EndpointSubsetNS result = KubernetesDiscoveryClientUtils.subsetsFromEndpoints(endpoints);
Assertions.assertNotNull(result);
Assertions.assertEquals(result.endpointSubset(), List.of());
Assertions.assertEquals(result.endpointSubset().size(), 1);
Assertions.assertEquals(result.endpointSubset().get(0).getPorts().get(0).getPort(), 8080);
Assertions.assertEquals(result.namespace(), "default");
}

Expand Down