Hotfix/10015 cloud formation lint error

[binkkatal]

connects #10015
The ./hack/update-expected.sh script generates some fields which are
required to be string fields and hence results in linting errors.

This PR changes those fields to string/*string and removes lint
warnings.

• Sort help for error
  E3031 CidrIp contains invalid characters (Pattern: x.x.x.x/y) at Resources/AWSEC2SecurityGroupIngressicmppmtuapielb20010850040/Properties/CidrIp
  tests/integration/update_cluster/complex/cloudformation.json:629:9

[k8s-ci-robot]

Welcome @binkkatal!

It looks like this is your first PR to kubernetes/kops 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kops has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @binkkatal. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[rifelpet]

Thanks for doing this! I think we should keep these types as integers (or int64) and only update the cloudformation (line 865) to string. We shouldn't modify the terraform types because those should use integers (see the terraform docs here).

Only modifying the cloudformationAutoscalingGroup type also means we don't have to update anything in pkg/model which I think is preferable for this particular issue.

This same suggestion applies to the loadbalancer and healthcheck types below

[binkkatal]

@rifelpet , the fields in the loadbalancer and health check also results in lint warnings,
should they be reverted back to int64 fields ?

E3012 Property Resources/AWSElasticLoadBalancingLoadBalancerapiprivateciliumadvancedexamplecom/Properties/Listeners/0/InstancePort should be of type String
E3012 Property Resources/AWSElasticLoadBalancingLoadBalancerapiprivateciliumadvancedexamplecom/Properties/Listeners/0/LoadBalancerPort should be of type String
E3012 Property Resources/AWSElasticLoadBalancingLoadBalancerapiprivateciliumadvancedexamplecom/Properties/HealthCheck/HealthyThreshold should be of type String
E3012 Property Resources/AWSElasticLoadBalancingLoadBalancerapiprivateciliumadvancedexamplecom/Properties/HealthCheck/UnhealthyThreshold should be of type String
E3012 Property Resources/AWSElasticLoadBalancingLoadBalancerapiprivateciliumadvancedexamplecom/Properties/HealthCheck/Interval should be of type String
E3012 Property Resources/AWSElasticLoadBalancingLoadBalancerapiprivateciliumadvancedexamplecom/Properties/HealthCheck/Timeout should be of type String

E3012 Property Resources/AWSElasticLoadBalancingLoadBalancerbastionprivateciliumadvancedexamplecom/Properties/Listeners/0/InstancePort should be of type String
tests/integration/update_cluster/privateciliumadvanced/cloudformation.json:1297:13

E3012 Property Resources/AWSElasticLoadBalancingLoadBalancerbastionprivateciliumadvancedexamplecom/Properties/Listeners/0/LoadBalancerPort should be of type String

[rifelpet]

These types should be updated to fix the lint warnings:

kops/upup/pkg/fi/cloudup/awstasks/load_balancer.go

Lines 789 to 829 in 0be8d4e

	type cloudformationLoadBalancer struct {
	LoadBalancerName *string `json:"LoadBalancerName,omitempty"`
	Listener []*cloudformationLoadBalancerListener `json:"Listeners,omitempty"`
	SecurityGroups []*cloudformation.Literal `json:"SecurityGroups,omitempty"`
	Subnets []*cloudformation.Literal `json:"Subnets,omitempty"`
	Scheme *string `json:"Scheme,omitempty"`
	HealthCheck *cloudformationLoadBalancerHealthCheck `json:"HealthCheck,omitempty"`
	AccessLog *cloudformationLoadBalancerAccessLog `json:"AccessLoggingPolicy,omitempty"`
	ConnectionDrainingPolicy *cloudformationConnectionDrainingPolicy `json:"ConnectionDrainingPolicy,omitempty"`
	ConnectionSettings *cloudformationConnectionSettings `json:"ConnectionSettings,omitempty"`
	CrossZoneLoadBalancing *bool `json:"CrossZone,omitempty"`
	Tags []cloudformationTag `json:"Tags,omitempty"`
	}
	type cloudformationLoadBalancerListener struct {
	InstancePort int `json:"InstancePort"`
	InstanceProtocol string `json:"InstanceProtocol"`
	LoadBalancerPort int64 `json:"LoadBalancerPort"`
	LoadBalancerProtocol string `json:"Protocol"`
	}
	type cloudformationLoadBalancerHealthCheck struct {
	Target *string `json:"Target"`
	HealthyThreshold *int64 `json:"HealthyThreshold"`
	UnhealthyThreshold *int64 `json:"UnhealthyThreshold"`
	Interval *int64 `json:"Interval"`
	Timeout *int64 `json:"Timeout"`
	}
	type cloudformationConnectionDrainingPolicy struct {
	Enabled *bool `json:"Enabled,omitempty"`
	Timeout *int64 `json:"Timeout,omitempty"`
	}
	type cloudformationConnectionSettings struct {
	IdleTimeout *int64 `json:"IdleTimeout,omitempty"`
	}

but the regular LoadBalancer, LoadBalancerHealthCheck types should not. This means the type conversion from int64 to string will need to happen in RenderCloudformation rather than in the pkg/model packages. The ./hack/update-expected.sh script should only be updating the cloudformation.json files, not the kubernetes.tf files.

[hakman]

/ok-to-test

[binkkatal]

@rifelpet , is it okay if i rebase and squash it to remove merge commits ?

[rifelpet]

@binkkatal yes, feel free to rebase and squash. The changes look good to me, so once you do we can get this merged

[binkkatal]

@rifelpet squashed and rebased the commits, ready to merge

[rifelpet]

confirmed that cfn-lint no longer reports those warnings. Thanks!

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: binkkatal, rifelpet

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:

• OWNERS [rifelpet]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment