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

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

Conversation

radekfojtik
Copy link

@radekfojtik radekfojtik commented Oct 31, 2023

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

  • validation remains only in webhook

Checklist

Fixes #5109

Relates to #

Signed-off-by: radekfojtik <68660951+radekfojtik@users.noreply.github.com>
@radekfojtik radekfojtik requested a review from a team as a code owner October 31, 2023 14:28
Copy link

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

While you are waiting, make sure to:

Learn more about:

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

@JorTurFer @tomkerkhove please take a look guys

Copy link
Member

@josefkarasek josefkarasek left a comment

Choose a reason for hiding this comment

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

I tested the PR locally with:

  1. Install keda v2.11.0 with helm
  2. Create TriggerAuthentication with identityId: ""
  3. Upgrade to keda v.2.12.0 - I was able to reproduce the issue
  4. 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.

return nil
}

func isEmptyString(str *string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

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
}

Copy link
Member

Choose a reason for hiding this comment

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

I think that this is a good feedback ❤️

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 \"\"")
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

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

@tomkerkhove
Copy link
Member

100%

@JorTurFer
Copy link
Member

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
Copy link
Member

JorTurFer commented Nov 21, 2023

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
Copy link
Member

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
Copy link
Author

@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
Copy link
Author

radekfojtik commented Nov 22, 2023

I tested the PR locally with:

  1. Install keda v2.11.0 with helm
  2. Create TriggerAuthentication with identityId: ""
  3. Upgrade to keda v.2.12.0 - I was able to reproduce the issue
  4. 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
Copy link
Member

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

@JorTurFer
Copy link
Member

@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:
image

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
Copy link
Member

JorTurFer commented Jan 22, 2024

We should push this topic @zroubalik @tomkerkhove

Copy link

stale bot commented Mar 22, 2024

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.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Mar 22, 2024
@zroubalik
Copy link
Member

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

@stale stale bot removed the stale All issues that are marked as stale due to inactivity label Mar 27, 2024
@tomkerkhove
Copy link
Member

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

@JorTurFer JorTurFer mentioned this pull request Apr 9, 2024
35 tasks
Copy link

stale bot commented Jun 1, 2024

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.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Jun 1, 2024
Copy link

stale bot commented Jun 8, 2024

This issue has been automatically closed due to inactivity.

@stale stale bot closed this Jun 8, 2024
@zroubalik zroubalik reopened this Jun 10, 2024
@stale stale bot removed the stale All issues that are marked as stale due to inactivity label Jun 10, 2024
Copy link

stale bot commented Aug 24, 2024

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.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Aug 24, 2024
Copy link

stale bot commented Aug 31, 2024

This issue has been automatically closed due to inactivity.

@stale stale bot closed this Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale All issues that are marked as stale due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TriggerAuthentication - spec.podIdentity.identityId as a pointer - breaking change in 2.12.0
5 participants