Add subnets field to IngressClassParams

[johngmyers]

Issue

#2920

Description

Adds a subnets field to IngressClassParams, to allow overriding the alb.ingress.kubernetes.io/subnets annotation for all Ingresses in an IngressClass.

At this point I'm proposing the API, to get feedback before implementing. I'm trying to take advantage of the ability to use the structure that exists in YAML but not annotation values.

Checklist

• Added tests that cover your change (if possible)
• Added/modified documentation as required (such as the README.md, or the docs directory)
• Manually tested
• Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

• Backfilled missing tests for code in same general area 🎉
• Refactored something and made the world a better place 🌟

[johngmyers]

I'm a bit stuck, having trouble compiling controller-gen v0.5.0, as specified in Makefile. Go 1.17 and above won't compile it. I tried using Go 1.13.15, but it wouldn't download:

can't load package: package sigs.k8s.io/controller-tools/cmd/controller-gen@v0.5.0: can only use path@version syntax with 'go get'

Which version of Go do I need to compile controller-gen per the Makefile?

[codecov-commenter]

Codecov Report

Base: 54.09% // Head: 53.97% // Decreases project coverage by -0.11% ⚠️

Coverage data is based on head (b3c3292) compared to base (c37ce96).
Patch coverage: 32.60% of modified lines in pull request are covered.

❗ Current head b3c3292 differs from pull request most recent head 2c7507f. Consider uploading reports for the commit 2c7507f to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2945      +/-   ##
==========================================
- Coverage   54.09%   53.97%   -0.12%     
==========================================
  Files         144      145       +1     
  Lines        8291     8383      +92     
==========================================
+ Hits         4485     4525      +40     
- Misses       3479     3531      +52     
  Partials      327      327              

Impacted Files	Coverage Δ
pkg/networking/subnet_resolver.go	74.55% <0.00%> (-18.23%)	⬇️
pkg/networking/subnet_resolver_mocks.go	0.00% <0.00%> (ø)
webhooks/elbv2/targetgroupbinding_validator.go	75.00% <ø> (ø)
pkg/ingress/model_build_load_balancer.go	68.47% <77.77%> (+4.52%)	⬆️
webhooks/elbv2/ingressclassparams_validator.go	80.00% <80.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[kishorj]

I'm a bit stuck, having trouble compiling controller-gen v0.5.0, as specified in Makefile. Go 1.17 and above won't compile it. I tried using Go 1.13.15, but it wouldn't download:

can't load package: package sigs.k8s.io/controller-tools/cmd/controller-gen@v0.5.0: can only use path@version syntax with 'go get'

Which version of Go do I need to compile controller-gen per the Makefile?

I was unable to install v0.5.0 on my dev setup as well, since I had the old version in my path, it didn't complain. PR #2953 updates the controller gen to latest.

[kishorj]

There is a possibility of multiple matches here and we need a strategy to break ties in case of multiple subnets per AZ. In case of subnet name/id in the annotation, controller currently expects all of the entries to be resolved to valid subnets, and no multiples in the same AZ.

[johngmyers]

I was thinking we'd have the same expectation; that the union of all SubnetSelectors contains at most one subnet per AZ. It is true that the situation could change between the time the IngressClassParams is last modified and the LB is created or modified.

[johngmyers]

Apparently the annotation expects exactly one match. I was expecting this to handle multiple matches, but still at most one subnet per AZ.

[johngmyers]

I think devising rules for multiple subnets in an AZ would be for a separate PR. This PR maintains the restriction of at most one subnet per AZ.

[kishorj]

Is this the same VPC as the ALB?

[johngmyers]

Good question! A better approach would probably be to add a vpc field to IngressClassParamsSpec instead of hanging it off of subnets.

[johngmyers]

Ah, DescribeSubnets doesn't allow for filtering a substring. I think it makes more sense to allow filtering on arbitrary tags:

type SubnetSelector struct {
	// ID specifies the resource ID of a subnet. Exactly one of this or `nameContains` must be specified.
	ID string `json:"id,omitempty"`

	TagKey string `json:"tagKey,omitempty"`
        TagValue string `json:"tagValue,omitempty"`
}

or somesuch.

[kishorj]

2 and 3 should have the same behavior - subnets annotation if defined, or else subnet discovery

[johngmyers]

I disagree. The admin should have a way of forcing use of subnet discovery, disallowing a single Ingress from changing the subnets for the entire IngressClass through an annotation.

[kishorj]

Could you provide some samples to differentiate empty list vs un-specified subnets with the current changes?

Forcing subnet discovery is a great idea. For clarity, we either need to provide explicit configuration or discover based on the admin specified labels.

[johngmyers]

One would force subnet discovery with:

spec:
    subnets: []

To get the default behavior of Ingress annotations falling back to subnet discovery, one would leave the subnets field unspecified.

In my use case, a soft-multitenant cluster, I will want to disable all ingress annotations with exclusive mergebehavior. My current plan is to specify a value for each of them in the IngressClassParams of each IngressClass.

[kishorj]

Since the describe is on a single tag key (on the given vpc), if there are multiple matching subnets per AZ, we make the tag feature unusable. It is more usable if we support multiple tag keys

[johngmyers]

The admin presumably has control over subnet tagging, so they can avoid putting the tag value on more than one subnet in an AZ.

We could make this:

type TagSelector struct {
    Key string
    Value string
}

type SubnetSelector struct {
    ID string
   Tags []TagSelector

to be fully generalized, but that seems overkill.

[kishorj]

We need to account for multiple matches per AZ, otherwise we make the tag selector feature unusable in those situations.

[johngmyers]

How is this different from the existing annotation's tag searching semantics? The admin presumably has control over both the assignment of tags to subnets and use of tags in subnet selection; they can assign and select appropriately.

If we do anything other than error out, we risk painting ourselves into a corner should ALBs start supporting multiple subnets per AZ. We could perhaps add a check for this condition in the validating admission webhook so that it fails earlier.

[kishorj]

With annotations, the user can rectify the situation by specifying appropriate subnet ids in the k8s resource. With tag labels, the user might have to modify the AWS resources.

As for possibility of ALBs supporting multiple subnets per AZ in future, I'm assuming it would still be backwards compatible and we have the opportunity to relax the restrictions in subsequent releases.

[johngmyers]

With IngressClassParams, one can also specify appropriate subnet IDs:

spec:
    subnets:
    - id: subnet-xxxx
    - tagKey: Name
      tagValue: mySubnet

to reproduce the example in the annotations documentation. (I'm thinking a followup PR could replace SubnetsResolver.ResolveViaNameOrIDSlice() with a call to ResolveSubnetSpec().)

[johngmyers]

If we provide semantics for multiple subnets in an AZ that are other than erroring out, then the semantics that ALB later gives multiple subnets in an AZ might turn out to not be backwards compatible with the semantics we provided.

The best LBC can do by itself is deterministically choose one of the subnets to use. ALB would be likely to allow the LB to grab IPs from either or both subnets. Replacing the former with the latter would cause LB IPs to be taken from subnets where they never were before.

[kishorj]

Why not a list of IDs or a list of tag key/values? We can limit one set of subnet selectors for the ingress group.

[johngmyers]

Is there a need to prohibit specifying both IDs and tag/values? I'd like the interface to be able to specify anything that the existing annotation can.

[kishorj]

For simplicity.
I'm thinking about either of the following:

• A list of subnet ids, this is similar to the annotation, user is still on the hook to meet the ALB requirements.
• List of tags and values. This is similar to subnet auto-discovery where we let the users override the tags/values used for subnet discovery

[johngmyers]

The current proposal cannot represent the rules for subnet discovery: It cannot specify that two tags must match. If we go with the TagSelector approach I mentioned above, then one could do:

spec:
    subnets:
    - tags:
      - key: kubernetes.io/role/elb
        value: "1"
      - key: kubernetes.io/cluster/the-cluster-name
        value: owned
    - tags:
      - key: kubernetes.io/role/elb
        value: "1"
      - key: kubernetes.io/cluster/the-cluster-name
        value: shared
    - tags:
      - key: kubernetes.io/role/elb
        value: ""
      - key: kubernetes.io/cluster/the-cluster-name
        value: owned
    - tags:
      - key: kubernetes.io/role/elb
        value: ""
      - key: kubernetes.io/cluster/the-cluster-name
        value: shared

assuming we allow a TagSelector to match zero subnets.

That seems overly complicated to me. It's much simpler to rely on the tags being provisioned appropriately:

spec:
    subnets:
    - tagKey: ingressclass/the-cluster-name
      tagValue: the-ingress-class

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers, kishorj

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 [kishorj]

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

[kishorj]

/lgtm