Update server acl init to create tokens for gateways

[adilyse]

This allows the server acl init job to create base tokens for any
provided ingress and terminating gateway names.

For terminating gateways, users will need to update these created
tokens to include permissions to the services they represent. To
do this, they should add separate policies or attach service identities
to avoid having this job overwrite their configuration.

When Consul namespaces are enabled, users can provide a namespace with
each gateway name to specify which Consul namespace to scope the gateway
token to. In this case, the gateway flag values can be provided as
<gateway-name>.<gateway-namespace>. If a namespace is not provided,
the token will be created for the default namespace.

[adilyse]

This currently creates gateway acl tokens in the Consul default namespace. This will work, but we should decide if there are cases where the gateway should be defined in another namespace (for example if -consul-inject-destination-namespace is defined).

[lkysow]

Looking good! I tested on a real kube cluster starting without namespaces and then upgrading to consul ent with namespaces.

The policies, tokens and secrets were all created as expected and upgraded as expected. I also tested attaching a new policy to one of the tokens and the upgrade happened without issue (as expected).

Couple of comments around naming and refactoring of rules.go.

[lkysow]

Adding this here and having to set it to "" in the non-gateway callers seems like a smell to me. What about splitting this and rulesData out into a new method/struct?

type rulesData struct {
	EnableNamespaces               bool
	ConsulSyncDestinationNamespace string
	EnableSyncK8SNSMirroring       bool
	SyncK8SNSMirroringPrefix       string
}

type gatewayRulesData struct {
	rulesData
	GatewayName string
}

func (c *Command) renderRules(tmpl string) (string, error) {
	return c.renderRulesGeneric(tmpl, c.rulesData())
}

func (c *Command) renderGatewayRules(tmpl, gatewayName string) (string, error) {
	// Populate the data that will be used in the template.
	// Not all templates will need all of the fields.
	data := gatewayRulesData{
		rulesData:   c.rulesData(),
		GatewayName: gatewayName,
	}

	return c.renderRulesGeneric(tmpl, data)
}

func (c *Command) rulesData() rulesData {
	return rulesData{
		EnableNamespaces:               c.flagEnableNamespaces,
		ConsulSyncDestinationNamespace: c.flagConsulSyncDestinationNamespace,
		EnableSyncK8SNSMirroring:       c.flagEnableSyncK8SNSMirroring,
		SyncK8SNSMirroringPrefix:       c.flagSyncK8SNSMirroringPrefix,
	}
}

func (c *Command) renderRulesGeneric(tmpl string, data interface{}) (string, error) {
	// Check that it's a valid template
	compiled, err := template.New("root").Parse(strings.TrimSpace(tmpl))
	if err != nil {
		return "", err
	}

	// Render the template
	var buf bytes.Buffer
	err = compiled.Execute(&buf, data)
	if err != nil {
		// Discard possible partial results on error return
		return "", err
	}

	return buf.String(), nil
}

[adilyse]

I've made the change because it seems like a reasonable suggestion, but nobody likes to hear that their code smells. You should try "what about <suggestion>?" or "it might be more modular/less repetitive/<specific reason> to <suggestion>" instead.

[lkysow]

This currently creates gateway acl tokens in the Consul default namespace. This will work, but we should decide if there are cases where the gateway should be defined in another namespace (for example if -consul-inject-destination-namespace is defined).

From the RFC I thought it had to be in the default ns for now?

[adilyse]

Besides updating things based on the review comments, I've also added support for gateways in non-default Consul namespace.

Since the gateways require Connect to be enabled, it's possible to use the flagConsulInjectDestinationNamespace when creating the rules to reflect the correct namespace. If Connect is set up to register all services in a specific Consul namespace, the gateway acl token will reflect that the gateway will be registered there as well. In all other cases, the token will be scoped for the default Consul namespace.

[adilyse]

I've made the change because it seems like a reasonable suggestion, but nobody likes to hear that their code smells. You should try "what about <suggestion>?" or "it might be more modular/less repetitive/<specific reason> to <suggestion>" instead.

[lkysow]

👍 LGTM. I'll wait for the change to using the default ns (if we're doing that).

[lkysow]

What about flagIngressGatewayNames? Looks like we follow the convention of matching these variables with the flags.

[adilyse]

Updated.

[adilyse]

Updated to allow the specification of namespaces for each gateway in the form of <gateway-name>.<gateway-namespace>.

[lkysow]

I tested this (after using the updated policy) and it works!!

[lkysow]

Just one last question about routing to services in other namespaces.

[lkysow]

Have you tested this with routing to services in a different ns from the gateway? I would have thought the node_prefix and service_prefix rules had to be within a namespace_prefix "" block to allow routing to services in different ns's or is routing to services in other ns's not expected to be supported?

[adilyse]

Given that it's easy to run multiple ingress gateways, I don't think we should support cross-namespace permissions at this point. They're pretty messy anyway and it's better to keep permissions as restricted as possible.

[lkysow]

I think we should support the use case of ingress gateway in default namespace being able to route to services in other consul namespaces but I think this may already work because of the cross-namespace policy we create in the default namespace.

Can you try it out and let me know if it does?