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

V1.1.0 is not compatible with v1.0.0 release due to setting up an optional field in the controller #3003

Closed
sedefsavas opened this issue Dec 6, 2021 · 2 comments · Fixed by #3004 or #3016
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@sedefsavas
Copy link
Contributor

In our community meetings, we have decided to only do minor releases starting from v1 and do a major release for breaking changes. So, all v1.x releases need to be compatible with each other.

In v1.1.0, we introduced a bug by adding an optional field

Name *string `json:"name,omitempty"`

and then setting it a value in the controller:

s.scope.ControlPlaneLoadBalancer().Name = aws.String(generatedName)

Now, external controllers that use CAPA API v1.0.0, is failing to update AWSCluster because admission webhook denies the request with invalid: spec.controlPlaneLoadBalancer.name: Invalid value: "null": field is immutable error

/kind bug
/priority critical-urgent
/milestone v1.2.0
/triage accepted

@k8s-ci-robot k8s-ci-robot added this to the v1.2.0 milestone Dec 6, 2021
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Dec 6, 2021
@sedefsavas sedefsavas self-assigned this Dec 6, 2021
@sedefsavas
Copy link
Contributor Author

sedefsavas commented Dec 6, 2021

@dlipovetsky What would not be working if we remove the LB name assignment in BYO LB?

@dlipovetsky
Copy link
Contributor

The external controller doesn't know about the Name field, and the defaulting webhook is setting it to nil, but the Name field has already been set by the v1.1.0 CAPA controller, so that's causing the validating webhook to reject the update as a change?

@sedefsavas and I talked about possible workarounds.

The first option I'll try is to generate the default ELB name in the code, and to not persist it in the AWSCluster Spec. The reasoning is: Any client that is not aware of the Name field would get the default value, and if the default value is not persisted, then that client can update the AWSCluster without hitting the validation error. Any client that is aware of the Name field can use it. And although the default value won't be persisted to the Spec, it will still be in the Status, and so it will be discoverable.

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. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
3 participants