Heuristic to guess resource name and operation misses plural names

[brycahta]

Describe the bug

• Code-generator ignores resources when the default form of the resource is plural. Ex: DhcpOptions, FlowLogs
• root cause: Resource generation is driven by a resource's Create operation. DhcpOptions does not have a Create operation bound to it after heuristic processing because GetOpTypeAndResourceNameFromOpID returns the tuple <OpTypeCreateBatch , dhcpOption>.

Steps to reproduce

• Attempt to generate DhcpOptions resource for ec2-controller
• No files are generated and resource is treated as if it were ignored

Expected outcome

• DhcpOptions should generate the same as other resources (as if it were singular).
• GetOpTypeAndResourceNameFromOpID should return <OpTypeCreate , dhcpOptions>

Environment

• Kubernetes version? 1.20
• Using EKS (yes/no), if so version? No
• AWS service targeted (S3, RDS, etc.)? EC2

[brycahta]

Proposal to Fix

• Add a resource-level override to indicate the plural-ness of a resource. This will signal to code-generator to not bind Batch operations with a plural resource.

Interface

• EC2-Controller::generator.yaml:

resource:
  DhcpOptions:
    contains_plural_suffix: true

Implementation

• Add cfg *ackgenconfig.Config parameter to GetOpTypeAndResourceNameFromOpID
• Check for plural override for any Batch code branches

func GetOpTypeAndResourceNameFromOpID(opID string, cfg *ackgenconfig.Config) (OpType, string) {
    ...
    } else if strings.HasPrefix(opID, "Create") {
        resName := strings.TrimPrefix(opID, "Create")
                if isSafePlural(resName) {
            return OpTypeCreateBatch, pluralize.Singular(resName)
        }
        return OpTypeCreate, resName
    ...
}

---

func isSafePlural(resName string, cfg *ackgenconfig.Config) bool {
    hasPluralOverride := <check cfg for override for this resName>
    return pluralize.IsPlural(resName) && !hasPluralOverride
}

• Names such as contains_plural_suffix and isSafePlural are more placeholders for now than permanent names, but intent remains the same. Open to any suggestions!

[vijtrip2]

The approach looks decent to me! 🚀

[RedbackThomson]

I don't see any particular value in the contains_plural_suffix resource configuration. I'd assume this to be true for all resources, and then any that don't match should be placed into the ignore list. For instance, CreateFlowLogs seems like a data plane operation, and should be ignored.

I'm concerned about using a plural as the Kind for a resource, though. If DHCPOptions is the kind, then the plural would also be DHCPOptions. I'm not sure where in the k8s core code, if there is any, that assumes Kind != Plural. However, I can't think of a way to singularise this, either.

[brycahta]

I don't see any particular value in the contains_plural_suffix resource configuration. I'd assume this to be true for all resources, and then any that don't match should be placed into the ignore list. For instance, CreateFlowLogs seems like a data plane operation, and should be ignored.

In other words you're thinking this kind of scenario is the exception and shouldn't occur throughout the APIs frequently? I was trying to get data for this and grep'd for all Create operations that end in 's' in aws-sdk-go/models/apis folder as a starting point. Results:

CreateAccess
CreateAccountAlias
CreateAccountStates
CreateAccountStatus
CreateAccountStatuses
CreateAddress
CreateAgentStatus
CreateAlias
CreateAnalysis
CreateAssessmentFrameworkControlSets
CreateAssessmentFrameworkControls
CreateAssociationBatchRequestEntries
CreateBotAlias
CreateChannelMembershipErrors
CreateConfigurationSetTrackingOptions
CreateConnectionAlias
CreateConnectionApiKeyAuthRequestParameters
CreateConnectionAuthRequestParameters
CreateConnectionBasicAuthRequestParameters
CreateConnectionOAuthClientRequestParameters
CreateConnectionOAuthRequestParameters
CreateControlMappingSources
CreateCrossAccountAuthorizationParameters
CreateDatabaseDefaultPermissions
CreateDelegationByAssessmentErrors
CreateDelegationRequests
CreateDhcpOptions
CreateDistributionWithTags
CreateEndpointAccess
CreateEnvironmentInputAirflowConfigurationOptions
CreateEventBus
CreateFleetLocations
CreateFlowLogs
CreateInProgress
CreateInstances
CreateInvitations
CreateJobOutputs
CreateJobPlaylists
CreateLabels
CreateLoadBalancerListeners
CreateLocationEfs
CreateLocationFsxWindows
CreateLocationNfs
CreateMeetingWithAttendees
CreateMembers
CreateOrUpdateParameters
CreateOrUpdateTags
CreateParameters
CreatePlayerSessions
CreateProcess
CreateRoleAlias
CreateSampleFindings
CreateSimulationJobRequests
CreateSnapshots
CreateStackInstances
CreateStreamingDistributionWithTags
CreateTableDefaultPermissions
CreateTableRows
CreateTags
CreateTapes
CreateTasks
CreateTemplateAlias
CreateThemeAlias
CreateThesaurus
CreateTransitGatewayConnectRequestOptions
CreateTransitGatewayMulticastDomainRequestOptions
CreateTransitGatewayVpcAttachmentRequestOptions
CreateVolumePermissionModifications
CreateVolumePermissions
CreateWorkspaceRequests
CreateWorkspaces
CreatedArtifacts

I'm not familiar with a lot of these so I deferred to a config for now, but I'll take more time to see which seem like resources and which data plane operations. If DhcpOptions is one of the only cases, then I agree the config won't have any value, and we could instead maintain a static list of these exceptional cases in ops.go.

I'm not sure where in the k8s core code, if there is any, that assumes Kind != Plural.

This is a good callout; I'll try to research this more.

[bwagner5]

Looks like there is precedent for plural kinds, most notably Endpoints in corev1: https://github.com/kubernetes/kubernetes/issues/8115 so shouldn't be any core k8s components that assume Kind != Plural.

I'd disagree that CreateFlowLogs is a data plane API. CreateFlowLogs is used to setup network traffic flows on a VPC, not to actually publish a flow log entry.

[RedbackThomson]

In other words you're thinking this kind of scenario is the exception and shouldn't occur throughout the APIs frequently? I was trying to get data for this and grep'd for all Create operations that end in 's' in aws-sdk-go/models/apis folder as a starting point. Results:

Removing all of the non-plural words that end with s in that list, I honestly don't think that's too bad. Given just how many freakin' AWS services there are, this averages out to only a few per service. So for things like CreateTableRows, we'd need to add an ignore, but most of these looks like valid CRDs.

I'd disagree that CreateFlowLogs is a data plane API. CreateFlowLogs is used to setup network traffic flows on a VPC, not to actually publish a flow log entry.

My apologies, I misunderstood this API. Valid point.

[brycahta]

Removing all of the non-plural words that end with s in that list, I honestly don't think that's too bad. Given just how many freakin' AWS services there are, this averages out to only a few per service.

Gotcha. My thoughts for approach were either:

• New config value
• A static collection of these plural names that should not be ignored
  • this collection would be referenced in a similar way as checking the config override above

Are you good with moving forward with the 2nd option given the data or did you have a different implementation in mind?

So for things like CreateTableRows, we'd need to add an ignore, but most of these looks like valid CRDs.

Could you expand on this a bit? What do you mean by add an ignore; I thought it is already being ignored today.

[jaypipes]

OK, sorry for delay in getting to this. Here is my recommendation.

I agree with @RedbackThomson that a new configuration option called contains_plural_suffix is clunky. Furthermore, a new config option actually isn't necessary. If you have a generator.yaml file that looks like this:

resources:
  DHCPOptions:
    ...

Then the code in GetOpTypeAndResourceNameFromOpID can simply check to see if the name of the resource in the generator config's Resources field exists. If it does, then return OpTypeCreate, .

So, basically, this:

func GetOpTypeAndResourceNameFromOpID(opID string, cfg *ackgenconfig.Config) (OpType, string) {
    ...
    } else if strings.HasPrefix(opID, "Create") {
        resName := strings.TrimPrefix(opID, "Create")
        // Check to see if the resName exists in the generator configuration's
        // list of resources. If it does, then just return OpTypeCreate and the
        // the resource name. This takes care of "pluralized singular" resource
        // names like EC2's DhcpOptions.
        for key, _ := cfg.Resources {
            if strings.EqualFold(resName, key) {
                return OpTypeCreate, key
            }
        }
                if isSafePlural(resName) {
            return OpTypeCreateBatch, pluralize.Singular(resName)
        }
        return OpTypeCreate, resName
    ...
}

[brycahta]

Closing; fix was merged