Skip to content

Credentials changes for v2.0.0 #1052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Dec 21, 2020
Merged

Credentials changes for v2.0.0 #1052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3be7d12
Remove load_config_file and support for KUBECONFIG environment variable
jrhouston Nov 2, 2020
5a16a65
Change config_path -> config_paths
jrhouston Nov 13, 2020
f82e5f2
Support config_path and config_paths
jrhouston Nov 18, 2020
70c644e
Use system path separator
jrhouston Nov 20, 2020
d7516bc
Update website/docs/index.html.markdown
jrhouston Nov 20, 2020
04fdfac
Add ConflictsWith for config_path
jrhouston Nov 23, 2020
b0bfaf8
Generate an error when the user does not supply any configuration
jrhouston Nov 25, 2020
975f492
Add better error message when provider is not configured
jrhouston Nov 27, 2020
75377d4
Add check for api token path
jrhouston Dec 18, 2020
b9bcdd2
Defer checking valid configuration to avoid progressive apply issue
jrhouston Dec 21, 2020
f16553e
Fix crash in kubernetes_job waiter
jrhouston Dec 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
171 changes: 123 additions & 48 deletions kubernetes/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@ import (
"bytes"
"context"
"fmt"
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"log"
"net/http"
"os"
"path/filepath"

"github.com/hashicorp/terraform-plugin-sdk/v2/diag"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/logging"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
Expand Down Expand Up @@ -67,16 +70,18 @@ func Provider() *schema.Provider {
DefaultFunc: schema.EnvDefaultFunc("KUBE_CLUSTER_CA_CERT_DATA", ""),
Description: "PEM-encoded root certificates bundle for TLS authentication.",
},
"config_paths": {
Type: schema.TypeList,
Elem: &schema.Schema{Type: schema.TypeString},
Optional: true,
Description: "A list of paths to kube config files. Can be set with KUBE_CONFIG_PATHS environment variable.",
},
"config_path": {
Type: schema.TypeString,
Optional: true,
DefaultFunc: schema.MultiEnvDefaultFunc(
[]string{
"KUBE_CONFIG",
"KUBECONFIG",
},
"~/.kube/config"),
Description: "Path to the kube config file, defaults to ~/.kube/config",
Type: schema.TypeString,
Optional: true,
DefaultFunc: schema.EnvDefaultFunc("KUBE_CONFIG_PATH", nil),
Description: "Path to the kube config file. Can be set with KUBE_CONFIG_PATH.",
ConflictsWith: []string{"config_paths"},
},
"config_context": {
Type: schema.TypeString,
Expand All @@ -101,12 +106,6 @@ func Provider() *schema.Provider {
DefaultFunc: schema.EnvDefaultFunc("KUBE_TOKEN", ""),
Description: "Token to authenticate an service account",
},
"load_config_file": {
Type: schema.TypeBool,
Optional: true,
DefaultFunc: schema.EnvDefaultFunc("KUBE_LOAD_CONFIG_FILE", true),
Description: "Load local kubeconfig.",
},
"exec": {
Type: schema.TypeList,
Optional: true,
Expand Down Expand Up @@ -204,12 +203,19 @@ type kubeClientsets struct {
config *restclient.Config
mainClientset *kubernetes.Clientset
aggregatorClientset *aggregator.Clientset

configData *schema.ResourceData
}

func (k kubeClientsets) MainClientset() (*kubernetes.Clientset, error) {
if k.mainClientset != nil {
return k.mainClientset, nil
}

if err := checkConfigurationValid(k.configData); err != nil {
return nil, err
}

if k.config != nil {
kc, err := kubernetes.NewForConfig(k.config)
if err != nil {
Expand All @@ -234,8 +240,53 @@ func (k kubeClientsets) AggregatorClientset() (*aggregator.Clientset, error) {
return k.aggregatorClientset, nil
}

func providerConfigure(ctx context.Context, d *schema.ResourceData, terraformVersion string) (interface{}, diag.Diagnostics) {
var apiTokenMountPath = "/var/run/secrets/kubernetes.io/serviceaccount"

func inCluster() bool {
host, port := os.Getenv("KUBERNETES_SERVICE_HOST"), os.Getenv("KUBERNETES_SERVICE_PORT")
if host == "" || port == "" {
return false
}

if _, err := os.Stat(apiTokenMountPath); err != nil {
return false
}
return true
}

var authDocumentationURL = "https://registry.terraform.io/providers/hashicorp/kubernetes/latest/docs#authentication"

func checkConfigurationValid(d *schema.ResourceData) error {
if inCluster() {
log.Printf("[DEBUG] Terraform appears to be running inside the Kubernetes cluster")
return nil
}

if os.Getenv("KUBE_CONFIG_PATHS") != "" {
return nil
}

atLeastOneOf := []string{
"host",
"config_path",
"config_paths",
"client_certificate",
"token",
"exec",
}
for _, a := range atLeastOneOf {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand this correctly, but it looks like having just "token" or "client_certificate" set would count as valid configuration. Not sure if I'm missing something else.

This could be confusing if not covering all combinations that make a valid configuration.

Also (not 100% sure) but it might be falsely triggered by progressive apply situations. Can we rule those out?

Copy link
Collaborator Author

@jrhouston jrhouston Dec 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The intent of this code is not to cover all valid possible configurations, it is to rule out that:

  1. The user has left the provider block is empty
  2. Terraform is running inside a cluster.

The purpose of this is so we can tell the user to explicitly configure instead of generating the confusing "could not connect" error at apply time when the client actually gets initialized. Once we fix this we should be able to generate more meaningful errors that come from client go, as it will warn you about these problems.

We can use ConflictsWith and RequiredWith to validate that the right combination of attributes is being used together, but I would propose doing that in another pull request.

Copy link
Member

@alexsomesan alexsomesan Dec 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good. I didn't catch those nuances while reading through it the first time. Let's try this approach out and see how it improves things. Looks like hashicorp/terraform#24055 is confirmed fixed so we're clear to move in that regard.
I agree with doing things incrementally. Let's re-evaluate after this is released.

if _, ok := d.GetOk(a); ok {
return nil
}
}

return fmt.Errorf(`provider not configured: you must configure a path to your kubeconfig
or explicitly supply credentials via the provider block or environment variables.

See our documentation at: %s`, authDocumentationURL)
}

func providerConfigure(ctx context.Context, d *schema.ResourceData, terraformVersion string) (interface{}, diag.Diagnostics) {
// Config initialization
cfg, err := initializeConfiguration(d)
if err != nil {
Expand All @@ -262,6 +313,7 @@ func providerConfigure(ctx context.Context, d *schema.ResourceData, terraformVer
config: cfg,
mainClientset: nil,
aggregatorClientset: nil,
configData: d,
}
return m, diag.Diagnostics{}
}
Expand All @@ -270,40 +322,64 @@ func initializeConfiguration(d *schema.ResourceData) (*restclient.Config, error)
overrides := &clientcmd.ConfigOverrides{}
loader := &clientcmd.ClientConfigLoadingRules{}

if d.Get("load_config_file").(bool) {
log.Printf("[DEBUG] Trying to load configuration from file")
if configPath, ok := d.GetOk("config_path"); ok && configPath.(string) != "" {
path, err := homedir.Expand(configPath.(string))
configPaths := []string{}

if v, ok := d.Get("config_path").(string); ok && v != "" {
configPaths = []string{v}
} else if v, ok := d.Get("config_paths").([]interface{}); ok && len(v) > 0 {
for _, p := range v {
configPaths = append(configPaths, p.(string))
}
} else if v := os.Getenv("KUBE_CONFIG_PATHS"); v != "" {
// NOTE we have to do this here because the schema
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's an open issue for this hashicorp/terraform-plugin-sdk#142

// does not yet allow you to set a default for a TypeList
configPaths = filepath.SplitList(v)
}

if len(configPaths) > 0 {
expandedPaths := []string{}
for _, p := range configPaths {
path, err := homedir.Expand(p)
if err != nil {
return nil, err
}
log.Printf("[DEBUG] Configuration file is: %s", path)
loader.ExplicitPath = path

ctxSuffix := "; default context"

kubectx, ctxOk := d.GetOk("config_context")
authInfo, authInfoOk := d.GetOk("config_context_auth_info")
cluster, clusterOk := d.GetOk("config_context_cluster")
if ctxOk || authInfoOk || clusterOk {
ctxSuffix = "; overriden context"
if ctxOk {
overrides.CurrentContext = kubectx.(string)
ctxSuffix += fmt.Sprintf("; config ctx: %s", overrides.CurrentContext)
log.Printf("[DEBUG] Using custom current context: %q", overrides.CurrentContext)
}

overrides.Context = clientcmdapi.Context{}
if authInfoOk {
overrides.Context.AuthInfo = authInfo.(string)
ctxSuffix += fmt.Sprintf("; auth_info: %s", overrides.Context.AuthInfo)
}
if clusterOk {
overrides.Context.Cluster = cluster.(string)
ctxSuffix += fmt.Sprintf("; cluster: %s", overrides.Context.Cluster)
}
log.Printf("[DEBUG] Using overidden context: %#v", overrides.Context)
if _, err := os.Stat(path); err != nil {
return nil, fmt.Errorf("could not open kubeconfig %q: %v", p, err)
Copy link
Member

Choose a reason for hiding this comment

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

Would this fail to configure the provider if a single file is missing?
IMO, we should consider all entries in the list in a best effort to find valid credentials.

Also, in case of multiple files present it could be argued that a valid CurrentContext should be present to eliminate ambiguities about which cluster would be chosen to connect to. Otherwise we might end up applying changes to an undesirable destination.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The behaviour of kubectl when using a KUBECONFIG with multiple paths is not to error if one of the paths does not exist. That being said I think if we're erring on the side of explicitness by not supporting KUBECONFIG anymore (and allowing this to be set as a list in the provider block) then we should also be more explicit here and tell the user that one of the paths they specified does not exist rather than silently skipping it.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good then. Let's go with this!
We might need to keep an eye out for corner cases where this list would be set but not all entries might be actually present in the file system (yet). It's a stretch and quite unlikely, but we've seen funny things before.

What do you think about requiring a CurrentContext to be set when this list has more than one entry? Just as a thought experiment for now. Doesn't need to be part of this PR.

}

log.Printf("[DEBUG] Using kubeconfig: %s", path)
expandedPaths = append(expandedPaths, path)
}

if len(expandedPaths) == 1 {
loader.ExplicitPath = expandedPaths[0]
} else {
loader.Precedence = expandedPaths
}

ctxSuffix := "; default context"

kubectx, ctxOk := d.GetOk("config_context")
authInfo, authInfoOk := d.GetOk("config_context_auth_info")
cluster, clusterOk := d.GetOk("config_context_cluster")
if ctxOk || authInfoOk || clusterOk {
ctxSuffix = "; overriden context"
if ctxOk {
overrides.CurrentContext = kubectx.(string)
ctxSuffix += fmt.Sprintf("; config ctx: %s", overrides.CurrentContext)
log.Printf("[DEBUG] Using custom current context: %q", overrides.CurrentContext)
}

overrides.Context = clientcmdapi.Context{}
if authInfoOk {
overrides.Context.AuthInfo = authInfo.(string)
ctxSuffix += fmt.Sprintf("; auth_info: %s", overrides.Context.AuthInfo)
}
if clusterOk {
overrides.Context.Cluster = cluster.(string)
ctxSuffix += fmt.Sprintf("; cluster: %s", overrides.Context.Cluster)
}
log.Printf("[DEBUG] Using overidden context: %#v", overrides.Context)
}
}

Expand Down Expand Up @@ -367,7 +443,6 @@ func initializeConfiguration(d *schema.ResourceData) (*restclient.Config, error)
return nil, nil
}

log.Printf("[INFO] Successfully initialized config")
return cfg, nil
}

Expand Down
Loading