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

WIP: Update enhancement for aws-load-balancer-operator #1099

Closed
wants to merge 3 commits into from
Closed

WIP: Update enhancement for aws-load-balancer-operator #1099

wants to merge 3 commits into from

Conversation

thejasn
Copy link
Contributor

@thejasn thejasn commented Apr 27, 2022 •
edited
Loading

Update enhancement for aws-load-balancer-operator

  • Updates some implementation details
  • Updates test plan and current caveats in testing
  • Removes non-applicable details for initial release

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 27, 2022
@thejasn
Copy link
Contributor Author

thejasn commented Apr 27, 2022

/assign @arjunrn

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 27, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign dtantsur after the PR has been reviewed.
You can assign the PR to them by writing /assign @dtantsur 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 files:

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

arjunrn
arjunrn reviewed Apr 28, 2022
View reviewed changes
Copy link
Contributor

@arjunrn arjunrn left a comment

Choose a reason for hiding this comment

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

Can you also update the implementation history section. In the drawbacks we should mention that services will also be reconciled. Also in the risk mitigation section for target type IP we can say that this won't work in OpenShift and so is not a risk.

enhancements/ingress/aws-load-balancer-operator.md Outdated
The webhook can be registered with a CA bundle which is used to verify the identity
of webhook by the API server. The [service-ca controller](https://docs.openshift.com/container-platform/4.10/security/certificate_types_descriptions/service-ca-certificates.html)
can be used to generate certificates and have them injected into the webhook
configurations. The operator will also watch the secret with the certificates so
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently the secret is not watched. Or rather the secret could be updated in the background and the controller would not cause an update in the deployment of the controller pods to reload the secret. The only way around this if the file is reloaded by controller(not sure if this functionality exists upstream) or if we update the deployment so that it causes a pod rollout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

@thejasn thejasn force-pushed the update/aws-load-balancer-operator branch from 33f1859 to 66a4316 Compare May 4, 2022 08:03
@thejasn thejasn force-pushed the update/aws-load-balancer-operator branch from 66a4316 to 32a6a34 Compare May 4, 2022 08:24
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 4, 2022

@thejasn: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/markdownlint 32a6a34 link true /test markdownlint

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@thejasn
Copy link
Contributor Author

thejasn commented May 5, 2022

Closing in favor of #1107

@thejasn thejasn closed this May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arjunrn arjunrn arjunrn left review comments

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@dtantsur dtantsur Awaiting requested review from dtantsur

Assignees

@arjunrn arjunrn

Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thejasn @arjunrn @omertuc