Skip to content

Conversation

@flowchartsman
Copy link
Contributor

Fixes #20451

Motivation

Go functions die when auth is turned on. I would like them to not die.

Modifications

Added transport of auth plugin and auth plugin params to go instance config.
Added parsing of these values into the pf sdk

Verifying this change

  • Make sure that the change passes the CI checks.
  • Cannot integration test functions with auth

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

No doc change needed, because the docs don't mention the exception, so now the intuitive assumption holds.

Matching PR in forked repository

PR in forked repository: flowchartsman#6

flowchartsman and others added 4 commits May 31, 2023 15:01
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 1, 2023
@flowchartsman
Copy link
Contributor Author

NOTE Unless I missed something there are no unit integration tests for functions that enable auth. The closest I could find was the tests in tests/integration/src/test/java/org/apache/pulsar/tests/integration/functions/PulsarFunctionsTest.java, however it makes no mention of auth. I'm not familiar enough with the testing framework of the product to know if there's a way to easily run one of these tests with auth enabled, so if we want those tests, I'm going to need some assistance.

Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

LGTM. The one detail I see that might be missing for the go function runtime is an ability to configure TLS. Token based auth is recommended to be performed over TLS.

That feature would go in another PR. At the very least, we'll want to document this limitation to make sure users are aware of the current state of go functions.

@flowchartsman flowchartsman marked this pull request as draft June 1, 2023 22:33
@flowchartsman
Copy link
Contributor Author

flowchartsman commented Jun 1, 2023

Converting this to draft to integrate TLS (see #20453 / PIP-273) Ready for review

@flowchartsman flowchartsman changed the title [fix][fn] enable Go function token auth [fix][fn] enable Go function token auth and TLS Jun 2, 2023
@flowchartsman flowchartsman marked this pull request as ready for review June 2, 2023 18:23
@flowchartsman
Copy link
Contributor Author

That feature would go in another PR.
Hold my 🍺

Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

Great work @flowchartsman! I left one comment, but it's very minor.

@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2023

Codecov Report

❌ Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.91%. Comparing base (5e6e6ce) to head (9537ccb).
⚠️ Report is 2214 commits behind head on master.

Files with missing lines Patch % Lines
.../apache/pulsar/functions/runtime/RuntimeUtils.java 76.92% 0 Missing and 3 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #20468      +/-   ##
============================================
- Coverage     72.95%   72.91%   -0.05%     
+ Complexity    31914    31907       -7     
============================================
  Files          1867     1867              
  Lines        138485   138540      +55     
  Branches      15202    15208       +6     
============================================
- Hits         101037   101014      -23     
- Misses        29438    29509      +71     
- Partials       8010     8017       +7     
Flag Coverage Δ
inttests 24.14% <0.00%> (+0.02%) ⬆️
systests 24.98% <33.33%> (-0.06%) ⬇️
unittests 72.20% <83.33%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...pulsar/functions/instance/go/GoInstanceConfig.java 100.00% <100.00%> (ø)
.../apache/pulsar/functions/runtime/RuntimeUtils.java 78.26% <76.92%> (-0.17%) ⬇️

... and 66 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tisonkun
Copy link
Member

tisonkun commented Jun 2, 2023

Merging...

Thanks for your contribution @flowchartsman!

@tisonkun tisonkun merged commit 8b3c085 into apache:master Jun 2, 2023
michaeljmarshall pushed a commit to datastax/pulsar that referenced this pull request Jun 6, 2023
Note: there were conflicts with the go.sum file.

In order to get things working, I cleared out the conflicts and then
ran `go mod tidy` in the `pulsar-funcion-go/examples` directory.

Co-authored-by: Andy Walker <andy@andy.dev>
(cherry picked from commit 8b3c085)
lhotari pushed a commit that referenced this pull request Mar 27, 2024
Co-authored-by: Andy Walker <andy@andy.dev>
(cherry picked from commit 8b3c085)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug][fn] Go functions runtime does not support any sort of auth.

5 participants