Add securityGroups field to IngressClassParams #3277

johngmyers · 2023-07-09T18:13:25Z

Issue

Competes with #2919.

Description

Adds a securityGroups field to IngressClassParams, to allow overriding the alb.ingress.kubernetes.io/security-groups and alb.ingress.kubernetes.io/manage-backend-security-group-rules annotations for all Ingresses in an IngressClass.

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 🌟

k8s-ci-robot · 2023-07-09T18:13:30Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: johngmyers
Once this PR has been reviewed and has the lgtm label, please assign m00nf1sh for approval. For more information see the Kubernetes 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:

OWNERS

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

johngmyers · 2023-07-09T18:19:03Z

I'm not entirely sure of the naming of the managedInbound and managedBackend fields. Suggestions?

codecov-commenter · 2023-07-09T18:23:59Z

Codecov Report

Patch coverage: 52.68% and project coverage change: +0.80% 🎉

Comparison is base (940efc7) 54.70% compared to head (6311bf9) 55.50%.
Report is 47 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3277      +/-   ##
==========================================
+ Coverage   54.70%   55.50%   +0.80%     
==========================================
  Files         148      149       +1     
  Lines        8590     8952     +362     
==========================================
+ Hits         4699     4969     +270     
- Misses       3559     3644      +85     
- Partials      332      339       +7

Files Changed	Coverage Δ
pkg/networking/security_group_resolver.go	`39.81% <0.00%> (-53.67%)`	⬇️
pkg/networking/security_group_resolver_mocks.go	`0.00% <0.00%> (ø)`
pkg/ingress/model_build_load_balancer.go	`75.00% <76.05%> (+2.75%)`	⬆️
pkg/networking/subnet_resolver.go	`88.37% <100.00%> (+0.62%)`	⬆️
webhooks/elbv2/ingressclassparams_validator.go	`95.83% <100.00%> (+2.61%)`	⬆️

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

johngmyers · 2023-07-21T15:32:05Z

/retest

johngmyers · 2023-07-21T19:03:57Z

/retest

johngmyers · 2023-07-21T19:27:17Z

/retest

johngmyers · 2023-07-25T23:00:41Z

/retest

johngmyers · 2023-07-28T21:42:29Z

/retest

oliviassss · 2023-08-22T18:00:26Z

config/crd/bases/elbv2.k8s.aws_ingressclassparams.yaml

+                      Exactly one of this, `managedInbound`, or `tags` must be specified.
+                    items:
+                      description: SecurityGroupID specifies a security group ID.
+                      pattern: sg-[0-9a-f]+


Can it also accept SG names just to keep consistent with the alb.ingress.kubernetes.io/security-groups annotation

No, the field should be strongly typed. If you want to search by the Name tag, use the tags field using a tag key of Name.

Using the spec field of a resource, one can specify precise semantics using structured data. Resources should not use the loosey-goosey semantics that the lack of structure in annotations tend to lead to.

pkg/ingress/model_build_load_balancer.go

pkg/ingress/model_build_load_balancer_test.go

pkg/networking/security_group_resolver.go

oliviassss · 2023-08-22T18:07:58Z

webhooks/elbv2/ingressclassparams_validator.go

+				return allErrs
+			}
+			if securityGroups.Tags != nil {
+				allErrs = append(allErrs, field.Forbidden(fieldPath.Child("tags"), "may not have both `ids` and `tags` set"))


I'm just wondering why would the ids and tags be exclusive? Let's say if a user specifies both (for fine-grained filtering purpose, or just by accident), can we just filter the SG by both field instead of error out?

What is the use case for specifying both ids and tags? An admin would typically have one way of communicating security groups from the IaC provisioning system to the deploy-to-Kubernetes system. Either they'd propagate a list of IDs or they'd provision the groups with tags according to some convention. Why would someone use one mechanism for some groups and another mechanism for others?

I don't see much point in having both. If you have id, there isn't much value in having the name (e.g. for the sake of documentation one can always put this information in comments or in some non-actionable annotation). Allowing both means you either have to make one take precedence when these don't agree; or still fail at runtime - but this is way more complicated to explain what's wrong (e.g. the message would have to read something like "sg-123 has name 'name-1', but the annotation specified 'name-2' - you have to either only use name/id; or if both are used - they should point to the same entity" - but having such a reasonable message is somewhat a security concern because you can now map security-group-id to a name which might have sensitive info)

At the same time clearly having a behavior to fail when both are specified allows using things like admission controllers to explicitly fail when resource are provisioned, rather than at runtime, so you can't even deploy incorrectly configured ingress.

johngmyers · 2023-08-23T02:53:32Z

/retest

apis/elbv2/v1beta1/ingressclassparams_types.go

k8s-triage-robot · 2024-01-21T10:19:55Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-ci-robot · 2024-02-07T01:01:39Z

PR needs rebase.

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/test-infra repository.

k8s-triage-robot · 2024-03-08T01:28:22Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2024-04-07T01:45:00Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2024-04-07T01:45:04Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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/test-infra repository.

johngmyers added 3 commits July 9, 2023 10:50

Add securityGroups field to IngressClassParams

c472a5f

Validate securityGroups field of IngressClassParams

2ba762d

Implement securityGroups field of IngressClassParams

1fc9fc8

k8s-ci-robot requested review from kishorj and M00nF1sh July 9, 2023 18:13

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 9, 2023

johngmyers mentioned this pull request Jul 9, 2023

Controller cfg for backend sg rule management #3266

Closed

4 tasks

oliviassss reviewed Aug 22, 2023

View reviewed changes

pkg/ingress/model_build_load_balancer.go Show resolved Hide resolved

oliviassss reviewed Aug 22, 2023

View reviewed changes

pkg/ingress/model_build_load_balancer_test.go Show resolved Hide resolved

oliviassss reviewed Aug 22, 2023

View reviewed changes

pkg/networking/security_group_resolver.go Show resolved Hide resolved

oliviassss reviewed Aug 22, 2023

View reviewed changes

johngmyers added 2 commits August 22, 2023 18:01

Extract some code to private receivers

28b7b8f

Provide better explanation of failure to find a security group

a806430

johngmyers force-pushed the security-groups branch from bbd806c to a806430 Compare August 23, 2023 02:46

oliviassss mentioned this pull request Aug 24, 2023

Ability to use Security Group Name and/or tags in alb.ingress.kubernetes.io/security-groups annotation #3359

Closed

oliviassss reviewed Aug 29, 2023

View reviewed changes

apis/elbv2/v1beta1/ingressclassparams_types.go Outdated Show resolved Hide resolved

Address review comments

6311bf9

johngmyers mentioned this pull request Oct 1, 2023

feat: add support for global accelerator endpoint groups #3405

Open

5 tasks

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 21, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 8, 2024

k8s-ci-robot closed this Apr 7, 2024

oliviassss mentioned this pull request Aug 8, 2024

Add securityGroups field to IngressClassParam #3765

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add securityGroups field to IngressClassParams #3277

Add securityGroups field to IngressClassParams #3277

johngmyers commented Jul 9, 2023

k8s-ci-robot commented Jul 9, 2023

johngmyers commented Jul 9, 2023

codecov-commenter commented Jul 9, 2023 •

edited

Loading

johngmyers commented Jul 21, 2023

johngmyers commented Jul 21, 2023

johngmyers commented Jul 21, 2023

johngmyers commented Jul 25, 2023

johngmyers commented Jul 28, 2023

oliviassss Aug 22, 2023

johngmyers Aug 23, 2023 •

edited

Loading

oliviassss Aug 22, 2023

johngmyers Aug 23, 2023

Aleksei-Poliakov Sep 5, 2023

johngmyers commented Aug 23, 2023

k8s-triage-robot commented Jan 21, 2024

k8s-ci-robot commented Feb 7, 2024

k8s-triage-robot commented Mar 8, 2024

k8s-triage-robot commented Apr 7, 2024

k8s-ci-robot commented Apr 7, 2024

Add securityGroups field to IngressClassParams #3277

Add securityGroups field to IngressClassParams #3277

Conversation

johngmyers commented Jul 9, 2023

Issue

Description

Checklist

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

k8s-ci-robot commented Jul 9, 2023

johngmyers commented Jul 9, 2023

codecov-commenter commented Jul 9, 2023 • edited Loading

Codecov Report

johngmyers commented Jul 21, 2023

johngmyers commented Jul 21, 2023

johngmyers commented Jul 21, 2023

johngmyers commented Jul 25, 2023

johngmyers commented Jul 28, 2023

oliviassss Aug 22, 2023

Choose a reason for hiding this comment

johngmyers Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

oliviassss Aug 22, 2023

Choose a reason for hiding this comment

johngmyers Aug 23, 2023

Choose a reason for hiding this comment

Aleksei-Poliakov Sep 5, 2023

Choose a reason for hiding this comment

johngmyers commented Aug 23, 2023

k8s-triage-robot commented Jan 21, 2024

k8s-ci-robot commented Feb 7, 2024

k8s-triage-robot commented Mar 8, 2024

k8s-triage-robot commented Apr 7, 2024

k8s-ci-robot commented Apr 7, 2024

codecov-commenter commented Jul 9, 2023 •

edited

Loading

johngmyers Aug 23, 2023 •

edited

Loading