-
Notifications
You must be signed in to change notification settings - Fork 133
feat: added skip-hash for basic auth for Konnect #1765
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1765 +/- ##
==========================================
- Coverage 32.79% 32.76% -0.03%
==========================================
Files 73 73
Lines 8093 8112 +19
==========================================
+ Hits 2654 2658 +4
- Misses 5273 5289 +16
+ Partials 166 165 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
"thus gaining some performance with large configs.\n"+ | ||
"Usage of this flag without apt select-tags and default-lookup-tags can be problematic.\n"+ | ||
"This flag is not valid with Konnect.") | ||
syncCmd.Flags().BoolVar(&dumpConfig.SkipHashForBasicAuth, "skip-hash-for-basic-auth", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not want this flag for gateway apply
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will add
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, in cases where the basic auth has been synced before without the flag, using this flag leads to no diff and so no action - that is expected, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. That is expected.
The GET /basic-auth endpoint doesn't have any skip-hash param. So, there's no way of knowing if the returned password is a hash or not. Earlier too we didn't compare for passwords while syncing/diffing. So, I am retaining the same behaviour.
067bc53
to
15b64a5
Compare
4baf58a
to
c14ff4f
Compare
return true | ||
} | ||
|
||
if targetContent.Info != nil && targetContent.Info.SkipHashForBasicAuth { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this block necessary? 🤔
This feature introduces a flag to be used with sync
command to skip-hashing of basic-auth credential
passwords when using with Konnect.
An info field is also added for the same.
Linked PRs:
Kong/go-kong#576
Kong/go-database-reconciler#342
For #1747