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

Hot Fix Release v4.6.1 #103

Closed
wants to merge 13 commits into from
Closed

Hot Fix Release v4.6.1 #103

wants to merge 13 commits into from

Conversation

azizbekxm
Copy link
Contributor

@azizbekxm azizbekxm commented May 23, 2024

Releasing these prs for EDGCOMMON-78, EDGCOMMON-79
#99
#100
#101
#102

azizbekxm and others added 10 commits May 23, 2024 11:33
…2 Compliant Cryptography (#99)

* EDGCOMMON-79.Add ssl/tls support

(cherry picked from commit e35fed0)
* EDGCOMMON-78. Add ssl for OkapiClient

(cherry picked from commit bd00a1e)
(cherry picked from commit 5251bf4)
…nt edge modules that use Net server

(cherry picked from commit e815cf2)
(cherry picked from commit 4fbc595)
(cherry picked from commit f7343f1)
(cherry picked from commit 9b23b27)
#102)

* [EDGORDERS-83-IT]. Add tls integration test, add test scope BC deps, keystore and trust store

(cherry picked from commit bc84d49)
NEWS.md Outdated Show resolved Hide resolved
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link
Contributor

@julianladisch julianladisch left a comment

Choose a reason for hiding this comment

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

We already have the spring.ssl.bundle.jks.web-server and folio.client.tls namespace for the TLS configuration variables:

Please do NOT introduce yet another namespace like http-server or web-client.

The TLS configuration variables should have the same name regardless of the underlying technology (Spring, Vert.x).

Using different namespaces results in a multitute of work for the sysops, and it is very error-prone.

Re-using the Spring namespace for Vert.x modules will also make it more easy to change an edge module from Vert.x to Spring.

@SerhiiNosko
Copy link
Contributor

@julianladisch these are some points here to keep naming as is:

  • Some spring config params have 'spring' prefix that is not correct param name for vertx module.
  • Vertx HttpServer Api and WebClient API have APIs that can be different that Spring API. For example, if Vertx WebClient has method setSsl() - better to use param 'ssl_enabled' instead of 'folio.client.tls.enabled'
  • We should follow existing naming strategy, if we have existing params named in this style like: 'request_timeout_ms', 'api_key_sources', 'log_level' - so new params should follow the same style, in our case 'ssl_enabled' instead of 'folio.client.tls.enabled'
  • My understanding is that if we migrate from Vertx to Spring - we should change some deployment options, params in any case, and ssl related params would be changed together with other params.

@julianladisch
Copy link
Contributor

Renaming the option names affects about 9 Vert.x edge modules only.

Keeping different names affects > 100 libraries and their sysops.

I agree that the spring prefix sounds somewhat wrong for non-Spring modules. However, nobody cares as long as the option works. We can explain this in the README and all sysops will love it if the options have the same name across all edge modules.

@julianladisch
Copy link
Contributor

https://docs.spring.io/spring-boot/docs/current/reference/html/features.html#features.external-config.typesafe-configuration-properties.relaxed-binding.environment-variables
explains that sysops cannot use environment variables with names like spring.ssl.bundle.jks.web-server and folio.client.tls.
This applies to FOLIO because we use Alpine for both Spring and Vert.x modules: https://github.com/folio-org/folio-tools/tree/master/folio-java-docker/openjdk17
Instead sysops need to use names like SPRING_SSL_BUNDLE_JDK_WEBSERVER and FOLIO_CLIENT_TLS.
Spring automatically converts them.
Vert.x based modules should accept these uppercase forms.
And READMEs of Spring and Vert.x based modules should show the correct uppercase forms.

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.

4 participants