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

EyalPazz · 2025-02-16T09:00:54Z

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 · 2025-02-16T09:01:03Z

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.

youngnick

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.

apis/v1/gateway_types.go

EyalPazz · 2025-02-19T14:09:16Z

@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 · 2025-02-19T14:16:50Z

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

youngnick · 2025-02-20T00:36:21Z

@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

Thanks for taking this on @EyalPazz!

robscott · 2025-02-20T01:57:19Z

apis/v1/gateway_types.go

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


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.

Yes, I agree that because empty string means the same thing as unspecified, I'm fine with not moving to a pointer, and just allowing this to default to "".

what are scenarios where we'll want to use a pointer?

Good question, I didn't make that very clear.

The pointer is useful when you want to be able to distinguish between unspecified (nil) and empty (""). In our case we don't so a string without a min length is fine but in some cases it is a useful distinction.

apis/v1/gateway_types.go

robscott · 2025-02-20T02:05:57Z

conformance/tests/gateway-optional-address-value.yaml

+  gatewayClassName: "{GATEWAY_CLASS_NAME}"
+  addresses:
+  # How should we inject a valid type here?
+  - type: "VALID_ADDRESS_TYPE_PLACEHOLDER"


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.

robscott · 2025-02-20T02:12:58Z

pkg/features/gateway.go

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


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

Gateway.spec.addresses unspecified - implementation automatically assigns address(es) of whatever type(s) they choose

Gateway.spec.addresses.value unspecified (new in this PR) - implementation automatically assigns address(es) of the type requested (if possible)

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.

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

How about GatewayAutoAddressAssignment (as opposed to the existing static assignment)?

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

Fair point.

How about GatewayAutoAddressAssignment (as opposed to the existing static assignment)?

Isn't this what happens by default with most/all Gateways when spec.addresses is empty?

Maybe a generic name like "GatewayAddressByType" would work, but even that doesn't feel quite right. The more I think about this, the more it seems like starting with names per address type like "GatewayIPAddressAllocation" would be the most future proof path here.

The more I think about this, the more it seems like starting with names per address type like "GatewayIPAddressAllocation" would be the most future proof path here.

and then add a feature for each type of each provider?

or have IPAddress | Hostname | Dynamic

and then add a feature for each type of each provider?

Theoretically yes, but I think all existing Gateway API implementations support IP Address, so that's probably a sufficient starting point.

or have IPAddress | Hostname | Dynamic

What would Dynamic mean here? My intention with GatewayIPAddressAllocation was that it would indicate that a Gateway would provision an IPAddress if requested in spec.addresses.type. (I think all do by default today without that request, but it's still a reasonable test to be sure).

Dynamic would mean everything that is provider-specific, but maybe that's not the correct term
Isn't it kinda weird that for each type you would need to have a new Feature Request instead of just letting the implementation worry about it, and just make sure it conforms from our side?

While I expect lots of implementation-specific values to exist here, I don't think there's really a good way to conformance test implementation-specific support like this, so I'd leave that out of scope.

EyalPazz · 2025-02-24T12:26:10Z

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

robscott · 2025-02-28T02:14:58Z

/ok-to-test

EyalPazz · 2025-03-02T08:52:46Z

/retest

robscott

Thanks @EyalPazz!

apis/v1/shared_types.go

apis/v1/gateway_types.go

robscott · 2025-03-05T17:45:25Z

Hey @EyalPazz, I chatted with @youngnick about this and we agreed that it would make sense to move the conformance test for this into a follow up PR. The rationale is that as long as we have a clearly defined API spec for this, the conformance test would be covering with 99+% of implementations already do (automatically assign an IP address to a Gateway), so it's not quite as critical as we'd initially thought.

Will you have time to update the PR in the next day or two to reflect this new smaller scope? Would love to get this into v1.3 if possible.

EyalPazz · 2025-03-05T18:57:51Z

Hey @EyalPazz, I chatted with @youngnick about this and we agreed that it would make sense to move the conformance test for this into a follow up PR. The rationale is that as long as we have a clearly defined API spec for this, the conformance test would be covering with 99+% of implementations already do (automatically assign an IP address to a Gateway), so it's not quite as critical as we'd initially thought.

Will you have time to update the PR in the next day or two to reflect this new smaller scope? Would love to get this into v1.3 if possible.

Yea for sure, i'll do it tomorrow morning

EyalPazz · 2025-03-06T08:48:17Z

Should be ok now :) @robscott

robscott · 2025-03-07T06:23:30Z

Thanks @EyalPazz! Will leave final LGTM for @youngnick or someone else.

/approve

k8s-ci-robot · 2025-03-07T06:23:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: EyalPazz, robscott

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [robscott]

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 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 requested review from robscott and shaneutt February 16, 2025 09:00

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 added this to the v1.3.0 milestone Feb 17, 2025

youngnick requested changes Feb 18, 2025

View reviewed changes

apis/v1/gateway_types.go Show resolved Hide resolved

shaneutt removed their request for review February 18, 2025 11:17

EyalPazz force-pushed the make-addresses-value-optional branch from 26365be to d39cc82 Compare February 19, 2025 14:04

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 requested a review from youngnick February 19, 2025 14:06

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

robscott reviewed Feb 20, 2025

View reviewed changes

EyalPazz requested a review from robscott February 24, 2025 12:26

youngnick mentioned this pull request Feb 27, 2025

Adding GEP-3539: Gateway API to Expose Pods on Cluster-Internal IP Address (ClusterIP Gateway) #3608

Open

EyalPazz force-pushed the make-addresses-value-optional branch from cdfd8c2 to 1487843 Compare February 27, 2025 08:37

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 27, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 28, 2025

EyalPazz added 4 commits March 3, 2025 09:35

feat: make Gateway.Spec.Addresses.Value optional

bcc53c1

wip: test mock

e3c5cb6

feat: flag injection & change GatewayAddress -> GatewaySpecAddress

9b4c8d1

add omitempty to the value

687de42

EyalPazz force-pushed the make-addresses-value-optional branch from 4cd24c2 to 687de42 Compare March 3, 2025 07:36

chore: update codegen

cc7a71b

robscott reviewed Mar 5, 2025

View reviewed changes

apis/v1/shared_types.go Outdated Show resolved Hide resolved

apis/v1/gateway_types.go Outdated Show resolved Hide resolved

EyalPazz added 3 commits March 6, 2025 10:32

fix: type validation and minLength

afb0490

chore: remove conformance test for now

1d7df0f

complete rename of GatewayAddress -> GatewaySpecAddress

35f890d

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

EyalPazz commented Feb 16, 2025

k8s-ci-robot commented Feb 16, 2025

youngnick left a comment

EyalPazz commented Feb 19, 2025

EyalPazz commented Feb 19, 2025

youngnick commented Feb 20, 2025

robscott left a comment

robscott Feb 20, 2025

youngnick Feb 26, 2025

EyalPazz Feb 27, 2025

robscott Feb 28, 2025

robscott Feb 20, 2025

robscott Feb 20, 2025

EyalPazz Feb 20, 2025

youngnick Feb 26, 2025

robscott Feb 26, 2025

EyalPazz Feb 27, 2025

EyalPazz Feb 27, 2025

robscott Feb 28, 2025

EyalPazz Mar 2, 2025 •

edited

Loading

robscott Mar 2, 2025

EyalPazz commented Feb 24, 2025

robscott commented Feb 28, 2025

EyalPazz commented Mar 2, 2025

robscott left a comment

robscott commented Mar 5, 2025

EyalPazz commented Mar 5, 2025

EyalPazz commented Mar 6, 2025

robscott commented Mar 7, 2025

k8s-ci-robot commented Mar 7, 2025

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

Are you sure you want to change the base?

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

Conversation

EyalPazz commented Feb 16, 2025

k8s-ci-robot commented Feb 16, 2025

youngnick left a comment

Choose a reason for hiding this comment

EyalPazz commented Feb 19, 2025

EyalPazz commented Feb 19, 2025

youngnick commented Feb 20, 2025

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EyalPazz Mar 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EyalPazz commented Feb 24, 2025

robscott commented Feb 28, 2025

EyalPazz commented Mar 2, 2025

robscott left a comment

Choose a reason for hiding this comment

robscott commented Mar 5, 2025

EyalPazz commented Mar 5, 2025

EyalPazz commented Mar 6, 2025

robscott commented Mar 7, 2025

k8s-ci-robot commented Mar 7, 2025

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

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

EyalPazz Mar 2, 2025 •

edited

Loading