cloudfoundry / ibm-websphere-liberty-buildpack

IBM WebSphere Application Server Liberty Buildpack
Apache License 2.0
72 stars 190 forks source link

Compression feature - 2nd reopen #487

Closed krismarc closed 3 years ago

krismarc commented 3 years ago

Hello @preetisawant, @kevin-ortega,

thanks for providing the fix and sorry for reopening this for the 2nd time.

I've just tested your change and this works for specific/individual http endpoint. However there's a second option to get this configured in the LIberty. It can be configured globally - outside http endpoints and referenced back in their configurations. With this option the empty tag is still being added in endpoints configurations.

Both options are described here (just scroll down to the bottom): https://openliberty.io/blog/2020/04/22/http-response-compression.html

if I am not mistaken, the individual configuration wins with the global one and the empty tag overrides the global config. However, I am not 100% sure. Do you know how this exactly works? Your feedback and quick responses are highly appreciated.

Before deployment: image

After deployment: image

update: We have done some tests. Looks like I am correct.

an extra empty tag overrides (takes control over compression specified outside httpEndpoint):

<httpEndpoint id="defaultHttpEndpoint" compressionRef="myCompressionID">
    <!-- Enable response compression -->
    <compression/>
</httpEndpoint>
<compression id="myCompressionID" serverPreferredAlgorithm="gzip">
    <types>+application/json</types>
    <types>-text/plain</types>
</compression>

so even if not specified by the user your feature still adds it there and breaks his configuration.

While the same app without an empty compression tag inside httEndpoint works as expected:

<httpEndpoint id="defaultHttpEndpoint" compressionRef="myCompressionID"/>
<compression id="myCompressionID" serverPreferredAlgorithm="gzip">
    <types>+application/json</types>
    <types>-text/plain</types>
</compression>

..as a workaround you might have to look for compressionRef in httpEndpoints and don't enable a default compression if it's present.

Best regards, K.M.

kevin-ortega commented 3 years ago

@KrzMar We have fixed this latest issue. Thank you for discovering and reporting this problem.

krismarc commented 3 years ago

Hi @kevin-ortega,

thank you for your efforts.

That's enough for us. As Long as it's not breaking users configuration then it's fine for us.

However, I'd say this piece of code (the logic) prevents adding empty compression tags but not always making the compression enabled by default anymore. Eg. If there's compression configured on the root level (out of http endpoint configuration - global one) but not referenced in the endpoint with compressionRef attribute, then the default compression will be skipped for that endpoint.

example (httpEndpoint without compressionRef):

<httpEndpoint id="defaultHttpEndpoint"/>
<compression id="myCompressionID" serverPreferredAlgorithm="gzip">
    <types>+application/json</types>
    <types>-text/plain</types>
</compression>
      compression = REXML::XPath.match(server_xml_doc, '/server/compression')
      if compression.empty?
        endpoint.add_element('compression') if endpoint.elements['compression'].nil?
      end

..so, the above http endpoint is not having compression enabled and your automated default will be skipped for it because 'global' configuration is present.

Anyway.. having it extremely enabled in all possible scenarios is not my goal.

Best regards, K.M.