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

GEP-1867: Per-Gateway Infrastructure #1868

Merged
merged 4 commits into from
May 30, 2023

Conversation

howardjohn
Copy link
Contributor

Reviewer note: this was split from #1757

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR introduces a new GEP on Per-Gateway Infrastructure. Read the GEP for details :-)

Which issue(s) this PR fixes:

Fixes #1867

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 22, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 22, 2023
@robscott
Copy link
Member

/retest

This doesn't scale beyond simple cases, however; even the example above requires 4 classes and exponentially grows as more use cases are needed.

Case 2 works fine, but greatly limits use cases, such as [Support namespace scoped implementations](https://github.com/kubernetes-sigs/gateway-api/issues/567).
Rather than having 1 big ingress for the entire cluster, it is common for clusters to have multiple smaller gateways ([example](https://istio.io/latest/docs/setup/additional-setup/gateway/#gateway-deployment-topologies).
Copy link
Contributor

Choose a reason for hiding this comment

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

why not create multiple GatewayClass for this use case ?

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 entire section is describing why not to do that. tl;dr - you need 1 GWC per usage anyways, RBAC permissions separation is broken, the separation is not useful, and no dynamic changes

Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Several comments which I would like to see resolved before we merge please, but otherwise as a Provisional first step I think this is an important issue you're helping to identify and fix and a good place to start.

I feel pretty strongly that we should drop the implementation details from this PR as commented below, but if you prefer not to do that then I just expect we'll need to invest more time here.

Hold for another maintainer review.

/hold

site-src/geps/gep-1867.md Outdated Show resolved Hide resolved
site-src/geps/gep-1867.md Outdated Show resolved Hide resolved
site-src/geps/gep-1867.md Outdated Show resolved Hide resolved
site-src/geps/gep-1867.md Outdated Show resolved Hide resolved
site-src/geps/gep-1867.md Outdated Show resolved Hide resolved
site-src/geps/gep-1867.md Outdated Show resolved Hide resolved
site-src/geps/gep-1867.md Outdated Show resolved Hide resolved
site-src/geps/gep-1867.md Outdated Show resolved Hide resolved
site-src/geps/gep-1867.md Outdated Show resolved Hide resolved
Comment on lines 81 to 128
## API

In order to address the concerns above, I propose a standard `infrastructure` API is added to `Gateway` and `GatewayClass`.
Note the important part of this is the `Gateway` change; the `GatewayClass` aspect is mostly for consistency.

Note that many of the fields under `GatewayInfrastructure` refer to WIP GEPs.
These fields are not tied to this GEP, and will only be included if the respective GEPs are approved.
However, they are shown below to give a sense of the use cases the field can support.

```go
type GatewaySpec struct {
// Infrastructure defines infrastructure level attributes about this Gateway instance.
Infrastructure GatewayInfrastructure `json:"infrastructure"`
// ...
}


type GatewayClassSpec struct {
// Infrastructure defines infrastructure level attributes for all Gateways in this class.
// A Gateway may provide configuration for the same values; as all fields in GatewayInfrastructure are implementation specific,
// the merging logic between these is as well.
Infrastructure GatewayInfrastructure `json:"infrastructure"`
// ...
}


type GatewayInfrastructure struct {
// AttachTo marks this Gateway as a child to the referenced parent Gateway.
// See GEP-1713 for details, which this depends on.
AttachTo LocalObjectReference

// Scope defines the "scope" of the Gateway's addresses.
// Valid options are ClusterLocal or Public
// See GEP-1651 for details, which this depends on.
// NOTE: that GEP is likely to be implemented in a different way, at which point this field can be removed from this struct.
Scope AddressScope

// Version defines the version of the Gateway infrastructure to use.
// This is an opaque string that is implementation specific.
// Some possible valid examples include "canary", "v1.2.3", "docker.io/example:latest".
Version string

// Size defines the size of the Gateway infrastructure.
// The meaning of this is implementation specific.
// GEP-1762 will recommend this to mean "Pod Replicas" for in-cluster deployments.
Size int

// ParametersRef provides a arbitrary implementation-specific configuration for
// fields not expressed directly in this struct.
// This follows the same semantics as GatewayClass's ParametersRef, but lives on the Gateway.
ParametersRef ParametersReference
}
```

The exact fields contained in `GatewayInfrastructure` are subject to change, and additional common fields can be added as use cases arise.
However, this will suffer from the common problem of a tension between providing common fields against offering extension points, so we should ensure that we are cautious to prevent excessive bloating of the field.
Fields in `GatewayInfrastructure` should be attributes of infrastructure that are relatively common across implementations.
Copy link
Member

@shaneutt shaneutt Mar 23, 2023

Choose a reason for hiding this comment

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

I would ask that we drop the implementation details from the first PR, as per the agreeing on the goals part of the GEP documentation:

Suggested change
## API
In order to address the concerns above, I propose a standard `infrastructure` API is added to `Gateway` and `GatewayClass`.
Note the important part of this is the `Gateway` change; the `GatewayClass` aspect is mostly for consistency.
Note that many of the fields under `GatewayInfrastructure` refer to WIP GEPs.
These fields are not tied to this GEP, and will only be included if the respective GEPs are approved.
However, they are shown below to give a sense of the use cases the field can support.
```go
type GatewaySpec struct {
// Infrastructure defines infrastructure level attributes about this Gateway instance.
Infrastructure GatewayInfrastructure `json:"infrastructure"`
// ...
}
type GatewayClassSpec struct {
// Infrastructure defines infrastructure level attributes for all Gateways in this class.
// A Gateway may provide configuration for the same values; as all fields in GatewayInfrastructure are implementation specific,
// the merging logic between these is as well.
Infrastructure GatewayInfrastructure `json:"infrastructure"`
// ...
}
type GatewayInfrastructure struct {
// AttachTo marks this Gateway as a child to the referenced parent Gateway.
// See GEP-1713 for details, which this depends on.
AttachTo LocalObjectReference
// Scope defines the "scope" of the Gateway's addresses.
// Valid options are ClusterLocal or Public
// See GEP-1651 for details, which this depends on.
// NOTE: that GEP is likely to be implemented in a different way, at which point this field can be removed from this struct.
Scope AddressScope
// Version defines the version of the Gateway infrastructure to use.
// This is an opaque string that is implementation specific.
// Some possible valid examples include "canary", "v1.2.3", "docker.io/example:latest".
Version string
// Size defines the size of the Gateway infrastructure.
// The meaning of this is implementation specific.
// GEP-1762 will recommend this to mean "Pod Replicas" for in-cluster deployments.
Size int
// ParametersRef provides a arbitrary implementation-specific configuration for
// fields not expressed directly in this struct.
// This follows the same semantics as GatewayClass's ParametersRef, but lives on the Gateway.
ParametersRef ParametersReference
}
```
The exact fields contained in `GatewayInfrastructure` are subject to change, and additional common fields can be added as use cases arise.
However, this will suffer from the common problem of a tension between providing common fields against offering extension points, so we should ensure that we are cautious to prevent excessive bloating of the field.
Fields in `GatewayInfrastructure` should be attributes of infrastructure that are relatively common across implementations.

If you don't want to that's OK it will just likely require more time for this PR than it may have for separate ones.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2023
@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 23, 2023
Comment on lines 114 to 107
// See GEP-1651 for details, which this depends on.
// NOTE: that GEP is likely to be implemented in a different way, at which point this field can be removed from this struct.
Scope AddressScope
Copy link
Member

Choose a reason for hiding this comment

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

Why would it be possible to request address scope both in spec.addresses and spec.infra? I can get the rationale for both independently, but think it would be confusing to allow this to be specified in different places within the same resource.

cc @dprotaso

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we do it in spec.addresses we should not do it in spec.infra. I meant to indicate that in my comment that we may remove this depending on how 1651 goes

Copy link
Member

Choose a reason for hiding this comment

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

I actually think I like it in infra since in many/most cases only a single address scope is going to be supported by a Gateway. Are there important use cases I'm missing where we'd want to be able to request multiple address scopes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Statically assign a LoadBalancer IP and a ClusterIP, I guess? not sure why you would actually do that, though

Probably a discussion for 1651

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think this raises the point that #1651 implies that it will be useful to have one Gateway with multiple address scopes.

Personally, I think it may be better to encourage users to have different Gateways with different scopes, to make decisions about where to expose things more explicit (you'll need to point your Routes at both Gateways if you want your route available in both scopes).

site-src/geps/gep-1867.md Outdated Show resolved Hide resolved
@arkodg
Copy link
Contributor

arkodg commented Mar 23, 2023

  • the Gateway API docs mentions that the GatewayClass resource should be used to provision infrastructure so imo it is a natural place to define intent saying - "dont provision infra but link to it using this config"
  • if there is a need to link infrastructure at the Gateway level, imo there should be some intent defined at the GatewayClass level saying - "dont provision infra but allow the Gateways to link to infrastructure using config defined at the Gateway level"

@howardjohn
Copy link
Contributor Author

  • the Gateway API docs mentions that the GatewayClass resource should be used to provision infrastructure so imo it is a natural place to define intent saying - "dont provision infra but link to it using this config"
  • if there is a need to link infrastructure at the Gateway level, imo there should be some intent defined at the GatewayClass level saying - "dont provision infra but allow the Gateways to link to infrastructure using config defined at the Gateway level"

Where is that mentioned? All of my understanding is that Gatewayclass is a template for infra, and Gateway is the infra.

https://gateway-api.sigs.k8s.io/api-types/gatewayclass/?h=gateway

This resource represents a class of Gateways that can be instantiated.

@arkodg
Copy link
Contributor

arkodg commented Mar 23, 2023

  • the Gateway API docs mentions that the GatewayClass resource should be used to provision infrastructure so imo it is a natural place to define intent saying - "dont provision infra but link to it using this config"
  • if there is a need to link infrastructure at the Gateway level, imo there should be some intent defined at the GatewayClass level saying - "dont provision infra but allow the Gateways to link to infrastructure using config defined at the Gateway level"

Where is that mentioned? All of my understanding is that Gatewayclass is a template for infra, and Gateway is the infra.

https://gateway-api.sigs.k8s.io/api-types/gatewayclass/?h=gateway

This resource represents a class of Gateways that can be instantiated.

based on https://gateway-api.sigs.k8s.io/api-types/gateway/#deployment-models, creating a Gateway could either create new infra or create new config for an existing infra, but it is tied to GatewayClass

Depending on the GatewayClass, the creation of a Gateway could do any of the following actions:

Use cloud APIs to create an LB instance.
Spawn a new instance of a software LB (in this or another cluster).
Add a configuration stanza to an already instantiated LB to handle the new routes.
Program the SDN to implement the configuration.
Something else we haven’t thought of yet...
The API does not specify which one of these actions will be taken.

@howardjohn
Copy link
Contributor Author

Depending on the GatewayClass, the creation of a Gateway could do any of the following actions:

None of those imply the GatewayClass actually provisions the infrastructure IMO, just that Gateways can be merged. if you want to pre-provision some infra when GatewayClass is created that is fine - it doesn't impact this PR IMO. The entire infrastructure API is implementation specific and can be rejected/ignored.

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.

I'm liking how this looks, but I think the interaction with #1651 is very interesting.

Changes to it are not expected to change deployed `Gateway`s.

This makes usage problematic in a declarative way.
For example, if I wanted to represent a `version` field and change that to trigger an upgrade, I would need to create an entirely new `Gateway`.
Copy link
Contributor

Choose a reason for hiding this comment

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

One of the things I like about this proposal is that this still allows people to create a second Gateway if the implementation can handle two running side-by-side, which will provide for smooth-and-slow transitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 fwiw Istio explicitly documents these two paths https://istio.io/latest/docs/setup/additional-setup/gateway/#upgrading-gateways (doc is using Istio API, though). Both are valid IMO.

Also it leaves room for a smarter controller to make a version change smooth-and-slow (flagger-style rollout, etc) as well

site-src/geps/gep-1867.md Outdated Show resolved Hide resolved
Comment on lines 114 to 107
// See GEP-1651 for details, which this depends on.
// NOTE: that GEP is likely to be implemented in a different way, at which point this field can be removed from this struct.
Scope AddressScope
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think this raises the point that #1651 implies that it will be useful to have one Gateway with multiple address scopes.

Personally, I think it may be better to encourage users to have different Gateways with different scopes, to make decisions about where to expose things more explicit (you'll need to point your Routes at both Gateways if you want your route available in both scopes).

Copy link
Contributor

@danehans danehans left a comment

Choose a reason for hiding this comment

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

In general, I'm in favor of exposing Gateway-specific configuration through gateway.spec. The semantics for doing so are still open for discussion. Primarily, I'm interested in better understanding how merging of GC/GW config parameters are handled.

site-src/geps/gep-1867.md Outdated Show resolved Hide resolved
site-src/geps/gep-1867.md Outdated Show resolved Hide resolved
site-src/geps/gep-1867.md Outdated Show resolved Hide resolved
site-src/geps/gep-1867.md Outdated Show resolved Hide resolved

### Separation of concerns

While there is value out of providing class-wide options as defaults, there is also value in providing these options on the object (Gateway) directly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since a Gateway must reference a GatewayClass, how would config merging be handled between the two resources. For example, I have a GatewayClass with parametersRef that specifies replicas: 2 and a Gateway infra with the same field set. Do Gateway infra parameters override GatewayClass params?

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 discussed in the API spec itself, see L92

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a very important question because this GEP introduces a lot of overlap between the two resources. @howardjohn do you have a link to the API spec you're referring to? The closest I could find was the GatewayClass spec, but it doesn't really seem to cover merging.

// ParametersRef is a reference to a resource that contains the configuration
// parameters corresponding to the GatewayClass. This is optional if the
// controller does not require any additional configuration.
//
// ParametersRef can reference a standard Kubernetes resource, i.e. ConfigMap,
// or an implementation-specific custom resource. The resource can be
// cluster-scoped or namespace-scoped.
//
// If the referent cannot be found, the GatewayClass's "InvalidParameters"
// status condition will be true.
//
// Support: Implementation-specific


## API

In order to address the concerns above, I propose a standard `infrastructure` API is added to `Gateway` and `GatewayClass`.
Copy link
Contributor

Choose a reason for hiding this comment

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

From a GC standpoint, I think it's difficult for users to differentiate parametersRef and infrastructure. IMO another field should not be introduced to expose config params for a GC. If GC parametersRef need to be updated dynamically, the spec already supports this (GC as a template is only a recommendation).

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 an established pattern in the API, IMO. To have core fields and extensions. For example, in HTTPRoute we have HeaderModifierFilter and ExtensionRefFilter.

Infrastructure provides core fields, and parametersRef provides an extension mechanism for vendor-specific fields

## API

In order to address the concerns above, I propose a standard `infrastructure` API is added to `Gateway` and `GatewayClass`.
Note the important part of this is the `Gateway` change; the `GatewayClass` aspect is mostly for consistency.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not have the new API align with the existing parametersRef instead of vice versa, i.e. parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am fine with any name that will get this approved :-)

site-src/geps/gep-1867.md Outdated Show resolved Hide resolved
site-src/geps/gep-1867.md Outdated Show resolved Hide resolved
@howardjohn
Copy link
Contributor Author

What is needed to push this forward?

@youngnick
Copy link
Contributor

This is ready to go for me at Provisional.

However, we should be able to move this to Implementable/Experimental pretty soon (probably after v0.7.0 final is cut). That will give the further GEPs that depend on this a place to put their config. I think the only things remaining for Implementable are:

  • making sure the new fields are optional and pointers
  • Making the defaulting behavior not implementation-specific (we should be moving any fields we put in to Extended as quickly as possible, I think)

This is already held, so I'll leave it held until @robscott has a review or unholds.

/lgtm

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 @howardjohn! Having trouble keeping track of all the related GEPs around this so let me know if any of these comments either don't make sense or have been addressed elsewhere.

geps/gep-1867.md Outdated Show resolved Hide resolved
geps/gep-1867.md Show resolved Hide resolved
geps/gep-1867.md Outdated Show resolved Hide resolved
geps/gep-1867.md Show resolved Hide resolved
geps/gep-1867.md Show resolved Hide resolved
geps/gep-1867.md Show resolved Hide resolved
Comment on lines +60 to +66
In core Kubernetes, Pods declare their requirements (for example, CPU requests) inline in the Pod resource; there is not a `ResourceClass` API that abstracts these further.
These higher level abstractions are handled by layered APIs (whether this is a CRD, an admission webhook, CI/CD tooling, etc).
This allows users the flexibility to easily configure things per-pod basis.
If the infrastructure admin wants to impose defaults or requirements on this flexibility, they are able to do so (in fact, `LimitRanger` provides a built in mechanism to do so).
Copy link
Member

@robscott robscott May 18, 2023

Choose a reason for hiding this comment

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

It would be helpful to provide some guidelines for when we'd want Gateway-level config to be able to override class-level config. I personally think there are many cases where admins would not want to allow config to be overridden. This feels especially problematic if we have something like parametersRef that could potentially allow all GatewayClass params to be overridden at the Gateway level, including something like implementation-specific firewall config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also responding to your other comment here

Why would we not want defaults and overrides for this at GatewayClass level? Are we sure that cluster admins are going to be fine with others creating custom variations of Gateways and overriding GatewayClass config?
Why would the merging logic be implementation specific?

I think there are a few options:

  1. default and overrides in Gateway, aligning with policy attachment

Makes paramRefs awkward, since it doesn't have these concepts

Gives GatewayClass editor ability to decide what Gateway admins can do. I don't think this is useful in most cases, since most users are not actually editing GC but are using defaults provided by implementations during install.

  1. Implementation specific merge

This aligns with paramRef, which already has implementation specific merging with Gateways (if they have per-gateway settings, of course -- its all impl specific so can be anything).

Gives the controller author ability to decide what gateway admins can do. I suspect this is often actually the person writing the GC, anyways, though, so this ends up being similar to (1) but simpler.

  1. No infrastructure field at GC

Not consistent with Gateway API. Users who want this at GC level would use a paramRef to something with the same fields, and then would have impl specific merging logic. So ends up the same as (2), roughly, but with an inconsistent UX I think.


So overall it seems like if we think GC owner wants overrides we should do (1), else we should do (2).

I don't really see why a GC would want to override TBH. For things like Version and Size, you probably don't want override -- you want a range. This is better addressed through other mechanisms like Gatekeeper (effectively what LimitRanger is in core). So I would lean towards (2).


I will note that the merging logic and the presence of the field are decoupled. Since the presence of the field is blocking 3 other GEPs, I can say the merging logic is WIP and a blocker for promotion beyond 'provisional' if it helps?

Copy link
Member

Choose a reason for hiding this comment

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

In general, implementation-specific has been reserved for concepts like RegEx that are broadly understood but have some nuanced differences. I'd hate to extend that to something important like hierarchical values that really could be solved by the API itself. This feels entirely different from policy in that we'll have a well-known list of config, and some of these values will be representable at both GW and GWC levels. I agree that for at least some, we'd want something like both a default and a range on GWC. So IMO there's a 4th option which is for the API to define merging semantics for every infra field that is present on both GW and GWC.

I think at a minimum we should aim to have some patterns or principles defined here that cover:

  1. When a field should be configurable on GW
  2. When a field should be configurable on GWC
  3. What kinds of interactions we want to support between GWC and GW infra fields

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a section on this

geps/gep-1867.md Show resolved Hide resolved
Comment on lines +24 to +25
* Provide the ability to configure arbitrary (implementation specific) attributes about a **specific Gateway**.
* Provide the ability to configure a standardized set of attributes about a **specific Gateway**.
Copy link
Member

Choose a reason for hiding this comment

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

These goals feel incomplete because you're also proposing standard infra-level config at the GatewayClass level but these only focus on individual Gateways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The implicit goal on the GEP is "Have consistency in the API". Providing infrastructure is purely to maintain consistency between the APIs; the entire goal of the GEP is to not use GatewayClass.

Copy link
Member

Choose a reason for hiding this comment

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

the entire goal of the GEP is to not use GatewayClass

This seems problematic. Not to use GatewayClass at all or just for this specific set of config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is to be able to configure infrastructure options on a per-gateway basis. Its not to stop users from using GC, but they can already do that today. They cannot do it on a per-GW basis today, so this GEP aims to enable this.

Copy link
Member

Choose a reason for hiding this comment

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

I do think a goal of this needs to be to standardize some infra level configuration at the GWC level as well. Although this can be configured via GWC params already, we don't have a standard way to represent shared concepts yet.

geps/gep-1867.md Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2023
@youngnick
Copy link
Contributor

Okay, I caught up on this one, and I think it's easier to just bring my comments back to the top because they're about a few of these threads. I've previously approved, but I think that @robscott has brought up some great points.

On reflection, what we're doing with this is graduating some classes of config from requiring a paramsRef to standard fields, and infrastructure is the place to put these fields. Because the things we're talking about are standard, we can also include an infrastructure stanza on Gateway as well as the one on GatewayClass.

So, I think that we should be clear that's what we're doing, and we need a few rules for how this whole thing will work.

I propose:

  • paramsRef always stays for implementation-specific support and early-mover experiments. infrastructure is the way in which we may graduate things from paramsRef to full fields.
  • GatewayClass infrastructure has defaults and overrides like a Policy Attachment, and any field we add in one, we add in both. Additionally, any field we add here, we add to Gateway infrastructure as well.
  • Gateway infrastructure may have additional fields that GatewayClass infrastructure doesn't, which are things that don't make sense to be defaulted or overridden (like attachTo from the merging GEP).
  • we include one field in this GEP: annotations, which will be the annotations to be used on any resources generated to fulfill the Gateway contract. This will be included in the GatewayClass infrastructure, in both defaults and overrides, as well as the Gateway infrastructure. We can then show how defaults and overrides work using this example. (For example, we'll need a Condition on Gateway to say if Gateway-level infrastructure has been overridden by GatewayClass-level infrastructure.)

This way, we can

  • create the infrastructure stanza with clarity about how it works
  • provide a path for paramsRef settings to graduate to full fields on GatewayClass
  • have a concrete illustration of how defaults and overrides can work in practice

geps/gep-1867.md Show resolved Hide resolved
geps/gep-1867.md Show resolved Hide resolved
@robscott
Copy link
Member

Thanks for all the updates @howardjohn! Mostly agree with @youngnick's comment, but have a couple slight disagreements:

GatewayClass infrastructure has defaults and overrides like a Policy Attachment, and any field we add in one, we add in both. Additionally, any field we add here, we add to Gateway infrastructure as well.

I think that the options we expose on GWC-infrastructure should be specific to each concept. For example, I think giving a min and max may be useful in some cases. As long as we're clear that that's a possibility for fields in the future, I don't think we need to solve it here.

we include one field in this GEP: annotations, which will be the annotations to be used on any resources generated to fulfill the Gateway contract. This will be included in the GatewayClass infrastructure, in both defaults and overrides, as well as the Gateway infrastructure. We can then show how defaults and overrides work using this example.

I think I actually like the most recent iteration of this GEP which only adds infrastructure and paramsRef fields and leaves the rest to future GEPs. I think we'd need at least one of those future GEPs to merge before we could mark this one as implementable though. I think even something as simple as annotations could open another can of worms that doesn't necessarily need to delay the simplified version of this GEP.

@youngnick
Copy link
Contributor

Ah, so the structure you're suggesting would be more like (using the annotations example even though I agree it's probably too hard for inclusion here, as well as some numerical thing):

  spec:
    infrastructure:
      annotations:
        defaults:
          <a map[string]string>
        overrides:
          <a map[string]string>
      numericalThing:
        max: 1000
        min: 1
        default: 30

Or something?

I can agree with that, although this PR might benefit from some arbitrary examples like the numericalThing one here to illustrate how this should work.

To be clear, I think I agree with leaving annotations out of this, it's probably more complicated than it seems, just like everythig else. Sigh.

@howardjohn
Copy link
Contributor Author

I updated this to include that we may want ranges or default+override, on a case by case basis

@robscott
Copy link
Member

Thanks @howardjohn! I'd still like to see a goal related to GatewayClass (https://github.com/kubernetes-sigs/gateway-api/pull/1868/files#r1204455976) but otherwise this LGTM.

/approve

@youngnick
Copy link
Contributor

This LGTM for provisional to me.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 29, 2023
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

I'm OK with moving forward with this as Provisional 👍

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: howardjohn, robscott, shaneutt

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:

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

@howardjohn
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 30, 2023
@howardjohn
Copy link
Contributor Author

/test pull-gateway-api-verify

@howardjohn
Copy link
Contributor Author

/test all

@mikemorris
Copy link
Contributor

Should this be promoted to Experimental (it appears that the Implementable GEP status is either gone or has nothing in it currently) now that GEP-1762: In Cluster Gateway Deployments using this field has merged as Experimental?

@robscott
Copy link
Member

robscott commented Jan 4, 2024

Good catch @mikemorris! It looks like this just needs a navigation update? https://gateway-api.sigs.k8s.io/geps/gep-1867/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GEP: Provide a standard way to configure per-Gateway Infrastructure
9 participants