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

Add more details in guideline for Auth Istio&Apisix #485

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

XinyaoWa
Copy link

Description

Add more details in guideline for Auth Istio&Apisix

Issues

List the issue or RFC link this PR is working on. If there is no such link, please mark it as n/a.

Type of change

List the type of change like below. Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would break existing design and interface)

Dependencies

List the newly introduced 3rd party dependency if exists.

Tests

Describe the tests that you ran to verify your changes.

@XinyaoWa XinyaoWa requested a review from Ruoyu-y as a code owner October 18, 2024 10:29
@XinyaoWa
Copy link
Author

@ckhened Please help to have a review~

authN-authZ/auth-istio/README.md Show resolved Hide resolved
First export the router service through istio ingress gateway.

```bash
kubectl apply -f $(pwd)/$DEPLOY_METHOD/chatQnA_router_gateway.yaml
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is only used for option: via JWT token generated by OIDC providers with curl
Why it is defined in the prerequisite section?

Copy link
Author

Choose a reason for hiding this comment

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

I think this has no relationship with the token generation way. It is just from istio gateway, and should be added for all megaservices if you want to do authentication.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, understand. But for authentication with the oauth service, another gateway configuration needs to get applied since there are some extra configuration. Applying both of them might introduce conflicts or complexity. So if you want to set the gateway anyway, please put the steps in each section, instead of putting the step in the Prerequisite section.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your suggestion. Putting the steps in each section will be duplicated. Thus I add "Optional" to this part and add a suggestion for this, do you think it's work?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for your suggestion. Putting the steps in each section will be duplicated. Thus I add "Optional" to this part and add a suggestion for this, do you think it's work?

Well. I think it is a must for all options to find the ingress port and ip address, but for the step kubectl apply -f $(pwd)/$DEPLOY_METHOD/chatQnA_router_gateway.yaml, it is ONLY used for the first two options of authentication, and NOT needed(instead of optional) for the third option, as it shall only use kubectl apply -f $(pwd)/$DEPLOY_METHOD/chatQnA_router_gateway_oauth.yaml instead.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your explanation!! I have just move this part to "Perform authentication and authorization via JWT tokens generated by OIDC provider" section as your original sequence, please have a check~

authN-authZ/auth-istio/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-apisix/README.md Outdated Show resolved Hide resolved
authN-authZ/auth-apisix/keycloak_install.yaml Outdated Show resolved Hide resolved
@Ruoyu-y Ruoyu-y requested a review from ckhened November 4, 2024 01:44
@XinyaoWa XinyaoWa requested a review from Ruoyu-y November 6, 2024 03:34
authN-authZ/auth-istio/README.md Outdated Show resolved Hide resolved
First export the router service through istio ingress gateway.

```bash
kubectl apply -f $(pwd)/$DEPLOY_METHOD/chatQnA_router_gateway.yaml
Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, understand. But for authentication with the oauth service, another gateway configuration needs to get applied since there are some extra configuration. Applying both of them might introduce conflicts or complexity. So if you want to set the gateway anyway, please put the steps in each section, instead of putting the step in the Prerequisite section.

authN-authZ/auth-istio/README.md Show resolved Hide resolved
authN-authZ/auth-apisix/keycloak_install.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@Ruoyu-y Ruoyu-y left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -0,0 +1,33 @@
# Copyright (C) 2024 Intel Corporation
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of adding this file, could you re use the one in templates and add a new values file like values_megaservice.yaml and update the values accordingly so that the implementation will be consistent with helm charts

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