Skip to content
This repository has been archived by the owner on Oct 11, 2024. It is now read-only.

Basic authentication #539

Closed

Conversation

bogumillaska
Copy link

@bogumillaska bogumillaska commented Nov 29, 2022

Description

Change allows to set basic authentication security option on an API Definition deployed using the tyk-operator.
Change allows only for simple setting of basic authentication option using default configuration. It does not allow setting additional basic auth metadata.

Related Issue

TT-7112 Add Basic Authentication option to ApiDefinition CRD

Motivation and Context

Basic authentication was on the list of not implemented features.

Test Coverage For This Change

Added unit test TestApiDefinitionBasicAuth in apidefinition_test.go followin a simillar strcuture to the mtls configuration.
Added config/samples/basic-auth/httpbin_basic_authentication.yaml
Executed manual tests on kind cluster using ce and pro modes

$ kubectl apply -f config/samples/basic-auth/httpbin_basic_authentication.yaml
$ apidefinition.tyk.tyk.io/httpbin-basic-auth created
$ curl http://localhost:8080/httpbin/headers
{
    "error": "Authorization field missing"
}

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [X ] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side). If PRing from your fork, don't come from your master!
  • Make sure you are making a pull request against our master branch (left side). Also, it would be best if you started your change off our latest master.
  • Make sure you are updating CHANGELOG.md based on your changes.
  • My change requires a change to the documentation.
    • If you've changed APIs, describe what needs to be updated in the documentation.
  • I have updated the documentation accordingly.
  • If you've changed API models, please update CRDs.
    • make manifests
    • make helm
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • Check your code additions will not fail linting checks:
    • gofmt -s -w .
    • go vet ./...
    • golangci-lint run

@bogumillaska bogumillaska requested a review from a team as a code owner November 29, 2022 20:55
@bogumillaska bogumillaska requested review from buraksekili and removed request for a team November 29, 2022 20:55
@bogumillaska bogumillaska reopened this Nov 30, 2022
@komalsukhani
Copy link
Collaborator

Thank you @bogumillaska for the PR.
I really like that you added integration tests for your changes! I want to get a feedback on it. Are they easy to write and understand?
Was there any reason for not enabling basic auth metadata fields?

@bogumillaska
Copy link
Author

Hi @komalsukhani
Yes - they were very easy to do. Mostly because mTLS option have an almost identical check. I guess it could be even extracted to some common functions because of those similarities. But I am just beginning my golang and tyk-operator journey so I did not do that at this time.
I left out metadata so far because of time restrictions. We are using basic authentication on the project and we wanted that capability - but so far we dont use any metadata.
We still want to add support for tag-headers and then if time permits we could think about adding metadata as a separate PR.
However - if you consider metadata a must to accept this PR then I can add that of course.

@komalsukhani
Copy link
Collaborator

Can you also update doc here
Do mention that metadata is not supported yet.

@komalsukhani
Copy link
Collaborator

We still want to add support for tag-headers and then if time permits we could think about adding metadata as a separate PR.
I think we do support tag-headers

@bogumillaska
Copy link
Author

We still want to add support for tag-headers and then if time permits we could think about adding metadata as a separate PR.
I think we do support tag-headers

Not according to https://github.com/TykTechnologies/tyk-operator/blob/master/docs/api_definitions.md.
But - true - I see that in the code now that you mention it. Let me verify that - Thank you!

@bogumillaska
Copy link
Author

I resolved all the comments.
Also - I confirm - tag headers seems to work - and they seems to be there for quite some time already - is api_definition.md mentioning some other functionality or it simply needs and update ?

@komalsukhani
Copy link
Collaborator

I resolved all the comments. Also - I confirm - tag headers seems to work - and they seems to be there for quite some time already - is api_definition.md mentioning some other functionality or it simply needs and update ?

Thank you!
api_definition.md file needs an update.

@komalsukhani
Copy link
Collaborator

@bogumillaska Will add your PR in our sprint. Once QA tests it, we will merge it to master.
Also, Next operator release is planned for end of the month.

@komalsukhani
Copy link
Collaborator

One last thing from my side.
Can you run make manifests and make helm and commit changed files?
These commands will update the description of the field in the CRD definition and helm charts.

@caroltyk caroltyk added this to the v0.13.0 milestone Dec 6, 2022
@andrei-tyk andrei-tyk mentioned this pull request Dec 14, 2022
18 tasks
@caroltyk
Copy link
Collaborator

Thank you for your contribution @bogumillaska . This is released in v0.13.0

@caroltyk caroltyk closed this Jan 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants