Remove Xstream dependency from eureka-client

[MrMarkW]

I would like to remove the xstream dependency from the eureka-client, but it is used in DiscoveryJerseyProvider. https://github.com/Netflix/eureka/blob/8009355993fedb15c82a67930f7354d4a28cb6c9/eureka-client/src/main/java/com/netflix/discovery/provider/DiscoveryJerseyProvider.java#L77-L81

I am not exactly clear why the client needs to have this provider? https://github.com/Netflix/eureka/blob/6efea5d3a0a8810c96a2d1fb6fb596e1c1c99f7f/eureka-client/src/main/java/com/netflix/discovery/shared/transport/jersey/EurekaJerseyClientImpl.java#L202-L203

If the client really needs the provider, could the eureka-client have a different provider for the client that only uses the Json codecs?

FYI: Eureka-client 1.8.7+ added the xstream security manager, which forces the use of Xstream 1.4.10. Unfortunately, our large code base requires 1.4.3 and it is not a simple effort to uplift to Xstream 1.4.10 because of non-passive changes.

[sabareeshkkanan]

I am voting for this as well because of future compatibility regarding reflection issue https://github.com/Netflix/eureka/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+xstream

[chriswhite199]

Plus JDK 11 issues:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.thoughtworks.xstream.core.util.Fields (file:/Users/me/.gradle/caches/modules-2/files-2.1/com.thoughtworks.xstream/xstream/1.4.10/dfecae23647abc9d9fd0416629a4213a3882b101/xstream-1.4.10.jar) to field java.util.TreeMap.comparator
WARNING: Please consider reporting this to the maintainers of com.thoughtworks.xstream.core.util.Fields
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

Which relates to this mess: https://github.com/x-stream/xstream/issues/101

[chriswhite199]

Is the solution as easy as overriding the setupConvertors method in the constructor of c.n.d.convertors.JsonXStream?:

https://github.com/Netflix/eureka/blob/master/eureka-client/src/main/java/com/netflix/discovery/converters/JsonXStream.java

    public JsonXStream() {
        super(new JettisonMappedXmlDriver() {
            private final NameCoder coder = initializeNameCoder();

            protected NameCoder getNameCoder() {
                return this.coder;
            }

            // add this as per x-stream/xstream#101?
            @Override
            protected void setupConverters() {
            }
        });

Oh apparently it's not that easy: https://github.com/x-stream/xstream/issues/101#issuecomment-432690259

[kuberr]

Plus JDK 11 issues:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.thoughtworks.xstream.core.util.Fields (file:/Users/me/.gradle/caches/modules-2/files-2.1/com.thoughtworks.xstream/xstream/1.4.10/dfecae23647abc9d9fd0416629a4213a3882b101/xstream-1.4.10.jar) to field java.util.TreeMap.comparator
WARNING: Please consider reporting this to the maintainers of com.thoughtworks.xstream.core.util.Fields
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

Which relates to this mess: x-stream/xstream#101

@troshko111 Why was this issue closed? From what I see, the EurekaClient still relies on the xstream library, and it still has the illegal access issue (which is ignorable for now, but not futureproof). Will it not eventually have to be fixed?

[troshko111]

There was no activity for a while, I take it as nobody is interested in removing it? I glanced at the usage and it looks removable (does not leak into public contract much) but we'd need to preserve serialization compatibility, check the perf / GC pressure does not degrade, etc.

We're open to discussing its removal if someone wants to take it on. I'll keep this open for some time then.

[SledgeHammer01]

@troshko111 I'm using JDK 11 right now. Isn't this going to be an issue if I want to upgrade to 13/14?

[troshko111]

Potentially, I did not investigate. Would appreciate if you reported back in case you do.

[lbourdages]

Reviving this issue.

There is a long thread on the xstream github page where it appears very evident that the maintainers do not want to make any effort to fix the illegal reflective access issue: https://github.com/x-stream/xstream/issues/101

Therefore, I think it would be worthwile to migrate to another dependency that will work in future relases.

[troshko111]

We're open to making xstream an opt-in (I would not want to completely remove the possibility of having it due to backward compatibility which we do not want to break). If someone is willing to take on this work I am happy to help, the end result should be xstream being used / loaded only if explicitly requested.

[pjfanning]

6 CVEs have been recently opened by OSS Fuzz guys against xstream and it doesn't seem likely they will be investigated soon. There are also questions about whether xstream will be reworked to work better with newer Java versions - xstream is very reliant on Java behaviours that are being phased out.

• https://github.com/x-stream/xstream/issues/262
• https://github.com/x-stream/xstream/issues/311

@troshko111 would it be possible to proceed with making xstream an optional dependency?

[wakingrufus]

I have solved this for myself in my projects in a way such that I can exclude xstream from my dependencies without a classnotfound exception occurring by replacing MyDefaultApacheHttpClient4Config with

/**
 * customization of
 * {@link com.netflix.discovery.shared.transport.jersey.EurekaJerseyClientImpl.EurekaJerseyClientBuilder.MyDefaultApacheHttpClient4Config}
 * to work with our custom Jersey client
 */
class JsonOnlyApacheHttpClient4Config extends DefaultApacheHttpClient4Config {
    public JsonOnlyApacheHttpClient4Config(EurekaClientConfig clientConfig) {
        MonitoredConnectionManager cm = createDefaultSslCM();
        getSingletons().add(new JsonOnlyJerseyProvider());

        // Common properties to all clients
        cm.setDefaultMaxPerRoute(clientConfig.getEurekaServerTotalConnectionsPerHost());
        cm.setMaxTotal(clientConfig.getEurekaServerTotalConnections());
        getProperties().put(ApacheHttpClient4Config.PROPERTY_CONNECTION_MANAGER, cm);

        String fullUserAgentName = ("Java-EurekaClient") + "/v" + buildVersion();
        getProperties().put(CoreProtocolPNames.USER_AGENT, fullUserAgentName);

        // To pin a client to specific server in case redirect happens, we handle redirects directly
        // (see DiscoveryClient.makeRemoteCall methods).
        getProperties().put(ClientConfig.PROPERTY_FOLLOW_REDIRECTS, Boolean.FALSE);
        getProperties().put(ClientPNames.HANDLE_REDIRECTS, Boolean.FALSE);
    }

    /**
     * @see SchemeRegistryFactory#createDefault()
     */
    private MonitoredConnectionManager createDefaultSslCM() {
        final SchemeRegistry registry = new SchemeRegistry();
        registry.register(
                new Scheme("http", 80, PlainSocketFactory.getSocketFactory()));
        registry.register(
                new Scheme("https", 443, new SSLSocketFactoryAdapter(SSLConnectionSocketFactory.getSocketFactory())));

        return new MonitoredConnectionManager("DiscoveryClient-HTTPClient", registry);
    }
}

which requires also an alternative to DiscoveryJerseyProvider that looks like

/**
 * Customization of {@link com.netflix.discovery.provider.DiscoveryJerseyProvider}
 * which removes the xml codecs and hardcodes the json codes to jackson in order to not reference xstream
 */
@Provider
@Produces({"application/json"})
@Consumes("*/*")
class JsonOnlyJerseyProvider implements MessageBodyWriter<Object>, MessageBodyReader<Object> {
    private static final Logger LOGGER = LoggerFactory.getLogger(com.netflix.discovery.provider.DiscoveryJerseyProvider.class);

    private final EncoderWrapper jsonEncoder;
    private final DecoderWrapper jsonDecoder;

    public JsonOnlyJerseyProvider() {
        this.jsonEncoder = CodecWrappers.getEncoder(CodecWrappers.LegacyJacksonJson.class);
        this.jsonDecoder = CodecWrappers.getDecoder(CodecWrappers.LegacyJacksonJson.class);
        LOGGER.info("Using JSON encoding codec {}", this.jsonEncoder.codecName());
        LOGGER.info("Using JSON decoding codec {}", this.jsonDecoder.codecName());
    }

    /**
     * As content is cached, we expect both ends use UTF-8 always. If no content charset encoding is explicitly
     * defined, UTF-8 is assumed as a default.
     * As legacy clients may use ISO 8859-1 we accept it as well, although result may be unspecified if
     * characters out of ASCII 0-127 range are used.
     */
    private static boolean isSupportedCharset(MediaType mediaType) {
        Map<String, String> parameters = mediaType.getParameters();
        if (parameters == null || parameters.isEmpty()) {
            return true;
        }
        String charset = parameters.get("charset");
        return charset == null
                || "UTF-8".equalsIgnoreCase(charset)
                || "ISO-8859-1".equalsIgnoreCase(charset);
    }

    /**
     * Checks for the {@link Serializer} annotation for the given class.
     *
     * @param entityType The class to be serialized/deserialized.
     * @return true if the annotation is present, false otherwise.
     */
    private static boolean isSupportedEntity(Class<?> entityType) {
        try {
            Annotation annotation = entityType.getAnnotation(Serializer.class);
            if (annotation != null) {
                return true;
            }
        } catch (Throwable th) {
            LOGGER.warn("Exception in checking for annotations", th);
        }
        return false;
    }

    private static Response createErrorReply(int status, Throwable cause, MediaType mediaType) {
        StringBuilder sb = new StringBuilder(cause.getClass().getName());
        if (cause.getMessage() != null) {
            sb.append(": ").append(cause.getMessage());
        }
        return createErrorReply(status, sb.toString(), mediaType);
    }

    private static Response createErrorReply(int status, String errorMessage, MediaType mediaType) {
        String message;
        if (MediaType.APPLICATION_JSON_TYPE.equals(mediaType)) {
            message = "{\"error\": \"" + errorMessage + "\"}";
        } else {
            message = "<error><message>" + errorMessage + "</message></error>";
        }
        return Response.status(status).entity(message).type(mediaType).build();
    }

    private static void closeInputOnError(InputStream inputStream) {
        if (inputStream != null) {
            LOGGER.error("Unexpected error occurred during de-serialization of discovery data, done connection cleanup");
            try {
                inputStream.close();
            } catch (IOException e) {
                LOGGER.debug("Cannot close input", e);
            }
        }
    }

    @Override
    public boolean isReadable(Class serializableClass, Type type, Annotation[] annotations, MediaType mediaType) {
        return isSupportedMediaType(mediaType) && isSupportedCharset(mediaType) && isSupportedEntity(serializableClass);
    }

    @Override
    public Object readFrom(
            Class serializableClass, Type type,
            Annotation[] annotations, MediaType mediaType,
            MultivaluedMap headers, InputStream inputStream) throws IOException {
        DecoderWrapper decoder;
        if (MediaType.MEDIA_TYPE_WILDCARD.equals(mediaType.getSubtype())) {
            throw new RuntimeException("xml not supported");
        } else if ("json".equalsIgnoreCase(mediaType.getSubtype())) {
            decoder = jsonDecoder;
        } else {
            decoder = jsonDecoder; // default
        }

        try {
            return decoder.decode(inputStream, serializableClass);
        } catch (Throwable e) {
            if (e instanceof Error) { // See issue: https://github.com/Netflix/eureka/issues/72 on why we catch Error here.
                closeInputOnError(inputStream);
                throw new WebApplicationException(e, createErrorReply(500, e, mediaType));
            }
            LOGGER.debug("Cannot parse request body", e);
            throw new WebApplicationException(e, createErrorReply(400, "cannot parse request body", mediaType));
        }
    }

    @Override
    public long getSize(
            Object serializableObject,
            Class serializableClass,
            Type type,
            Annotation[] annotations,
            MediaType mediaType) {
        return -1;
    }

    @Override
    public boolean isWriteable(Class serializableClass, Type type, Annotation[] annotations, MediaType mediaType) {
        return isSupportedMediaType(mediaType) && isSupportedEntity(serializableClass);
    }

    @Override
    public void writeTo(
            Object serializableObject, Class serializableClass,
            Type type, Annotation[] annotations, MediaType mediaType,
            MultivaluedMap headers, OutputStream outputStream) throws IOException, WebApplicationException {
        EncoderWrapper encoder = "json".equalsIgnoreCase(mediaType.getSubtype()) ? jsonEncoder : null;

        // XML codec may not be available
        if (encoder == null) {
            throw new WebApplicationException(createErrorReply(400, "No codec available to serialize content type " + mediaType, mediaType));
        }

        encoder.encode(serializableObject, outputStream);
    }

    private boolean isSupportedMediaType(MediaType mediaType) {
        return MediaType.APPLICATION_JSON_TYPE.isCompatible(mediaType);
    }
}

and then building my client as such:

    public static DiscoveryClient create(
            EurekaClientConfig clientConfig,
            ApplicationInfoManager applicationInfoManager) {
        val args = new DiscoveryClient.DiscoveryClientOptionalArgs();
        ClientConfig httpClientConfig = new JsonOnlyApacheHttpClient4Config(clientConfig);
        args.setEurekaJerseyClient(new EurekaJerseyClientImpl(
                clientConfig.getEurekaServerConnectTimeoutSeconds() * 1000,
                clientConfig.getEurekaServerReadTimeoutSeconds() * 1000,
                clientConfig.getEurekaConnectionIdleTimeoutSeconds() * 1000, httpClientConfig));
        return new DiscoveryClient(applicationInfoManager, clientConfig, args);
    }

if it is decided to make similar changes in upstream, Id be happy to open a PR