Remove deprecated 'securityGroupId' property from createEC2InstanceInput

[AlexVulaj]

Removes deprecated securityGroupId property from createEc2InstanceInput struct.

Depends on PRs to the three known downstream consumers to stop using the field:
osdctl: openshift/osdctl#592 (merged)
CAD: openshift/configuration-anomaly-detection#300 (merged)
CS: No uses detected

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 31.27%. Comparing base (705bbe5) to head (46bbee7).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #247      +/-   ##
==========================================
+ Coverage   31.18%   31.27%   +0.08%     
==========================================
  Files          12       12              
  Lines        1119     1116       -3     
==========================================
  Hits          349      349              
+ Misses        752      749       -3     
  Partials       18       18              

Files	Coverage Δ
pkg/verifier/aws/aws_verifier.go	26.66% <ø> (+0.19%)	⬆️
pkg/verifier/package_verifier.go	0.00% <ø> (ø)
pkg/verifier/aws/entry_point.go	0.00% <0.00%> (ø)

... and 5 files with indirect coverage changes

[abyrne55]

Unfortunately, I think there's more that has to happen here. Consumers of the verifier are still setting the deprecated SecurityGroupId field of the AwsEgressConfig type instead of the SecurityGroupIDs field, and currently this PR leaves that deprecated field silently unhandled AFAICT. We either have to:

• preserve AwsEgressConfig.SecurityGroupId and replace the compatibility shim removed by this PR with another shim somewhere else
• yank AwsEgressConfig.SecurityGroupId, which is a breaking API change

I'm not terribly opposed to breaking API changes at the moment, considering the verifier's next release will likely bump the x- and/or y-stream version, but we might want to be careful about changing too much all at once.

[AlexVulaj]

@abyrne55 good catch on these concerns. My take is that - considering this is still in "beta" technically and only has 3 known consumers, we should update those consumers to use the new field instead and then come back to this PR. Curious to hear your thoughts or any other maintainers'!

[AlexVulaj]

@abyrne55 I've updated osdctl and CAD with PRs linked in the description here. I didn't find any uses in clusters service.

[abyrne55]

Nice work with those consumer PRs. One more thing here: could you just remove the securityGroupId field of the egressConfig struct type on cmd/egress/cmd.go:37, as well as the line that sets up a CLI flag for it near the bottom of the same file?

[openshift-ci]

@AlexVulaj: all tests passed!

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-sigs/prow repository. I understand the commands that are listed here.

[abyrne55]

Tested locally and confirmed use of removed flag produces Error: unknown flag: --security-group-id. Also confirmed via AWS console that correct SG is applied when --security-group-ids is used

/lgtm
/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abyrne55

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 [abyrne55]

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