introduce per-stack hostname suffix as a CLI config parameter

[demonCoder95]

This supersedes the work in #693.

The approach there was to be able to "disable" per-stack hostnames. This, instead introduces configuration suffixes for per-stack hostnames allowing us to enforce .ingress.cluster.local domain suffix for per-stack ingresses and routegroups.

Change Description

• Introduce a CLI parameter per-stack-domain that allows specifying a suffix for using in per-stack hostnames. The default is ingress.cluster.local.
• Propagate the PerStackDomain field through the StackSetContainer to StackContainer.
• Use the PerStackDomain field in the StackContainer in the hostname generation logic instead of the ClusterDomains slice that was used before.
• Adjust the tests accordingly to match the new PerStackDomain suffix for stack hostnames.

To-do:

• Adjust the e2e tests with the new logic of per-stack hostname generation

[linki]

What is this doing? It used to add an entry for each cluster domain, now it adds one with a fixed suffix (and duplicates if there are multiple cluster domains).

Why not something like this?

if strings.HasSuffix(host, clusterDomainInternal) {
  result.Insert(fmt.Sprintf("%s.%s", sc.Name(), clusterDomainInternal))
}

to limit the per-stack domains to the internal one?

[linki]

Ok, it's not duplicated because result is a set. But maybe we still can make it more clear.

[demonCoder95]

I'll simplify as suggested.

[demonCoder95]

Thinking some more about it, since at the end of the day we're only providing a hostname that's a combination of the stack name and the per-stack domain suffix, I think we can maybe drop the loop altogether? Like simply reduce it from this:

// Old-style autogenerated hostnames
	for _, host := range spec.GetHosts() {
		for _, domain := range sc.clusterDomains {
			if strings.HasSuffix(host, domain) {
				// generate the per-stack hostname using the per-stack domain suffix
				result.Insert(fmt.Sprintf("%s.%s", sc.Name(), sc.perStackDomain))
			} else {
				log.Debugf(
					"Ingress host: %s suffix did not match cluster-domain %s",
					host,
					domain,
				)
			}
		}
	}

to this:

	// generate the per-stack hostname using the per-stack domain suffix
	result.Insert(fmt.Sprintf("%s.%s", sc.Name(), sc.perStackDomain))

We're only providing a single hostname per stack, and that is of the pattern <stackName>.<perStackDomain>. Maybe we don't need the results list either?

[demonCoder95]

Or maybe we can keep the list pattern, in case we might want to support multiple suffixes in the future?

[linki]

I would agree. But it depends where the main hostname comes from (e.g. foo.teapot.zalan.do). If this comes from another place, e.g. the StackSet, then we can drop it here. But it could be that it's needed for this. We can just try it out by running locally or via e2e tests.

[demonCoder95]

Hmm, makes sense. In that case, keeping the code the way it is makes sense, as checking for a clusterDomain for suffix is part of the validation that we want to ensure hosts coming from the spec are verified before we generate per-stack hosts.

[linki]

I extracted the CRD changes in another PR. Now it's much smaller to review here.

Do we have a real e2e test for this feature somewhere?

[demonCoder95]

I extracted the CRD changes in another PR. Now it's much smaller to review here.

Thanks!

Do we have a real e2e test for this feature somewhere?

I'll create one now to test the change as you suggested in the chat.