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

Add Accepted condition to BackendTLSPolicy #6151

Merged

Conversation

christianang
Copy link
Contributor

This PR adds the Accepted Condition to the BackendTLSPolicy.

The Status contains an array of Ancestor Policy Statuses. The Ancestor Ref will be set to the gateway similar to what happens on routes now.
If the BackendTLSPolicy is successfully applied to the Gateway for a given HTTPRoute and Backend Service the "Accepted" condition status will be set to true with the "Accepted" Reason.
If the BackendTLSPolicy is found, but is invalid due to any validation reasons (e.g unsupported certRefs, missing certs, malformed hostname, etc) the status will be set to false with the "Invalid" Reason.

Fixes #6137

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (943d5e2) 78.58% compared to head (d5d71cb) 79.45%.
Report is 6 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6151      +/-   ##
==========================================
+ Coverage   78.58%   79.45%   +0.87%     
==========================================
  Files         141      142       +1     
  Lines       20187    16219    -3968     
==========================================
- Hits        15864    12887    -2977     
+ Misses       4012     3019     -993     
- Partials      311      313       +2     
Files Coverage Δ
internal/dag/gatewayapi_processor.go 93.32% <100.00%> (-0.63%) ⬇️
internal/status/cache.go 93.27% <93.33%> (+0.25%) ⬆️
internal/status/backendtlspolicyconditions.go 91.37% <91.37%> (ø)

... and 132 files with indirect coverage changes

@christianang
Copy link
Contributor Author

christianang commented Feb 1, 2024

One bit of missing implementation at the moment is adding a condition if the TargetRef is missing or invalid. Couple of open questions to work through:

  • What would be the ancestor ref of the status for this condition. Since I didn't match this BackendTLSPolicy normally from the BackendRef its not really connect to a gateway yet. I could use whatever gateway is already setup, but since there could be multiple gateways should I add the condition multiple times? Or should we just ignore backendtlspolicies that haven't been matched and not add the condition.
  • An invalid TargetRef may be invalid to our gateway, but perhaps not to another implmentation, so perhaps we shouldn't assume a non Service TargetRef is invalid and therefore not add a condition in this case.

Any thoughts here @sunjayBhatia @skriss?

@sunjayBhatia
Copy link
Member

One bit of missing implementation at the moment is adding a condition if the TargetRef is missing or invalid. Couple of open questions to work through:

  • What would be the ancestor ref of the status for this condition. Since I didn't match this BackendTLSPolicy normally from the BackendRef its not really connect to a gateway yet. I could use whatever gateway is already setup, but since there could be multiple gateways should I add the condition multiple times? Or should we just ignore backendtlspolicies that haven't been matched and not add the condition.
  • An invalid TargetRef may be invalid to our gateway, but perhaps not to another implmentation, so perhaps we shouldn't assume a non Service TargetRef is invalid and therefore not add a condition in this case.

Any thoughts here @sunjayBhatia @skriss?

I see Note also that implementations MUST ONLY populate ancestor status for the Ancestor resources they are responsible for. in the spec which makes me think that if a Policy does not successfully target any resource in the Gateway->Route->Service hierarchy then we can/should ignore it

This is a bit of a bad experience for users who happen to make a mistake in their target reference config, i.e. typo a name and not see the Policy status get updated but that sounds a bit more like an issue the spec needs to solve (worth opening an issue upstream perhaps). Up for interpretation but I don't think it is the responsibility of every Gateway implementation to reconcile every instance of a Policy resource kind it knows about that shows up in the cluster, because as you say, it could be related to a completely different implementations Gateway/etc.

@christianang
Copy link
Contributor Author

Opened an issue here: kubernetes-sigs/gateway-api#2755

@christianang christianang marked this pull request as ready for review February 5, 2024 19:02
@christianang christianang requested a review from a team as a code owner February 5, 2024 19:02
@christianang christianang requested review from skriss and sunjayBhatia and removed request for a team February 5, 2024 19:02
@sunjayBhatia sunjayBhatia requested review from a team, izturn and clayton-gonsalves and removed request for a team February 5, 2024 19:02
@christianang christianang force-pushed the gateway-api-btp-conditions branch from 43e3205 to 248c895 Compare February 5, 2024 19:07
@christianang christianang added the release-note/small A small change that needs one line of explanation in the release notes. label Feb 5, 2024
internal/status/cache.go Outdated Show resolved Hide resolved
internal/dag/status_test.go Outdated Show resolved Hide resolved
internal/dag/status_test.go Show resolved Hide resolved
@christianang christianang force-pushed the gateway-api-btp-conditions branch from 248c895 to 4bfc825 Compare February 7, 2024 22:34
@christianang
Copy link
Contributor Author

Thanks for the feedback @sunjayBhatia! Addressed all feedback and it should be ready for another review.

Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

As far as I can tell, just one last tiny thing to add otherwise looks great 👍🏽

internal/status/backendtlspolicyconditions.go Outdated Show resolved Hide resolved
internal/status/backendtlspolicyconditions.go Show resolved Hide resolved
Signed-off-by: Christian Ang <christian.ang@broadcom.com>
@christianang christianang force-pushed the gateway-api-btp-conditions branch from 4bfc825 to d5d71cb Compare February 7, 2024 23:17
@sunjayBhatia
Copy link
Member

@projectcontour/maintainers any thoughts about getting this into 1.28?

@skriss
Copy link
Member

skriss commented Feb 9, 2024

@projectcontour/maintainers any thoughts about getting this into 1.28?

I haven't had a chance to review but if you're happy with it I'm fine with including.

@sunjayBhatia sunjayBhatia merged commit 7d6831b into projectcontour:main Feb 12, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/small A small change that needs one line of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Status Condition logic for BackendTLSPolicies
3 participants