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.

Sign up for GitHub

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

Jump to bottom

HPCC-31394 WsSMC send roxie control cmd to ssl port if available #18396

Merged
merged 1 commit into from
Jun 13, 2024
Merged

HPCC-31394 WsSMC send roxie control cmd to ssl port if available #18396

merged 1 commit into from
Jun 13, 2024

Conversation

asselitx
Copy link
Contributor

@asselitx asselitx commented Mar 11, 2024 •
edited
Loading

Rather than default to port 9876, use the roxie farmer ports configured for sending control messages to. Prefer an ssl port if available.

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change improves the code (refactor or other change that does not change the functionality)
  • This change fixes warnings (the fix does not alter the functionality or the generated code)
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Cloud-compatibility
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browsers
    • The component(s) render as expected

Smoketest:

  • Send notifications about my Pull Request position in Smoketest queue.
  • Test my draft Pull Request.

Testing:

@asselitx asselitx requested a review from mckellyln March 11, 2024 16:52
@github-actions GitHub Actions
Copy link

https://track.hpccsystems.com/browse/HPCC-31394
Jira updated

@mckellyln
Copy link
Contributor

I am still working on adding to this PR for using the same port in cascade/lock that was used to reach roxie.

@mckellyln
Copy link
Contributor

@asselitx I approve this PR and I have added: #18571
So these two PRs really go together.

mckellyln
mckellyln approved these changes Apr 22, 2024
View reviewed changes
Copy link
Contributor

@mckellyln mckellyln left a comment

Choose a reason for hiding this comment

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

Approved. I added some code to PR: #18571 for HPCC-31395 for this for testsocket to Roxie.

@mckellyln mckellyln mentioned this pull request Apr 22, 2024
Merged
39 tasks
@asselitx asselitx marked this pull request as ready for review April 24, 2024 14:32
ghalliday
ghalliday reviewed Apr 26, 2024
View reviewed changes
Copy link
Member

@ghalliday ghalliday left a comment

Choose a reason for hiding this comment

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

The change looks good. One question about the way the socket factory is managed etc.

esp/smc/SMCLib/TpWrapper.cpp Outdated
public:
TlsSmartSocketFactory(const char *_socklist) : CSmartSocketFactory(_socklist)
{
tlsService = true;
Copy link
Member

Choose a reason for hiding this comment

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

The encapsulation could be better - it is using an implementation detail of the class. Should this either be an extra parameter to createSmartSocketFactory(), or should it be using createSecureSmartSocketFactory()?

Is there other configuration information (e.g., issuer for mtls) that needs to be provided?

@asselitx asselitx force-pushed the smc-ssl-hpcc-31394 branch from 1484215 to ab71964 Compare May 7, 2024 18:22
@asselitx asselitx changed the base branch from candidate-9.4.x to candidate-9.6.x May 7, 2024 18:25
@asselitx asselitx requested a review from ghalliday May 7, 2024 18:25
ghalliday
ghalliday reviewed May 9, 2024
View reviewed changes
Copy link
Member

@ghalliday ghalliday left a comment

Choose a reason for hiding this comment

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

One initial question. I think the code is assuming that a non-null factory is returned from createSecureSocketConfig(nullptr, nullptr, nullptr). Is that a problem?

roxie/roxiepipe/roxiepipe.cpp
@@ -677,7 +677,7 @@ int main(int argc, char *argv[])
{
#ifdef _USE_OPENSSL
if (useSSL)
smartSocketFactory = createSecureSmartSocketFactory(hosts.str(), retryMode);
smartSocketFactory = createSecureSmartSocketFactory(hosts.str(), createSecureSocketConfig(nullptr, nullptr, nullptr), retryMode);
Copy link
Member

Choose a reason for hiding this comment

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

createSecureSocketConfig(nullptr, nullptr, nullptr) currently returns null. Is this doing what you expect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I expect but it may not be clear, and I wasn't sure if other changes would make it better. I thought this could be a placeholder until the usage of the smart socket classes is regularized. If it would be better to have an empty rather than null object here I could adjust the createSecureSocketConfig to do so or create an alternate function. I can see how this approach may not be clear and will adjust if you'd like.

The use of the smart sockets and factories isn't entirely consistent. In some cases the users of the classes interrogate the base interface for TLS status, and in others the user knows the TLS state and uses the correct secure/insecure flavor based on its own knowledge. roxiepipe is in the second case and it doesn't rely on the smart socket class itself having a valid config, rather it has already created a secure socket version based on the ssl argument (and compile switch).

@asselitx asselitx force-pushed the smc-ssl-hpcc-31394 branch from ab71964 to d7dc37d Compare May 10, 2024 15:04
@ghalliday ghalliday self-requested a review May 14, 2024 12:54
ghalliday
ghalliday reviewed May 17, 2024
View reviewed changes
Copy link
Member

@ghalliday ghalliday left a comment

Choose a reason for hiding this comment

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

@asselitx looks good. One question.

system/jlib/jsmartsock.cpp Outdated
SmartSocketListParser slp(_socklist);
if (slp.getSockets(sockArray) == 0)
throw createSmartSocketException(0, "no endpoints defined");

if (_tlsConfig && _tlsConfig->queryName() && streq(_tlsConfig->queryName(), "ssl"))
Copy link
Member

Choose a reason for hiding this comment

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

Why the check of the name against ssl? It could do with a comment.

(For instance the value created with createIssuerTlsConfig doesn't have that name - but it isn't going through this code path...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The check does not add value on reconsideration, so I'll remove it and squash my commits.

Initially it seemed reasonable to ensure we had a configuration for SSL/TLS but given the other constructor doesn't explicitly check and has a different format it makes little sense.

@asselitx asselitx force-pushed the smc-ssl-hpcc-31394 branch from d7dc37d to 83a1255 Compare June 7, 2024 15:09
@asselitx asselitx requested a review from ghalliday June 7, 2024 15:10
@asselitx
Copy link
Contributor Author

asselitx commented Jun 7, 2024 •
edited
Loading

I see two failures. The first windows runner build failing in vcpkg step doesn't appear to be related to my changes: building icu:x64-windows. It failed again after a rebase and push. The second are roxie smoketest failures I'm looking into.

@asselitx
HPCC-31394 WsSMC send roxie control cmd to ssl port if available
14357e8 
Rather than default to port 9876, use the roxie farmer ports configured
for sending control messages to. Prefer an ssl port if available.

Update the smart pointer factory and ctors where required to accept a tls
config from bare-metal configurations. These configurations using transitional
APIs don't support all the properties and sync features of the containerized
versions.

A separate ticket would be required to determine if and how to add the full
complement of tls related configuration, behavior and secrets support to bare
metal that is currently in place in containerized code.

Signed-off-by: Terrence Asselin <[email protected]>
@asselitx asselitx force-pushed the smc-ssl-hpcc-31394 branch from 83a1255 to 14357e8 Compare June 10, 2024 21:58
@asselitx
Copy link
Contributor Author

asselitx commented Jun 11, 2024 •
edited
Loading

@ghalliday Fixed issue when creating tlsConfig in WsECL. Remaining check that is failing is related to vcpkg on window failing on icu. Other PRs have the same problem such as Mark's: https://github.com/hpcc-systems/HPCC-Platform/actions/runs/9455013251/job/26043854433?pr=18749

@ghalliday ghalliday merged commit 3f6374e into hpcc-systems:candidate-9.6.x Jun 13, 2024
50 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mckellyln mckellyln mckellyln approved these changes

@ghalliday ghalliday ghalliday approved these changes

Assignees

@ghalliday ghalliday

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@asselitx @mckellyln @ghalliday