Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

builtin/k8s: Fix ‘ports’ configurability #1650

Merged
merged 1 commit into from
Jun 14, 2021

Conversation

rykov
Copy link
Contributor

@rykov rykov commented Jun 12, 2021

Looks like 0.4.0 release broke multi-port K8S configuration by setting ServicePort = 3000 if it is not specified in the config. By forcing ServicePort to always defined, a multi-port configuration using Ports always fails the mutual-exclusivity check with the following error:

* Cannot define both 'service_port' and 'ports'. Use 'ports' for configuring multiple container ports.

This PR ensures the default port is used only if neither service_port nor ports is configured.

Copy link
Member

@briancain briancain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that regression! I left a note on the PR, thanks for taking the time to open a pull request to fix this up 🙏🏻

Comment on lines 603 to 605
if p.config.ServicePort == 0 {
if p.config.ServicePort == 0 && p.config.Ports == nil {
p.config.ServicePort = 3000
}
Copy link
Member

@briancain briancain Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this entire if-block might not be required at all, it looks like it showed up during the k8s resource manager conversion. ​I thought this if-block below in the resource managers create was covering configuring the single and multi-port case:

// Setup our port configuration
if p.config.ServicePort == 0 && p.config.Ports == nil {
// nothing defined, set up the defaults
p.config.Ports = make([]map[string]string, 1)
p.config.Ports[0] = map[string]string{"port": strconv.Itoa(DefaultServicePort), "name": "http"}
} else if p.config.ServicePort > 0 && p.config.Ports == nil {
// old ServicePort var is used, so set it up in our Ports map to be used
p.config.Ports = make([]map[string]string, 1)
p.config.Ports[0] = map[string]string{"port": strconv.Itoa(int(p.config.ServicePort)), "name": "http"}
} else if p.config.ServicePort > 0 && len(p.config.Ports) > 0 {
// both defined, this is an error
return fmt.Errorf("Cannot define both 'service_port' and 'ports'. Use" +
" 'ports' for configuring multiple container ports.")
}

If removing the service port check on 603 doesn't fix it, we should try to fix up the port conversion instead over in resourceDeploymentCreate and remove this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@briancain You're absolutely right: I've removed that code, and things work without the error.

@briancain briancain added bug Something isn't working regression labels Jun 14, 2021
@briancain briancain added this to the 0.4.x milestone Jun 14, 2021
Copy link
Member

@briancain briancain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I gave it a shot myself, was able to reproduce the issue and see the fix work. Thanks again! ✨

@briancain briancain merged commit 9df1732 into hashicorp:main Jun 14, 2021
@rykov
Copy link
Contributor Author

rykov commented Jun 15, 2021

Thanks @briancain

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants