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

fix/373 Replace ARN format validation procedure #374

Merged

Conversation

srosenberg-apptio
Copy link
Contributor

Fixes #373

Feedback is welcome.

coverage: 83.8% of statements
ok      github.com/keikoproj/instance-manager/controllers/provisioners/eksmanaged       0.057s  coverage: 83.8% of statements
=== RUN   TestInstanceGroupSpecValidate
=== RUN   TestInstanceGroupSpecValidate/eks-fargate_with_managed_strategy
=== RUN   TestInstanceGroupSpecValidate/eks-bogus_provisioner
=== RUN   TestInstanceGroupSpecValidate/eks-fargate_with_bad_strategy
=== RUN   TestInstanceGroupSpecValidate/eks_with_empty_strings_in_licenseSpecifications
=== RUN   TestInstanceGroupSpecValidate/eks_with_invalid_licenseSpecification
=== RUN   TestInstanceGroupSpecValidate/eks_with_invalid_container_runtime
=== RUN   TestInstanceGroupSpecValidate/eks_with_valid_Placement
=== RUN   TestInstanceGroupSpecValidate/eks_with_invalid_combination_of_HostResourceGroupArn_and_Tenancy_in_Placement
=== RUN   TestInstanceGroupSpecValidate/eks_with_invalid_HostResourceGroupArn_in_Placement
=== RUN   TestInstanceGroupSpecValidate/eks_with_invalid_Tenancy_in_Placement
=== RUN   TestInstanceGroupSpecValidate/eks_with_gp3_volume_validates
=== RUN   TestInstanceGroupSpecValidate/eks_with_gp2_volume_with_provisioned_throughput_fails
=== RUN   TestInstanceGroupSpecValidate/eks_with_gp2_volume_with_provisioned_iops_fails
=== RUN   TestInstanceGroupSpecValidate/eks_with_metadataoptions_validates
=== RUN   TestInstanceGroupSpecValidate/eks_with_valid_configuration_for_any_random_partition
=== RUN   TestInstanceGroupSpecValidate/eks_with_invalid_LicenseSpecifications_for_any_random_partition
=== RUN   TestInstanceGroupSpecValidate/eks_with_invalid_combination_of_HostResourceGroupArn_and_Tenancy_in_Placement_for_any_random_partition
=== RUN   TestInstanceGroupSpecValidate/eks_with_invalid_HostResourceGroupArn_in_Placement_for_any_random_partition
--- PASS: TestInstanceGroupSpecValidate (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks-fargate_with_managed_strategy (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks-bogus_provisioner (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks-fargate_with_bad_strategy (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks_with_empty_strings_in_licenseSpecifications (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks_with_invalid_licenseSpecification (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks_with_invalid_container_runtime (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks_with_valid_Placement (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks_with_invalid_combination_of_HostResourceGroupArn_and_Tenancy_in_Placement (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks_with_invalid_HostResourceGroupArn_in_Placement (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks_with_invalid_Tenancy_in_Placement (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks_with_gp3_volume_validates (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks_with_gp2_volume_with_provisioned_throughput_fails (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks_with_gp2_volume_with_provisioned_iops_fails (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks_with_metadataoptions_validates (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks_with_valid_configuration_for_any_random_partition (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks_with_invalid_LicenseSpecifications_for_any_random_partition (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks_with_invalid_combination_of_HostResourceGroupArn_and_Tenancy_in_Placement_for_any_random_partition (0.00s)
    --- PASS: TestInstanceGroupSpecValidate/eks_with_invalid_HostResourceGroupArn_in_Placement_for_any_random_partition (0.00s)
=== RUN   TestLockedAnnotation
=== RUN   TestLockedAnnotation/Locked
=== RUN   TestLockedAnnotation/Unlocked
--- PASS: TestLockedAnnotation (0.00s)
    --- PASS: TestLockedAnnotation/Locked (0.00s)
    --- PASS: TestLockedAnnotation/Unlocked (0.00s)
PASS
coverage: 13.0% of statements
ok      github.com/keikoproj/instance-manager/api/v1alpha1      0.074s  coverage: 13.0% of statements
go build -o bin/manager main.go

return errors.Errorf("validation failed, 'notificationArn' must be a valid IAM role ARN")
}
if !common.StringEmpty(h.RoleArn) && !strings.HasPrefix(h.NotificationArn, awsprovider.ARNPrefix) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we also delete the const awsprovider.ARNPrefix since it's not used any longer

@eytan-avisror
Copy link
Collaborator

Thanks @srosenberg-apptio
Great fix, minor comment but LGTM otherwise

@codecov
Copy link

codecov bot commented Oct 14, 2022

Codecov Report

Merging #374 (9f8f352) into master (ae893c4) will decrease coverage by 0.02%.
The diff coverage is 60.00%.

❗ Current head 9f8f352 differs from pull request most recent head 4ab98e3. Consider uploading reports for the commit 4ab98e3 to get more accurate results

@@            Coverage Diff             @@
##           master     #374      +/-   ##
==========================================
- Coverage   51.43%   51.41%   -0.03%     
==========================================
  Files          33       33              
  Lines        4555     4553       -2     
==========================================
- Hits         2343     2341       -2     
  Misses       2065     2065              
  Partials      147      147              
Impacted Files Coverage Δ
controllers/providers/aws/aws.go 0.00% <ø> (ø)
api/v1alpha1/instancegroup_types.go 18.69% <50.00%> (ø)
controllers/provisioners/eks/helpers.go 91.66% <100.00%> (-0.03%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@srosenberg-apptio
Copy link
Contributor Author

Great fix, minor comment but LGTM otherwise

I will apply that fix, and I believe there is another issue I found downstream that I will tackle.

@srosenberg-apptio srosenberg-apptio changed the title fix/373 Replace ARN format validation procedure wip:fix/373 Replace ARN format validation procedure Oct 20, 2022
@srosenberg-apptio srosenberg-apptio changed the title wip:fix/373 Replace ARN format validation procedure fix/373 Replace ARN format validation procedure Oct 20, 2022
@srosenberg-apptio srosenberg-apptio changed the title fix/373 Replace ARN format validation procedure wip:fix/373 Replace ARN format validation procedure Oct 20, 2022
@srosenberg-apptio srosenberg-apptio changed the title wip:fix/373 Replace ARN format validation procedure fix/373 Replace ARN format validation procedure Oct 28, 2022
@srosenberg-apptio
Copy link
Contributor Author

@eytan-avisror
Ready for review. I deployed a build running a cluster in aws-us-gov partition. It did precisely what was expected: created/updated autoscaling groups and launch templates with the appropriate licensing configuration and host tenancy. New nodes were successfully assigned to dedicated hosts.

@eytan-avisror eytan-avisror self-requested a review October 28, 2022 15:20
Copy link
Collaborator

@eytan-avisror eytan-avisror left a comment

Choose a reason for hiding this comment

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

LGTM, you might need to rebase/sign your commits so DCO passes

@srosenberg-apptio srosenberg-apptio force-pushed the fix/ArnFormatValidation branch 2 times, most recently from 6271d8f to 63ad5ef Compare November 4, 2022 18:11
Signed-off-by: Shaunn Rosenberg <shaunnrosenberg@gmail.com>
Signed-off-by: Shaunn Rosenberg <srosenberg@apptio.com>
@srosenberg-apptio
Copy link
Contributor Author

DCO is passing.

@eytan-avisror eytan-avisror merged commit 5eb4421 into keikoproj:master Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IGs in special AWS partitions always fails ARN format validation in instance_types.go
2 participants