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

Container config update for repose upgrade #96

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

richarxt
Copy link
Collaborator

@richarxt richarxt commented Mar 7, 2024

While upgrading repose for blueflood-chef, we found out that some attributes of container deployment config are depricated after repose version 7.3.8. Attributes depricated are connection-timeout, read-timeout and client-request-logging.

For connection-timeout and read-timeout, we found out that we can set these properties in http-pool-connection.cfg filter file present in repose code

For read-timeout, we can set attribute http.socket-timeout as mentioned in its schema.
Documentation for this is present here

PR which mentioned attributes are depricated:
Depricate Container attributes

Client-request-logging attribute is not filtering out information. We can map client-request-logging attribute to HttpClient as mentioned here.

@shawnashlee
Copy link
Contributor

this needs some type of version check so that it will also work with the existing deployments in case we have to deploy the current version again.

@richarxt richarxt force-pushed the repose-upgrade-container-cfg branch from ef94d0b to 33c1490 Compare March 11, 2024 16:46
Copy link
Collaborator

@iWebi iWebi left a comment

Choose a reason for hiding this comment

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

LGTM

# After version 7.3.8, client_request_logging, connection_timeout, read-timeout & proxy-thread-pool attributes are depricated.
# So here we had added check to add these attributes only for version smaller than 7.3.8.
if (!@version.nil? &&
(majorVersion < 7 ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

won't the normal string comparison work here? if version < '7.3.8'. test ruby code shows it might be an option

version='7.3.8'
['1.2.3', '1.2', '7.0', '8.0', '7.4.1', '7.3.9', '7.2.1', '8.0.0'].each do |i|
  if version < i
    puts "#{ version } is less than #{i}" 
  else
    puts "#{ version } is greater than #{i}" 
  end
end

OUTPUT:
7.3.8 is greater than 1.2.3
7.3.8 is greater than 1.2
7.3.8 is greater than 7.0
7.3.8 is less than 8.0
7.3.8 is less than 7.4.1
7.3.8 is less than 7.3.9
7.3.8 is greater than 7.2.1
7.3.8 is less than 8.0.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DOne the changes

While upgrading repose for blueflood-chef, we found out that some attributes of container deployment config are depricated after repose version 7.3.8.
Attributes depricated are connection-timeout,  read-timeout and client-request-logging.

For connection-timeout and read-timeout, we found out that we can set these properties in http-pool-connection.cfg filter file
present in repose code https://github.com/rackerlabs/repose/blob/master/repose-aggregator/artifacts/valve/src/config/filters/http-connection-pool.cfg.xml

For read-timeout, we can set attribute http.socket-timeout as mentioned in its schema https://github.com/rackerlabs/repose/blob/f9a5ddcce0e2da4c1fcacd9f8f70d3dfb403c1bc/repose-aggregator/components/services/http-client-service/http-client-service-api/src/main/resources/META-INF/schema/config/http-connection-pool.xsd#L117
Documentation for this is present here https://www.openrepose.org/versions/8.10.0.0/services/http-connection-pool.html

PR which mentioned attributes are depricated:
rackerlabs/repose@a48bab7?diff=split&w=1

Client-request-logging attribute is not filtering out information. We can map client-request-logging attribute to HttpClient as mentioned below:
https://github.com/rackerlabs/repose/blob/master/repose-aggregator/tests/functional-tests/src/integrationTest/groovy/features/core/proxy/ClientRequestLogging.groovy#L35
@richarxt richarxt force-pushed the repose-upgrade-container-cfg branch from 33c1490 to 78aef1a Compare March 12, 2024 16:34
Copy link
Collaborator

@iWebi iWebi left a comment

Choose a reason for hiding this comment

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

+1

@richarxt richarxt merged commit bd861dd into master Mar 12, 2024
@richarxt richarxt deleted the repose-upgrade-container-cfg branch March 12, 2024 16:46
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants