TriggerAuth-podIdentity.identityId - validation removed (operator) #5142

radekfojtik · 2023-10-31T14:28:10Z

TriggerAuthentication.podIdentity - validation of identityId (empty string "") removed (operator)

validation remains only in webhook

Checklist

When introducing a new scaler, I agree with the scaling governance policy
I have verified that my change is according to the deprecations & breaking changes policy
Tests have been added
Changelog has been updated and is aligned with our changelog requirements
A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
A PR is opened to update the documentation on (repo) (if applicable)
Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #5109

Relates to #

Signed-off-by: radekfojtik <68660951+radekfojtik@users.noreply.github.com>

github-actions · 2023-10-31T14:28:28Z

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

Add an entry in our changelog in alphabetical order and link related issue
Update the documentation, if needed
Add unit & e2e tests for your changes
GitHub checks are passing
Is the DCO check failing? Here is how you can fix DCO issues

Learn more about:

Our contribution guide

apis/keda/v1alpha1/triggerauthentication_webhook.go

Signed-off-by: radekfojtik <68660951+radekfojtik@users.noreply.github.com>

radekfojtik · 2023-11-17T09:19:38Z

@JorTurFer @tomkerkhove please take a look guys

josefkarasek

I tested the PR locally with:

Install keda v2.11.0 with helm
Create TriggerAuthentication with identityId: ""
Upgrade to keda v.2.12.0 - I was able to reproduce the issue
Deploy patched keda and validation webhook images, include ValidatingWebhookConfiguration from charts:main branch

Old TriggerAuthentications with identityId: "" don't cause issues and the validation webhook blocks the creation of new ones.

josefkarasek · 2023-11-20T14:07:57Z

apis/keda/v1alpha1/triggerauthentication_webhook.go

+	return nil
+}
+
+func isEmptyString(str *string) bool {


Util functions like this one have a high potential for code duplicity. Can you add a new file to pkg/util and move the function there? Ideally with generics, something like

func IsDefaultValue[T comparable](value *T) bool { var def T return value != nil && *value == def }

josefkarasek · 2023-11-20T14:10:18Z

apis/keda/v1alpha1/triggerauthentication_webhook.go

-			if spec.PodIdentity.IdentityID != nil && *spec.PodIdentity.IdentityID == "" {
-				return nil, fmt.Errorf("identityid of PodIdentity should not be empty. If it's set, identityId has to be different than \"\"")
+			if isEmptyString(podIdentity.IdentityID) {
+				return fmt.Errorf("identityid of PodIdentity should not be empty. If it's set, identityId has to be different than \"\"")


Suggested change

return fmt.Errorf("identityid of PodIdentity should not be empty. If it's set, identityId has to be different than \"\"")

return fmt.Errorf("identityId of PodIdentity should not be empty. If it's set, identityId has to be different than \"\"")

zroubalik · 2023-11-20T14:37:26Z

Just a side note, we shouldn't rely on validation webhooks that much. If possible, this should work withouth validation as well.

tomkerkhove · 2023-11-21T07:52:23Z

100%

JorTurFer · 2023-11-21T20:55:45Z

Just a side note, we shouldn't rely on validation webhooks that much. If possible, this should work withouth validation as well.

I have to disagree with this point. KEDA has to work properly handling the scenario, but the data validation is the responsibility of the webhooks, don't you think? I mean, why do we use validating webhooks for validating the data plane, if we are going to raise validation messages in the operator too?

KEDA operator will fail with a handled error if the user is providing the item wrongly (it won't panic), so other validations over the data constraints should be managed by the webhooks and users have to assume that validations are done by webhooks. As user, you can't skip the validating webhooks and also expect a well-formed validation with a lot of info about the validating error

JorTurFer · 2023-11-21T20:58:13Z

LGTM but let's wait until @zroubalik and @tomkerkhove agree with it (or not xD), because this moves the responsibility from the operator to webhooks

tomkerkhove · 2023-11-22T08:22:44Z

I have to disagree with this point. KEDA has to work properly handling the scenario, but the data validation is the responsibility of the webhooks, don't you think?
[...]
As user, you can't skip the validating webhooks and also expect a well-formed validation with a lot of info about the validating error

No, because webhooks are optional. If it would be mandatory then I'd agree.

Validation webhooks are more for enforcing best practices in the current state of KEDA, not for ensuring things are valid. If we want to go that route than making it mandatory is the only viable option.

radekfojtik · 2023-11-22T13:21:25Z

@zroubalik @tomkerkhove Having this validation on the operator side is a breaking change introduced in v2.12 (is not backwards-compatible). I would say it needs to be removed now anyway. Alternatively, it is possible to add a warning and introduce the validation in a later version. But interesting - the operator will warn the user about a non-valid value that was not added by the user but by default by the operator.

radekfojtik · 2023-11-22T15:57:39Z

I tested the PR locally with:

Install keda v2.11.0 with helm

Create TriggerAuthentication with identityId: ""

Upgrade to keda v.2.12.0 - I was able to reproduce the issue

Deploy patched keda and validation webhook images, include ValidatingWebhookConfiguration from charts:main branch

Old TriggerAuthentications with identityId: "" don't cause issues and the validation webhook blocks the creation of new ones.

@josefkarasek There is no need to set the identityId: "", the operator does it for you in case you omit this property (when the finalizer is added). Thanks for suggestions.

zroubalik · 2023-12-21T14:24:33Z

@JorTurFer could you please look at this? We should resolve this already :) Thanks

JorTurFer · 2023-12-21T17:59:02Z

@JorTurFer could you please look at this? We should resolve this already :) Thanks

I reviewed here: #5142 (comment)

From my perspective, webhooks have to validate the information and the operator has to just handle errors if they are. No having VALIDATING webhooks IMHO explicitly means that you don't have validations (because you are not installing them), just fails if you introduce something wrong.
In my opinion, this removal is 100% correct:

It moves the responsibility from the operator to the webhooks. If the operator sees and empty pod identity, it doesn't matter who sets it, and it continues with its work, failing if it has to fail

JorTurFer · 2024-01-22T10:32:42Z

We should push this topic @zroubalik @tomkerkhove

stale · 2024-03-22T11:50:45Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

zroubalik · 2024-03-27T16:10:32Z

@JorTurFer let's add it to the agenda for the next meeting

tomkerkhove · 2024-04-02T05:49:53Z

Sorry, I did not see the pings - Let's discuss indeed

TriggerAuth-podIdentity.identityId - validation removed (operator)

8091a30

Signed-off-by: radekfojtik <68660951+radekfojtik@users.noreply.github.com>

radekfojtik requested a review from a team as a code owner October 31, 2023 14:28

semgrep-app bot reviewed Oct 31, 2023

View reviewed changes

apis/keda/v1alpha1/triggerauthentication_webhook.go Outdated Show resolved Hide resolved

TriggerAuthWebhook-superfluous nil err check before return (fix)

4befa5f

Signed-off-by: radekfojtik <68660951+radekfojtik@users.noreply.github.com>

radekfojtik mentioned this pull request Oct 31, 2023

TriggerAuthentication - spec.podIdentity.identityId as a pointer - breaking change in 2.12.0 #5109

Open

radekfojtik added 3 commits October 31, 2023 15:47

TriggerAuthWebhook-ci static check (fix)

357fd70

Signed-off-by: radekfojtik <68660951+radekfojtik@users.noreply.github.com>

Changelog added

ecbd6a7

Signed-off-by: radekfojtik <68660951+radekfojtik@users.noreply.github.com>

Merge branch 'main' into feature/rfo-main-triggerauth-validation

d7b49c5

josefkarasek approved these changes Nov 20, 2023

View reviewed changes

stale bot added the stale All issues that are marked as stale due to inactivity label Mar 22, 2024

stale bot removed the stale All issues that are marked as stale due to inactivity label Mar 27, 2024

JorTurFer mentioned this pull request Apr 9, 2024

Release: 2.14 #5671

Closed

35 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TriggerAuth-podIdentity.identityId - validation removed (operator) #5142

TriggerAuth-podIdentity.identityId - validation removed (operator) #5142

radekfojtik commented Oct 31, 2023 •

edited

github-actions bot commented Oct 31, 2023

radekfojtik commented Nov 17, 2023

josefkarasek left a comment

josefkarasek Nov 20, 2023

josefkarasek Nov 20, 2023

zroubalik commented Nov 20, 2023

tomkerkhove commented Nov 21, 2023

JorTurFer commented Nov 21, 2023

JorTurFer commented Nov 21, 2023 •

edited

tomkerkhove commented Nov 22, 2023

radekfojtik commented Nov 22, 2023

radekfojtik commented Nov 22, 2023 •

edited

zroubalik commented Dec 21, 2023

JorTurFer commented Dec 21, 2023

JorTurFer commented Jan 22, 2024 •

edited

stale bot commented Mar 22, 2024

zroubalik commented Mar 27, 2024

tomkerkhove commented Apr 2, 2024

	return fmt.Errorf("identityid of PodIdentity should not be empty. If it's set, identityId has to be different than \"\"")
	return fmt.Errorf("identityId of PodIdentity should not be empty. If it's set, identityId has to be different than \"\"")

TriggerAuth-podIdentity.identityId - validation removed (operator) #5142

Are you sure you want to change the base?

TriggerAuth-podIdentity.identityId - validation removed (operator) #5142

Conversation

radekfojtik commented Oct 31, 2023 • edited

Checklist

github-actions bot commented Oct 31, 2023

radekfojtik commented Nov 17, 2023

josefkarasek left a comment

Choose a reason for hiding this comment

josefkarasek Nov 20, 2023

Choose a reason for hiding this comment

josefkarasek Nov 20, 2023

Choose a reason for hiding this comment

zroubalik commented Nov 20, 2023

tomkerkhove commented Nov 21, 2023

JorTurFer commented Nov 21, 2023

JorTurFer commented Nov 21, 2023 • edited

tomkerkhove commented Nov 22, 2023

radekfojtik commented Nov 22, 2023

radekfojtik commented Nov 22, 2023 • edited

zroubalik commented Dec 21, 2023

JorTurFer commented Dec 21, 2023

JorTurFer commented Jan 22, 2024 • edited

stale bot commented Mar 22, 2024

zroubalik commented Mar 27, 2024

tomkerkhove commented Apr 2, 2024

radekfojtik commented Oct 31, 2023 •

edited

JorTurFer commented Nov 21, 2023 •

edited

radekfojtik commented Nov 22, 2023 •

edited

JorTurFer commented Jan 22, 2024 •

edited