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

[functions] Default functionAuthProvider when running in k8s #6203

Merged
merged 1 commit into from
Mar 18, 2020

Conversation

addisonj
Copy link
Contributor

@addisonj addisonj commented Feb 4, 2020

Motivation

In 2.4.x, when running with the KubernetesRuntime, it default to always
using the KubernetesSecretAuthProvider class. With the change in 2.5 to
making this behavior pluggable, there is currently a bug in that it
doesn't keep this behavior and requires a new configuration option to be
passed.

Modifications

This commit changes the config so that it defaults to the correct class
when we are running with a kubernetes runtime. This restores the
behavior match that of earlier versions

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Added a test that ensures that we get the correct default based on other config values

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

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): n
  • The public API: n
  • The schema: n
  • The default values of configurations: y
  • The wire protocol:n
  • The rest endpoints: n
  • The admin cli options: n
  • Anything that affects deployment: n

Documentation

  • Does this pull request introduce a new feature? n
  • If yes, how is the feature documented? n/a
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@sijie
Copy link
Member

sijie commented Feb 5, 2020

@addisonj can you rebase this pull request to include some improvements in the github action files?

@addisonj
Copy link
Contributor Author

addisonj commented Feb 5, 2020

@sijie I did the rebase, looks to have still failed some

@addisonj addisonj closed this Feb 6, 2020
@addisonj addisonj reopened this Feb 6, 2020
@sijie
Copy link
Member

sijie commented Feb 7, 2020

@addisonj can you try to rebase again? I just merge a pull request with bunch of improvements in integration tests.

@tuteng
Copy link
Member

tuteng commented Feb 15, 2020

Please take a look at this test: https://github.com/apache/pulsar/pull/6203/checks?check_run_id=446244014 @addisonj

The exception was thrown with the wrong message: expected "Function language runtime is either not set or cannot be determined" but got "null"
	at org.testng.internal.ExpectedExceptionsHolder.wrongException(ExpectedExceptionsHolder.java:71)
	at org.testng.internal.Invoker.considerExceptions(Invoker.java:1130)
	at org.testng.internal.Invoker.invokeMethod(Invoker.java:615)
	at org.testng.internal.Invoker.retryFailed(Invoker.java:839)
	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1010)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:125)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:109)
	at org.testng.TestRunner.privateRun(TestRunner.java:648)
	at org.testng.TestRunner.run(TestRunner.java:505)
	at org.testng.SuiteRunner.runTest(SuiteRunner.java:455)
	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:450)
	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:415)
	at org.testng.SuiteRunner.run(SuiteRunner.java:364)
	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:84)
	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1208)
	at org.testng.TestNG.runSuitesLocally(TestNG.java:1137)

@tuteng
Copy link
Member

tuteng commented Feb 24, 2020

ping @addisonj Please try rebase master.

@addisonj
Copy link
Contributor Author

@tuteng rebased and ran the tests locally and they appear to pass

@addisonj
Copy link
Contributor Author

whoops, ignore the last comment, am able to repro now, will fix

@addisonj addisonj force-pushed the default_k8s_secrets branch 2 times, most recently from 2746814 to baa928c Compare March 2, 2020 22:14
In 2.4.x, when running with the KubernetesRuntime, it default to always
using the KubernetesSecretAuthProvider class. With the change in 2.5 to
making this behavior pluggable, there is currently a bug in that it
doesn't keep this behavior and requires a new configuration option to be
passed.

This commit changes the config so that it defaults to the correct class
when we are running with a kubernetes runtime. This restores the
behavior match that of earlier versions

This also moves the WorkerConfig test to the same package where the
workerConfig resides after the refactor and re-arranges the resources
files and copied via a maven task
@addisonj
Copy link
Contributor Author

addisonj commented Mar 5, 2020

/pulsarbot run-failure-checks

2 similar comments
@addisonj
Copy link
Contributor Author

addisonj commented Mar 5, 2020

/pulsarbot run-failure-checks

@addisonj
Copy link
Contributor Author

addisonj commented Mar 6, 2020

/pulsarbot run-failure-checks

@addisonj
Copy link
Contributor Author

addisonj commented Mar 6, 2020

@tuteng @sijie It looks like all the tests passed at least once, notice the CPP tests are on the list twice, one passed? I think this is ready for merge

@addisonj
Copy link
Contributor Author

@sijie you still want this for 2.5.1? All tests passed on this, it just looks like github is showing the checks twice

@addisonj
Copy link
Contributor Author

this looks good to merge @sijie, not sure if you want for 2.5.1?

@sijie sijie merged commit 3a3174b into apache:master Mar 18, 2020
@sijie
Copy link
Member

sijie commented Mar 18, 2020

@addisonj thank you for your contribution!

tuteng pushed a commit to AmateurEvents/pulsar that referenced this pull request Mar 21, 2020
…6203)

In 2.4.x, when running with the KubernetesRuntime, it default to always
using the KubernetesSecretAuthProvider class. With the change in 2.5 to
making this behavior pluggable, there is currently a bug in that it
doesn't keep this behavior and requires a new configuration option to be
passed.

This commit changes the config so that it defaults to the correct class
when we are running with a kubernetes runtime. This restores the
behavior match that of earlier versions

This also moves the WorkerConfig test to the same package where the
workerConfig resides after the refactor and re-arranges the resources
files and copied via a maven task

Co-authored-by: Addison Higham <ahigham@instructure.com>
(cherry picked from commit 3a3174b)
tuteng pushed a commit that referenced this pull request Apr 13, 2020
In 2.4.x, when running with the KubernetesRuntime, it default to always
using the KubernetesSecretAuthProvider class. With the change in 2.5 to
making this behavior pluggable, there is currently a bug in that it
doesn't keep this behavior and requires a new configuration option to be
passed.

This commit changes the config so that it defaults to the correct class
when we are running with a kubernetes runtime. This restores the
behavior match that of earlier versions

This also moves the WorkerConfig test to the same package where the
workerConfig resides after the refactor and re-arranges the resources
files and copied via a maven task

Co-authored-by: Addison Higham <ahigham@instructure.com>
(cherry picked from commit 3a3174b)
jiazhai pushed a commit to jiazhai/pulsar that referenced this pull request May 18, 2020
…6203)

In 2.4.x, when running with the KubernetesRuntime, it default to always
using the KubernetesSecretAuthProvider class. With the change in 2.5 to
making this behavior pluggable, there is currently a bug in that it
doesn't keep this behavior and requires a new configuration option to be
passed.

This commit changes the config so that it defaults to the correct class
when we are running with a kubernetes runtime. This restores the
behavior match that of earlier versions

This also moves the WorkerConfig test to the same package where the
workerConfig resides after the refactor and re-arranges the resources
files and copied via a maven task

Co-authored-by: Addison Higham <ahigham@instructure.com>(cherry picked from commit 3a3174b)
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
…6203)

In 2.4.x, when running with the KubernetesRuntime, it default to always
using the KubernetesSecretAuthProvider class. With the change in 2.5 to
making this behavior pluggable, there is currently a bug in that it
doesn't keep this behavior and requires a new configuration option to be
passed.

This commit changes the config so that it defaults to the correct class
when we are running with a kubernetes runtime. This restores the
behavior match that of earlier versions

This also moves the WorkerConfig test to the same package where the
workerConfig resides after the refactor and re-arranges the resources
files and copied via a maven task

Co-authored-by: Addison Higham <ahigham@instructure.com>
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.

3 participants