8000 fix: Correct error message on missing path in path validation by zachomedia · Pull Request #2971 · nginx/kubernetes-ingress · GitHub
[go: up one dir, main page]
Skip to content

fix: Correct error message on missing path in path validation #2971

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
Merged
merged 5 commits into from
Sep 19, 2022
Merged

fix: Correct error message on missing path in path validation #2971

merged 5 commits into from
Sep 19, 2022

Conversation

zachomedia
Copy link
Contributor
@zachomedia zachomedia commented Aug 25, 2022 •
edited
Loading

Proposed changes

Describe the use case and detail of the change. If this PR addresses an issue on GitHub, make sure to include a link to that issue here in this description (not in the title of the PR).

In #2775 some path validation was added, including making the path field within the rules required (Kubernetes does not require it). The error message we were receiving was spec: Required field, which didn't point obviously to the issue.

The PR fixes the error message to point to the correct place. With this fix, you'll now see spec.rules[0].http.path[0].path: Required value. Additionally, I have noted the required value in the Restrictions section of the documentation.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

lucacome
lucacome reviewed Aug 25, 2022
View reviewed changes
Copy link
@lucacome lucacome left a comment

Choose a reason for hiding this comment

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

Hi @zachomedia

Thanks for the PR! I'm just trying to understand your config, did you use an empty path before v2.3.0 with the pathType as ImplementationSpecific? Because I think that's the only way to not get an error form k8s when applying the Ingress with recent versions of k8s. Are you using an old version? Any additional info would be appreciated 😄

@zachomedia @lucacome
fix: Fix field path in documentation
1e0078e 
Co-authored-by: Luca Comellini <luca.com@gmail.com>
@zachomedia
Copy link
Contributor Author
zachomedia commented Aug 25, 2022 •
edited
Loading

@lucacome Your guess would be correct :) We had some old ingresses which were using pathType: ImplementationSpecific and no path. Most recently, I had tested it with v2.2.2 and the ingresses were accepted by it. The upgrade to v2.3.0 caused those old ingresses to not be accepted.

We fixed up the ingresses which brought them back, this PR should help anyone else who encounters this find the issue a bit quicker since it'll point to the right spot.

@lucacome lucacome requested review from a team, lucacome, ciarams87 and jjngx September 8, 2022 17:49
@lucacome lucacome added the enhancement Pull requests for new features/feature enhancements label Sep 8, 2022
@lucacome lucacome added this to the v2.4.0 milestone Sep 8, 2022
lucacome
lucacome approved these changes Sep 8, 2022
View reviewed changes
Copy link
@lucacome lucacome left a comment

Choose a reason for hiding this comment

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

I feel like we might revisit the enforcement when the pathType is ImplementationSpecific since technically the path shouldn't be required in that case, but for now we should merge this so at least the error is clear.

Thanks again @zachomedia !

@lucacome lucacome added bug An issue reporting a potential bug and removed enhancement Pull requests for new features/feature enhancements labels Sep 8, 2022
@lucacome lucacome requested a review from a team September 9, 2022 19:00
@github-actions github-actions bot added documentation Pull requests/issues for documentation and removed bug An issue reporting a potential bug labels Sep 13, 2022
@codecov-commenter
Copy link
codecov-commenter commented Sep 13, 2022 •
edited
Loading

Codecov Report

Merging #2971 (a8143d4) into main (ac75bfa) will not change coverage.
The diff coverage is 100.00%.

❗ Current head a8143d4 differs from pull request most recent head 3440b50. Consider uploading reports for the commit 3440b50 to get more accurate results

@@           Coverage Diff           @@
##             main    #2971   +/-   ##
=======================================
  Coverage   52.31%   52.31%           
=======================================
  Files          58       58           
  Lines       15995    15995           
=======================================
  Hits         8368     8368           
  Misses       7350     7350           
  Partials      277      277
Impacted Files Coverage Δ
internal/k8s/validation.go 93.31% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@lucacome lucacome self-assigned this Sep 19, 2022
@lucacome lucacome added the bug An issue reporting a potential bug label Sep 19, 2022
@lucacome lucacome merged commit df6ed7c into nginx:main Sep 19, 2022
@mkoperator mkoperator mentioned this pull request Jan 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haywoodsh haywoodsh haywoodsh approved these changes

@ciarams87 ciarams87 Awaiting requested review from ciarams87 ciarams87 was automatically assigned from nginxinc/kic

+2 more reviewers

@lucacome lucacome lucacome approved these changes

@jjngx jjngx jjngx approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@lucacome lucacome

Labels
bug An issue reporting a potential bug documentation Pull requests/issues for documentation
Projects
None yet
Milestone
v2.4.0
Development

Successfully merging this pull request may close these issues.
5 participants
@zachomedia @codecov-commenter @lucacome @haywoodsh @jjngx
0