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

controller: fix watch resources that are created by the controller #160

Conversation

sivanantha321
Copy link
Contributor

@sivanantha321 sivanantha321 commented Jan 22, 2025

Fixes #134

This PR makes the AIGatewayRoute controller to watch the resources that are created by it and trigger reconcilation.

@sivanantha321 sivanantha321 requested a review from a team as a code owner January 22, 2025 13:10
@sivanantha321 sivanantha321 changed the title fix: AIGatewayRoute controller does not watches for HTTPRoute events controller: fix AIGatewayRoute controller does not watches for HTTPRoute events Jan 22, 2025
@mathetake
Copy link
Member

sorry i can't accept basically any fix without proper tests

@mathetake mathetake self-assigned this Jan 22, 2025
@mathetake
Copy link
Member

checking - are you up for writing test or we have to take this over

@sivanantha321
Copy link
Contributor Author

checking - are you up for writing test or we have to take this over

Yeah I can write a test for this.

@mathetake
Copy link
Member

great, simple one would be enough

@sivanantha321 sivanantha321 force-pushed the fix/aigatewayroute-not-watches-httproute branch from e20cdbb to 87148ec Compare January 25, 2025 17:24
Copy link

netlify bot commented Jan 25, 2025

Deploy Preview for envoy-ai-gateway ready!

Name Link
🔨 Latest commit 87148ec
🔍 Latest deploy log https://app.netlify.com/sites/envoy-ai-gateway/deploys/67951e3d0283d70008679dae
😎 Deploy Preview https://deploy-preview-160--envoy-ai-gateway.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.

@sivanantha321
Copy link
Contributor Author

sivanantha321 commented Jan 25, 2025

@mathetake Added a test. Let me know if anything else needs to be done.

@@ -159,6 +160,10 @@ func (c *configSink) syncAIGatewayRoute(aiGatewayRoute *aigv1a1.AIGatewayRoute)
},
Spec: gwapiv1.HTTPRouteSpec{},
}
err := ctrlutil.SetControllerReference(aiGatewayRoute, &httpRoute, c.client.Scheme())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I remove the owner reference as we have controller reference ?

Copy link
Member

Choose a reason for hiding this comment

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

http route is not the only thing we create, so i think the change here will be a partial solution. so you could either delete this line or make changes to all resources not only the http route in this pr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is required for the fix to work, so i will update all other resources to use controller reference.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good

@mathetake mathetake added this to the v.0.1.0 milestone Jan 26, 2025
Signed-off-by: Sivanantham Chinnaiyan <[email protected]>
Signed-off-by: Sivanantham Chinnaiyan <[email protected]>
Signed-off-by: Sivanantham Chinnaiyan <[email protected]>
@sivanantha321 sivanantha321 force-pushed the fix/aigatewayroute-not-watches-httproute branch from 87148ec to f58a7b7 Compare January 28, 2025 11:47
@sivanantha321 sivanantha321 changed the title controller: fix AIGatewayRoute controller does not watches for HTTPRoute events controller: fix watch resources that are created by the controller Jan 28, 2025
@sivanantha321
Copy link
Contributor Author

@mathetake I noticed the controller creating HTTPRouteFilter without owner reference. I believe this because the resource is created per namespace. This resource is not watched by the controller. So If it gets deleted accidentally, It won't be recreated until the next reconcilation triggered by some other resource. The controller reference based approach used in this PR may not be appropriate for this resource.

@mathetake
Copy link
Member

the controller creating HTTPRouteFilter without owner reference. I believe this because the resource is created per namespace.

yeah the HTTPRouteFilter is another issue for sure.

Copy link
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

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

excellent! thank you for working on this

@mathetake mathetake merged commit 20b2107 into envoyproxy:main Jan 28, 2025
30 checks passed
@mathetake
Copy link
Member

@sivanantha321 thank you for your contribution, nice work!

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.

Delete HTTPRoute does not trigger reconciliation
2 participants