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

Implement a secured debug interface for istio #31338

Merged
merged 6 commits into from
Mar 19, 2021

Conversation

irisdingbj
Copy link
Member

@irisdingbj irisdingbj commented Mar 9, 2021

make the debug interface secured via xds.

@howardjohn @esnible I put an initial version here to get early feedbacks. Thanks!

@irisdingbj irisdingbj requested review from a team as code owners March 9, 2021 10:07
@google-cla google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Mar 9, 2021
@istio-testing istio-testing added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 9, 2021
@irisdingbj irisdingbj changed the title Implement a secured debug interface for istio [WIP]Implement a secured debug interface for istio Mar 9, 2021
@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Mar 9, 2021
istioctl/cmd/debug.go Outdated Show resolved Hide resolved
pilot/pkg/features/pilot.go Outdated Show resolved Hide resolved
pilot/pkg/xds/debuggen.go Show resolved Hide resolved
pilot/pkg/xds/debuggen.go Outdated Show resolved Hide resolved
istioctl/cmd/debug.go Outdated Show resolved Hide resolved
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 11, 2021
@irisdingbj irisdingbj changed the title [WIP]Implement a secured debug interface for istio Implement a secured debug interface for istio Mar 11, 2021
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Mar 11, 2021
pilot/pkg/xds/debuggen.go Outdated Show resolved Hide resolved
pilot/pkg/xds/debuggen.go Outdated Show resolved Hide resolved
pilot/pkg/xds/debuggen.go Outdated Show resolved Hide resolved
pilot/pkg/xds/debuggen.go Outdated Show resolved Hide resolved
pilot/pkg/xds/debuggen.go Outdated Show resolved Hide resolved
pilot/pkg/xds/debuggen.go Outdated Show resolved Hide resolved
pilot/pkg/xds/debuggen.go Outdated Show resolved Hide resolved
pilot/pkg/xds/debuggen.go Show resolved Hide resolved
pilot/pkg/xds/debuggen.go Outdated Show resolved Hide resolved
pilot/pkg/xds/debuggen.go Outdated Show resolved Hide resolved
@howardjohn
Copy link
Member

Looks great overall!

@irisdingbj
Copy link
Member Author

/test unit-tests_istio

@irisdingbj irisdingbj added the release-notes-none Indicates a PR that does not require release notes. label Mar 15, 2021
@irisdingbj
Copy link
Member Author

/retest

Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

Looks great

pilot/pkg/xds/debuggen.go Outdated Show resolved Hide resolved
pilot/pkg/xds/debuggen.go Outdated Show resolved Hide resolved
pilot/pkg/xds/debuggen.go Outdated Show resolved Hide resolved
@irisdingbj
Copy link
Member Author

@howardjohn PTAL, Thanks!

pilot/pkg/xds/debug.go Outdated Show resolved Hide resolved
pilot/pkg/xds/debug.go Outdated Show resolved Hide resolved
pilot/pkg/xds/debuggen.go Outdated Show resolved Hide resolved
By default it will use the default serviceAccount from (istio-system) namespace if the pod is not specified.
`,
Example: ` # Retrieve sync status for all Envoys in a mesh
istioctl x debug syncz
Copy link
Member

Choose a reason for hiding this comment

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

I am getting

$ istioctl x debug syncz
Error: resource name may not be empty

Copy link
Member Author

Choose a reason for hiding this comment

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

you need specify --xds-address xxxx:15012

Copy link
Member

Choose a reason for hiding this comment

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

can we improve the error message somehow? Also I am not sure why I need to specify that, given the example doesn't. I want to access incluster istiod

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, will address this in the following up PR

istioctl/pkg/xds/client.go Outdated Show resolved Hide resolved
By default it will use the default serviceAccount from (istio-system) namespace if the pod is not specified.
`,
Example: ` # Retrieve sync status for all Envoys in a mesh
istioctl x debug syncz
Copy link
Member

Choose a reason for hiding this comment

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

can we improve the error message somehow? Also I am not sure why I need to specify that, given the example doesn't. I want to access incluster istiod

}
if len(args) == 0 {
return CommandParseError{
e: fmt.Errorf("debug type is required"),
Copy link
Member

Choose a reason for hiding this comment

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

nit: there is no format specifier here.

Copy link
Member

@shamsher31 shamsher31 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. release-notes-none Indicates a PR that does not require release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants