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

Add defaulter & conversion for Internet-facing to internet-facing ELB scheme change #2801

Closed
sedefsavas opened this issue Sep 24, 2021 · 7 comments · Fixed by #2832
Closed
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. kind/release-blocking Issues or PRs that need to be closed before the next release priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now.
Milestone

Comments

@sedefsavas
Copy link
Contributor

/kind bug

What steps did you take and what happened:
We fixed an issue about ELB scheme enum values #2768, but forgot to add the conversion for the clusters.

We should fix this before doing a release with #2768 fix in.
Also, we need to backport it to v1alpha3 too.

What did you expect to happen:

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]

Environment:

  • Cluster-api-provider-aws version:
  • Kubernetes version: (use kubectl version):
  • OS (e.g. from /etc/os-release):
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. needs-priority labels Sep 24, 2021
@sedefsavas sedefsavas added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Sep 24, 2021
@sedefsavas sedefsavas added the kind/release-blocking Issues or PRs that need to be closed before the next release label Sep 24, 2021
@sedefsavas
Copy link
Contributor Author

cc @dilyevsky

@randomvariable
Copy link
Member

I think we also need this in the defaulter so that it applies to existing v1alpha3 or existing v1alpha4 clusters.

@randomvariable randomvariable changed the title Add conversion for Internet-facing to internet-facing ELB scheme change Add defaulter & conversion for Internet-facing to internet-facing ELB scheme change Sep 27, 2021
@randomvariable
Copy link
Member

randomvariable commented Sep 27, 2021

Discussing with @sedefsavas , I think what we want is:

v1alpha3 --> v1alpha3 & v1alpha4 --> v1alpha4, default on Update to corrected value
v1alpha3 --> v1alpha4 , v1alpha3 --> v1beta1, v1alpha4 --> v1beta1, correct in conversion

We may also want to call this particular defaulter inside the controller, such that it allows the ELB to complete reconciliation. Calling patch will then update with the corrected value. This can then be removed in v1beta1 such that v1beta1 enforces the correct terminology.

@sedefsavas
Copy link
Contributor Author

/assign

@sedefsavas
Copy link
Contributor Author

Changes for the v1alpha4 release is described in the open PR: see

TODO:

Changes to be added to the next v1alpha3 release:
No change in the API or webhooks, just use "Internet-facing" scheme and translate it to "internet-facing" internally in the controllers. This is enough to unblock existing users without any change to their objects.

Changes to be added to the next v1beta1 release:
Internet-facing to internet-facing modification will only happen in the conversion webhook and remove the logic that accepts Internet-facing scheme from the controllers. No need for defaulting.

@randomvariable
Copy link
Member

/milestone v1.0.0

@k8s-ci-robot k8s-ci-robot added this to the v1.0.0 milestone Oct 4, 2021
@dlipovetsky
Copy link
Contributor

Thanks for catching this @sedefsavas!

(I missed this issue because of a tag typo above 😬 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. kind/release-blocking Issues or PRs that need to be closed before the next release priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now.
Projects
None yet
4 participants