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.

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

Move service instance to commons #1399

Merged

Conversation

wind57
Copy link
Contributor

@wind57 wind57 commented Aug 4, 2023

No description provided.

@@ -150,116 +142,4 @@ void testBothAddressesTaken() {
Assertions.assertEquals(hostNames, List.of("one", "three", "two"));
}

@Test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tests were moved to commons, not deleted

@@ -130,6 +135,32 @@ public static ServicePortNameAndNumber endpointsPort(LinkedHashMap<String, Integ
}
}

public static ServiceInstance serviceInstance(@Nullable ServicePortSecureResolver servicePortSecureResolver,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this method has changed from the way it used to exist in fabric8 implementation. For example it takes as input a Supplier<InstanceIdHostPodName>, where:

/**
 * computes instanceId, host and podName. All needed when calculating ServiceInstance.
 * @author wind57
 */
public record InstanceIdHostPodName(String instanceId, String host, String podName) {
}

instanceId, host and podName are computed based on specific fabric8 EndpointAddress/Service:

final class Fabric8InstanceIdHostPodNameSupplier implements Supplier<InstanceIdHostPodName> {

	private final EndpointAddress endpointAddress;

	private final Service service;

	Fabric8InstanceIdHostPodNameSupplier(EndpointAddress endpointAddress, Service service) {
		this.endpointAddress = endpointAddress;
		this.service = service;
	}

	@Override
	public InstanceIdHostPodName get() {
		return new InstanceIdHostPodName(instanceId(), host(), podName());
	}

	// instanceId is usually the pod-uid as seen in the .metadata.uid
	private String instanceId() {
		return Optional.ofNullable(endpointAddress).map(EndpointAddress::getTargetRef).map(ObjectReference::getUid)
				.orElseGet(() -> service.getMetadata().getUid());
	}

	private String host() {
		return Optional.ofNullable(endpointAddress).map(EndpointAddress::getIp)
				.orElseGet(() -> service.getSpec().getExternalName());
	}

	private String podName() {
		return Optional.ofNullable(endpointAddress).map(EndpointAddress::getTargetRef)
				.filter(objectReference -> "Pod".equals(objectReference.getKind())).map(ObjectReference::getName)
				.orElse(null);
	}
}

The logic is exactly the same as it used to be, but a Supplier/Function is needed so that I can pass into this method from commons both fabric8 and in the nearest future: k8s code.

@wind57
Copy link
Contributor Author

wind57 commented Aug 4, 2023

Last PR before I start working on k8s integration with all these moved methods

@wind57 wind57 marked this pull request as ready for review August 4, 2023 20:09
@@ -154,6 +185,32 @@ static String primaryPortName(KubernetesDiscoveryProperties properties, Map<Stri
return primaryPortName;
}

static Map<String, Map<String, String>> podMetadata(String podName, Map<String, String> serviceMetadata,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

exactly the same logic is preserved, tests ensure this

@wind57
Copy link
Contributor Author

wind57 commented Aug 6, 2023

@ryanjbaxter ready to be looked at. This is a bit more involved, but in essence it does not change anything "business related". It just makes sure that instead of passing fabric8 specific stuff to serviceInstance common method, it passes things independent of the client. For example instead of a:

  • service name
  • service labels
  • service annotations

it now expects a ServiceMetadataForServiceInstance - that encapsulates those 3 fields above.


Another example would be that serviceMetadata that now lives in commons package, expects a Function<String, PodLabelsAndAnnotations>, or simpler a way to get:

  • pod labels
  • pod annotations

from a String (podName). This being a Function actually will call:

Fabric8PodLabelsAndAnnotationsSupplier for fabric8 and K8sPodLabelsAndAnnotationsSupplier in one of the future PRs.

This is needed so that commons does not care what client we use, thus I can delegate to the same common code.
I hope this makes sense.

@ryanjbaxter ryanjbaxter added this to the 3.0.5 milestone Aug 7, 2023
@ryanjbaxter ryanjbaxter merged commit ce9db7b into spring-cloud:3.0.x Aug 7, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants