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.

Sign up for GitHub

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

Jump to bottom

Add --kubernetes flag to make dapr invoke cmd support kubernetes #862

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

Add --kubernetes flag to make dapr invoke cmd support kubernetes #862

wants to merge 36 commits into from

Conversation

imneov
Copy link
Contributor

@imneov imneov commented Dec 26, 2021 •
edited
Loading

Description

Support the invoke cmd on kubernetes mode.

The process is similar:

  1. Get the Pod corresponding to the App
  2. Complete the specific communication through the SubResource("proxy") of RESTClient
  3. Direct access to App's AppPort port or access to daprd's HttpPort port

                                      +-----------------+
                                      |                 |
                     AppPort          | +-------------+ |
         +----------------------------> |    APP      | |
  +----------+                        | +-------------+ |
  | dapr cli |                        |                 |
  +----------+                        | +-------------+ |
         +----------------------------> |    Daprd    | |
                    HttpPort          | +-------------+ |
                                      |                 |
                                      |            pod  |
                                      +-----------------+

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #[436] #[848]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@imneov imneov requested review from a team as code owners December 26, 2021 03:36
@imneov imneov changed the title Add --kubernetes flag to make dapr invoke cmd support kubernetes mode Add --kubernetes flag to make dapr invoke cmd support kubernetes Dec 26, 2021
@imneov imneov changed the title Add --kubernetes flag to make dapr invoke cmd support kubernetes [WIP] Add --kubernetes flag to make dapr invoke cmd support kubernetes Dec 26, 2021
tanvigour
tanvigour reviewed Dec 27, 2021
View reviewed changes
Copy link
Contributor

@tanvigour tanvigour left a comment

Choose a reason for hiding this comment

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

Can we add test case for this?

@imneov
Copy link
Contributor Author

imneov commented Dec 30, 2021

Can we add test case for this?

Sorry, just saw the reply.

Thank you tanvigour for review. I wrote part of the test code. There may be problems with code style or edge cases. Please review.

Welcome to add test cases.

@imneov
Copy link
Contributor Author

imneov commented Dec 30, 2021

There is a problem with using proxy to access the HttpPort, mainly because the address that daprd listens to is 127.0.0.1, and the proxy cannot access this port.

So now our project(tkeel-io/cli#74) uses portforward for invoke. The basic process is as follows:

  1. Get the Pod corresponding to the App
  2. Build a tunnel between the local and target pod through RESTClient's SubResource("portforward")
  3. Access daprd's HttpPort port through the local tunnel port

This part of the implementation needs to modify portforward.go to implement random ports (#863).

@imneov imneov changed the title [WIP] Add --kubernetes flag to make dapr invoke cmd support kubernetes Add --kubernetes flag to make dapr invoke cmd support kubernetes Jan 28, 2022
@mukundansundar
Copy link
Collaborator

@imneov Can you signoff on your commits? https://github.com/dapr/cli/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-signing-your-work

imneov and others added 7 commits March 2, 2022 09:02
@mukundansundar
Copy link
Collaborator

@imneov Can you sign off all your commits ? https://github.com/dapr/cli/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-signing-your-work

anhldbk
anhldbk reviewed Mar 2, 2022
View reviewed changes
cmd/invoke.go Outdated Show resolved Hide resolved
pkg/kubernetes/invoke.go Show resolved Hide resolved
@imneov imneov force-pushed the feat/interact-with-kubernetes-dapr-apps branch 4 times, most recently from 8d399b4 to 8df3681 Compare March 22, 2022 01:44
@mukundansundar
Copy link
Collaborator

@imneov Can you fix conflicts in the PR?

@imneov
Copy link
Contributor Author

imneov commented Apr 6, 2022

@imneov Can you fix conflicts in the PR?

Yes, I fix it.

@mukundansundar
Copy link
Collaborator

Thanks for the changes ... We will try to review this soon ...

shubham1172
shubham1172 reviewed Apr 7, 2022
View reviewed changes
cmd/invoke.go Outdated Show resolved Hide resolved
@imneov imneov force-pushed the feat/interact-with-kubernetes-dapr-apps branch from ad44543 to 9dad732 Compare April 10, 2022 03:38
@imneov
Copy link
Contributor Author

imneov commented Sep 27, 2022

@pravinpushkar HI~ Is there anything else I need to do?

@pravinpushkar
Copy link
Contributor

@imneov Please fix the small lint issue. PR looks fine. We need to update this here also to be in sync - https://docs.dapr.io/reference/cli/dapr-invoke/

Do I need to submit a PR for docs?

Yes, If not done already then please create a doc issue and PR and link it here.
I have triggered the PR checks let's see.

@pravinpushkar pravinpushkar added the docs-needed Docs PR needed before this PR can be merged label Sep 27, 2022
@imneov
Copy link
Contributor Author

imneov commented Sep 30, 2022

Support the invoke cmd on kubernetes mode.

OK , Thanks for you reply. @pravinpushkar

There is doc issue(dapr/docs#2846) and PR(dapr/docs#2847)

@pravinpushkar pravinpushkar removed the docs-needed Docs PR needed before this PR can be merged label Sep 30, 2022
@imneov
Copy link
Contributor Author

imneov commented Oct 17, 2022

@pravinpushkar Hi~ Is there any work I need to do?

@dapr-bot
Copy link
Collaborator

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@dapr-bot dapr-bot added the stale label Nov 25, 2022
@dapr-bot
Copy link
Collaborator

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@dapr-bot dapr-bot added the stale label Dec 26, 2022
mukundansundar
mukundansundar reviewed Dec 28, 2022
View reviewed changes
Copy link
Collaborator

@mukundansundar mukundansundar left a comment

Choose a reason for hiding this comment

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

please add E2E for this feature.

@msfussell
Copy link
Member

@mukundansundar - What is happening with this issue/PR?

@cicoyle
Copy link
Contributor

cicoyle commented Jul 26, 2024

@imneov - looks like there are some e2e failures, mind looking into and fixing those?
@dapr/maintainers-cli - looks like this PR has been open quite a while, we could use a review on this please :)

That being said, given the time constraint for this release, I will push this to the next release

@cicoyle cicoyle added this to the v1.15 milestone Jul 26, 2024
@cicoyle cicoyle requested a review from a team July 26, 2024 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mukundansundar mukundansundar mukundansundar left review comments

@tanvigour tanvigour tanvigour left review comments

@shubham1172 shubham1172 shubham1172 left review comments

@anhldbk anhldbk anhldbk left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
pinned
Projects
None yet
Milestone
v1.15
Development

Successfully merging this pull request may close these issues.