Owls88611 make sure REST calls are made using the expected port and protocol #2301

doxiao · 2021-04-07T17:47:25Z

The changes in this PR fix an issue where, in some configurations, the operator may choose a wrong port and/or a wrong protocol (mistakenly considering a non-sure port to be secure) for making REST calls to get WLS server health states.

The issue is caused by the following bugs in operator.

The code relies on the port names in a server service and WlsServerConfig#getAdminProtocolChannelName to determine if a port has "admin" privilege. This is not reliable because for example, the ssl port is exposed by the server service as "default-secure" by default, but as "https-secure" if istio is enabled. As a result, when istio is enabled, the server SSL listenport is not chosen when it should be the one to use.
If there is no "admin" port to use for a server, the logic picks the first port in the server service, instead of considering the one that exposes the server listen port first. As a result, the port of a channel may be chosen even the server listen port exists. When this happens, a non-secure port may be treated as a secure port because the code treat all non default listen port as secure.

The changes fix these issues by

removing the dependence to server services;
Use getLocalAdminProtocolChannelPort to get the right port;
add a validation check for the case where the default listen port, SSL port, admin port are disabled/unconfigured, and there is not a channel with admin privileges.

Unit test cases are added to cover different configuration permutations.

Integrations test run on Jenkins (had no additional failures except for the known failures in main): https://build.weblogick8s.org:8443/job/weblogic-kubernetes-operator-kind-new/4623/.

Jenkins run with the latest version: https://build.weblogick8s.org:8443/job/weblogic-kubernetes-operator-kind-new/4658/.

rosemarymarano

LGTM

russgold · 2021-04-09T16:41:18Z

I could use some more explanations, here. There are a fair number of places where we need to select the REST port, including for the health check and the metrics. WlsServerConfig.getLocalAdminProtocolChannelPort as its own logic, which may not match that used in other places. I see fairly high level tests, but nothing that pertains directly to the algorithm for doing this selection. I could be missing something.

doxiao · 2021-04-09T16:59:07Z

I could use some more explanations, here. There are a fair number of places where we need to select the REST port, including for the health check and the metrics. WlsServerConfig.getLocalAdminProtocolChannelPort as its own logic, which may not match that used in other places. I see fairly high level tests, but nothing that pertains directly to the algorithm for doing this selection. I could be missing something.

WlsServerConfig.getLocalAdminProtocolChannelPort has exactly what we need here: it chooses a port to use in the following order:

the channel that has admin privileges ;
the server's admin port;
the server's SSL port
the server's default listenport.

The original logic uses the ports on the server's K8S service, which is not needed.

As to the test cases, they do test if the correct port/protocol is picked/used given a server configuration. The expected URL is specified in the line like this: defineResponse(200, "", "http://dyn-managed-server2.Test:7001");. This means the test expects that the default listenport 7001 is picked. I noticed that the test names do not make this obvious. I can make some changes to make it clearer.

operator/src/test/java/oracle/kubernetes/weblogic/domain/model/DomainValidationTest.java

operator/src/main/java/oracle/kubernetes/operator/steps/ReadHealthStep.java

ankedia

LGTM

ankedia · 2021-04-09T19:19:11Z

One minor comment - It looks like there are still some test methods with unused clusterName variable -
String clusterName = "TestClusterForFRest";

russgold · 2021-04-12T13:22:25Z

operator/src/main/java/oracle/kubernetes/weblogic/domain/model/DomainValidationMessages.java

@@ -131,4 +131,8 @@ public static String exceedMaxExternalServiceName(
      String domainUid, String adminServerName, String result, int limit) {
    return getMessage(MessageKeys.ILLEGAL_EXTERNAL_SERVICE_NAME_LENGTH, domainUid, adminServerName, result, limit);
  }
+
+  public static String noAvaiablePortToUse(String domainUid, String serverName) {


Spelling: "available" not "avaiable"

russgold · 2021-04-12T13:23:50Z

operator/src/main/resources/Operator.properties

@@ -167,9 +167,11 @@ WLSDO-0019=DomainUID ''{0}'' and admin server name ''{1}'' combination ''{2}'' e
 WLSDO-0020=Online WebLogic configuration updates complete \
  but there are pending non-dynamic changes that require \
  pod restarts to take effect. The changes are:
-
+WLSDO-0021=DomainUID ''{0}'' server ''{1}'' does not have a port available for the operator to send REST calls. \
+  The default listen port and SSL port are disabled, the admin port is not configured and there is not a channel with the admin privileges.\


Phrasing is clumsy. You could check with Rosemary, but I would recommend, "... and there is no channel with admin privileges."

russgold · 2021-04-12T13:29:39Z

Mostly looks good; some questions/comments:

You said that the names weren't a reliable way to select the right port, but I didn't see code/tests that explicitly dealt with that. The tests I see appear to be done at a much higher level. I'd have expected tests on WlsServerConfig at the least.
The monitoring exporter also needs to compute the REST port, but I don't see any changes/tests that deal with that.

rjeberhard · 2021-04-12T14:16:55Z

@doxiao, @russgold, @ankedia, @jshum2479, can someone provide the list of all code points that need this information? I can think of the monitoring exporter, the server health check through REST, and the dynamic update in the introspector (this likely will be a different code base since it's behavior from WDT). Are there other cases? These then need to give the correct answer with and without Istio.

doxiao · 2021-04-12T15:23:56Z

The problem exists in code paths where the result of WlsServerConfig#getAdminProtocolChannelName method is used to match a port among server service's ports. The result of getAdminProtocolChannelName does not reflect the server port names in the istio enabled case. As far as I can tell, only REST (ReadHealthStep) and exporter (PodHelper) do this. I have changed both places to NOT use server service ports to determine which port to use, instead just call getLocalAdminProtocolChannelPort method, which is the same no matter if istio is enabled or not.

doxiao · 2021-04-12T15:37:01Z

Mostly looks good; some questions/comments:

You said that the names weren't a reliable way to select the right port, but I didn't see code/tests that explicitly dealt with that. The tests I see appear to be done at a much higher level. I'd have expected tests on WlsServerConfig at the least.

The problem and fix are not in WlsServerConfig layer, instead, in ReadHealthStep. Therefore the newly added test cases are at the ReadHealthStep layer.

The monitoring exporter also needs to compute the REST port, but I don't see any changes/tests that deal with that.

I changed PodHelper to do the same as in ReadHealthStep. Note that the exporter related code did not have the problem that ReadHealthStep had because it had different orElse handling (when the port chosen by name does not match anything), but it had a different problem - it choose server's listen port over SSL port. I changed it to match exactly what we do now in ReadHealthStep.

doxiao · 2021-04-12T16:39:37Z

@russgold I noticed that in PodHelper#createMonitoringExporterContainer method, the logic of choosing a port for the Java options -DWLS_PORT is different from what is for choosing a port for the ContainerPort. The former looks for a channel with admin privileges before looking for server's admin port, ssl port and listen port, but the latter does not. Changes in this PR only impacts the former where WlsServerConfig#getAdminProtocolChannelName is used. I think we are fine there because the WLS_PORT is the one that the exporter is going to use to talk to WLS side. But want to double check with you.

doxiao added 3 commits April 6, 2021 23:03

choose correct port and protocl

f775707

refactor unit tests

525c7c8

Merge remote-tracking branch 'origin/main' into owls88611-rest-url-fix

fcde766

doxiao requested review from russgold, rjeberhard and ankedia April 7, 2021 17:47

doxiao changed the title ~~Owls88611 make sure REST calls are made usign the expected port and protocol~~ WIP Owls88611 make sure REST calls are made usign the expected port and protocol Apr 7, 2021

doxiao added 4 commits April 7, 2021 17:42

rework the fix to not depend on service

66d849b

cleanup

92494bb

more cleanup

35c92e9

more cleanup

369a1c4

doxiao changed the title ~~WIP Owls88611 make sure REST calls are made usign the expected port and protocol~~ Owls88611 make sure REST calls are made usign the expected port and protocol Apr 8, 2021

doxiao changed the title ~~Owls88611 make sure REST calls are made usign the expected port and protocol~~ Owls88611 make sure REST calls are made using the expected port and protocol Apr 8, 2021

doxiao changed the title ~~Owls88611 make sure REST calls are made using the expected port and protocol~~ WIP Owls88611 make sure REST calls are made using the expected port and protocol Apr 8, 2021

doxiao added 3 commits April 8, 2021 21:45

add validation for no port to use cases

e2d35d7

merge

c25d04a

reword validation failure message

e0383ea

doxiao changed the title ~~WIP Owls88611 make sure REST calls are made using the expected port and protocol~~ Owls88611 make sure REST calls are made using the expected port and protocol Apr 9, 2021

doxiao requested a review from rosemarymarano April 9, 2021 12:09

rosemarymarano approved these changes Apr 9, 2021

View reviewed changes

modify unit test cases to make their logic more clear

a8434ad

ankedia reviewed Apr 9, 2021

View reviewed changes

review comments

e9dc102

ankedia approved these changes Apr 9, 2021

View reviewed changes

clean up unused variables in unit test code

a2fd095

russgold reviewed Apr 12, 2021

View reviewed changes

address review comments

7048d48

minor change to validation failure message

6fa6fc7

doxiao requested a review from russgold April 12, 2021 16:44

russgold approved these changes Apr 13, 2021

View reviewed changes

rjeberhard merged commit 5fa552e into main Apr 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Owls88611 make sure REST calls are made using the expected port and protocol #2301

Owls88611 make sure REST calls are made using the expected port and protocol #2301

Uh oh!

doxiao commented Apr 7, 2021 •

edited

Loading

Uh oh!

rosemarymarano left a comment

Uh oh!

russgold commented Apr 9, 2021

Uh oh!

doxiao commented Apr 9, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ankedia left a comment

Uh oh!

ankedia commented Apr 9, 2021 •

edited

Loading

Uh oh!

russgold Apr 12, 2021

Uh oh!

russgold Apr 12, 2021

Uh oh!

russgold commented Apr 12, 2021

Uh oh!

rjeberhard commented Apr 12, 2021

Uh oh!

doxiao commented Apr 12, 2021 •

edited

Loading

Uh oh!

doxiao commented Apr 12, 2021 •

edited

Loading

Uh oh!

doxiao commented Apr 12, 2021 •

edited

Loading

Uh oh!

Uh oh!

Owls88611 make sure REST calls are made using the expected port and protocol #2301

Owls88611 make sure REST calls are made using the expected port and protocol #2301

Uh oh!

Conversation

doxiao commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rosemarymarano left a comment

Choose a reason for hiding this comment

Uh oh!

russgold commented Apr 9, 2021

Uh oh!

doxiao commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ankedia left a comment

Choose a reason for hiding this comment

Uh oh!

ankedia commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

russgold Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

russgold Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

russgold commented Apr 12, 2021

Uh oh!

rjeberhard commented Apr 12, 2021

Uh oh!

doxiao commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doxiao commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doxiao commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

doxiao commented Apr 7, 2021 •

edited

Loading

doxiao commented Apr 9, 2021 •

edited

Loading

ankedia commented Apr 9, 2021 •

edited

Loading

doxiao commented Apr 12, 2021 •

edited

Loading

doxiao commented Apr 12, 2021 •

edited

Loading

doxiao commented Apr 12, 2021 •

edited

Loading