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.

Sign up for GitHub

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

Jump to bottom

feat: make Gateway.Spec.Addresses.Value optional #3616

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: make Gateway.Spec.Addresses.Value optional #3616

wants to merge 3 commits into from

Conversation

EyalPazz
Copy link
Contributor

What type of PR is this?
/kind feature

Which issue(s) this PR fixes:
Fixes #3612

Does this PR introduce a user-facing change?:

Make the `value` field in Gateway.Spec.Addresses array optional

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 16, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: EyalPazz
Once this PR has been reviewed and has the lgtm label, please assign danwinship for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot
Copy link
Contributor

Hi @EyalPazz. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 16, 2025
@shaneutt shaneutt added this to the v1.3.0 milestone Feb 17, 2025
youngnick
youngnick requested changes Feb 18, 2025
View reviewed changes
Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

This needs more specification; changing a field to optional is not a breaking change, but it is a big one that will require code updates from all controllers. If you are proposing this, including how to handle empty values for implementations that support it and do not will be important.

We may also need an extended feature to cover this GatewayAddressEmpty or something, so that we can slice the conformance tests accordingly.

@shaneutt shaneutt removed their request for review February 18, 2025 11:17
@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 19, 2025
@EyalPazz EyalPazz force-pushed the make-addresses-value-optional branch from 26365be to d39cc82 Compare February 19, 2025 14:04
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Feb 19, 2025
@EyalPazz EyalPazz requested a review from youngnick February 19, 2025 14:06
@EyalPazz
Copy link
Contributor Author

@youngnick, this isn't ready for merge yet, i wanted your opinion on injecting a valid type value to the test suite
Should we take a similar approach to the GatewayStaticAddresses test?

@EyalPazz EyalPazz force-pushed the make-addresses-value-optional branch from d39cc82 to 581ba80 Compare February 19, 2025 14:11
@EyalPazz
Copy link
Contributor Author

@shaneutt , i'd also appreciate your opinion here, as i saw you wrote the PLACEHOLDER_**_ADDRS hacks

@youngnick
Copy link
Contributor

@youngnick, this isn't ready for merge yet, i wanted your opinion on injecting a valid type value to the test suite Should we take a similar approach to the GatewayStaticAddresses test?

That seems reasonable, yes.

robscott
robscott reviewed Feb 20, 2025
View reviewed changes
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on @EyalPazz!

apis/v1/gateway_types.go
Comment on lines 676 to 678
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
Value string `json:"value"`
Copy link
Member

Choose a reason for hiding this comment

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

Although the +optional is a convention, we have two options to make this optional:

A) A pointer to allow for nil/unspecified values

Suggested change
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
Value string `json:"value"`
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
Value *string `json:"value,omitempty"`

B) Allow empty string (no need to distinguish between empty string and unspecified here, so this is likely better)

Suggested change
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
Value string `json:"value"`
// +kubebuilder:validation:MaxLength=253
Value string `json:"value,omitempty"`

Would appreciate some other input on this from others like @youngnick before committing to an approach.

conformance/tests/gateway-optional-address-value.yaml Outdated
gatewayClassName: "{GATEWAY_CLASS_NAME}"
addresses:
# How should we inject a valid type here?
- type: "VALID_ADDRESS_TYPE_PLACEHOLDER"
Copy link
Member

Choose a reason for hiding this comment

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

This seems like the fundamental challenge of writing a conformance test for this. As far as I know, none of our conformance tests so far require a specific address type. I like your idea of allowing this to be a configurable flag on the test suite and that's probably sufficient as a starting point. It could be useful to expand this in the future to have a separate SupportedFeature per type though.

pkg/features/gateway.go

// SupportGatewayAddressEmpty option indicates support for an empty
// spec.addresses.value field
SupportGatewayAddressEmpty FeatureName = "GatewayAddressEmpty"
Copy link
Member

Choose a reason for hiding this comment

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

This feature name doesn't quite seem right. Maybe useful to describe the address options that will be available after this PR merges:

  1. Gateway.spec.addresses unspecified - implementation automatically assigns address(es) of whatever type(s) they choose
  2. Gateway.spec.addresses.value unspecified (new in this PR) - implementation automatically assigns address(es) of the type requested (if possible)
  3. Gateway.spec.addresses fully specified - implementation automatically assigns address requested (if possible)

So that's a long way of saying that the thing that's new here is the ability to request a type of address without specifying the desired value. So maybe a better name would be GatewayTypedAddressAllocation although that's awfully verbose. Naming is decidedly not my specialty, but something in that general direction would probably be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can't a fully specified Gateway.spec.addresses also be considered a TypedAddress?

@EyalPazz
Copy link
Contributor Author

@youngnick @robscott can you please re-review this?

@EyalPazz EyalPazz requested a review from robscott February 24, 2025 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youngnick youngnick Awaiting requested review from youngnick

@robscott robscott Awaiting requested review from robscott

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Gateway API Pipeline
Status: Review
Milestone
v1.3.0
Development

Successfully merging this pull request may close these issues.

Gateway.Spec.Addresses.Value should be optional
5 participants
@EyalPazz @k8s-ci-robot @youngnick @robscott @shaneutt