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.

Sign up for GitHub

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

Jump to bottom

Tag subnets on AWS so load balancers are properly created #3527

Closed
wants to merge 1 commit into from
Closed

Tag subnets on AWS so load balancers are properly created #3527

wants to merge 1 commit into from

Conversation

geojaz
Copy link
Member

@geojaz geojaz commented Oct 3, 2017

Without having these tags, internal load balancers are nondeterministically created and sometimes end up in inappropriate subnets.

Closes #2011

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 3, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: kris-nova

Assign the PR to them by writing /assign @kris-nova in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

chrislovecnm
chrislovecnm reviewed Oct 3, 2017
View reviewed changes
Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question for you

upup/pkg/fi/cloudup/awstasks/subnet.go
// https://github.com/kubernetes/kops/issues/2011
glog.V(4).Infoln("Found a subnet of type: %s and tagging it for proper ELB placement", e.Type)
if e.Type == kops.SubnetTypePrivate {
e.Tags["kubernetes.io/role/internal-elb"] = ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do tagname = 1 elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont we need another tag as well. Checking

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also need the tags from here kubernetes/kubernetes#41695 shared and owned? I am not certain, but since you are messing with tags anyways

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those tags are generated in https://github.com/kubernetes/kops/blob/master/pkg/resources/aws.go

I'll change values to 1. It also occurred to me that folks who use TF and cloud formation probably also want this, so refactoring a bit.

@chrislovecnm chrislovecnm added this to the 1.8.0 milestone Oct 6, 2017
chrislovecnm
chrislovecnm suggested changes Oct 6, 2017
View reviewed changes
Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need e.Tags["kubernetes.io/role/internal-elb"] = "1" to have conformity between our tags. Looks great, sorry for the nit pick. Let me know if you do not have time, I can put in a follow-up PR.

@justinsb
Copy link
Member

Good spot. I'd say rather than passing Type into the subnet task, specify the Tag when you create the Subnet Task. (i.e. the Subnet Task doesn't need to know about the Type, just the Tags!)

justinsb
justinsb reviewed Oct 10, 2017
View reviewed changes
pkg/model/network.go
@@ -155,6 +155,7 @@ func (b *NetworkModelBuilder) Build(c *fi.ModelBuilderContext) error {
CIDR: s(subnetSpec.CIDR),
Shared: fi.Bool(sharedSubnet),
Tags: tags,
Type: subnetSpec.Type,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of passing Type here, I would put the Tag logic here.

@geojaz geojaz mentioned this pull request Oct 22, 2017
Merged
@geojaz
Copy link
Member Author

geojaz commented Oct 22, 2017

Sorry for the delay. I reworked this PR in #3682. Addresses the same idea and I think it takes into consideration these comments.

@geojaz geojaz closed this Oct 22, 2017
k8s-github-robot pushed a commit that referenced this pull request Oct 22, 2017
Merge pull request #3682 from reactiveops/tag_subnets_2011
eca1653 
Automatic merge from submit-queue.

[AWS] Properly tag public and private subnets for ELB creation

This is a replacement for #3527 that I think makes more sense. Thanks!

Closes #2011
@shavo007
Copy link
Contributor

just came across this issue using kops 1.7 now!

I get error:
Error creating load balancer (will retry): Failed to create load balancer for service ingress-nginx/ingress-nginx: could not find any suitable subnets for creating the ELB

Good to know its fixed in 1.8. But i will wait for GA release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinsb justinsb justinsb left review comments

@chrislovecnm chrislovecnm chrislovecnm requested changes

Assignees
No one assigned
Labels
blocks-next cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-changes size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
1.8.0
Development

Successfully merging this pull request may close these issues.
6 participants
@geojaz @k8s-github-robot @justinsb @shavo007 @chrislovecnm @k8s-ci-robot