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

Add support for GSSAPI in Kafka scaler #4851

Merged
merged 9 commits into from
Oct 10, 2023
Merged

Conversation

novicr
Copy link
Contributor

@novicr novicr commented Aug 2, 2023

Added Kerberos authentication to Kafka scaler. TriggerAuthentication can now accept additional parameters that will be passed to Sarama to authenticate scaler via Kerberos.

Checklist

  • [N/A] 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
  • [N/A] 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 #4836

Relates to PR: kedacore/keda-docs#1201

@novicr novicr requested a review from a team as a code owner August 2, 2023 03:21
@github-actions
Copy link

github-actions bot commented Aug 2, 2023

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

🏖️ Over the summer, the response time will be longer than usual due to maintainers taking time off so please bear with us.

While you are waiting, make sure to:

Learn more about:

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Nice contribution!
Can we cover this new feature with an e2e tests somehow?

@novicr
Copy link
Contributor Author

novicr commented Aug 7, 2023

My changes only consider the kafka scaler (based on sarama). The change is to collect relevant information in TriggerAuthentication and pass it along to sarama. Not sure about the e2e test - would require standing up kerberos infra as part of the test. I'd rather trust that sarama will do the right thing with inputs (it does). Unit tests ensure that correct values are passed along.

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Is there any way to add an e2e tests which tests this?

config/manager/manager.yaml Outdated Show resolved Hide resolved
pkg/scalers/kafka_scaler.go Show resolved Hide resolved
pkg/scalers/kafka_scaler.go Show resolved Hide resolved
@sansmoraxz
Copy link
Contributor

FYI As far as I see as of yet strimzi doesn't have support for kerberos: strimzi/strimzi-kafka-operator#3088

@zroubalik
Copy link
Member

Also, how do we deal with the token refreshment? I am not Kerberors expert at all, though I think that it needs to be refreshed after some period of time.

@novicr
Copy link
Contributor Author

novicr commented Aug 17, 2023

Also, how do we deal with the token refreshment? I am not Kerberors expert at all, though I think that it needs to be refreshed after some period of time.

Sarama takes care of it. We just provide the path to key tab

@novicr
Copy link
Contributor Author

novicr commented Aug 19, 2023

I moved temp kerberos files into dedicated directory, just in case someone already mounts /tmp into their container. Don't think we are likely to run into any conflicts with /tmp/kerberos, especially since kerberos isn't supported by keda yet

@sansmoraxz
Copy link
Contributor

sansmoraxz commented Aug 19, 2023

Once this merges, I will take a dig with kafka-go

@novicr
Copy link
Contributor Author

novicr commented Aug 23, 2023

What are next steps on this? There were couple small commits after approval. Need someone to approve again. Also, how do we make sure this goes hand in hand with docs and charts PR's?

@sansmoraxz
Copy link
Contributor

E2E tests run by one of the core maintainers. Ignore my above approval, that was a misclick.

@sansmoraxz
Copy link
Contributor

They are still on vacation. So progress is slow.

@JorTurFer
Copy link
Member

What are next steps on this? There were couple small commits after approval. Need someone to approve again. Also, how do we make sure this goes hand in hand with docs and charts PR's?

We check that all the PRs are ready to merge before merging it on any repo. It's a manual action that we do before merging.
I guess that during next days we will address all the reviews that we have because we have slowed the speed during summer vacations (personally, I had more than 100 notifications waiting review in GH xD)

Signed-off-by: Roman Novichenok <roman.novichenok@gmail.com>
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

I am still not sure we should add another mount point. If the file needs to be stored, can't we use some in-memory go filesystem implementation?

@novicr
Copy link
Contributor Author

novicr commented Sep 16, 2023

I am still not sure we should add another mount point. If the file needs to be stored, can't we use some in-memory go filesystem implementation?

Not sure what you mean, but open to any suggestions. I can spin up in-mem fs in the code and create a file in there, but I don't see any way to pass that file to sarama. Sarama accepts a parameter that if a file system path to the required files (kyetab and krb5.conf). Any files created in my in-memory filesystem will not be visible to sarama.

@zroubalik
Copy link
Member

I am still not sure we should add another mount point. If the file needs to be stored, can't we use some in-memory go filesystem implementation?

Not sure what you mean, but open to any suggestions. I can spin up in-mem fs in the code and create a file in there, but I don't see any way to pass that file to sarama. Sarama accepts a parameter that if a file system path to the required files (kyetab and krb5.conf). Any files created in my in-memory filesystem will not be visible to sarama.

Right.

@sansmoraxz how does kafka-go client deal with kerberos auth? Does it need a file in a filesystem as well?

@sansmoraxz
Copy link
Contributor

Strictly speaking it's not something that's there in the official repo. Well there was a PR but not much progress was made.

The sasl interface should be loose enough to implement without issues. Have to look more in dept.

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

@novicr Hi, the PR is looking good. The only concern is the new volume mount to host Kerberos files.

We talked about the issue on our community call today and the suggestion is:

  • don't make this feature default (the volume mount part in config/manager.yaml
  • in the documentation add a section with an explanation, that if users want to use GSSAPI in Kafka scaler, they need to update the deployment with the volume mount (a code snippet that can be copy pasted is the way to go)
  • the scaler code should reflect this and print an error message in case kerberos files are not available with a suggestion on adding a mount
  • the existing Helm Chart supports adding arbitrary volume mounts, so it should be good from this side, CC @JorTurFer to confirm

Thanks!

FYI we plan to do a release on Wednesday

CHANGELOG.md Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

  • the existing Helm Chart supports adding arbitrary volume mounts, so it should be good from this side, CC @JorTurFer to confirm

yep!
https://github.com/kedacore/charts/blob/274c330fa559e594d2eab1e897b581de6bc23e6d/keda/values.yaml#L475-L492

Signed-off-by: Roman Novichenok <roman.novichenok@gmail.com>
Signed-off-by: Roman Novichenok <roman.novichenok@gmail.com>
Signed-off-by: Roman Novichenok <roman.novichenok@gmail.com>
Signed-off-by: Roman Novichenok <roman.novichenok@gmail.com>
pkg/scalers/kafka_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/kafka_scaler.go Outdated Show resolved Hide resolved
@novicr
Copy link
Contributor Author

novicr commented Sep 30, 2023

Removed kerberos mount from config/manager.yaml
Added instructions to keda-docs to add dedicated mount when turning on kerberos auth (both via manager.yaml and charts). Not sure my description in the docs is sufficient, please provide suggestions there.

When /tmp is not writable and no writable /tmp/kerberos is provided, error message now has instructions to follow documentation.

Signed-off-by: Roman Novichenok <roman.novichenok@gmail.com>
CHANGELOG.md Outdated Show resolved Hide resolved
@zroubalik
Copy link
Member

zroubalik commented Oct 10, 2023

/run-e2e kafka
Update: You can check the progress here

novicr and others added 2 commits October 10, 2023 09:49
Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: novicr <43680008+novicr@users.noreply.github.com>
Signed-off-by: Roman Novichenok <roman.novichenok@gmail.com>
@zroubalik
Copy link
Member

zroubalik commented Oct 10, 2023

/run-e2e kafka
Update: You can check the progress here

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution!

@zroubalik zroubalik enabled auto-merge (squash) October 10, 2023 13:56
@zroubalik zroubalik merged commit bfdb931 into kedacore:main Oct 10, 2023
19 checks passed
toniiiik pushed a commit to toniiiik/keda that referenced this pull request Jan 15, 2024
Signed-off-by: Roman Novichenok <roman.novichenok@gmail.com>
Signed-off-by: novicr <43680008+novicr@users.noreply.github.com>
Co-authored-by: Roman Novichenok <roman.novichenok@gmail.com>
Co-authored-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: anton.lysina <alysina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GSSAPI sasl mechanism for Kafka scaler
6 participants