🐛 Default AWSMachine in admission controller instead of during reconciliation #5748
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
k8s-ci-robot
added
needs-priority
size/L
fiunchinho
changed the title
fiunchinho
force-pushed
the
default-awsmachine
branch
from
7040319 to
5d1d25a
Compare
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/test all |
|
/test pull-cluster-api-provider-aws-e2e-blocking |
|
/test pull-cluster-api-provider-aws-e2e |
|
/test pull-cluster-api-provider-aws-e2e-blocking |
controllers/awsmachine_controller.go
Outdated
| // If InstanceMetadataOptions is not set in the spec, don't modify the instance | ||
| // The webhook will set defaults on new resources, but we shouldn't force defaults on existing instances |
There was a problem hiding this comment.
| // If InstanceMetadataOptions is not set in the spec, don't modify the instance | |
| // The webhook will set defaults on new resources, but we shouldn't force defaults on existing instances | |
| // If InstanceMetadataOptions is not set in the spec, don't modify the AWSMachine. | |
| // The webhook will set defaults on new AWSMachines, but we cannot write defaults to existing objects | |
| // because validation doesn't allow changes to the spec. |
| Attribute: ec2types.NetworkInterfaceAttributeGroupSet, | ||
| })).Return(&ec2.DescribeNetworkInterfaceAttributeOutput{Groups: []ec2types.GroupIdentifier{{GroupId: aws.String("3")}}}, nil).MaxTimes(1) | ||
| m.ModifyNetworkInterfaceAttribute(context.TODO(), gomock.Any()).AnyTimes() | ||
| m.ModifyInstanceMetadataOptions(context.TODO(), gomock.Any()).MaxTimes(1) |
There was a problem hiding this comment.
That's IMO not expected while creating an instance. The metadata should be set once in RunInstances, and not modified afterwards.
There was a problem hiding this comment.
I think you're right. I gave it another shot, I completely removed ModifyInstanceMetadataOptions now, and rely only on the admission controller to change the metadata options.
fiunchinho
force-pushed
the
default-awsmachine
branch
from
5d1d25a to
9f2b6d6
Compare
fiunchinho
force-pushed
the
default-awsmachine
branch
from
9f2b6d6 to
7f241e1
Compare
|
/test all |
|
/test pull-cluster-api-provider-aws-e2e |
1 similar comment
|
/test pull-cluster-api-provider-aws-e2e |
|
@fiunchinho: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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-sigs/prow repository. I understand the commands that are listed here. |
3 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
While reconciling newly created
AWSMachines, I noticed that this error usually happensThe problem is that we are setting the defaults on the existing
AWSMachinewhile reconciling it. I think it'd be better to use the defaulting admission controller instead, and avoid default during the reconciliation.Checklist:
Release note: