Add AWS Marketplace Entitlement verification

[lucacome]

AWS Entitlement verification

[soneillf5]

Minor changes and some questions

[pleshakov]

Right now this function is called every time the binary is executed. However, the binary can be executed without the intention of starting the IC - for example, a troubleshooting /nginx-ingress version command to determine the IC version. With the current init() implementation, that troubleshooting command with also register usage.

However, there is a bit bigger concern I see - the function has a lot of logic which we don't test at all.

To address those concerns I suggest the following:

(1) Let's call this function indirectly. This way we will only call it when the IC is meant to start.
(2) Let's remove all log Fatal statement. Instead, the function will return an error. If the error is not nil, we will exit in main.go

in main.go

var startupCheckFn func() error

func main() {
	flag.Parse()

	err := flag.Lookup("logtostderr").Value.Set("true")
	if err != nil {
		glog.Fatalf("Error setting logtostderr to true: %v", err)
	}

	versionInfo := fmt.Sprintf("Version=%v GitCommit=%v Date=%v", version, commit, date)
	if *versionFlag {
		fmt.Println(versionInfo)
		os.Exit(0)
	}

	if startupCheckFn != nil {
		err := startupCheckFn()
		if err != nil {
			glog.Fatalf("Failed startup check: %v", err)
		}
		glog.Info("Startup check is successful")
	}
	
	. . .

in aws.go

func init() {
	startupCheckFn = checkAWSEntitlement
}

func checkAWSEntitlement() error {
	. . .
}

this way we will be able to test checkAWSEntitlement.

Since we only make one external API call, we will be able to test it by wrapping it into an interface like this:

type awsMeter interface {
	RegisterUsage(params *marketplacemetering.RegisterUsageInput) (*marketplacemetering.RegisterUsageOutput, error)
}

In unit tests, we will be able to create a fake implementation that will return the right output.

[lucacome]

What kind of logic would you want to test?

[pleshakov]

the ifs in init()

[soneillf5]

I don't think init as a whole needs a test as there's so little logic, just ifs for the errors. You could wrap and test the logic around the token claims and unit test that?

The question around the binary is interesting. Why would this code need a version check?

[pleshakov]

I don't think init as a whole needs a test as there's so little logic, just ifs for the errors. You could wrap and test the logic around the token claims and unit test that?

we have a happy path when nothing returns an error and we a number places where the error needs to be checked. why would we not want to test it? 🤔
yeah, some additional refactoring of init can also help

The question around the binary is interesting. Why would this code need a version check?

this init code is always called, even if the version check command is invoked on the binary.

[pleshakov]

supporting calling the registration from main is easy:

main.go

var startupCheckFn func() error

. . .
       if startupCheckFn != nil {
          startupCheckFn()
       }

aws.go

func init() {
	startupCheckFn = checkAWSEntitlement
}

func checkAWSEntitlement() {
	. . .
}

However, the result is that (1) it allows using the same logger (glog) and (2) it will not affect the version check at all.

The point of using init() is to run it before main() automatically, without modifying main

the requirement is to run that code as part of the Ingress Controller startup, so that the pod is registered. However, we don't have the requirement that it must be run from the init() function. when implementing new features, in general we should not break the existing ones (in this case version check) and make sure that the new functionally fits nicely with the existing one (in this case, use the same logger for consistent format).

[lucacome]

• The version check still works
• The log is only one line. I just pushed a change to only print on failure, so that would be the only line printed and there won't be any inconsistency.

[pleshakov]

The version check still works

yep. but it works with a side effect - it will register a pod

The log is only one line. I just pushed a change to only print on failure, so that would be the only line printed and there won't be any inconsistency.

Below is an example of inconsistent format:
fatal log in the init

2021/05/21 14:58:08 operation error Marketplace Metering: RegisterUsage, failed to resolve service endpoint, an AWS region is required, but was not found

fatal log in the main.go

I0526 15:28:24.173615   45447 main.go:259] Starting NGINX Ingress controller Version=my-version GitCommit= Date= PlusFlag=false
F0526 15:28:24.173819   45447 main.go:275] error creating client configuration: unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined

[bonnyr]

I would like to offer some other considerations, for why @pleshakov's points merit consideration:

1. init may be called before main but there's no ordering guarantee for which init is called before another between packages. Not really the case here but it could be if more pakages are using init. This is implementation-dependent and the golang authors are free the change that implementation leading to silent breaks
2. init is magic and we should strive for simplicity rather than things happening automagically.
3. whilst init can be used for setting up structures etc, since it's very early in the lifecycle of the process, using it to interact with state (the registration here) is beyond what would be good practice. IMO it would be better to explicitly initialize and control initialization from main
4. using log.Fatal in init is a recipe for surprise by developers down the line. I would not use that.
5. Testing should definitely cover the cases in the initialization however simple (or at least create tech debt to record this)
6. I would choose consistency (in this case agreeing on the particular log library) so whichever version allows producing consistent outcome for users is preferrable. You never know how users are going to use the logs for their own purposes

my 2c

[lucacome]

The best solutions are always the most obvious that don't make you think and obviously this made a lot of people think! Thanks for all the great feedback, I will go with the suggested solution.