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

Only include API server additional security groups in InstanceGroups for masters #10519

Conversation

seh
Copy link
Contributor

@seh seh commented Jan 3, 2021

When using an AWS NLB in front of the Kubernetes API servers, we can't attach the EC2 security groups nominated in the Cluster "spec.api.loadBalancer.additionalSecurityGroups" field directly to the load balancer, as NLBs don't have associated security groups. Instead, we intend to attach those nominated security groups to the machines that will receive network traffic forwarded from the NLB's listeners. For the API servers, since that program runs only on the master or control plane machines, we need only attach those security groups to the machines that will host the kube-apiserver program, by way of the ASG launch templates that come from kOps InstanceGroups of role "master."

We were mistakenly including these security groups in launch templates derived from InstanceGroups of all of our three current roles: "bastion," "master," and "node." Instead, skip InstanceGroups of the "bastion" and "node" roles and only target those of role "master."

Fixes #10517.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 3, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @seh. 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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 3, 2021
@k8s-ci-robot k8s-ci-robot added area/provider/aws Issues or PRs related to aws provider size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 3, 2021
@seh seh force-pushed the restrict-api-server-security-groups-to-masters branch from 19baedf to 301d1de Compare January 3, 2021 16:55
@rifelpet
Copy link
Member

rifelpet commented Jan 4, 2021

This fix looks good to me, can you drop your last commit to address the merge conflicts? copyright year issues were fixed in #10520

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 4, 2021
@seh
Copy link
Contributor Author

seh commented Jan 4, 2021

can you drop your last commit to address the merge conflicts?

With glee. That last commit saddened me greatly to see the changed file count climb so high. I'll take care of this on Monday morning.

@hakman
Copy link
Member

hakman commented Jan 4, 2021

Looks ok to me too, so...
/ok-to-test

@k8s-ci-robot 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 4, 2021
When using an AWS NLB in front of the Kubernetes API servers, we can't
attach the EC2 security groups nominated in the Cluster
"spec.api.loadBalancer.additionalSecurityGroups" field directly to the
load balancer, as NLBs don't have associated security groups. Instead,
we intend to attach those nominated security groups to the machines
that will receive network traffic forwarded from the NLB's
listeners. For the API servers, since that program runs only on the
master or control plane machines, we need only attach those security
groups to the machines that will host the "kube-apiserver" program, by
way of the ASG launch templates that come from kOps InstanceGroups of
role "master."

We were mistakenly including these security groups in launch templates
derived from InstanceGroups of all of our three current roles:
"bastion," "master," and "node." Instead, skip InstanceGroups of the
"bastion" and "node" roles and only target those of role "master."
@seh seh force-pushed the restrict-api-server-security-groups-to-masters branch from 301d1de to 76feb2e Compare January 4, 2021 13:43
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 4, 2021
@rifelpet
Copy link
Member

rifelpet commented Jan 4, 2021

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rifelpet, seh

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

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 4, 2021
@k8s-ci-robot k8s-ci-robot merged commit 22c5975 into kubernetes:master Jan 4, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Jan 4, 2021
k8s-ci-robot added a commit that referenced this pull request Jan 4, 2021
…19-origin-release-1.19

Automated cherry pick of #10519: Test that AWS launch templates include wrong SG
@seh seh deleted the restrict-api-server-security-groups-to-masters branch January 4, 2021 22:21
@seh
Copy link
Contributor Author

seh commented Jan 4, 2021

Can we backport this for inclusion in kOps version 1.19?

Oh, it's already done in #10528.

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. area/api area/provider/aws Issues or PRs related to aws provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.

EC2 security groups meant only for NLB-fronted API server are attached to all machines in the cluster
4 participants