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

AWS: More ELB attributes via service annotations #30695

Merged
merged 1 commit into from Aug 22, 2016

Conversation

Projects
None yet
6 participants
@krancour
Member

krancour commented Aug 16, 2016

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

@googlebot googlebot added the cla: yes label Aug 16, 2016

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot 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 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

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot 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 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

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot 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 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

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot 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 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

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot 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 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

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot 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 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

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot 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 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.

@krancour

This comment has been minimized.

Show comment
Hide comment
@krancour

krancour Aug 19, 2016

Member

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

Member

krancour commented Aug 19, 2016

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

foundAttributes := &describeAttributesOutput.LoadBalancerAttributes
// Update attributes if they're dirty
if !reflect.DeepEqual(loadBalancerAttributes, foundAttributes) {

This comment has been minimized.

@justinsb

justinsb Aug 19, 2016

Member

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

@justinsb

justinsb Aug 19, 2016

Member

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

@justinsb

This comment has been minimized.

Show comment
Hide comment
@justinsb

justinsb Aug 19, 2016

Member

ok to test

Member

justinsb commented Aug 19, 2016

ok to test

@justinsb

This comment has been minimized.

Show comment
Hide comment
@justinsb

justinsb Aug 19, 2016

Member

lgtm

Member

justinsb commented Aug 19, 2016

lgtm

@justinsb

This comment has been minimized.

Show comment
Hide comment
@justinsb

justinsb Aug 19, 2016

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.

Member

justinsb commented Aug 19, 2016

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 changed the title from Add support for managing ELB attributes with service annotations to AWS: More ELB attributes via service annotations Aug 19, 2016

@k8s-bot

This comment has been minimized.

Show comment
Hide comment

k8s-bot commented Aug 19, 2016

GCE e2e build/test passed for commit 96dad1f.

@krancour

This comment has been minimized.

Show comment
Hide comment
@krancour

krancour Aug 20, 2016

Member

@justinsb... I will first thing Monday.

Member

krancour commented Aug 20, 2016

@justinsb... I will first thing Monday.

@justinsb

This comment has been minimized.

Show comment
Hide comment
@justinsb

justinsb Aug 22, 2016

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!).

Member

justinsb commented Aug 22, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@krancour

krancour Aug 22, 2016

Member

@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!

Member

krancour commented Aug 22, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@justinsb

justinsb Aug 22, 2016

Member

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

Member

justinsb commented Aug 22, 2016

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

@justinsb justinsb added the lgtm label Aug 22, 2016

@justinsb justinsb added this to the v1.4 milestone Aug 22, 2016

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 22, 2016

Contributor

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

Contributor

k8s-merge-robot commented Aug 22, 2016

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

@k8s-bot

This comment has been minimized.

Show comment
Hide comment

k8s-bot commented Aug 22, 2016

GCE e2e build/test passed for commit 96dad1f.

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Aug 22, 2016

Contributor

Automatic merge from submit-queue

Contributor

k8s-merge-robot commented Aug 22, 2016

Automatic merge from submit-queue

@k8s-merge-robot k8s-merge-robot merged commit bfafb6f into kubernetes:master Aug 22, 2016

6 of 7 checks passed

Submit Queue Github CI tests are not green.
Details
Jenkins GCE Node e2e Build finished. 626 tests run, 140 skipped, 0 failed.
Details
Jenkins GCE e2e Build finished. 368 tests run, 166 skipped, 0 failed.
Details
Jenkins GKE smoke e2e Build finished. 368 tests run, 367 skipped, 0 failed.
Details
Jenkins unit/integration Build finished. 3891 tests run, 24 skipped, 0 failed.
Details
Jenkins verification Build finished.
Details
cla/google All necessary CLAs are signed
@therc

This comment has been minimized.

Show comment
Hide comment
@therc

therc Sep 24, 2016

Contributor

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.

Contributor

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 krancour:manage-elb-attributes branch Sep 27, 2016

felixbuenemann added a commit to felixbuenemann/workflow that referenced this pull request Feb 25, 2017

docs(managing-workflow) Document k8s annotation for AWS ELB idle timeout
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.

bacongobbler added a commit to bacongobbler/workflow that referenced this pull request Feb 28, 2017

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