Rename useForwardedHeaders to useRemoteIpOptions #33202
Conversation
|
!build (view Open Liberty Personal Build - ❌ completed with errors/failures.) Note: Target locations of links might be accessible only to IBM employees. |
Code analysis and actionsDO NOT DELETE THIS COMMENT.
|
volosied
force-pushed
the
32407-useForwardedHeaders-rename
branch
from
629d17b to
7d35080
Compare
| if (Objects.nonNull(serviceContext)) { | ||
|
|
||
| hostAddress = serviceContext.useForwardedHeadersInAccessLog() ? serviceContext.getForwardedRemoteHost() : null; | ||
| hostAddress = serviceContext.useRemoteIpOptionsInAccessLog() ? serviceContext.getForwardedRemoteHost() : null; |
There was a problem hiding this comment.
I'm unsure that this is more descriptive since the object that will be used is getForwardedRemoteHost() . While it is true that the remote host is calculated based on remote IP options, it itself is a forwarded (or x-forwarded) type header. So as it stands it would read : "use forwarded header or host address for the access log remote host" . We don't configure a remoteIP option that gives us a host, rather those options help determine if we consider the forwarded headers. There is one singular option for the access log to consider these headers, which is what this maps to, so I'd lean towards leaving at least this one as is or perhaps something like hasRemoteIpAccessLogEnabled/ isRemoteIpAccessLogEnabled / hasRemoteIpAccessLogOption
There was a problem hiding this comment.
or we could simply use the same name as the actual remote IP option for it: useRemoteIpInAccessLog . The more I think about it, the more I'd lean towards just using the same name as the property itself for this.
There was a problem hiding this comment.
I agree, it's better for the public config to match. I've fixed it.
...com.ibm.ws.transport.http/src/com/ibm/ws/http/channel/internal/values/AccessLogRemoteIP.java
Outdated
Show resolved
Hide resolved
....ws.transport.http/src/com/ibm/ws/http/channel/internal/values/AccessLogRequestProtocol.java
Outdated
Show resolved
Hide resolved
|
!build (view Open Liberty Personal Build - ❌ completed with errors/failures.) Note: Target locations of links might be accessible only to IBM employees. |
Code analysis and actionsDO NOT DELETE THIS COMMENT.
|
3 participants
release buglabel if applicable: https://github.com/OpenLiberty/open-liberty/wiki/Open-Liberty-Conventions).fixes #32407
This does change an SPI class, so further review is needed.