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

AWS: More ELB attributes via service annotations #30695

Merged
merged 1 commit into from
Aug 22, 2016
Merged

AWS: More ELB attributes via service annotations #30695

merged 1 commit into from
Aug 22, 2016

Conversation

krancour
Copy link
Member

@krancour krancour commented Aug 16, 2016 •
edited by k8s-oncall
Loading

Replaces #25015 and addresses all of @justinsb's feedback therein. This is a new PR because I was unable to reopen #25015 to amend it.

I noticed recently that there is existing (but undocumented) precedent for the AWS cloud provider to manage ELB-specifc load balancer configuration based on service annotations. In particular, one can already designate an ELB as "internal" or enable PROXY protocol.

This PR extends this capability to the management of ELB attributes, which includes the following items:

  • Access logs:
    • Enabled / disabled
    • Emit interval
    • S3 bucket name
    • S3 bucket prefix
  • Connection draining:
    • Enabled / disabled
    • Timeout
  • Connection:
    • Idle timeout
  • Cross-zone load balancing:
    • Enabled / disabled

Some of these are possibly more useful than others. Use cases that immediately come to mind:

  • Enabling cross-zone load balancing is potentially useful for "Ubernetes Light," or anyone otherwise attempting to spread worker nodes around multiple AZs.
  • Increasing idle timeout is useful for the benefit of anyone dealing with long-running requests. An example I personally care about would be git pushes to Deis' builder component.

This change is Reviewable

@k8s-bot
Copy link

k8s-bot commented Aug 16, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

6 similar comments
@k8s-bot
Copy link

k8s-bot commented Aug 16, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Aug 16, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Aug 16, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Aug 16, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Aug 16, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Aug 16, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Aug 16, 2016
@krancour
Copy link
Member Author

@justinsb, could you possibly mark this as "ok to test?"

justinsb
justinsb reviewed Aug 19, 2016
View reviewed changes
pkg/cloudprovider/providers/aws/aws_loadbalancer.go
foundAttributes := &describeAttributesOutput.LoadBalancerAttributes

// Update attributes if they're dirty
if !reflect.DeepEqual(loadBalancerAttributes, foundAttributes) {
Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to log here if they are unequal, just in case reflect.DeepEqual has false positives

@justinsb
Copy link
Member

ok to test

@justinsb
Copy link
Member

lgtm

@justinsb
Copy link
Member

Thanks @krancour this looks great. If you have time it would be great to add a glog when updating attributes, but not a blocker for merge.

@justinsb justinsb added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Aug 19, 2016
@justinsb justinsb changed the title Add support for managing ELB attributes with service annotations AWS: More ELB attributes via service annotations Aug 19, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 19, 2016

GCE e2e build/test passed for commit 96dad1f.

@krancour
Copy link
Member Author

@justinsb... I will first thing Monday.

@justinsb
Copy link
Member

@krancour thanks - ping me when you've done it and I'll LGTM. Also if you don't have time I would like to still get this into 1.4, so just LMK. Adding the logging can be done post feature-freeze (today!).

@krancour
Copy link
Member Author

@justinsb if feature freeze is today, I'd say we're better off putting that last log-line in ex post facto-- just given what I have on my plate this morning. Happy to hear this can make it into 1.4!

@justinsb justinsb mentioned this pull request Aug 22, 2016
Closed
@justinsb
Copy link
Member

Awesome @krancour . Opened #31127 to track the logging. This LGTM

@justinsb justinsb added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2016
@justinsb justinsb added this to the v1.4 milestone Aug 22, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Aug 22, 2016

GCE e2e build/test passed for commit 96dad1f.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit bfafb6f into kubernetes:master Aug 22, 2016
@therc
Copy link
Member

therc commented Sep 24, 2016

Are these documented anywhere? I'll be the first to confess that the only user-visible documentation for my annotations are buried deep in user-guide/services/ ("SSL support on AWS"). We should find a home for them all.

@krancour krancour deleted the manage-elb-attributes branch September 27, 2016 13:54
@davidw davidw mentioned this pull request Feb 24, 2017
Closed
felixbuenemann added a commit to felixbuenemann/workflow that referenced this pull request Feb 25, 2017
@felixbuenemann
docs(managing-workflow) Document k8s annotation for AWS ELB idle timeout
2511d0a 
This updates the docs to describe how to persist the AWS ELB idle timeout by using the proper k8s service annotation instead of following the manual instructions, which get reset if k8s re-configures the ELB.

The annotation was added in kubernetes/kubernetes#30695 and merged targeting k8s v1.4 in August 2016. I have verified that it works as expected on k8s v1.4.6.
@felixbuenemann felixbuenemann mentioned this pull request Feb 25, 2017
Merged
bacongobbler pushed a commit to bacongobbler/workflow that referenced this pull request Feb 28, 2017
feat(configuring-load-balancers): use kubernetes idle timeout annotation
0b357f4 
This annotation is undocumented, but was introduced in kubernetes/kubernetes#30695.
@bacongobbler bacongobbler mentioned this pull request Feb 28, 2017
Closed
@brendandburns brendandburns mentioned this pull request Oct 10, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@justinsb justinsb

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
6 participants
@krancour @k8s-bot @justinsb @k8s-github-robot @therc @googlebot