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

Set SupportedVersion on GatewayClass #1301

Merged
merged 11 commits into from
Dec 5, 2023

Conversation

kate-osborn
Copy link
Contributor

Proposed changes

Problem: NGF does not set the SupportedVersion condition on the GatewayClass.
From the spec:

The version of Gateway API CRDs that is installed in a cluster can be determined
by looking at the gateway.networking.k8s.io/bundle-version annotation on each
CRD. Each implementation MUST compare that with the list of versions that it
recognizes and supports. Implementations with a GatewayClass MUST publish the
SupportedVersion condition on the GatewayClass to indicate whether the CRDs
installed in the cluster are supported.

Solution: Set the SupportedVersion condition on the GatewayClass.

This PR adds a new controller that watches the metadata of CRDs. CRD events are filtered using a custom predicate that inspects the gateway.networking.k8s.io/bundle-version annotation. CRDs without this annotation will be ignored. Updates to this annotation will trigger a state change and build a new graph. This required some changes to how we determine if the state has changed in the changeTrackingUpdater.

Below is a table that shows how the SupportedVersion and Accepted conditions are set for different combinations of major and minor versions (using our current supported version v1.0.0):

Major Version Minor Version SupportedVersion Accepted Config Generated
1 0 True True Yes
1 any False True Yes
> 1 any False False No

When we update the Gateway API version, we will have to update the supported version in the code.

Testing:

  • Ran conformance tests and verified that the Provisioner sets the SupportedVersion condition on the GatewayClass.
  • Verified updates to the bundle-version on the Gateway API CRDs update the GatewayClass status accordingly.

Closes #1178

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

@kate-osborn kate-osborn requested a review from a team as a code owner November 30, 2023 22:12
Copy link

netlify bot commented Nov 30, 2023

Deploy Preview for nginx-gateway-fabric canceled.

Name Link
🔨 Latest commit 8b45826
🔍 Latest deploy log https://app.netlify.com/sites/nginx-gateway-fabric/deploys/656f4c9b8b79fc0008f3842d

@github-actions github-actions bot added documentation Improvements or additions to documentation enhancement New feature or request helm-chart Relates to helm chart labels Nov 30, 2023
@kate-osborn
Copy link
Contributor Author

@nginxinc/nginx-gateway-fabric part of the acceptance criteria of this issue is to "Update the upgrade docs to warn users about ensuring both NGF and the Gateway CRDs are updated together."

I looked at the upgrade instructions and don't think this is necessary anymore. Let me know if you disagree.

@kate-osborn kate-osborn requested a review from a team November 30, 2023 22:22
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.

introducing predicates in the store is 👍

internal/framework/controller/register.go Show resolved Hide resolved
internal/framework/gatewayclass/validate.go Show resolved Hide resolved
internal/framework/gatewayclass/validate.go Outdated Show resolved Hide resolved
internal/framework/conditions/conditions.go Show resolved Hide resolved
internal/framework/gatewayclass/validate.go Show resolved Hide resolved
internal/mode/static/state/changed_predicate.go Outdated Show resolved Hide resolved
internal/mode/static/state/store.go Outdated Show resolved Hide resolved
@pleshakov
Copy link
Contributor

@kate-osborn

@nginxinc/nginx-gateway-fabric part of the acceptance criteria of this issue is to "Update the upgrade docs to warn users about ensuring both NGF and the Gateway CRDs are updated together."

I looked at the upgrade instructions and don't think this is necessary anymore. Let me know if you disagree.

What if we update the following docs (and similarly for Helm) with something like below?

Upgrade Gateway API resources:

Verify that your NGINX Gateway Fabric version is compatible with the Gateway API resources. Refer to the [Technical Specifications]({{< relref "reference/technical-specifications.md" >}}) for details.

from https://github.com/nginxinc/nginx-gateway-fabric/blob/main/site/content/installation/installing-ngf/manifests.md#upgrade-nginx-gateway-fabric

Verify that the current and the new NGINX Gateway Fabric version is compatible with the Gateway API resources. Refer to the [Technical Specifications]({{< relref "reference/technical-specifications.md" >}}) for details. If the current version is not compatible, NGINX Gateway Fabric will stop serving traffic during a period after your upgrade the Gateway API resources and before you upgrade NGINX Gateway Fabric to the new version.

@kate-osborn
Copy link
Contributor Author

@kate-osborn

@nginxinc/nginx-gateway-fabric part of the acceptance criteria of this issue is to "Update the upgrade docs to warn users about ensuring both NGF and the Gateway CRDs are updated together."

I looked at the upgrade instructions and don't think this is necessary anymore. Let me know if you disagree.

What if we update the following docs (and similarly for Helm) with something like below?

Upgrade Gateway API resources:

Verify that your NGINX Gateway Fabric version is compatible with the Gateway API resources. Refer to the [Technical Specifications]({{< relref "reference/technical-specifications.md" >}}) for details.

from https://github.com/nginxinc/nginx-gateway-fabric/blob/main/site/content/installation/installing-ngf/manifests.md#upgrade-nginx-gateway-fabric

Verify that the current and the new NGINX Gateway Fabric version is compatible with the Gateway API resources. Refer to the [Technical Specifications]({{< relref "reference/technical-specifications.md" >}}) for details. If the current version is not compatible, NGINX Gateway Fabric will stop serving traffic during a period after your upgrade the Gateway API resources and before you upgrade NGINX Gateway Fabric to the new version.

@pleshakov I think this note makes more sense in the release notes when there's a known issue upgrading to the new version.

@pleshakov
Copy link
Contributor

@pleshakov I think this note makes more sense in the release notes when there's a known issue upgrading to the new version.

@kate-osborn

Let's say current NFG supports Gateway API X, but doesn't support Y. The next NGF supports Gateway API Y.
What will we say in the release notes for the next NGF?

@Jcahilltorre
Copy link
Contributor

@pleshakov I think this note makes more sense in the release notes when there's a known issue upgrading to the new version.

@kate-osborn

Let's say current NFG supports Gateway API X, but doesn't support Y. The next NGF supports Gateway API Y. What will we say in the release notes for the next NGF?

This release of NGINX Gateway Fabric adds support for Gateway API Y.

@Jcahilltorre
Copy link
Contributor

added some style suggestions

@bjee19
Copy link
Contributor

bjee19 commented Dec 5, 2023

What was the reason for introducing predicates in the store? Having a little difficulty understanding if its like a refactor thing or if it ties into the SupportedVersion on the GatewayClass.

@kate-osborn
Copy link
Contributor Author

What was the reason for introducing predicates in the store? Having a little difficulty understanding if its like a refactor thing or if it ties into the SupportedVersion on the GatewayClass.

@bjee19 it's both 😄

The bundle version annotation on the Gateway API CRDs contains the installed version that we need to validate. For these CRDs, we want to trigger a change when the bundle version annotation changes. However, the changeTrackingUpdater did not consider changes to the annotations as a "change" because the resource generation does not change. Resource generation is only incremented when the spec changes. So, I had to come up with a way to trigger a change when the bundle version annotation of the Gateway API CRD changes. Rather than add branching or inject another function like triggerStateChange, I added predicates.

Predicates allow us to apply custom logic for determining if a resource changes. For Gateway API CRDs, we only want to trigger a change if the bundle version annotation changes. For a Gateway, we only want to trigger a change if the generation changes. For a Secret, we only want to trigger a change if it is referenced by the last graph.

@kate-osborn kate-osborn merged commit d6bbdba into nginxinc:main Dec 5, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request helm-chart Relates to helm chart
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

SupportedVersion condition for GatewayClass
5 participants