Graduating Ingress to V1 - #88041

Can we inline this since we don't have a docstring associated with this?

This pattern actually came from what I was doing for EndpointSlice validation. I was initially using inlined vars here, but Tim recommended moving to this style in that API review. I've grown to appreciate key values like this being highly visible like this, but can switch back to inline if that's preferred.

we should be more specific concerning marking as default. We don't want to be mysterious here, people will have a hard time finding it.

I'm not sure what Bowei's means here but marking an IngressClass as default works only if most of the implementations do respect this. If that's not the case, then you end up in a situation where user expects one thing, based on the k8s core contract but the reality is different.

We should force all controllers to respect it. Maybe this goes into the KEP.

@bowei I have more specific documentation on the IngressClass resource - it was missing in one of the versions when you reviewed this. I was hoping to avoid duplicating that copy on both the Class field and the Class resource, but I can do that if you think it will help make this easier to understand.

@hbagdi This was a last minute addition to the KEP per Tim's suggestion. It mirrors the behavior of StorageClass. An admission controller watches Ingresses and sets a default value for Class if it's not already specified on the Ingress and there is an IngressClass resource that has been marked as default.

I'm not saying the addition doesn't make sense and the behavior is incorrect.

But, this:

Implementations of this API may choose to ignore Ingresses without a class specified.

This seems like a problematic may to me.
We should define the behavior of controllers around what to do when the class is not set at all. If one controller ignores them, and user installs another one, and it takes over those Ingress resources, then that leads to a bad user experience. The guidelines here should be more strict.

Yeah, I agree that it would be nice if we could be more specific here. Unfortunately we're limited to some extent by what already exists. Ingresses have already been valid without a class specified, and we can't make that use case invalid and still maintain backwards compatibility. At the same time, the behavior when this was unspecified was also not specified and therefore was inconsistent, so we're limited with what we can say here without requiring behavior to change dramatically here.

If there were a way to require class to be specified here, that sure would be ideal, but I just don't think we can unfortunately. I'm also open to ideas for how we could further specify how the lack of a class should be handled, I just haven't found anything that works well within the constraints we have (especially backwards compatibility).

However, this is a V1 spec for which (AFAIK) every ingress controller will need code changes to support (even if it is just adding informers for the new types). This seems like the best opportunity to make another incompatible change :)

James, the problem is that Ingress has been v1beta1 for so long that is essentially a GA API, and beta is only in the name. People rely on it like a v1 GA API and breaking the behavior will not go well for most users of the community.

Rob, you have a good point.
I get that we can't enforce things in the API but we can have stricter guidelines.

A solution I can think of is to replace may with should and then detail on the behavior we want implementors to follow.

👍 I like that idea, thanks! I've replaced the may with should and I think that at least gets us closer to the goal here.

tests can become fragile if you test for the error message

That's definitely true, in this case I thought it was worthwhile though. This is an admission controller, and it's important not just that it prevents invalid configuration, but that it includes a clear explanation for why the configuration is invalid. In this case, this is testing an error message that is specific to this admission controller, something that would only change if the admission controller itself changed.

typo: assigned to this

What should be the behavior when two Ingress classes have this behavior?
As simple as two controllers fight over it sounds good enough.

NVM. Found that we do have validation around this.

Yeah this is all just running through an admission controller that mirrors what was already done for StorageClass. If there are more than 1 IngressClasses marked as default in a cluster, we actually block creation of Ingresses until that has been resolved. I think a link will break when I push again, but this is all in plugin/pkg/admission/ingressclass.

I'm not sure what Bowei's means here but marking an IngressClass as default works only if most of the implementations do respect this. If that's not the case, then you end up in a situation where user expects one thing, based on the k8s core contract but the reality is different.

We should force all controllers to respect it. Maybe this goes into the KEP.

Question for my learning: Why do we add the new fields to older resources here?
Is it for some round-tripping behavior?

We do this in networking.v1beta1 as well. 🤔

Yep, it's a strange requirement to enable round-tripping as I understand it. Definitely makes this PR pretty repetitive.

Thank you!
Just to make sure, if one goes through the upgrade, and the old and new resource both are available, then magically your old resource (extensions.Ingress) have the new fields as well, correct?

I'm not saying this is a problem but just want to understand this internal detail.

other places we have names of referenced objects, we call it <kind>Name, e.g.:

• pod.spec.runtimeClassName
• pod.spec.priorityClassName
• pod.spec.serviceAccountName
• pvc.spec.storageClassName

Is there a reason not to call this IngressClassName? That would also avoid potential pitfalls of generated clients in various languages choking on a field name that is a common keyword.

Do we also expect ingress controllers to just watch for Ingress objects that match their class name? If so, we should add fieldSelector support for this (so a controller could list/watch ingresses?fieldSelector=spec.ingressClassName=foo, for example).

To do that:

1. Make methods in the ingress strategy to:
   1. Build a map of selectable fields for an ingress object (example)
   2. Return that map along with labels for matching field/label selectors (example)
   3. Construct a match predicate from those label/field sets (example)
2. Assign the match predicate function as the ingress storage PredicateFunc (example)
3. Call AddFieldLabelConversionFunc for each external version (example), mapping the fieldSelector field names to the internal field names. In this case, the external and shared field names would be identical: metadata.name, metadata.namespace, and spec.ingressClassName (or whatever we end up naming the field)

When that's done, the following should work:

kubectl get ingresses.v1beta1.extensions --field-selector=metadata.name=$myname
kubectl get ingresses.v1beta1.extensions --field-selector=metadata.namespace=$mynamespace
kubectl get ingresses.v1beta1.extensions --field-selector=spec.ingressClassName=$myclassname

kubectl get ingresses.v1beta1.networking.k8s.io --field-selector=metadata.name=$myname
kubectl get ingresses.v1beta1.networking.k8s.io --field-selector=metadata.namespace=$mynamespace
kubectl get ingresses.v1beta1.networking.k8s.io --field-selector=spec.ingressClassName=$myclassname

kubectl get ingresses.v1.networking.k8s.io --field-selector=metadata.name=$myname
kubectl get ingresses.v1.networking.k8s.io --field-selector=metadata.namespace=$mynamespace
kubectl get ingresses.v1.networking.k8s.io --field-selector=spec.ingressClassName=$myclassname

Agree that ingressClassName works better here, will work on changing that.

The relationship between class name and controllers is not as straightforward as it might seem. The class name here refers to the name of the IngressClass resource, which then specifies the controller name. The idea was to allow for a single controller to implement multiple classes of ingress, such as external and internal load balancers. So in this case, we'd likely want IngressClass to be selectable in this same way. At that point, controllers would watch IngressClass resources with their controller name, then modify their watch on Ingress resources to include any with those class names. Does that sound reasonable?

Spec without status is interesting... no other *Class types I see have a spec/status split. Is there a particular future use case you have in mind here?

We originally proposed a status as part of this resource but pulled it for now with the idea that it could be added in the future. We want this resource to mirror the GatewayClass resource as closely as possible, and the status there is still a work in progress. Ideally once that stabilized, we'd bring the same status back into IngressClass. The two major things we want to enable a controller to indicate are:

1. That it is implementing a given class
2. That parameters associated with the given class are valid/invalid

That may evolve as the discussion around GatewayClass evolves, but we wanted to leave room for status here as well.

Is this the default we want for v1 or do we want to require pathType to be explicit when creating via v1 (no default, make required in validation)?

Although I'd initially been working towards making pathType a required field with no default, we ended up deciding against that to avoid making such a significant change as part of a v1 release. This would just make it more work for end users to move to the v1 API - this field would need to be set multiple times (1 per path) for each Ingress.

I was also hesitant to drag out the Ingress v1 graduation process as the work on Service APIs was gaining momentum. As I understand it, requiring the pathType field would require another beta release in 1.18 before we could reach v1 in 1.19. I think there's relatively minimal value gained by that approach. For more context:

kubernetes/enhancements#1413 (comment)

This would just make it more work for end users to move to the v1 API - this field would need to be set multiple times (1 per path) for each Ingress.

Propagating an ambiguous meaning of path as a default to v1 doesn't seem great.

As I understand it, requiring the pathType field would require another beta release in 1.18 before we could reach v1 in 1.19.

If the v1beta1 versions default it, and v1 does not, then validation can require it without affecting v1beta1 API users. No additional release is required.

revert this to v1beta1 for 1.18

This can't change to v1 until v1.19 (open a tracking issue to switch it). Changing it in a single release means writes via a 1.18 server in a skewed multi-API-server cluster will make ingresses unreadable by a 1.17 server that doesn't understand v1. The etcd storage integration test catches this and should change back to expecting v1beta1 to be stored for 1.18.

drop printlns

If we get here this is an error we should log (we thought we recognized the resource, but the object type did not match).

That also means there are no integration or e2e tests verifying this works, since all actual invocations of this admission plugin would have returned here

is this intentionally only setting the class field for requests made to the v1 API? I expected it for requests to the v1beta1 APIs as well

admission deals with internal types, so this will be a networking.Ingress object

If an update of an ingress with a spec.class value drops the spec.class value in the new incoming object (which a client using an old version of client-go would do), should we preserve the existing value in PrepareForUpdate? alternately, should the ingressclass admission plugin also populate the default class on update for ingresses that don't have it or the class annotation set?

nit / query; does this mean:

?

nit:

SSL doesn't have SNI; TLS does.

nit:

nit:

nit:

nit: