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

krt: initial debug interface #53597

Merged
merged 6 commits into from
Nov 5, 2024
Merged

krt: initial debug interface #53597

merged 6 commits into from
Nov 5, 2024

Conversation

howardjohn
Copy link
Member

@howardjohn howardjohn commented Oct 21, 2024 •
edited
Loading

This adds an initial krtz debug interface.

Example output:

name: ServiceEntryWorkloads
state:
  inputCollection: ServiceEntries
  inputs:
    default/external-svc-https:
      dependencies:
      - AuthzDerivedPolicies
      - AuthzDerivedPolicies
      - MeshConfig
      - PeerAuthentications
      - PeerAuthentications
      - static
      - static
      outputs:
      - Kubernetes/networking.istio.io/ServiceEntry/default/external-svc-https/api.dropboxapi.com
      - Kubernetes/networking.istio.io/ServiceEntry/default/external-svc-https/www.googleapis.com
  outputs:
    Kubernetes/networking.istio.io/ServiceEntry/default/external-svc-https/api.dropboxapi.com:
      canonicalName: external-svc-https
      canonicalRevision: latest
      clusterId: Kubernetes
      hostname: api.dropboxapi.com
      name: external-svc-https
      namespace: default
      services:
        default/api.dropboxapi.com:
          ports:
          - servicePort: 443
            targetPort: 443
      uid: Kubernetes/networking.istio.io/ServiceEntry/default/external-svc-https/api.dropboxapi.com
      workloadName: external-svc-https
      workloadType: POD
    Kubernetes/networking.istio.io/ServiceEntry/default/external-svc-https/www.googleapis.com:
      canonicalName: external-svc-https
      canonicalRevision: latest
      clusterId: Kubernetes
      hostname: www.googleapis.com
      name: external-svc-https
      namespace: default
      services:
        default/www.googleapis.com:
          ports:
          - servicePort: 443
            targetPort: 443
      uid: Kubernetes/networking.istio.io/ServiceEntry/default/external-svc-https/www.googleapis.com
      workloadName: external-svc-https
      workloadType: POD

@howardjohn howardjohn added the release-notes-none Indicates a PR that does not require release notes. label Oct 21, 2024
@howardjohn howardjohn requested a review from a team as a code owner October 21, 2024 22:51
@howardjohn howardjohn added the do-not-merge/hold Block automatic merging of a PR. label Oct 21, 2024
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 21, 2024
howardjohn
howardjohn commented Oct 21, 2024
View reviewed changes
pkg/kube/krt/informer.go Outdated
@@ -184,6 +185,7 @@ func WrapClient[I controllers.ComparableObject](c kclient.Informer[I], opts ...C
close(h.synced)
h.log.Infof("%v synced", h.name())
}()
RegisterCollectionForDebugging(h)
Copy link
Member Author

Choose a reason for hiding this comment

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

Should probably move this to collection creation and not have a global var

@stevenctl
Copy link
Contributor

stevenctl commented Oct 22, 2024 •
edited
Loading

Can we have a debug output that shows only the collection dependencies and not the full list of actual inputs/output items.

@hzxuzhonghu
Copy link
Member

hzxuzhonghu commented Oct 23, 2024 •
edited
Loading

Without looking at the code it is hard to understand the meaning. Can you add a key beforedefault/external-svc-https:

And also for dependencies,

      - static with filter {}
      - static with filter {}

@howardjohn
Copy link
Member Author

Can you add a key beforedefault/external-svc-https:

I am not sure I understand. that string is the key.. what key would we add?

@hzxuzhonghu
Copy link
Member

I mean like kubectl output, you can add a explicit kind ServiceEntry, etc

@howardjohn
Copy link
Member Author

ah good point. let me see if I can pass the input type through. ill clean up the filters to be more clear as well

@ramaraochavali
Copy link
Contributor

What does inputs and outputs signify?

@howardjohn
Copy link
Member Author

What does inputs and outputs signify?

istio/pilot/pkg/serviceregistry/kube/controller/ambient/workloads.go

Line 134 in 915ac2b

return func(ctx krt.HandlerContext, wle *networkingclient.WorkloadEntry) *model.WorkloadInfo {

Input: WorkloadEntrys
Output: WorkloadInfos

@lgadban lgadban mentioned this pull request Oct 28, 2024
Open
@howardjohn
Copy link
Member Author

OK I have implemented all the TODOs and cleaned things up a bit, this is ready to go from my perspective. See the original description for the new output

@hzxuzhonghu
Copy link
Member 

default/external-svc-https:    
  dependencies:
  - AuthzDerivedPolicies          // can this be set to user aware istio API 
  - AuthzDerivedPolicies   // duplicate
  - MeshConfig
  - PeerAuthentications
  - PeerAuthentications
  - static                       // what does static mean
  - static

hzxuzhonghu
hzxuzhonghu reviewed Oct 31, 2024
View reviewed changes
pkg/kube/krt/collection.go Outdated
//for os, o := range h.collectionState.outputs {
// h.log.Errorf("Output %v -> %v", os, o)
//}
//h.log.Errorf("<<< END DUMP")
Copy link
Member

Choose a reason for hiding this comment

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

nit: remove

@howardjohn
Copy link
Member Author 

default/external-svc-https:    
  dependencies:
  - AuthzDerivedPolicies          // can this be set to user aware istio API 
  - AuthzDerivedPolicies   // duplicate
  - MeshConfig
  - PeerAuthentications
  - PeerAuthentications
  - static                       // what does static mean
  - static

oops, my bad - I fixed this already but forgot to update the description after fixing that one. Now it says NetworkTrigger I believe

@howardjohn howardjohn removed the do-not-merge/hold Block automatic merging of a PR. label Nov 5, 2024
stevenctl
stevenctl approved these changes Nov 5, 2024
View reviewed changes
pkg/kube/krt/debug.go

// maybeRegisterCollectionForDebugging registers the collection in the debugger, if one is enabled
func maybeRegisterCollectionForDebugging[T any](c Collection[T], handler *DebugHandler) {
if handler == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

it could be nice to have an env like KRT_GLOBAL_DEBUG that allows enabling debug on all collections without modifying code to apss in GlobalDebugHandler

@istio-testing istio-testing merged commit 7ac0508 into istio:master Nov 5, 2024
27 checks passed
MaYuan-02 pushed a commit to MaYuan-02/istio that referenced this pull request Dec 8, 2024
@howardjohn
krt: initial debug interface (istio#53597)
9b81de0 
* krt: initial debug interface

* make it more explicit

* fixes

* banner

* revert api change

* lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevenctl stevenctl stevenctl approved these changes

@costinm costinm Awaiting requested review from costinm

@linsun linsun Awaiting requested review from linsun

@ericvn ericvn Awaiting requested review from ericvn

@hzxuzhonghu hzxuzhonghu Awaiting requested review from hzxuzhonghu

Assignees

@stevenctl stevenctl

Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@howardjohn @stevenctl @hzxuzhonghu @ramaraochavali @istio-testing