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

Extend SSL/TLS support to st2stream and st2api - plus move common API code into common library #6204

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jk464
Copy link
Contributor

@jk464 jk464 commented May 24, 2024

In StackStorm/stackstorm-k8s#401, I'm adding support for SSL/TLS between internal components of StackStorm. st2auth has out-of-the-box support for SSL/TLS. Meanwhile st2stream and st2api did not and required placing a TLS proxy layer infront of the stream/api containers.

@cognifloyd pointed out in the st2 core code where it should be fairly straightforward to copy the SSL/TLS support from st2auth to st2stream and st2api. This PR does that.

The PR containers two parts as two commits:

Extend SSL/TLS support to st2stream and st2api

This is the primary commit of the PR, it is a basic implementation of SSL/TLS support as suggested by @cognifloyd - and works perfectly fine, nothing fancy beyond that.

Move common API code into shared library

While implementing support for SSL/TLS I notice between the 3 components api, auth and stream that the following set of 3 files for each component:

st2auth:

st2auth/st2auth/config.py
st2auth/st2auth/app.py
st2auth/st2auth/cmd/api.py

st2api:

st2api/st2api/config.py
st2api/st2api/app.py
st2api/st2api/cmd/api.py

st2stream:

st2stream/st2stream/config.py
st2stream/st2stream/app.py
st2stream/st2stream/cmd/api.py

Are all very similar in function, with only minor functional differences between each one. However there was also some inconsistency in convention between the 3 due to the functionally between separately implement for each (for example auth being the only one with SSL/TLS support).

Therefore in an attempt to "streamline" the code base, I've moved this code into a new separate st2common library openapi (name open to suggestions since this isn't maybe the best description of its function, I just went for this as a "working" name), the library contains 3 files

  • st2common/st2common/openapi/api.py
  • st2common/st2common/openapi/app.py
  • st2common/st2common/openapi/config.py

Which moves the common code from the respectively named files in st2api, st2auth, and st2stream components and updates those libraries to pull in this new library.

This will hopefully simplify support/maintenance going forward - and ensure any new features/fixes that would apply to all 3 components gets implement to all 3 (for example if we had this originally, would we have gotten SSL/TLS support for all 3 components at the same time, instead of just for auth - until this PR?)


As an immediate piece of self-feedback there's a few places in the st2common/openapi library I've done something along the lines of:

if service_name == "foo":
  <do_edge_case_for_service_foo>

I don't particularly like that, but these where minor differences between services of an otherwise identical function and I was unsure how to handle it otherwise without having to needlessly replicate code. So any suggestion on how to handle this without having to bake edgecases in the common library would be appreciated.

@pull-request-size pull-request-size bot added the size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review. label May 24, 2024
@jk464 jk464 requested a review from cognifloyd May 24, 2024 11:13
@jk464 jk464 changed the title Extent SSL/TLS support to st2stream and st2api - plus move common API code into common library Extend SSL/TLS support to st2stream and st2api - plus move common API code into common library May 24, 2024
@jk464 jk464 force-pushed the feature/st2api-stream-tls branch from 0c311d7 to 217a90b Compare May 24, 2024 11:17
@jk464 jk464 force-pushed the feature/st2api-stream-tls branch from 217a90b to 58fdc7e Compare May 24, 2024 11:34
@jk464 jk464 force-pushed the feature/st2api-stream-tls branch from 58fdc7e to 53d2e92 Compare May 24, 2024 14:00
@guzzijones
Copy link
Contributor

can you please fix the lint checks.

@guzzijones guzzijones self-requested a review September 13, 2024 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants