Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
032014e
test
wind57 Dec 4, 2021
c3d0ad2
Merge branch 'main' of https://github.com/spring-cloud/spring-cloud-k…
wind57 Dec 13, 2021
10889fd
fix @Nested tests not running
wind57 Dec 16, 2021
8f0375a
merged main
wind57 Dec 17, 2021
96ebf42
Merge branch 'main' of https://github.com/spring-cloud/spring-cloud-k…
wind57 Jan 5, 2022
db8403e
Merge branch 'main' of https://github.com/spring-cloud/spring-cloud-k…
wind57 Jan 12, 2022
90a5345
Merge branch 'main' of https://github.com/spring-cloud/spring-cloud-k…
wind57 Jan 12, 2022
b041c00
trigger again
wind57 Jan 13, 2022
9580444
Merge branch 'main' of https://github.com/spring-cloud/spring-cloud-k…
wind57 Mar 15, 2022
a34ac47
Add renovate.json
renovate[bot] Jul 13, 2022
6613f78
Merge branch 'spring-cloud:main' into main
wind57 Jul 13, 2022
1b3eae9
Merge pull request #1 from wind57/renovate/configure
wind57 Jul 13, 2022
48ba4cb
started work
wind57 Mar 12, 2023
ff026e8
continue work
wind57 Mar 12, 2023
845eff0
continue work
wind57 Mar 12, 2023
146d191
before integration tests
wind57 Mar 12, 2023
cc278e1
add integration test
wind57 Mar 12, 2023
f26d4aa
add documentation
wind57 Mar 12, 2023
80b49a8
review comments
wind57 Mar 13, 2023
6f8e59d
fix test
wind57 Mar 13, 2023
0c595d6
fix test
wind57 Mar 13, 2023
a2a0af1
trigger build
wind57 Mar 13, 2023
4275382
Merge branch 'spring-cloud:main' into main
wind57 Mar 13, 2023
618f25a
Delete renovate.json
wind57 Mar 14, 2023
100a9cd
Delete delme.sh
wind57 Mar 14, 2023
5d3d2a8
Merge branch 'main' into fix-1170
wind57 Mar 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
4 changes: 4 additions & 0 deletions docs/src/main/asciidoc/discovery-client.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,10 @@ By default all of the ports and their names will be added to the metadata of the

As said before, if you want to get the list of `ServiceInstance` to also include the `ExternalName` type services, you need to enable that support via: `spring.cloud.kubernetes.discovery.include-external-name-services=true`. As such, when calling `DiscoveryClient::getInstances` those will be returned also. You can distinguish between `ExternalName` and any other types by inspecting `ServiceInstance::getMetadata` and lookup for a field called `type`. This will be the type of the service returned : `ExternalName`/`ClusterIP`, etc.

`ServiceInstance` can include the labels and annotations of specific pods from the underlying service instance. To obtain such information, you need to also enable:

`spring.cloud.kubernetes.discovery.metadata.add-pod-labels=true` and/or `spring.cloud.kubernetes.discovery.metadata.add-pod-annotations=true`. At the moment, such functionality is present only in the fabric8 client implementation, but will be added to the kubernetes native client in a later release.

If, for any reason, you need to disable the `DiscoveryClient`, you can set the following property in `application.properties`:

====
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@
* @param cluster the cluster the service resides in.
*/
public record DefaultKubernetesServiceInstance(String instanceId, String serviceId, String host, int port,
Map<String, String> metadata, boolean secure, String namespace,
String cluster) implements KubernetesServiceInstance {
Map<String, String> metadata, boolean secure, String namespace, String cluster, Map<String, String> podLabels,
Map<String, String> podAnnotations) implements KubernetesServiceInstance {

/**
* @param instanceId the id of the instance.
Expand All @@ -48,7 +48,12 @@ public record DefaultKubernetesServiceInstance(String instanceId, String service
*/
public DefaultKubernetesServiceInstance(String instanceId, String serviceId, String host, int port,
Map<String, String> metadata, boolean secure) {
this(instanceId, serviceId, host, port, metadata, secure, null, null);
this(instanceId, serviceId, host, port, metadata, secure, null, null, Map.of(), Map.of());
}

public DefaultKubernetesServiceInstance(String instanceId, String serviceId, String host, int port,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

keep the previous constructor intact and introduce a new one

Map<String, String> metadata, boolean secure, String namespace, String cluster) {
this(instanceId, serviceId, host, port, metadata, secure, namespace, cluster, Map.of(), Map.of());
}

@Override
Expand Down Expand Up @@ -101,6 +106,16 @@ public String getCluster() {
return this.cluster;
}

@Override
public Map<String, String> podLabels() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to not just put this in the metadata?

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 was afraid that service labels and pod labels could create a mess if put together, like what if labels colide? i thought that separating them would be cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that is kind of what I was saying here....I suppose there is no other way, we either run the risk of colliding or we separate things.

Since we have some freedom here with this new property could we make it Map<String, Map<String, String> call it podMetadata and put both labels and annotations in that map?

Copy link
Contributor Author

@wind57 wind57 Mar 13, 2023

Choose a reason for hiding this comment

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

this is where I was hesitant too, I thought not everyone is interested in one or the other... I don't know. I'll listen to your advice here. I mean what if you want to separate your logic on labels only, for example?

Copy link
Contributor

Choose a reason for hiding this comment

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

if you enable one or the other only that one would be added to the map

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my turn to miss-read!

Map<String, Map<String, String>

so something like : labels = {a = b} and annotations = {c = d}, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

correct

return podLabels;
}

@Override
public Map<String, String> podAnnotations() {
return podAnnotations;
}

private URI createUri(String scheme, String host, int port) {
// assume ExternalName type of service
if (port == -1) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,15 +92,30 @@ public KubernetesDiscoveryProperties(@DefaultValue("true") boolean enabled, bool
* @param annotationsPrefix prefix for the annotations
* @param addPorts include ports as metadata
* @param portsPrefix prefix for the ports, by default it is "port."
* @param addPodLabels add pod labels as part of the response.
* @param addPodAnnotations add pod annotations as part of the response.
*/
public record Metadata(@DefaultValue("true") boolean addLabels, String labelsPrefix,
@DefaultValue("true") boolean addAnnotations, String annotationsPrefix,
@DefaultValue("true") boolean addPorts, @DefaultValue("port.") String portsPrefix) {
@DefaultValue("true") boolean addPorts, @DefaultValue("port.") String portsPrefix, boolean addPodLabels,
boolean addPodAnnotations) {

@ConstructorBinding
public Metadata {

}

public Metadata(@DefaultValue("true") boolean addLabels, String labelsPrefix,
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 a new constructor not to break the public existing one

@DefaultValue("true") boolean addAnnotations, String annotationsPrefix,
@DefaultValue("true") boolean addPorts, @DefaultValue("port.") String portsPrefix) {

this(addLabels, labelsPrefix, addAnnotations, annotationsPrefix, addPorts, portsPrefix, false, false);
}

/**
* Default instance.
*/
public static final Metadata DEFAULT = new Metadata(true, null, true, null, true, "port.");
public static final Metadata DEFAULT = new Metadata(true, null, true, null, true, "port.", false, false);

}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package org.springframework.cloud.kubernetes.commons.discovery;

import java.util.Map;

import org.springframework.cloud.client.ServiceInstance;

/**
Expand All @@ -29,4 +31,12 @@ sealed public interface KubernetesServiceInstance extends ServiceInstance permit

String getCluster();

default Map<String, String> podLabels() {
return Map.of();
}

default Map<String, String> podAnnotations() {
return Map.of();
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* Copyright 2013-2023 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.cloud.kubernetes.commons.discovery;

import java.util.Map;

/**
* @author wind57
*/
public record PodMetadata(Map<String, String> podLabels, Map<String, String> podAnnotations) {

/**
* pod labels and annotations are empty.
*/
public static PodMetadata EMPTY = new PodMetadata(Map.of(), Map.of());

}
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,24 @@ void testDefaultConstructor() {
assertThat(m.annotationsPrefix()).isNull();
assertThat(m.addPorts()).isTrue();
assertThat(m.portsPrefix()).isEqualTo("port.");
assertThat(m.addPodLabels()).isFalse();
assertThat(m.addPodAnnotations()).isFalse();
}

@Test
void testSpringBindingFields() {
new ApplicationContextRunner().withUserConfiguration(Config.class)
.withPropertyValues("spring.cloud.kubernetes.discovery.metadata.labelsPrefix=labelsPrefix")
.withPropertyValues("spring.cloud.kubernetes.discovery.metadata.labelsPrefix=labelsPrefix",
"spring.cloud.kubernetes.discovery.metadata.add-pod-annotations=true",
"spring.cloud.kubernetes.discovery.metadata.add-pod-labels=true")
.run(context -> {
KubernetesDiscoveryProperties props = context.getBean(KubernetesDiscoveryProperties.class);
assertThat(props).isNotNull();
assertThat(props.metadata().labelsPrefix()).isEqualTo("labelsPrefix");
assertThat(props.metadata().addPorts()).isTrue();
assertThat(props.metadata().portsPrefix()).isEqualTo("port.");
assertThat(props.metadata().addPodLabels()).isTrue();
assertThat(props.metadata().addPodAnnotations()).isTrue();
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ void testFirstConstructor() {
assertThat(instance.getScheme()).isEqualTo("https");
assertThat(instance.getNamespace()).isEqualTo("spring-k8s");
assertThat(instance.getCluster()).isNull();
assertThat(instance.podLabels()).isEqualTo(Map.of());
assertThat(instance.podAnnotations()).isEqualTo(Map.of());
}

@Test
Expand All @@ -61,6 +63,27 @@ void testSecondConstructor() {
assertThat(instance.getScheme()).isEqualTo("https");
assertThat(instance.getNamespace()).isEqualTo("spring-k8s");
assertThat(instance.getCluster()).isEqualTo("cluster");
assertThat(instance.podLabels()).isEqualTo(Map.of());
assertThat(instance.podAnnotations()).isEqualTo(Map.of());
}

@Test
void testThirdConstructor() {
DefaultKubernetesServiceInstance instance = new DefaultKubernetesServiceInstance("instanceId", "serviceId",
"host", 8080, Map.of("a", "b"), true, "spring-k8s", "cluster", Map.of("a", "b"), Map.of("c", "d"));

assertThat(instance.getInstanceId()).isEqualTo("instanceId");
assertThat(instance.getServiceId()).isEqualTo("serviceId");
assertThat(instance.getHost()).isEqualTo("host");
assertThat(instance.getPort()).isEqualTo(8080);
assertThat(instance.isSecure()).isTrue();
assertThat(instance.getUri()).isEqualTo(URI.create("https://host:8080"));
assertThat(instance.getMetadata()).isEqualTo(Map.of("a", "b"));
assertThat(instance.getScheme()).isEqualTo("https");
assertThat(instance.getNamespace()).isEqualTo("spring-k8s");
assertThat(instance.getCluster()).isEqualTo("cluster");
assertThat(instance.podLabels()).isEqualTo(Map.of("a", "b"));
assertThat(instance.podAnnotations()).isEqualTo(Map.of("c", "d"));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ public List<ServiceInstance> getInstances(String serviceId) {
Map<String, String> serviceMetadata = serviceMetadata(serviceId, service, properties, List.of(),
service.getMetadata().getNamespace());
ServiceInstance externalNameServiceInstance = serviceInstance(null, service, null, -1, serviceId,
serviceMetadata, service.getMetadata().getNamespace());
serviceMetadata, service.getMetadata().getNamespace(), properties, client);
instances.add(externalNameServiceInstance);
}
}
Expand Down Expand Up @@ -152,7 +152,7 @@ private List<ServiceInstance> getNamespaceServiceInstances(EndpointSubsetNS es,
List<EndpointAddress> addresses = addresses(endpointSubset, properties);
for (EndpointAddress endpointAddress : addresses) {
ServiceInstance serviceInstance = serviceInstance(servicePortSecureResolver, service, endpointAddress,
endpointPort, serviceId, serviceMetadata, namespace);
endpointPort, serviceId, serviceMetadata, namespace, properties, client);
instances.add(serviceInstance);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@
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.ObjectMeta;
import io.fabric8.kubernetes.api.model.ObjectReference;
import io.fabric8.kubernetes.api.model.Pod;
import io.fabric8.kubernetes.api.model.Service;
import io.fabric8.kubernetes.api.model.ServiceList;
import io.fabric8.kubernetes.client.KubernetesClient;
Expand All @@ -44,13 +46,15 @@
import org.springframework.cloud.kubernetes.commons.KubernetesNamespaceProvider;
import org.springframework.cloud.kubernetes.commons.discovery.DefaultKubernetesServiceInstance;
import org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryProperties;
import org.springframework.cloud.kubernetes.commons.discovery.PodMetadata;
import org.springframework.cloud.kubernetes.fabric8.Fabric8Utils;
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.EXTERNAL_NAME;
import static org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryConstants.HTTP;
import static org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryConstants.HTTPS;
import static org.springframework.cloud.kubernetes.commons.discovery.KubernetesDiscoveryConstants.NAMESPACE_METADATA_KEY;
Expand Down Expand Up @@ -236,7 +240,8 @@ static List<EndpointAddress> addresses(EndpointSubset endpointSubset, Kubernetes

static ServiceInstance serviceInstance(@Nullable ServicePortSecureResolver servicePortSecureResolver,
Service service, @Nullable EndpointAddress endpointAddress, int endpointPort, String serviceId,
Map<String, String> serviceMetadata, String namespace) {
Map<String, String> serviceMetadata, String namespace, KubernetesDiscoveryProperties properties,
KubernetesClient client) {
// instanceId is usually the pod-uid as seen in the .metadata.uid
String instanceId = Optional.ofNullable(endpointAddress).map(EndpointAddress::getTargetRef)
.map(ObjectReference::getUid).orElseGet(() -> service.getMetadata().getUid());
Expand All @@ -254,8 +259,10 @@ static ServiceInstance serviceInstance(@Nullable ServicePortSecureResolver servi
String host = Optional.ofNullable(endpointAddress).map(EndpointAddress::getIp)
.orElseGet(() -> service.getSpec().getExternalName());

PodMetadata podMetadata = podMetadata(client, serviceMetadata, properties, endpointAddress, namespace);

return new DefaultKubernetesServiceInstance(instanceId, serviceId, host, endpointPort, serviceMetadata, secured,
namespace, null);
namespace, null, podMetadata.podLabels(), podMetadata.podAnnotations());
}

static List<Service> services(KubernetesDiscoveryProperties properties, KubernetesClient client,
Expand Down Expand Up @@ -287,6 +294,39 @@ else if (!properties.namespaces().isEmpty()) {
return services;
}

static PodMetadata podMetadata(KubernetesClient client, Map<String, String> serviceMetadata,
KubernetesDiscoveryProperties properties, EndpointAddress endpointAddress, String namespace) {
if (!EXTERNAL_NAME.equals(serviceMetadata.get(SERVICE_TYPE))) {
if (properties.metadata().addPodLabels() || properties.metadata().addPodAnnotations()) {
String podName = Optional.ofNullable(endpointAddress).map(EndpointAddress::getTargetRef)
.filter(objectReference -> "Pod".equals(objectReference.getKind()))
.map(ObjectReference::getName).orElse(null);

if (podName != null) {
ObjectMeta metadata = Optional
.ofNullable(client.pods().inNamespace(namespace).withName(podName).get())
.map(Pod::getMetadata).orElse(new ObjectMeta());
Map<String, String> podLabels = Map.of();
Map<String, String> podAnnotations = Map.of();
if (properties.metadata().addPodLabels()) {
podLabels = metadata.getLabels();
}

if (properties.metadata().addPodAnnotations()) {
podAnnotations = metadata.getAnnotations();
}

PodMetadata podMetadata = new PodMetadata(podLabels, podAnnotations);
LOG.debug(() -> "adding podMetadata : " + podMetadata + " from pod : " + podName);
return podMetadata;
}

}
}

return PodMetadata.EMPTY;
}

/**
* serviceName can be null, in which case, such a filter will not be applied.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,22 @@
import java.util.Map;
import java.util.Set;

import io.fabric8.kubernetes.api.model.EndpointAddressBuilder;
import io.fabric8.kubernetes.api.model.EndpointPortBuilder;
import io.fabric8.kubernetes.api.model.EndpointSubsetBuilder;
import io.fabric8.kubernetes.api.model.Endpoints;
import io.fabric8.kubernetes.api.model.EndpointsBuilder;
import io.fabric8.kubernetes.api.model.ObjectMeta;
import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
import io.fabric8.kubernetes.api.model.ObjectReferenceBuilder;
import io.fabric8.kubernetes.api.model.PodBuilder;
import io.fabric8.kubernetes.api.model.Service;
import io.fabric8.kubernetes.api.model.ServiceBuilder;
import io.fabric8.kubernetes.api.model.ServiceSpecBuilder;
import io.fabric8.kubernetes.client.Config;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.server.mock.EnableKubernetesMockClient;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;

Expand All @@ -51,17 +54,6 @@ class KubernetesDiscoveryClientTests {

private static KubernetesClient client;

@BeforeAll
static void setUp() {
// Configure the kubernetes master url to point to the mock server
System.setProperty(Config.KUBERNETES_MASTER_SYSTEM_PROPERTY, client.getConfiguration().getMasterUrl());
System.setProperty(Config.KUBERNETES_TRUST_CERT_SYSTEM_PROPERTY, "true");
System.setProperty(Config.KUBERNETES_AUTH_TRYKUBECONFIG_SYSTEM_PROPERTY, "false");
System.setProperty(Config.KUBERNETES_AUTH_TRYSERVICEACCOUNT_SYSTEM_PROPERTY, "false");
System.setProperty(Config.KUBERNETES_NAMESPACE_SYSTEM_PROPERTY, "test");
System.setProperty(Config.KUBERNETES_HTTP2_DISABLE, "true");
}

@AfterEach
void afterEach() {
client.endpoints().inAnyNamespace().delete();
Expand Down Expand Up @@ -529,6 +521,44 @@ void testExternalNameService() {
"labels-prefix-label-key", "label-value", "annotations-prefix-abc", "def", "type", "ExternalName"));
}

@Test
void testPodMetadata() {
Service nonExternalNameService = new ServiceBuilder()
.withSpec(new ServiceSpecBuilder().withType("ClusterIP").build()).withNewMetadata()
.withName("blue-service").withNamespace("a").endMetadata().build();
client.services().inNamespace("a").resource(nonExternalNameService).create();

client.endpoints().inNamespace("a").resource(new EndpointsBuilder()
.withMetadata(new ObjectMetaBuilder().withName("blue-service").build())
.withSubsets(new EndpointSubsetBuilder().withPorts(new EndpointPortBuilder().withPort(8080).build())
.withAddresses(new EndpointAddressBuilder().withIp("127.0.0.1")
.withTargetRef(new ObjectReferenceBuilder().withKind("Pod").withName("my-pod").build())
.build())
.build())
.build()).create();

client.pods().inNamespace("a").resource(new PodBuilder().withMetadata(new ObjectMetaBuilder().withName("my-pod")
.withLabels(Map.of("a", "b")).withAnnotations(Map.of("c", "d")).build()).build()).create();

KubernetesDiscoveryProperties.Metadata metadata = new KubernetesDiscoveryProperties.Metadata(true,
"labels-prefix-", true, "annotations-prefix-", true, "ports-prefix", true, true);
KubernetesDiscoveryProperties properties = new KubernetesDiscoveryProperties(true, true, Set.of("a", "b"), true,
60L, false, "", Set.of(), Map.of(), "", metadata, 0, false, true);

KubernetesDiscoveryClient discoveryClient = new KubernetesDiscoveryClient(client, properties, null, null, null);
List<ServiceInstance> result = discoveryClient.getInstances("blue-service");
Assertions.assertEquals(result.size(), 1);
DefaultKubernetesServiceInstance serviceInstance = (DefaultKubernetesServiceInstance) result.get(0);
Assertions.assertEquals(serviceInstance.getServiceId(), "blue-service");
Assertions.assertEquals(serviceInstance.getHost(), "127.0.0.1");
Assertions.assertEquals(serviceInstance.getPort(), 8080);
Assertions.assertFalse(serviceInstance.isSecure());
Assertions.assertEquals(serviceInstance.getUri().toASCIIString(), "http://127.0.0.1:8080");
Assertions.assertEquals(serviceInstance.getMetadata(), Map.of("k8s_namespace", "a", "type", "ClusterIP"));
Assertions.assertEquals(serviceInstance.podLabels(), Map.of("a", "b"));
Assertions.assertEquals(serviceInstance.podAnnotations(), Map.of("c", "d"));
}

private void createEndpoints(String namespace, String name, Map<String, String> labels) {
client.endpoints().inNamespace(namespace)
.resource(new EndpointsBuilder()
Expand Down
Loading