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

Namespace resources are tracked by graph #1320

Merged
merged 27 commits into from
Dec 15, 2023

Conversation

bjee19
Copy link
Contributor

@bjee19 bjee19 commented Dec 1, 2023

Refactor code such that Namespace resources are tracked by the graph, thus letting us remove dependency on using the Capturer to do so.

Problem: The Capturer is currently tracking relevant Namespaces and we would like to instead use the Graph.

Solution: Refactor codebase so Graph tracks referenced namespaces and remove dependency on the Capturer.

Testing: Added additional unit tests to graph, change processor, and new tests to namespaces. Ensure tests pass.

Closes #1263

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

Copy link

netlify bot commented Dec 1, 2023

Deploy Preview for nginx-gateway-fabric ready!

Name Link
🔨 Latest commit 3372c8b4c8d57d8dcc3ba616a931f8105045f8c4
🔍 Latest deploy log https://app.netlify.com/sites/nginx-gateway-fabric/deploys/657b59f4e962cf0008a1fd2e
😎 Deploy Preview https://deploy-preview-1320--nginx-gateway-fabric.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added the tech-debt Short-term pain, long-term benefit label Dec 1, 2023
@bjee19
Copy link
Contributor Author

bjee19 commented Dec 1, 2023

Have a couple of questions:

  1. Do you think having a separate Namespace resource and subsequent namespaceHolder resource stored in namespace.go is necessary? I was trying to follow the Secret resource stored in secret.go but realized that since I hadn't planned for any validation of namespace resources, I could make ReferencedNamespaces in graph.go a map[types.NamespacedName]*v1.Namespace and just have a helper function in graph.go which populates ReferencedNamespaces with the Kubernetes resource namespace instead of our own.
  2. A follow up / related question to the above question, is validation on namespaces necessary? I wasn't sure if that was covered in a different place in the code such that the ClusterState's Namespaces were validated. If so I would keep the separate Namespace resource I created.
  3. Where should I put test cases that cover some of the logic that I removed from capturer_test.go? I was thinking they would go in the change_processor.test but wasn't sure.
  4. With the recent changes in PR Fix attaching of HTTPRoutes to Gateway Listeners #1275 and the addition of the Attachable field, does that change how I currently check for referenced namespaces? Also should I be checking for the Valid field and not consider it a referencedNamespace if the Listener isn't Valid?

@sjberman
Copy link
Collaborator

sjberman commented Dec 4, 2023

@bjee19

  1. I feel like we don't need to have our own namespace resource, it's probably overkill.
  2. Validation on namespaces isn't necessary. These aren't resources that we are generating config for, we're purely looking at labels.
  3. change_processor_test.go probably makes sense, seems like that's where we do a lot of the resource event testing.
  4. Since we don't generate any config for namespaces specifically, we can keep tracking them even if the Listener isn't Valid. Ultimately, the state of the Listener is what determines the config we generate, the namespace is essentially metadata.

@bjee19 bjee19 force-pushed the debt/namespace-resources-tracked-by-graph branch from 23a89a1 to 0e8f1a1 Compare December 4, 2023 22:44
@bjee19
Copy link
Contributor Author

bjee19 commented Dec 4, 2023

Have a couple of test cases that were removed from capturer_test that deal with changing the label selector on a Gateway Listener to match/unmatch a Namespace's labels and seeing if that is reflected accordingly. In the Capturer it would test to see if a relationship existed, but with the new changes, I'm unsure where to put these test cases or if they are needed at all.

change_processor_test doesn't seem to fit as upserting a gateway to update its listener's selector would trigger the change itself.

@sjberman
Copy link
Collaborator

sjberman commented Dec 5, 2023

Is this testing things in namespace.go? If so, you can create namespace_test.go.

@bjee19 bjee19 force-pushed the debt/namespace-resources-tracked-by-graph branch from 7475337 to f8d4bba Compare December 6, 2023 22:31
@bjee19 bjee19 marked this pull request as ready for review December 6, 2023 22:33
@bjee19 bjee19 requested a review from a team as a code owner December 6, 2023 22:34
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

great refactor!
please see my comments, suggestions and requests

internal/mode/static/state/change_processor_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/change_processor_test.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/graph.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/graph.go Show resolved Hide resolved
internal/mode/static/state/graph/namespace.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/namespace.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/namespace.go Show resolved Hide resolved
internal/mode/static/state/graph/namespace.go Outdated Show resolved Hide resolved
internal/mode/static/state/graph/graph_test.go Outdated Show resolved Hide resolved
@bjee19 bjee19 requested a review from pleshakov December 14, 2023 19:03
@bjee19 bjee19 force-pushed the debt/namespace-resources-tracked-by-graph branch from 0d3406f to 3372c8b Compare December 14, 2023 19:39
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

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

🚀 🚀 🚀

@bjee19 bjee19 force-pushed the debt/namespace-resources-tracked-by-graph branch from 05f6bd2 to 5159893 Compare December 15, 2023 22:41
@bjee19 bjee19 merged commit ae48c2b into nginx:main Dec 15, 2023
22 checks passed
@bjee19 bjee19 deleted the debt/namespace-resources-tracked-by-graph branch May 7, 2024 16:38
miledxz added a commit to miledxz/nginx-gateway-fabric that referenced this pull request Jan 14, 2025
Refactor code such that Namespace resources are tracked by the graph, thus letting us remove dependency on using the Capturer to do so.

Problem: The Capturer is currently tracking relevant Namespaces and we would like to instead use the Graph.

Solution: Refactor codebase so Graph tracks referenced namespaces and remove dependency on the Capturer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech-debt Short-term pain, long-term benefit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Namespace resources are tracked by Graph
4 participants