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

feat(crd): add CredentialBasicAuth #83

Merged
merged 3 commits into from
Sep 19, 2024
Merged

feat(crd): add CredentialBasicAuth #83

merged 3 commits into from
Sep 19, 2024

Conversation

pmalek
Copy link
Member

@pmalek pmalek commented Sep 18, 2024

What this PR does / why we need it:

This PR adds CredentialBasicAuth CRD which will be used by KGO.

Resources of this type will be created when KongConsumer will use a Secret as credential. This will result in CredentialBasicAuth being created, constituting a 1:1 mapping with a credential resource in Konnect.

Which issue this PR fixes

Related to Kong/gateway-operator#617

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect significant changes

@pmalek pmalek added this to the KGO 1.4 milestone Sep 18, 2024
@pmalek pmalek self-assigned this Sep 18, 2024
@pmalek pmalek force-pushed the credentials-basic-auth branch 3 times, most recently from be3e20e to a847515 Compare September 19, 2024 08:09
@pmalek pmalek marked this pull request as ready for review September 19, 2024 08:09
@pmalek pmalek requested a review from a team as a code owner September 19, 2024 08:09
@pmalek pmalek enabled auto-merge (squash) September 19, 2024 08:10
Copy link
Contributor

@randmonkey randmonkey left a comment

Choose a reason for hiding this comment

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

So a CredentialBasicAuth represents a relationship that a KongConsumer referencing a Secret as a basic auth credential attached to the consumer?

api/configuration/v1alpha1/credential_basic_auth_types.go Outdated Show resolved Hide resolved
@pmalek
Copy link
Member Author

pmalek commented Sep 19, 2024

So a CredentialBasicAuth represents a relationship that a KongConsumer referencing a Secret as a basic auth credential attached to the consumer?

Yes, that's the idea. If more Consumers use the same credential (not sure how realistic use case this is) this will create multiple credential objects for each usage.

@pmalek
Copy link
Member Author

pmalek commented Sep 19, 2024

Blocked for now before we get consensus on how this should look like:

https://kongstrong.slack.com/archives/C079GK3KB9R/p1726735862213059

@pmalek pmalek force-pushed the credentials-basic-auth branch 2 times, most recently from 0bf6bf1 to 27d7191 Compare September 19, 2024 11:44
@pmalek
Copy link
Member Author

pmalek commented Sep 19, 2024

Unblocked.

This for now will contain only the consumer ref and verbatim credential field.

This will replace the "old" way of binding consumers to credentials (Secrets) using the credentials field on the consumer. Instead users will be able to use this (and other types of credential CRDs) to bind consumers to credentials.

In the future we'll be able to provide support for "old" way of binding - using the credentials field - by an implementation of an additional reconciler as in Kong/gateway-operator#623.

@pmalek pmalek merged commit 21b39dc into main Sep 19, 2024
7 checks passed
@pmalek pmalek deleted the credentials-basic-auth branch September 19, 2024 16:03
@czeslavo
Copy link
Contributor

One afterthought I just had: shouldn't we name the CRD KongCredentialBasicAuth (with the Kong prefix) for the sake of consistency with the rest of the resources?

@pmalek
Copy link
Member Author

pmalek commented Sep 20, 2024

One afterthought I just had: shouldn't we name the CRD KongCredentialBasicAuth (with the Kong prefix) for the sake of consistency with the rest of the resources?

I was thinking about this one and went with no prefix. If you feel that would make it more consistent (which in hindsight seems like it would) I can make the change 👍

@czeslavo
Copy link
Contributor

Yeah, I think we should keep it consistent. 👍

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.

3 participants