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

Ensure Ref is not overwritten in Spec #128

Merged

Conversation

michaelhtm
Copy link
Member

Issue #1898

Description of changes:
Similarly as the ec2-controller, the ref stored in a
struct is being overwritten during create/update.
With this fix we ensure the ref would not be discarded.

Currently there isn't a way to test this fix, as we can't
run two controllers simultaneously and reference subnet
names in cluster directly.
Instead I will document an example with the fix working in this PR

provided:

apiVersion: eks.services.k8s.aws/v1alpha1
kind: Cluster
metadata:
  name: my-clust
spec:
  name: my-clust
  roleARN: <removed>
  version: "1.30"
  resourcesVPCConfig:
    endpointPrivateAccess: true
    endpointPublicAccess: false
    subnetRefs:
      - from:
          name: sub1
      - from:
          name: sub2

Creating .spec and status:

spec:
  accessConfig:
    authenticationMode: CONFIG_MAP
    bootstrapClusterCreatorAdminPermissions: true
  kubernetesNetworkConfig:
    ipFamily: ipv4
    serviceIPv4CIDR: 172.20.0.0/16
  logging:
    clusterLogging:
    - enabled: false
      types:
      - api
      - audit
      - authenticator
      - controllerManager
      - scheduler
  name: my-clust
  resourcesVPCConfig:
    endpointPrivateAccess: true
    endpointPublicAccess: true
    publicAccessCIDRs:
    - 0.0.0.0/0
    subnetRefs:
    - from:
        name: sub1
    - from:
        name: sub2
  roleARN: <removed>
  version: "1.30"
status:
  ackResourceMetadata:
    arn: <removed>
    ownerAccountID: <removed>
    region: us-west-2
  certificateAuthority: {}
  conditions:
  - lastTransitionTime: "2024-09-18T20:56:50Z"
    status: "True"
    type: ACK.ReferencesResolved
  - lastTransitionTime: "2024-09-18T20:56:51Z"
    status: "False"
    type: ACK.ResourceSynced
  createdAt: "2024-09-18T20:56:50Z"
  health: {}
  platformVersion: eks.8
  status: CREATING

Active:

spec:
  accessConfig:
    authenticationMode: CONFIG_MAP
    bootstrapClusterCreatorAdminPermissions: true
  kubernetesNetworkConfig:
    ipFamily: ipv4
    serviceIPv4CIDR: 172.20.0.0/16
  logging:
    clusterLogging:
    - enabled: false
      types:
      - api
      - audit
      - authenticator
      - controllerManager
      - scheduler
  name: my-clust
  resourcesVPCConfig:
    endpointPrivateAccess: true
    endpointPublicAccess: true
    publicAccessCIDRs:
    - 0.0.0.0/0
    subnetRefs:
    - from:
        name: sub1
    - from:
        name: sub2
  roleARN: <removed>
  version: "1.30"
status:
  ackResourceMetadata:
    <removed>
    ownerAccountID: <removed>
    region: us-west-2
  certificateAuthority:
    data: <removed>
  conditions:
  - lastTransitionTime: "2024-09-18T22:00:27Z"
    status: "True"
    type: ACK.ReferencesResolved
  - lastTransitionTime: "2024-09-18T22:00:28Z"
    status: "True"
    type: ACK.ResourceSynced
  createdAt: "2024-09-18T20:56:50Z"
  endpoint: https://DFD284F7337766E87670192E4EB46565.gr7.us-west-2.eks.amazonaws.com
  health: {}
  identity:
    oidc:
      issuer: https://oidc.eks.us-west-2.amazonaws.com/id/DFD284F7337766E87670192E4EB46565
  platformVersion: eks.8
  status: ACTIVE

Updating:

spec:
  accessConfig:
    authenticationMode: CONFIG_MAP
    bootstrapClusterCreatorAdminPermissions: true
  kubernetesNetworkConfig:
    ipFamily: ipv4
    serviceIPv4CIDR: 172.20.0.0/16
  logging:
    clusterLogging:
    - enabled: false
      types:
      - api
      - audit
      - authenticator
      - controllerManager
      - scheduler
  name: my-clust
  resourcesVPCConfig:
    endpointPrivateAccess: true
    endpointPublicAccess: false
    publicAccessCIDRs:
    - 0.0.0.0/0
    subnetRefs:
    - from:
        name: sub1
    - from:
        name: sub2
  roleARN: <removed>
  version: "1.30"
status:
  ackResourceMetadata:
    arn: <removed>
    ownerAccountID: <removed>
    region: us-west-2
  certificateAuthority:
    data: <removed>
  conditions:
  - lastTransitionTime: "2024-09-18T22:04:38Z"
    status: "True"
    type: ACK.ReferencesResolved
  - lastTransitionTime: "2024-09-18T22:04:38Z"
    message: Cluster is in 'UPDATING' status
    status: "False"
    type: ACK.ResourceSynced
  - message: cluster in 'UPDATING' state, cannot be modified until 'ACTIVE'
    status: "True"
    type: ACK.Recoverable
  createdAt: "2024-09-18T20:56:50Z"
  endpoint: https://DFD284F7337766E87670192E4EB46565.gr7.us-west-2.eks.amazonaws.com
  health: {}
  identity:
    oidc:
      issuer: https://oidc.eks.us-west-2.amazonaws.com/id/DFD284F7337766E87670192E4EB46565
  platformVersion: eks.8
  status: UPDATING

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Similarly as the ec2-controller, the ref stored in a struct
is being overwritten during create/update. With this fix
we ensure the ref would not be discarded.

Currently there isn't a way to test this fix, as we can't
run two controllers simultaneously and reference subnet names
in cluster directly.
Instead I will document an example with the fix working in
this PR
@michaelhtm michaelhtm force-pushed the maintain-resource-reference branch from d5c868d to 282983a Compare September 19, 2024 16:45
Copy link

ack-prow bot commented Sep 19, 2024

@michaelhtm: 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
eks-verify-attribution 282983a link false /test eks-verify-attribution

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.

Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

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

Good job @michaelhtm !
/lgtm

@ack-prow ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2024
Copy link

ack-prow bot commented Sep 19, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, michaelhtm

The full list of commands accepted by this bot can be found here.

The pull request process is described 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

@ack-prow ack-prow bot added the approved label Sep 19, 2024
@ack-prow ack-prow bot merged commit f02dfb8 into aws-controllers-k8s:main Sep 19, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants