Allow PrefixList for sshAccess and kubernetesApiAccess #13113

hierynomus · 2022-01-16T22:21:19Z

Signed-off-by: Jeroen van Erp jeroen@hierynomus.com

k8s-ci-robot · 2022-01-16T22:21:27Z

Hi @hierynomus. Thanks for your PR.

I'm waiting for a kubernetes 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/test-infra repository.

rifelpet · 2022-01-16T22:30:43Z

/ok-to-test

rifelpet · 2022-01-16T22:42:29Z

kops' cloudmock packages will need to be updated to recognize the new prefix list field. All of the changes needed should be in here

hierynomus · 2022-01-17T09:41:49Z

Thanks for the pointer @rifelpet. Awaiting new build results now ;)

hierynomus · 2022-01-17T10:14:18Z

pkg/apis/kops/validation/validation.go

@@ -85,17 +85,23 @@ func validateClusterSpec(spec *kops.ClusterSpec, c *kops.Cluster, fieldPath *fie

 	// SSHAccess
 	for i, cidr := range spec.SSHAccess {
-		allErrs = append(allErrs, validateCIDR(cidr, fieldPath.Child("sshAccess").Index(i))...)
+		if !strings.HasPrefix(cidr, "pl-") {


Should we (can we) add validation that the PrefixListID is actually valid by interrogating AWS, or don't we do that during validation?

Shouldn't this only be allowed if the cloudprovider is AWS?

hierynomus · 2022-01-17T10:19:17Z

@rifelpet Another question: Should we also add the PrefixList for the "ICMP for PMTU discovery" block? See:

kops/pkg/model/awsmodel/api_loadbalancer.go

Lines 379 to 381 in 7e64518

    
           // Allow ICMP traffic required for PMTU discovery 
        
           if utils.IsIPv6CIDR(cidr) { 
        
           	c.AddTask(&awstasks.SecurityGroupRule{

If so, what should be the FromPort and ToPort?

hierynomus · 2022-01-17T10:24:16Z

/retest

johngmyers

Perhaps modify one of the IPv6 e2e tests to use this?

johngmyers · 2022-01-19T07:08:51Z

pkg/apis/kops/validation/validation.go

@@ -85,17 +85,23 @@ func validateClusterSpec(spec *kops.ClusterSpec, c *kops.Cluster, fieldPath *fie

 	// SSHAccess
 	for i, cidr := range spec.SSHAccess {
-		allErrs = append(allErrs, validateCIDR(cidr, fieldPath.Child("sshAccess").Index(i))...)
+		if !strings.HasPrefix(cidr, "pl-") {


Shouldn't this only be allowed if the cloudprovider is AWS?

johngmyers · 2022-01-19T07:16:57Z

pkg/model/awsmodel/utils.go

+	return strings.HasPrefix(id, "pl-")
+}
+
+func LimitIngress(t *awstasks.SecurityGroupRule, cidr string) *awstasks.SecurityGroupRule {


Perhaps make this a receiver on awstasks.SecurityGroupRule? Perhaps call it SetPrefix()?

I've added that as SetCidrOrPrefix, as I thought that SetPrefix wasn't fully covering the functionality.

My thinking was that a CIDR specifies a prefix. A "pl-" string is a prefix list. Both CIDR and PrefixList specify the subset of the address space that the SGR applies to.

I suppose "Prefix" is incorrectly singular when applied to prefix lists. I'm trying to find a good term to express the concept of the subset of address space.

Yes agreed, basically either limit the ingress to certain address(blocks).

So that's why I initially chose LimitIngress

Except a security group rule can be for egress instead.

docs/cluster_spec.md

johngmyers · 2022-01-19T07:26:28Z

As to ICMP/ICMPv6, you would need to query whether the prefix list in question contains IPv4 or IPv6 addresses, then add the appropriate rule. Per AWS documentation, a prefix list may contain only one address family.

hierynomus · 2022-01-19T10:20:36Z

As to ICMP/ICMPv6, you would need to query whether the prefix list in question contains IPv4 or IPv6 addresses, then add the appropriate rule. Per AWS documentation, a prefix list may contain only one address family.

I've added this functionality, by passing the fi.Cloud.(awsup.AWSCloud) into the builder struct. Feel free to suggest a better way to obtain the AWS API.

I've kept the commit separate for now to ease the review process, I will squash it once you've OK'ed the implementation 😄

hierynomus · 2022-01-21T09:15:47Z

Any ideas already @johngmyers on the naming of the method? Furthermore besides that, is there anything else needed? I've updated the complexe2e test to include 2 prefix lists (1 ipv4, 1 ipv6), which other e2e test needs something extra?

johngmyers · 2022-01-21T16:31:28Z

Sorry, I haven't been able to get back to this due to the large review backlog.

hierynomus · 2022-01-31T14:17:23Z

Hi @johngmyers Did you have any chance to come back to this PR again?

hakman

Hi @hierynomus. I did a first pass of the PR.
There are mostly small changes from my point of view, but would like to not do cloud calls in model.

pkg/apis/kops/validation/validation.go

upup/pkg/fi/cloudup/awstasks/securitygrouprule.go

hakman · 2022-02-15T08:23:20Z

pkg/model/awsmodel/api_loadbalancer.go

+				icmpIpv6 := false
+				if strings.HasPrefix(cidr, "pl-") {
+					b, err := b.IsIPv6PrefixList(cidr)
+					if err != nil {
+						return err
+					}
+					icmpIpv6 = b
+				} else {
+					icmpIpv6 = utils.IsIPv6CIDR(cidr)
+				}


I don't think model should check the kind of prefix with AWS. I would rather skip the ICMP rules for prefixes, for now.

As a curiosity, any idea what happens if you set IPv4 PrefixList to ICMPv6 protocol or IPV6 PrefixList to ICMPv4 protocol?

Just tried that out, AWS does allow to configure it like that, but not sure what the net effect is. I added a ICMPv4 inbound rule with an IPv6 type PrefixList, and the console accepted and created that.

So, let's skip this for now, to get it into 1.23 before the release. We can try later to add the ICMP rules and see what happens.

I'm not sure what the effect would be of not rendering ICMP rules when a prefix list is in use? I took the approach of querying the AWS API from the IAM model, where that was already used, so I figured that was allowed ;).

Are you sure we can skip them?

It is required for Path MTU Discovery.
I am sure I don't want to call the cloud for this. 😄

docs/cluster_spec.md

pkg/model/awsmodel/api_loadbalancer.go

cloudmock/aws/mockec2/prefixlists.go

hakman · 2022-02-15T13:21:17Z

Great. Would you mind also squashing the commits before merging the PR?

hierynomus · 2022-02-15T13:36:41Z

Great. Would you mind also squashing the commits before merging the PR?

Sure, will do that now!

Signed-off-by: Jeroen van Erp <jeroen@hierynomus.com>

hakman

Looks great. Thanks for taking the time to add this feature!

k8s-ci-robot · 2022-02-15T15:18:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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~~ [hakman]

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

…-upstream-release-1.23 Automated cherry pick of #13113: Allow PrefixList for sshAccess and kubernetesApiAccess

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 16, 2022

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 16, 2022

k8s-ci-robot requested review from johngmyers and zetaab January 16, 2022 22:21

k8s-ci-robot added area/api area/provider/aws Issues or PRs related to aws provider do-not-merge/contains-merge-commits and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 16, 2022

hierynomus force-pushed the issue-12925 branch from ae306bc to fd72b2f Compare January 16, 2022 22:29

k8s-ci-robot removed the do-not-merge/contains-merge-commits label Jan 16, 2022

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 Jan 16, 2022

hierynomus force-pushed the issue-12925 branch from fd72b2f to 401097b Compare January 16, 2022 22:32

hierynomus force-pushed the issue-12925 branch from 401097b to 988f720 Compare January 17, 2022 09:41

hierynomus force-pushed the issue-12925 branch from 988f720 to 55ffb84 Compare January 17, 2022 09:58

k8s-ci-robot added the area/documentation label Jan 17, 2022

hierynomus commented Jan 17, 2022

View reviewed changes

johngmyers requested changes Jan 19, 2022

View reviewed changes

k8s-ci-robot assigned johngmyers Jan 19, 2022

hierynomus force-pushed the issue-12925 branch from aced1cb to c460deb Compare January 19, 2022 12:55

hierynomus force-pushed the issue-12925 branch from c460deb to 78b28c6 Compare January 19, 2022 15:29

hakman requested changes Feb 15, 2022

View reviewed changes

k8s-ci-robot assigned hakman Feb 15, 2022

hierynomus force-pushed the issue-12925 branch from 9e9d53c to 11c1137 Compare February 15, 2022 10:16

hakman requested changes Feb 15, 2022

View reviewed changes

pkg/model/awsmodel/api_loadbalancer.go Outdated Show resolved Hide resolved

cloudmock/aws/mockec2/prefixlists.go Outdated Show resolved Hide resolved

hierynomus force-pushed the issue-12925 branch from 04be693 to 86a4811 Compare February 15, 2022 12:43

Allow PrefixList for sshAccess and kubernetesApiAccess

255a032

Signed-off-by: Jeroen van Erp <jeroen@hierynomus.com>

hierynomus force-pushed the issue-12925 branch from 86a4811 to 255a032 Compare February 15, 2022 13:37

hakman approved these changes Feb 15, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 15, 2022

hakman mentioned this pull request Feb 15, 2022

Automated cherry pick of #13113: Allow PrefixList for sshAccess and kubernetesApiAccess #13260

Merged

k8s-ci-robot merged commit bffc602 into kubernetes:master Feb 15, 2022

k8s-ci-robot added this to the v1.24 milestone Feb 15, 2022

hierynomus deleted the issue-12925 branch February 15, 2022 15:27

hierynomus mentioned this pull request Feb 15, 2022

Allow AWS Prefix Lists for sshAccess and kubernetesApiAccess #12925

Closed

k8s-ci-robot added a commit that referenced this pull request Feb 17, 2022

Merge pull request #13260 from hakman/automated-cherry-pick-of-#13113…

7cef12e

…-upstream-release-1.23 Automated cherry pick of #13113: Allow PrefixList for sshAccess and kubernetesApiAccess

rifelpet mentioned this pull request Mar 16, 2022

Possibility to add additional routes in route tables of subnets #13318

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow PrefixList for sshAccess and kubernetesApiAccess #13113

Allow PrefixList for sshAccess and kubernetesApiAccess #13113

hierynomus commented Jan 16, 2022

k8s-ci-robot commented Jan 16, 2022

rifelpet commented Jan 16, 2022

rifelpet commented Jan 16, 2022

hierynomus commented Jan 17, 2022

hierynomus Jan 17, 2022

johngmyers Jan 19, 2022

hierynomus commented Jan 17, 2022

hierynomus commented Jan 17, 2022

johngmyers left a comment

johngmyers Jan 19, 2022

johngmyers Jan 19, 2022

hierynomus Jan 19, 2022

johngmyers Jan 19, 2022

hierynomus Jan 19, 2022

johngmyers Jan 19, 2022

johngmyers commented Jan 19, 2022

hierynomus commented Jan 19, 2022

hierynomus commented Jan 21, 2022

johngmyers commented Jan 21, 2022

hierynomus commented Jan 31, 2022

hakman left a comment

hakman Feb 15, 2022

hierynomus Feb 15, 2022

hakman Feb 15, 2022

hierynomus Feb 15, 2022

hakman Feb 15, 2022

hakman commented Feb 15, 2022

hierynomus commented Feb 15, 2022

hakman left a comment

k8s-ci-robot commented Feb 15, 2022

Allow PrefixList for sshAccess and kubernetesApiAccess #13113

Allow PrefixList for sshAccess and kubernetesApiAccess #13113

Conversation

hierynomus commented Jan 16, 2022

k8s-ci-robot commented Jan 16, 2022

rifelpet commented Jan 16, 2022

rifelpet commented Jan 16, 2022

hierynomus commented Jan 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hierynomus commented Jan 17, 2022

hierynomus commented Jan 17, 2022

johngmyers 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

johngmyers commented Jan 19, 2022

hierynomus commented Jan 19, 2022

hierynomus commented Jan 21, 2022

johngmyers commented Jan 21, 2022

hierynomus commented Jan 31, 2022

hakman 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

hakman commented Feb 15, 2022

hierynomus commented Feb 15, 2022

hakman left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 15, 2022