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

enhancement: Add token based auth as alternative to basic-auth for http requests. #192

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

jansalapatek
Copy link

Added two new variables to manage token based authentication which for example is required for using k8s-sidecar with git(lab), since it does not support basic-auth.

@jansalapatek jansalapatek marked this pull request as ready for review June 7, 2022 15:56
@jansalapatek
Copy link
Author

I cannot label the PR, but it is an enhancement.

@jekkel jekkel added the enhancement New feature or request label Jun 9, 2022
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@jekkel
Copy link
Member

jekkel commented Jun 9, 2022

Would you be able to add a test for your scenario?

Fixed description on auth mechanism
fixed linebreak
Copy link
Member

@jekkel jekkel left a comment

Choose a reason for hiding this comment

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

I really wished for a test case ;)

src/helpers.py Outdated Show resolved Hide resolved
src/helpers.py Outdated Show resolved Hide resolved
src/helpers.py Outdated Show resolved Hide resolved
src/helpers.py Outdated Show resolved Hide resolved
src/helpers.py Show resolved Hide resolved
src/helpers.py Outdated Show resolved Hide resolved
src/helpers.py Outdated Show resolved Hide resolved
@jansalapatek
Copy link
Author

I really wished for a test case ;)

Not sure about the currently implemented test concept. Implementing it will cost more time on my end, until I understood it.

@jekkel
Copy link
Member

jekkel commented Jun 13, 2022

There's no real test concept. What we have right now is a e2e test using Kind and a longish bash script whose exit code is relevant for fail or pass. Adding a test case would mean to adapt the script accordingly or introduce a sound test concept. I'd be ok with either.

@jansalapatek
Copy link
Author

I do not exactly know how to test it locally, but I think I got the general concept.

@jansalapatek jansalapatek requested a review from jekkel June 14, 2022 07:20
jekkel
jekkel previously approved these changes Jun 14, 2022
Copy link
Member

@jekkel jekkel left a comment

Choose a reason for hiding this comment

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

Great job, thank you for your contribution! Approving already but do you think a negative test for wrong api key would be worth the effort?

volumeMounts:
- name: shared-volume
mountPath: /tmp/
- name: script-volume
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 we don't need the script here.

Copy link
Author

Choose a reason for hiding this comment

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

good point. removed the script

value: /tmp/
- name: RESOURCE
value: both
- name: SCRIPT
Copy link
Member

Choose a reason for hiding this comment

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

and here

Copy link
Author

Choose a reason for hiding this comment

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

good point. removed the script

Copy link
Author

Choose a reason for hiding this comment

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

also switched resource to configmap

volumes:
- name: shared-volume
emptyDir: {}
- name: script-volume
Copy link
Member

Choose a reason for hiding this comment

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

and here

Copy link
Author

Choose a reason for hiding this comment

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

good point. removed the script

@@ -27,3 +38,9 @@ async def read_item():
@app.post("/503", status_code=503)
async def read_item():
return 503


@app.get("/200/api_key")
Copy link
Member

Choose a reason for hiding this comment

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

could you add another endpoint with a different API key so we can add a negative test case as well? That would be awesome!

Copy link
Author

Choose a reason for hiding this comment

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

added a new container with wrong credentials/secret and used the same endpoint

@jekkel
Copy link
Member

jekkel commented Jun 14, 2022

@jekkel
Copy link
Member

jekkel commented Jun 14, 2022

jekkel
jekkel previously approved these changes Jun 14, 2022
test/server/server.py Outdated Show resolved Hide resolved
jekkel
jekkel previously approved these changes Jun 14, 2022
@jekkel
Copy link
Member

jekkel commented Jun 14, 2022

I think we only need to get the build green before merging/releasing.

jekkel
jekkel previously approved these changes Jun 15, 2022
@jansalapatek
Copy link
Author

It seems, the test file is not present in the pod, but I do not know where to download the pod logs to check, why.

@jansalapatek
Copy link
Author

Can you run the build pipe once more? thx!

@jansalapatek
Copy link
Author

Build failed, because python image could not be fetched from registry :-/ server had a 503.

@jekkel
Copy link
Member

jekkel commented Nov 7, 2022

Build failed, because python image could not be fetched from registry :-/ server had a 503.

New build running...

... and failing.

@jansalapatek please investigate. You can download the pods log from the action summary page (can you?)

@github-actions github-actions bot added the stale close issues and PRs after 60 days of inactivity label Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request stale close issues and PRs after 60 days of inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants