Adding tests for dex plus restructuring and formatting #2815
Conversation
|
@kromanow94 @kimwnasptd can you take a look at this. Somehow no one noticed it during distribution testing. If this is a problem, we need to have a test for the DEX login flow. Interestingly it has not been changed in 7 months https://github.com/kubeflow/manifests/commits/master/common/dex/base/config-map.yaml |
|
@tzabbi can you extend the dex test, such that this does not happen again? |
|
@juliusvonkohout not sure how to imporve the test. If I see it correctly your dex test is just to build and deploy the kubernetes manifests right? https://github.com/kubeflow/manifests/blob/master/.github/workflows/dex_test.yaml#L31 In this case we have to log into the system. |
|
I will try a curl. |
|
Does it only apply the manifests so far? Yes. manifests/.github/workflows/dex_test.yaml Line 31 in ace875b https://www.kubeflow.org/docs/components/pipelines/user-guides/core-functions/connect-api/#example-for-dex might have some information. |
|
Hello all, This wasn't an issue because the Please see: But, I suppose with the changed default from the oidc-authservice to the oauth2-proxy, we can as well just delete the oidc-authservice from Having said the above, @tzabbi's issue is no longer a bug but a cleanup and refactoring. @tzabbi , since you already started work in here, could you make the changes described above (remove oidc-autheservice definition and references)? |
During the cleanup/refactoring we also have to address #2734 (comment) https://github.com/kubeflow/manifests/blob/master/apps/centraldashboard/upstream/overlays/oauth2-proxy/kustomization.yaml is not allowed in the upstream folder. This breaks every time when we synchronize the upstream manifest. It is tracked here as well #2804. |
@kromanow94 you are totally right. I cloned this repo again and get the same result. I might be the case that I modified this with a patch... Yes, I will make this changes but not before tomorrow. |
|
@juliusvonkohout it is still required to add a test? since my "bug" wasn't a bug. |
If you have the time yes. More inportant might be "During the cleanup/refactoring we also have to address #2734 (comment) https://github.com/kubeflow/manifests/blob/master/apps/centraldashboard/upstream/overlays/oauth2-proxy/kustomization.yaml is not allowed in the upstream folder. This breaks every time when we synchronize the upstream manifest. It is tracked here as well #2804." |
There was a problem hiding this comment.
During the cleanup/refactoring we also have to address #2734 (comment)
https://github.com/kubeflow/manifests/blob/master/apps/centraldashboard/upstream/overlays/oauth2-proxy/kustomization.yaml is not allowed in the upstream folder. This breaks every time when we synchronize the upstream manifest. It is tracked here as well #2804.
There was a problem hiding this comment.
I've created a new directory called manual-patches (the name might be not the best) so you can make proposals what do you prefer.
common/oidc-client/README.md
Outdated
There was a problem hiding this comment.
@kromanow94 are we sure that all this information is in the other readme? @tzabbi I think we have to merge them properly
There was a problem hiding this comment.
Especially the section ## Kubeflow Pipelines User and M2M Authentication and Authorization and below
There was a problem hiding this comment.
You can also just leave it in for now @tzabbi if it is too much effort to merge it.
There was a problem hiding this comment.
Added this content again, but I'm not sure how to adjust it properly.
|
/ok-to-test |
|
Regarding i think it should be ../apps/centraldashboard/overlays/oauth2-proxy in /example/kustomization.yaml |
|
@hansinikarunarathne it is better now, but still not 100% fixed I think |
|
@juliusvonkohout what is with the yaml linter should i fix it? |
I think @tzabbi has not taken the changes I made to his branch. Can you check it @tzabbi ? |
|
Hi @hansinikarunarathne which change do you mean? were can i found your changes? |
Hi @tzabbi. I did the fix of trying to linting deleted YAML files in this PR #2823. Please feel free to check my changes. Changes are in the master. You can take it from the master. If you further face issues regarding this, let me know |
|
@hansinikarunarathne on master? I already pulled it :) |
In your master, I see this But I changed it into So that's why I mentioned that your branch is not up to master of Kubelow manifest. If I am wrong , make me correct |
|
Looks like you are right. I will check the inconsistency tomorrow. |
Signed-off-by: Tom Zaspel <[email protected]>
|
Added your changes @hansinikarunarathne can you confirm that all your changes are now in the PR? |
|
@hansinikarunarathne @tzabbi i rebased it 20 hours ago after merging your linting PR, as you can see from the history. |
|
It is still quite similar at least the errors have to be fixed. |
|
But please do not make this PR bigger than it needs to be. Lets only fix files that we have to from now on. The more files the more cumbersome and slow will be my review. |
juliusvonkohout
changed the title
|
Sorry for the whole syntax missmatches i have to check my linter... |
I have added guidelines for lining in the lining Github action. I think It will help you to fix those issues.
|
Signed-off-by: Tom Zaspel <[email protected]>
|
Fixed all errors. |
|
I see Maybe a chmod +x is needed. |
|
In general @tzabbi please fix the dex test in a follow up PR |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: juliusvonkohout The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Let us continue in #2827 |
* fix redirectURIs for in dex config since kubeflow uses oauth2 Signed-off-by: Tom Zaspel <[email protected]> * remove authservice and refactore code Signed-off-by: Tom Zaspel <[email protected]> * remove authservice from github actions and refactore code Signed-off-by: Tom Zaspel <[email protected]> * adjust name of oauth2 in FAQ Signed-off-by: Tom Zaspel <[email protected]> * create new directory in central dashboard called manuel-patches for oauth2-proxy Signed-off-by: Tom Zaspel <[email protected]> * change manuel-patches to overlay Signed-off-by: Tom Zaspel <[email protected]> * Test if user can login to dex Signed-off-by: Tom Zaspel <[email protected]> * Add kubeflow authentication oidc-authservice README.md Signed-off-by: Tom Zaspel <[email protected]> * Fix yaml linter error Signed-off-by: Tom Zaspel <[email protected]> * Fix yaml linting Signed-off-by: Tom Zaspel <[email protected]> * Add changes from comit: 08f217c Signed-off-by: Tom Zaspel <[email protected]> * Fix even more lint issues Signed-off-by: Tom Zaspel <[email protected]> --------- Signed-off-by: Tom Zaspel <[email protected]> Signed-off-by: Tom Zaspel <[email protected]> Co-authored-by: Hansini Karunarathne <[email protected]> Signed-off-by: Patrick Schönthaler <[email protected]>
Pull Request Template for Kubeflow manifests Issues
✏️ A brief description of the changes
📦 List any dependencies that are required for this change
🐛 If this PR is related to an issue, please put the link to the issue here.
✅ Contributor checklist
DCOcheck) ✅cla/googlecheck) ✅