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

Fix mii wdt filterpy #2379

Merged
merged 4 commits into from
Jun 16, 2021
Merged

Fix mii wdt filterpy #2379

merged 4 commits into from
Jun 16, 2021

Conversation

jshum2479
Copy link
Member

We need to set the default listenPort/ssl listenPort if one is not provided, otherwise the istio nap injected will use 0 for the port, resulting in validation error during introspection.

@tbarnes-us
Copy link

General comment #1: Does introspectDomain.py have a similar issue?

@tbarnes-us
Copy link

tbarnes-us commented May 26, 2021

General comment #2: IIRC, the default behavior changes based on whether security is enabled for the domain. Is that applicable here?

@jshum2479
Copy link
Member Author

General comment #1: Does introspectDomain.py have a similar issue?

No, introspectDomain.py still use the config override which reads config.xml and replace with real value. Mii no longer use config overrides.

@jshum2479
Copy link
Member Author

General comment #2: IIRC, the default behavior changes based on whether security is enabled for the domain. Is that applicable here?

The change is for user not providing listenPort and/or ssl listenPort when it is enabled which causes the injected nap with no value - that becomes 0 which is not valid.

# Set the default if it is not provided to avoid nap default to 0 which fails validation.

if admin_server_port is None:
admin_server_port = 7001
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the domain is in SecuredMode?

Copy link
Member Author

Choose a reason for hiding this comment

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

istio won't work with secure mode yet

Copy link
Member

Choose a reason for hiding this comment

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

Oh! That seems like an issue. Do we have a JIRA to complete that support? I'd like to resolve that gap before the Verrazzano team hits it.

Copy link
Member

Choose a reason for hiding this comment

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

@ddsharpe @jshum2479, what would be the correct behavior for secure mode? Does this "None" check let us differentiate between when the customer has left it blank or if the admin server's default port is disabled because of secure mode?

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: the filter code already have provision to setup the ssl port if the secure mode is enabled (line 423).

But, ultimately it will not work in istio environment.

  1. whenever admin port (same for secure mode) is enabled, the readiness probe /weblogic/ready is treated as management function because it started with /weblogic and it must be accessed directly read-address:adminport and not proxied it through localhost:adminport
  2. Istio always proxy it through unless there is annotation to forbid the rewrite the port traffic (essentially take it out of the mesh).
  3. operator current implementation always use the plain readiness port in the domain spec, while it can be fixed in PodStepContext but it won't fix (2).

Copy link
Member Author

Choose a reason for hiding this comment

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

As for the correct behavior, I am not sure what's the correct behavior. If secure mode is enabled, does it mean regular listen port is disabled?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

So the core should automatically disabled the listenPort. The question is whether the operator needs to do anything special about it. I suggest creating another issue to handle secure mode as I suspect there maybe issue in non-istio case. This PR is for missing listenPort only. For adminport/secure mode it's a dead end for istio for now.

@rjeberhard rjeberhard merged commit 6c9a8c9 into main Jun 16, 2021
@jshum2479 jshum2479 deleted the fix-mii-wdt-filterpy branch December 2, 2021 16:38
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.

None yet

5 participants