-
Notifications
You must be signed in to change notification settings - Fork 421
RFE-7406: Include the K8s ingress resource in namespace inspect collection #2004
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
Open
oarribas
wants to merge
2
commits into
openshift:main
Choose a base branch
from
oarribas:gather-k8s-ingress
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Verify the resource format and collection.
The format
{Resource: "ingresses.networking"}may not correctly specify the Kubernetes Ingress resource from thenetworking.k8s.ioAPI group. Standardschema.GroupResourceusage would be{Group: "networking.k8s.io", Resource: "ingresses"}. While other entries also omit the Group field, it's unclear if the dot-notation in the Resource field will be properly resolved by the code consuming this list.Run the following script to verify how this resource is consumed and whether ingresses are actually collected:
Additionally, please test the inspect command with a namespace containing Ingress resources to confirm they are collected in the output.
🏁 Script executed:
Length of output: 239324
Fix resource specification for Kubernetes Ingress resource.
The format
{Resource: "ingresses.networking"}is incomplete. The Kubernetes Ingress resource belongs to thenetworking.k8s.ioAPI group (full group), not justnetworking.Change line 34 to one of these correct formats:
{Group: "networking.k8s.io", Resource: "ingresses"}Or if the resource can be discovered by name alone:
{Resource: "ingresses"}The current dot-notation
ingresses.networkingmay not resolve correctly during resource discovery and collection. Testing the inspect command with a namespace containing Ingress resources is necessary to confirm the fix works.🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like ^^ comment is valid. Is there any particular reason of using
ingresses.networking?.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Length of output: 8012
🏁 Script executed:
Length of output: 1632
🏁 Script executed:
Length of output: 3626
@ardaguclu Great question! After investigating the codebase, I found that
{Resource: "ingresses.networking"}is inconsistent with the established pattern in this file.Key Finding
Look at line 29 in the same function:
{Resource: "networkpolicies"},NetworkPolicyis also from thenetworking.k8s.ioAPI group, yet it's specified as just{Resource: "networkpolicies"}without any group specification. This proves that simple resource names work fine fornetworking.k8s.ioresources through kubectl/oc's resource discovery mechanism.How the Code Works
In
pkg/cli/admin/inspect/util.go:90-94, thegroupResourceToInfosfunction constructs the dot-notation only when the Group field is populated:Recommendation
To match the existing pattern and be consistent with
networkpolicies, change line 34 to:{Resource: "ingresses"},Alternatively, if explicit group specification is preferred:
{Group: "networking.k8s.io", Resource: "ingresses"},The current
{Resource: "ingresses.networking"}hardcodes what should be constructed by the code, breaking the established pattern where all other entries use simple resource names.🧬 Code Graph Analysis Results
[File: pkg/cli/admin/inspect/namespace.go, Lines 19-36]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API group must be specified because there is more than one
ingressresource in OpenShift clusters and in some old versions, the priorities were different, so API group must be explicitly specified.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oarribas maybe you can just use the form with group and resource as suggested by the bot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ardaguclu , as @palonsoro commented, it was added as there are more than one
ingressresource with different APIs.On the other hand, I haven't see the form with group used anywhere else, so not sure if the suggested one is the correct way. If it is, I can change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please try
ingresses.networking.k8s.iowhether it works or not?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking the
GroupResourcetype, it looks like the right way to go is the first suggested by the bot, as theGropuResourcehas theGroupfor API group and theResourcefor the resource name.@oarribas maybe you didn't see it because it was not required for the collected API groups. But if you do it like the bot suggests (with group and resource) and it fails, that would be a bug by itself, because the
GroupResourcetype is something coming from upstream Kubernetes that should be usable in the way expected by upstream alongside oc.