Increase unit test coverage

[sedefsavas]

This issue is to track improving our current unit-test coverage which is as follows:

Prow link for the interactive UI above.

Artifacts of the test-coverage prow job are here for more details.

[sedefsavas]

/triage accepted
/priority important-long-term

[k8s-ci-robot]

@sedefsavas: The label(s) priority/important-long-term cannot be applied, because the repository doesn't have them.

In response to this:

/triage accepted
/priority important-long-term

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.

[sedefsavas]

This is issue should be covered here: #2569

[Ankitasw]

WIP list for the scenarios covered in AWSCluster controller tests :

• Reconcile AWSCluster successfully:
  - Should set LoadBalancerReady on cluster object if load balancer created successfully
  - Should set awscluster status as ready if reconcile is successful
  - Should add finalizer to awscluster
• Reconcile AWSCluster failures:
  - Should not set ClusterSecurityGroupsReady on cluster object if reconcile security groups fails
  - Should not set BastionHostReady on cluster object if reconcile bastion fails
  - Should not set LoadBalancerReady on cluster object if reconcile load balancer fails
• Test Reconcile AWSCluster func:
  - Should Reconcile AWSCluster with requeue
  - Should fail Reconcile with GetOwnerCluster failure
  - Should Reconcile successfully if no AWSCluster found
  - Should not Reconcile if cluster is paused
• Test create requeue requests For Unpaused Cluster:
  - Should not create reconcile request for deleted clusters
  - Should not create reconcile request if infrastructure ref for AWSCluster on owner cluster is not set
  - Should not create reconcile request if infrastructure ref type on owner cluster is not AWSCluster
  - Should not create reconcile request if AWSCluster not found
  - Should not create reconcile request if AWSCluster is externally managed
  - Should create reconcile request successfully

Integration tests:

• Reconcile AWSCluster, Should not create VPC if using unmanaged VPC
• Reconcile delete AWSCluster, Should delete route tables while deleting VPC
• Reconcile AWSCluster, Should fail VPC creation if VPC limit exceeded

[Ankitasw]

/assign
I will be working on controllers package and will be listing scenarios for each controller in this issue

[shivi28]

/assign
I will start working on pkd/cloud/services/ec2 package

[Madhur97]

/assign
Will start working on pkg/cloud/services/autoscaling package

[Ankitasw]

WIP list for scenarios covered for AWSMachine controller:

• Reconcile AWSMachine, should fail to find instance if no provider ID provided
• Reconcile AWSMachine, when instance creation succeeds,
  1. should not do anything if control plane ELB is already registered
  2. should attach control plane ELB to instance
  3. Should store userdata using AWS Secrets Manager
  4. should fail to delete bootstrap data secret if AWSMachine state is updated
• Reconcile AWSMachine, when instance creation fails,
  1. Should fail while getting userdata
  2. should fail to determine the registration status of control plane ELB
  3. should fail to attach control plane ELB to instance
  4. should fail to delete bootstrap data secret if AWSMachine is in failed state
• Reconcile AWSMachine, when instance creation fails, should fail in ensureTag,
  1. Should fail to return machine annotations after instance is created
  2. Should fail to update resource tags after instance is created
• Reconcile AWSMachine, when instance creation fails, while ensuring SecurityGroups,
  1. Should fail to return machine annotations
  2. Should fail to fetch core security groups
  3. Should return silently if ensureSecurityGroups fails to fetch additional security groups
  4. Should fail to update security group
• Deleting AWSMachine, reconcile LB detachment
  1. should fail to determine registration status of ELB
  2. should fail to detach control plane ELB from instance
  3. should fail if secretPrefix present, but secretCount is not set
  4. should fail if secrets backend is invalid
  5. should fail if deleting entries from AWS Secret fails
• Deleting AWSMachine, when instance can be shut down
  1. should detach control plane ELB from instance
  2. should not do anything if control plane ELB is already detached from instance
• TestAWSMachineReconciler AWSClusterToAWSMachines func
  1. Should create reconcile request successfully
  2. Should not create reconcile request for deleted clusters
  3. Should not create reconcile request if ownerCluster not found
  4. Should not create reconcile request if owned Machines not found
  5. Should not create reconcile request if owned Machine type is not AWSMachine
  6. Should not create reconcile request if name for machine in infrastructure ref not found
• TestAWSMachineReconciler requeueAWSMachinesForUnpausedCluster func
  1. Should not create reconcile request for deleted clusters
  2. Should create reconcile request successfully
• TestAWSMachineReconciler indexAWSMachineByInstanceID func
  1. Should not return instance id if cluster type is not AWSCluster
  2. Should return instance id successfully
  3. Should not return instance id if instance id is not present
• TestAWSMachineReconciler Reconcile func
  1. Should Reconcile AWSMachine with requeue
  2. Should fail Reconcile with GetOwnerMachine failure
  3. Should not Reconcile if machine does not contain cluster label
  4. Should not Reconcile if cluster is paused
  5. Should not Reconcile if AWSManagedControlPlane is not ready
  6. Should not Reconcile if AWSCluster is not ready
  7. Should fail to reconcile while fetching infra cluster
  8. Should Reconcile successfully if no AWSMachine found
     Integration tests:
• Should delete AWSMachine when no ELB found
• Should delete instance if using managed VPC

[Madhur97]

I will start working on pkd/cloud/services/ssm package

[Madhur97]

WIP list for the scenarios covered in AWSCluster controller tests :

• Reconcile AWSCluster successfully:
  • Should set LoadBalancerReady on cluster object if load balancer created successfully
    ..
    ..
    ..
• Reconcile AWSCluster, Should not create VPC if using unmanaged VPC
• Reconcile delete AWSCluster, Should delete route tables while deleting VPC
• Reconcile AWSCluster, Should fail VPC creation if VPC limit exceeded

Adding more cases which can be covered as part of cluster controller tests:

1. Should not reconcile if owner Cluster is marked as paused
2. Should fail reconciliation and patch PrincipalCredentialRetrieved condition as unknown when source identity is not authorized
3. Should fail reconciliation if AWSClusterControllerIdentity’s name is not default
4. Should fail reconciliation and mark PrincipalUsageNotAllowedCondition as false if cluster namespace is neither present in allowedNamespaces nor matches any selector of IdentityReference
5. Should add session entry to cache if new identity provider is found
6. Should load session from cache if session is already present
7. Should refresh credentials if credentials have expired when identity is AWSRolePrincipalType/AWSStaticPrincipalType
8. Should reconcile successfully if identity is ClusterRoleIdentityKind with a sourceIdentityRef
9. Should reconcile successfully if identity is ClusterRoleIdentityKind without a sourceIdentityRef
10. Should set ClusterStaticPrincipal as Secret's owner reference (in case identity is AWSClusterStaticIdentity)
11. Should set ManagedVPCAttributes if VPC is managed
12. Should associate VPC with secondary CIDR, if provided.
13. Should fail if subnets are not specified when VPC is unmanaged
14. Should fail if Security group overrides specified in case of managed VPC
15. Should remove undesired rules and add missing rules in security groups during their reconciliation
16. Should skip bastion host if No private subnets available
17. Should fail reconciliation of bastion host, if public subnets unavailable
18. Should change ELB scheme to “internet-facing” If it is set to “Internet-facing”
19. Should generate ELB name if it is not present
20. Should add security groups, tags, AZs, subnets to ELB as given in spec
21. AWSCluster should satisfy below checks (apart from the ones already mentioned in above comment) after successful reconciliation

• Conditions to be set:
  1. PrincipalCredentialRetrieved
  2. PrincipalUsageAllowedCondition
  3. VpcReadyCondition
  4. InternetGatewayReadyCondition
  5. SubnetsReadyCondition
  6. RouteTablesReadyCondition
  7. NatGatewaysReadyCondition
  8. ClusterSecurityGroupsReadyCondition
  9. BastionHostReadyCondition
  10. LoadBalancerReadyCondition
• ControlPlaneEndpoint and port should be set

[Ankitasw]

@Madhur97 PTAL

Adding more cases which can be covered as part of cluster controller tests:

1. Should not reconcile if owner Cluster is marked as paused

This is already covered in the test cases I added

2. Should fail reconciliation and patch PrincipalCredentialRetrieved condition as unknown when source identity is not authorized
3. Should fail reconciliation if AWSClusterControllerIdentity’s name is not default
4. Should fail reconciliation and mark PrincipalUsageNotAllowedCondition as false if cluster namespace is neither present in allowedNamespaces nor matches any selector of IdentityReference
5. Should add session entry to cache if new identity provider is found
6. Should load session from cache if session is already present
7. Should refresh credentials if credentials have expired when identity is AWSRolePrincipalType/AWSStaticPrincipalType
8. Should reconcile successfully if identity is ClusterRoleIdentityKind with a sourceIdentityRef
9. Should reconcile successfully if identity is ClusterRoleIdentityKind without a sourceIdentityRef
10. Should set ClusterStaticPrincipal as Secret's owner reference (in case identity is AWSClusterStaticIdentity)

I think 2-10 can be taken care in scope package(session_test.go), wdyt?

11. Should set ManagedVPCAttributes if VPC is managed

Already covered in vpc_test.go

12. Should associate VPC with secondary CIDR, if provided.

Can be taken care in vpc_test.go if not already covered.

13. Should fail if subnets are not specified when VPC is unmanaged

Already covered in subnets_test.go

14. Should fail if Security group overrides specified in case of managed VPC

Already covered in securitygroups_test.go

15. Should remove undesired rules and add missing rules in security groups during their reconciliation

Can be covered in securitygroups_test.go if not already covered.

16. Should skip bastion host if No private subnets available

Can be covered in bastion_test.go, anyways getting covered in the test case (Should not create VPC in case of unmanaged VPC)

17. Should fail reconciliation of bastion host, if public subnets unavailable

Can be covered in bastion_test.go, anyways getting covered in the test case (Should not create VPC in case of unmanaged VPC)

18. Should change ELB scheme to “internet-facing” If it is set to “Internet-facing”

Can be covered in loadbalancer_test.go

19. Should generate ELB name if it is not present

Already covered in loadbalancer_test.go

20. Should add security groups, tags, AZs, subnets to ELB as given in spec

Can be covered in loadbalancer_test.go

21. AWSCluster should satisfy below checks (apart from the ones already mentioned in above comment) after successful reconciliation

• Conditions to be set:
  1. PrincipalCredentialRetrieved
  2. PrincipalUsageAllowedCondition
  3. VpcReadyCondition
  4. InternetGatewayReadyCondition
  5. SubnetsReadyCondition
  6. RouteTablesReadyCondition
  7. NatGatewaysReadyCondition
  8. ClusterSecurityGroupsReadyCondition
  9. BastionHostReadyCondition
  10. LoadBalancerReadyCondition
• ControlPlaneEndpoint and port should be set

I think all conditions can be checked(except PrincipalCredentialRetrieved, PrincipalUsageAllowedCondition as these are specific to scope) in current test cases only, I will add those checks

[sedefsavas]

@Madhur97 @Ankitasw
In general, we cannot cover every edge case in the integration tests, we should be mindful of test durations.

Our aim is to make the code base well tested by using webhook tests, integration, unit, and e2e tests wherever suitable. For instance, #3 can be a webhook test, #4, #5, #6, #7 etc could all be tested well with unit tests.

[sedefsavas]

Also, in integration tests, we can try to merge multiple scenarios into a single test, which saves us time. Also, for testing specific situations, we can use fakeClient too (in scenarios where testing with envtest becomes more complicated).

[Ankitasw]

Missed scenarios in launchtemplate.go unit tests:

• Should return computed userdata if userdata is nil in launchtemplate
• In TestService_LaunchTemplateNeedsUpdate func:
  • Should return true if incoming IamInstanceProfile is not same as existing IamInstanceProfile
  • Should return true if incoming InstanceType is not same as existing InstanceType
  • Should return false if failed to get core node security groups
• Should fail to form launch template data during creation/deletion
• Should fetch imageLookupFormat from scope if not present in launchtemplate
• Should fetch ImageLookupOrg from scope if not present in launchtemplate
• Should fetch ImageLookupBaseOS from scope if not present in launchtemplate
• Should do eks EMI lookup in case of EKSManaged cluster(think of covering eksAMILookup() func here with different combinations to cover all edge cases)

[Ankitasw]

I will work on package pkg/cloud/tags

[Madhur97]

pkg/cloud/services/autoscaling:
Need to check past issues occurred related to autoscaling package and cover them in test files.

[shivi28]

Missed scenarios in launchtemplate.go unit tests:

• Should return computed userdata if userdata is nil in launchtemplate

• In TestService_LaunchTemplateNeedsUpdate func:

  • Should return true if incoming IamInstanceProfile is not same as existing IamInstanceProfile
  • Should return true if incoming InstanceType is not same as existing InstanceType
  • Should return false if failed to get core node security groups

• Should fail to form launch template data during creation/deletion

• Should fetch imageLookupFormat from scope if not present in launchtemplate

• Should fetch ImageLookupOrg from scope if not present in launchtemplate

• Should fetch ImageLookupBaseOS from scope if not present in launchtemplate

• Should do eks EMI lookup in case of EKSManaged cluster(think of covering eksAMILookup() func here with different combinations to cover all edge cases)

Added all the above scenarios in the updated PR

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten