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

Ingress V1: Parse Ingress V1 entries from Store. #832

Merged
merged 9 commits into from
Sep 14, 2020

Conversation

mflendrich
Copy link
Contributor

@mflendrich mflendrich commented Sep 10, 2020

closes #789
closes #791

This PR:

  • extends parser.Build() with fromIngressV1(): as of now, parser.Build includes v1 ingresses in the resulting KongState. There are three bits that we still need after this PR for full KEP compatibility:
  • makes some drive-by cleanups (see individual commits)

After this PR, KIC practically supports Ingress V1 (in a scope specified above).

Also, fromIngressV1 is unit-tested but there's no high level test for parser.Build actually handling ingressv1 correctly. I'd like to spark a discussion about the right way to test this change (on a higher level than unit testing).

Notes to reviewers:

  • consider reviewing commit by commit
  • consider not squashing

@mflendrich mflendrich changed the base branch from main to feat/ingressv1-informers-store September 10, 2020 14:37
Base automatically changed from feat/ingressv1-informers-store to next September 11, 2020 01:05
@mflendrich mflendrich changed the base branch from next to feat/ingressv1-statussync September 11, 2020 01:44
@mflendrich mflendrich force-pushed the feat/ingressv1-parser branch 3 times, most recently from 68d1782 to 3bb41da Compare September 11, 2020 12:10
Base automatically changed from feat/ingressv1-statussync to next September 11, 2020 12:11
@mflendrich mflendrich force-pushed the feat/ingressv1-parser branch 2 times, most recently from bdb59f1 to 0a0a1cb Compare September 11, 2020 13:26
@mflendrich mflendrich marked this pull request as ready for review September 11, 2020 14:55
@mflendrich mflendrich changed the title [WIP] Ingress V1: Parse Ingress V1 entries from Store. Ingress V1: Parse Ingress V1 entries from Store. Sep 11, 2020
Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

Based on my work in #843, I think this is reasonably good to go. There are issues elsewhere (raised in #843), but not with these changes, more things alongside them that were in the controller before and whose limitations are now becoming apparent.

+1 on most of Harry's comments, but I believe they've now been addressed. I'll wait for his follow-up if he has any remaining items that we should continue to discuss here.

Copy link
Member

@hbagdi hbagdi left a comment

Choose a reason for hiding this comment

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

I'm still not sure about route priorities. They come into effect only if there is a regex route (they don't have any effect for regular paths).

Let's revisit this later. For most users, this is intuitive enough that it should not cause problems. Famous last words.

@mflendrich mflendrich merged commit d40ef57 into next Sep 14, 2020
@mflendrich mflendrich deleted the feat/ingressv1-parser branch September 14, 2020 20:17
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