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

fix(bug): Encode name in filter by name FIQL Query #484

Closed
wants to merge 2 commits into from

Conversation

thunderboltsid
Copy link
Contributor

@thunderboltsid thunderboltsid commented Oct 2, 2024

Not encoding the name can cause errors when non-standard characters are present in the name.

How has this been tested?
Unter testing

  • Create a new subnet with escapable characters in the name: vlan173|ncn|dev|sandbox
  • Create a new cluster with a machine deployment pointing to that subnet
  • Observe the cluster come up successfully

Current status: with these changes we are still seeing failed machines with:

  failureMessage: 'Failure detected from referenced resource infrastructure.cluster.x-k8s.io/v1beta1,
    Kind=NutanixMachine with name "quick-start-0j5mcd-zb6l8-k6fqs": failed to retrieve
    subnet by name vlan173|ncn|dev|sandbox'

Not encoding the name can cause errors when non-standard characters
are present in the name.
Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 33.14%. Comparing base (78f61d1) to head (cfbda29).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #484      +/-   ##
==========================================
+ Coverage   33.00%   33.14%   +0.14%     
==========================================
  Files          17       17              
  Lines        1809     1810       +1     
==========================================
+ Hits          597      600       +3     
+ Misses       1192     1190       -2     
  Partials       20       20              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@adiantum adiantum left a comment

Choose a reason for hiding this comment

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

lgtm
but you need to add some tests

@thunderboltsid
Copy link
Contributor Author

thunderboltsid commented Oct 2, 2024

Closing this PR as URL encoding doesn't take care of all escaping. e.g. /subnets/list is a POST endpoint and the filter is passed as a json object where the | has to be escaped as \\|. We have decided to make a tradeoff in favor correctness in exchange for performance and do all filtering client-side. We might revisit a more exhaustive FIQL filtering in the future but for now switch to client side filters See #485

@thunderboltsid thunderboltsid deleted the jira/NCN-102972 branch October 2, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants