From 05453ef0dfbd03f28464b2e8a9c31046818003a7 Mon Sep 17 00:00:00 2001 From: Mike Fedosin Date: Fri, 27 Nov 2020 14:01:25 +0100 Subject: [PATCH] Bug 1813949: ignore local env variables when we create a service client This commit explicitly disables reading auth data from env variables by setting an invalid EnvPrefix. By doing this, we make sure that the data from clouds.yaml is enough to authenticate. After this change we don't have to unset OS_CLOUD env variable explicitly anymore. Ref https://issues.redhat.com/browse/OSASINFRA-2152 --- pkg/asset/installconfig/openstack/openstack.go | 6 ------ pkg/asset/installconfig/openstack/session.go | 17 +++-------------- .../openstack/validation/cloudinfo.go | 9 ++------- .../openstack/validvaluesfetcher.go | 14 +++++--------- pkg/asset/machines/openstack/machines.go | 12 ++++-------- pkg/asset/machines/openstack/machinesets.go | 5 ++--- pkg/asset/machines/worker.go | 3 +-- pkg/destroy/openstack/glance.go | 8 +++----- pkg/destroy/openstack/openstack.go | 11 ++--------- pkg/tfvars/openstack/bootstrap_ignition.go | 12 +++--------- pkg/tfvars/openstack/openstack.go | 13 +++---------- pkg/tfvars/openstack/rhcos_image.go | 14 ++++---------- pkg/types/openstack/defaults/clientopts.go | 16 ++++++++++++++++ 13 files changed, 48 insertions(+), 92 deletions(-) create mode 100644 pkg/types/openstack/defaults/clientopts.go diff --git a/pkg/asset/installconfig/openstack/openstack.go b/pkg/asset/installconfig/openstack/openstack.go index 098b6802276..bde9aee1582 100644 --- a/pkg/asset/installconfig/openstack/openstack.go +++ b/pkg/asset/installconfig/openstack/openstack.go @@ -2,7 +2,6 @@ package openstack import ( - "os" "sort" "strings" @@ -46,11 +45,6 @@ func Platform() (*openstack.Platform, error) { return nil, errors.Wrap(err, "failed UserInput") } - // We should unset OS_CLOUD env variable here, because the real cloud name was defined - // on the previous step. OS_CLOUD has more priority, so the value from "cloud" variable - // will be ignored if OS_CLOUD contains something. - os.Unsetenv("OS_CLOUD") - networkNames, err := getExternalNetworkNames(cloud) if err != nil { return nil, err diff --git a/pkg/asset/installconfig/openstack/session.go b/pkg/asset/installconfig/openstack/session.go index 9388ef5ed28..16a27338081 100644 --- a/pkg/asset/installconfig/openstack/session.go +++ b/pkg/asset/installconfig/openstack/session.go @@ -2,9 +2,9 @@ package openstack import ( - "os" "sync" + openstackdefaults "github.com/openshift/installer/pkg/types/openstack/defaults" "github.com/pkg/errors" "github.com/ghodss/yaml" @@ -21,12 +21,8 @@ type Session struct { // GetSession returns an OpenStack session for a given cloud name in clouds.yaml. func GetSession(cloudName string) (*Session, error) { - opts := defaultClientOpts(cloudName) - - // We should unset OS_CLOUD env variable here, because the real cloud name was - // defined on the previous step. OS_CLOUD has more priority, so the value from - // "opts" variable will be ignored if OS_CLOUD contains something. - os.Unsetenv("OS_CLOUD") + opts := openstackdefaults.DefaultClientOpts(cloudName) + opts.YAMLOpts = new(yamlLoadOpts) cloudConfig, err := clientconfig.GetCloudFromYAML(opts) if err != nil { @@ -37,13 +33,6 @@ func GetSession(cloudName string) (*Session, error) { }, nil } -func defaultClientOpts(cloudName string) *clientconfig.ClientOpts { - opts := new(clientconfig.ClientOpts) - opts.Cloud = cloudName - opts.YAMLOpts = new(yamlLoadOpts) - return opts -} - type yamlLoadOpts struct{} func (opts yamlLoadOpts) LoadCloudsYAML() (map[string]clientconfig.Cloud, error) { diff --git a/pkg/asset/installconfig/openstack/validation/cloudinfo.go b/pkg/asset/installconfig/openstack/validation/cloudinfo.go index eaffff7836f..08de4d7b761 100644 --- a/pkg/asset/installconfig/openstack/validation/cloudinfo.go +++ b/pkg/asset/installconfig/openstack/validation/cloudinfo.go @@ -2,7 +2,6 @@ package validation import ( "net/url" - "os" "strings" "github.com/gophercloud/gophercloud" @@ -24,6 +23,7 @@ import ( "github.com/openshift/installer/pkg/quota" "github.com/openshift/installer/pkg/types" + openstackdefaults "github.com/openshift/installer/pkg/types/openstack/defaults" ) // CloudInfo caches data fetched from the user's openstack cloud @@ -68,12 +68,7 @@ func GetCloudInfo(ic *types.InstallConfig) (*CloudInfo, error) { Flavors: map[string]Flavor{}, } - opts := &clientconfig.ClientOpts{Cloud: ic.OpenStack.Cloud} - - // We should unset OS_CLOUD env variable here, because the real cloud name was - // defined on the previous step. OS_CLOUD has more priority, so the value from - // "opts" variable will be ignored if OS_CLOUD contains something. - os.Unsetenv("OS_CLOUD") + opts := openstackdefaults.DefaultClientOpts(ic.OpenStack.Cloud) ci.clients.networkClient, err = clientconfig.NewServiceClient("network", opts) if err != nil { diff --git a/pkg/asset/installconfig/openstack/validvaluesfetcher.go b/pkg/asset/installconfig/openstack/validvaluesfetcher.go index 3c92debfe85..1c75b3f2ea4 100644 --- a/pkg/asset/installconfig/openstack/validvaluesfetcher.go +++ b/pkg/asset/installconfig/openstack/validvaluesfetcher.go @@ -9,6 +9,8 @@ import ( "github.com/gophercloud/gophercloud/openstack/networking/v2/networks" "github.com/gophercloud/utils/openstack/clientconfig" networkutils "github.com/gophercloud/utils/openstack/networking/v2/networks" + + openstackdefaults "github.com/openshift/installer/pkg/types/openstack/defaults" ) // getCloudNames gets the valid cloud names. These are read from clouds.yaml. @@ -28,9 +30,7 @@ func getCloudNames() ([]string, error) { // getExternalNetworkNames interrogates OpenStack to get the external network // names. func getExternalNetworkNames(cloud string) ([]string, error) { - conn, err := clientconfig.NewServiceClient("network", &clientconfig.ClientOpts{ - Cloud: cloud, - }) + conn, err := clientconfig.NewServiceClient("network", openstackdefaults.DefaultClientOpts(cloud)) if err != nil { return nil, err } @@ -61,9 +61,7 @@ func getExternalNetworkNames(cloud string) ([]string, error) { // getFlavorNames gets a list of valid flavor names. func getFlavorNames(cloud string) ([]string, error) { - conn, err := clientconfig.NewServiceClient("compute", &clientconfig.ClientOpts{ - Cloud: cloud, - }) + conn, err := clientconfig.NewServiceClient("compute", openstackdefaults.DefaultClientOpts(cloud)) if err != nil { return nil, err } @@ -91,9 +89,7 @@ func getFlavorNames(cloud string) ([]string, error) { return flavorNames, nil } func getFloatingIPNames(cloud string, floatingNetworkName string) ([]string, error) { - conn, err := clientconfig.NewServiceClient("network", &clientconfig.ClientOpts{ - Cloud: cloud, - }) + conn, err := clientconfig.NewServiceClient("network", openstackdefaults.DefaultClientOpts(cloud)) if err != nil { return nil, err } diff --git a/pkg/asset/machines/openstack/machines.go b/pkg/asset/machines/openstack/machines.go index fa10fe062fc..0678db41cad 100644 --- a/pkg/asset/machines/openstack/machines.go +++ b/pkg/asset/machines/openstack/machines.go @@ -15,6 +15,7 @@ import ( "github.com/openshift/installer/pkg/types" "github.com/openshift/installer/pkg/types/openstack" + openstackdefaults "github.com/openshift/installer/pkg/types/openstack/defaults" ) const ( @@ -40,7 +41,7 @@ func Machines(clusterID string, config *types.InstallConfig, pool *types.Machine mpool := pool.Platform.OpenStack platform := config.Platform.OpenStack - trunkSupport, err := checkNetworkExtensionAvailability(platform.Cloud, "trunk", nil) + trunkSupport, err := checkNetworkExtensionAvailability(platform.Cloud, "trunk") if err != nil { return nil, err } @@ -175,13 +176,8 @@ func generateProvider(clusterID string, platform *openstack.Platform, mpool *ope return &spec, nil } -func checkNetworkExtensionAvailability(cloud, alias string, opts *clientconfig.ClientOpts) (bool, error) { - if opts == nil { - opts = &clientconfig.ClientOpts{} - } - opts.Cloud = cloud - - conn, err := clientconfig.NewServiceClient("network", opts) +func checkNetworkExtensionAvailability(cloud, alias string) (bool, error) { + conn, err := clientconfig.NewServiceClient("network", openstackdefaults.DefaultClientOpts(cloud)) if err != nil { return false, err } diff --git a/pkg/asset/machines/openstack/machinesets.go b/pkg/asset/machines/openstack/machinesets.go index c7d5e0edbb9..5ed9dc8dd5e 100644 --- a/pkg/asset/machines/openstack/machinesets.go +++ b/pkg/asset/machines/openstack/machinesets.go @@ -4,7 +4,6 @@ package openstack import ( "fmt" - "github.com/gophercloud/utils/openstack/clientconfig" clusterapi "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -14,7 +13,7 @@ import ( ) // MachineSets returns a list of machinesets for a machinepool. -func MachineSets(clusterID string, config *types.InstallConfig, pool *types.MachinePool, osImage, role, userDataSecret string, clientOpts *clientconfig.ClientOpts) ([]*clusterapi.MachineSet, error) { +func MachineSets(clusterID string, config *types.InstallConfig, pool *types.MachinePool, osImage, role, userDataSecret string) ([]*clusterapi.MachineSet, error) { if configPlatform := config.Platform.Name(); configPlatform != openstack.Name { return nil, fmt.Errorf("non-OpenStack configuration: %q", configPlatform) } @@ -23,7 +22,7 @@ func MachineSets(clusterID string, config *types.InstallConfig, pool *types.Mach } platform := config.Platform.OpenStack mpool := pool.Platform.OpenStack - trunkSupport, err := checkNetworkExtensionAvailability(platform.Cloud, "trunk", clientOpts) + trunkSupport, err := checkNetworkExtensionAvailability(platform.Cloud, "trunk") if err != nil { return nil, err } diff --git a/pkg/asset/machines/worker.go b/pkg/asset/machines/worker.go index 968b409049e..932e6296c8a 100644 --- a/pkg/asset/machines/worker.go +++ b/pkg/asset/machines/worker.go @@ -8,7 +8,6 @@ import ( "strings" "github.com/ghodss/yaml" - openstackclientconfig "github.com/gophercloud/utils/openstack/clientconfig" baremetalapi "github.com/metal3-io/cluster-api-provider-baremetal/pkg/apis" baremetalprovider "github.com/metal3-io/cluster-api-provider-baremetal/pkg/apis/baremetal/v1alpha1" gcpapi "github.com/openshift/cluster-api-provider-gcp/pkg/apis" @@ -364,7 +363,7 @@ func (w *Worker) Generate(dependencies asset.Parents) error { imageName, _ := rhcosutils.GenerateOpenStackImageName(string(*rhcosImage), clusterID.InfraID) - sets, err := openstack.MachineSets(clusterID.InfraID, ic, &pool, imageName, "worker", "worker-user-data", &openstackclientconfig.ClientOpts{}) + sets, err := openstack.MachineSets(clusterID.InfraID, ic, &pool, imageName, "worker", "worker-user-data") if err != nil { return errors.Wrap(err, "failed to create worker machine objects") } diff --git a/pkg/destroy/openstack/glance.go b/pkg/destroy/openstack/glance.go index fe510d9364f..594bb3b8570 100644 --- a/pkg/destroy/openstack/glance.go +++ b/pkg/destroy/openstack/glance.go @@ -8,6 +8,8 @@ import ( "github.com/pkg/errors" "github.com/sirupsen/logrus" + openstackdefaults "github.com/openshift/installer/pkg/types/openstack/defaults" + "k8s.io/apimachinery/pkg/util/wait" ) @@ -29,11 +31,7 @@ func DeleteGlanceImage(name string, cloud string) error { } func deleteGlanceImage(name string, cloud string) (bool, error) { - opts := clientconfig.ClientOpts{ - Cloud: cloud, - } - - conn, err := clientconfig.NewServiceClient("image", &opts) + conn, err := clientconfig.NewServiceClient("image", openstackdefaults.DefaultClientOpts(cloud)) if err != nil { logrus.Warningf("There was an error during the image removal: %v", err) return false, nil diff --git a/pkg/destroy/openstack/openstack.go b/pkg/destroy/openstack/openstack.go index c5e36197e97..f47b8c3f9e4 100644 --- a/pkg/destroy/openstack/openstack.go +++ b/pkg/destroy/openstack/openstack.go @@ -1,12 +1,12 @@ package openstack import ( - "os" "strings" "time" "github.com/openshift/installer/pkg/destroy/providers" "github.com/openshift/installer/pkg/types" + openstackdefaults "github.com/openshift/installer/pkg/types/openstack/defaults" "github.com/gophercloud/gophercloud" "github.com/gophercloud/gophercloud/openstack/blockstorage/v3/volumes" @@ -101,14 +101,7 @@ func (o *ClusterUninstaller) Run() error { } returnChannel := make(chan string) - opts := &clientconfig.ClientOpts{ - Cloud: o.Cloud, - } - - // We should unset OS_CLOUD env variable here, because the real cloud name was - // defined on the previous step. OS_CLOUD has more priority, so the value from - // "opts" variable will be ignored if OS_CLOUD contains something. - os.Unsetenv("OS_CLOUD") + opts := openstackdefaults.DefaultClientOpts(o.Cloud) // launch goroutines for name, function := range deleteFuncs { diff --git a/pkg/tfvars/openstack/bootstrap_ignition.go b/pkg/tfvars/openstack/bootstrap_ignition.go index 998b205f309..13174bcb05e 100644 --- a/pkg/tfvars/openstack/bootstrap_ignition.go +++ b/pkg/tfvars/openstack/bootstrap_ignition.go @@ -16,6 +16,7 @@ import ( "github.com/vincent-petithory/dataurl" "github.com/openshift/installer/pkg/asset/ignition" + openstackdefaults "github.com/openshift/installer/pkg/types/openstack/defaults" ) // Starting from OpenShift 4.4 we store bootstrap Ignition configs in Glance. @@ -23,11 +24,8 @@ import ( // uploadBootstrapConfig uploads the bootstrap Ignition config in Glance and returns its location func uploadBootstrapConfig(cloud string, bootstrapIgn string, clusterID string) (string, error) { logrus.Debugln("Creating a Glance image for your bootstrap ignition config...") - opts := clientconfig.ClientOpts{ - Cloud: cloud, - } - conn, err := clientconfig.NewServiceClient("image", &opts) + conn, err := clientconfig.NewServiceClient("image", openstackdefaults.DefaultClientOpts(cloud)) if err != nil { return "", err } @@ -179,11 +177,7 @@ func generateIgnitionShim(userCA string, clusterID string, bootstrapConfigURL st // getAuthToken fetches valid OpenStack authentication token ID func getAuthToken(cloud string) (string, error) { - opts := &clientconfig.ClientOpts{ - Cloud: cloud, - } - - conn, err := clientconfig.NewServiceClient("identity", opts) + conn, err := clientconfig.NewServiceClient("identity", openstackdefaults.DefaultClientOpts(cloud)) if err != nil { return "", err } diff --git a/pkg/tfvars/openstack/openstack.go b/pkg/tfvars/openstack/openstack.go index 54bb0b46aba..03957be7968 100644 --- a/pkg/tfvars/openstack/openstack.go +++ b/pkg/tfvars/openstack/openstack.go @@ -15,6 +15,7 @@ import ( "github.com/openshift/installer/pkg/rhcos" "github.com/openshift/installer/pkg/tfvars/internal/cache" types_openstack "github.com/openshift/installer/pkg/types/openstack" + openstackdefaults "github.com/openshift/installer/pkg/types/openstack/defaults" "github.com/pkg/errors" "sigs.k8s.io/cluster-api-provider-openstack/pkg/apis/openstackproviderconfig/v1alpha1" @@ -197,11 +198,7 @@ func getGlancePublicURL(serviceCatalog *tokens.ServiceCatalog) (string, error) { // getServiceCatalog fetches OpenStack service catalog with service endpoints func getServiceCatalog(cloud string) (*tokens.ServiceCatalog, error) { - opts := &clientconfig.ClientOpts{ - Cloud: cloud, - } - - conn, err := clientconfig.NewServiceClient("identity", opts) + conn, err := clientconfig.NewServiceClient("identity", openstackdefaults.DefaultClientOpts(cloud)) if err != nil { return nil, err } @@ -222,11 +219,7 @@ func getServiceCatalog(cloud string) (*tokens.ServiceCatalog, error) { // getNetworkFromSubnet looks up a subnet in openstack and returns the ID of the network it's a part of func getNetworkFromSubnet(cloud string, subnetID string) (string, error) { - opts := &clientconfig.ClientOpts{ - Cloud: cloud, - } - - networkClient, err := clientconfig.NewServiceClient("network", opts) + networkClient, err := clientconfig.NewServiceClient("network", openstackdefaults.DefaultClientOpts(cloud)) if err != nil { return "", err } diff --git a/pkg/tfvars/openstack/rhcos_image.go b/pkg/tfvars/openstack/rhcos_image.go index 8b013a22348..0ecbc0678c9 100644 --- a/pkg/tfvars/openstack/rhcos_image.go +++ b/pkg/tfvars/openstack/rhcos_image.go @@ -13,6 +13,8 @@ import ( "github.com/gophercloud/utils/openstack/clientconfig" "github.com/pkg/errors" "github.com/sirupsen/logrus" + + openstackdefaults "github.com/openshift/installer/pkg/types/openstack/defaults" ) // uploadBaseImage creates a new image in Glance and uploads the RHCOS image there @@ -25,11 +27,7 @@ func uploadBaseImage(cloud string, localFilePath string, imageName string, clust } defer f.Close() - opts := clientconfig.ClientOpts{ - Cloud: cloud, - } - - conn, err := clientconfig.NewServiceClient("image", &opts) + conn, err := clientconfig.NewServiceClient("image", openstackdefaults.DefaultClientOpts(cloud)) if err != nil { return err } @@ -125,11 +123,7 @@ func isImageImportSupported(cloud string) (bool, error) { // https://docs.openstack.org/api-ref/image/v2/?expanded=#image-service-info-discovery logrus.Debugln("Checking if the image import mechanism is supported") - opts := clientconfig.ClientOpts{ - Cloud: cloud, - } - - conn, err := clientconfig.NewServiceClient("image", &opts) + conn, err := clientconfig.NewServiceClient("image", openstackdefaults.DefaultClientOpts(cloud)) if err != nil { return false, err } diff --git a/pkg/types/openstack/defaults/clientopts.go b/pkg/types/openstack/defaults/clientopts.go new file mode 100644 index 00000000000..fb64632755b --- /dev/null +++ b/pkg/types/openstack/defaults/clientopts.go @@ -0,0 +1,16 @@ +package defaults + +import ( + "github.com/gophercloud/utils/openstack/clientconfig" +) + +// DefaultClientOpts generates default client opts based on cloud name +func DefaultClientOpts(cloudName string) *clientconfig.ClientOpts { + opts := new(clientconfig.ClientOpts) + opts.Cloud = cloudName + // We explicitly disable reading auth data from env variables by setting an invalid EnvPrefix. + // By doing this, we make sure that the data from clouds.yaml is enough to authenticate. + // For more information: https://github.com/gophercloud/utils/blob/8677e053dcf1f05d0fa0a616094aace04690eb94/openstack/clientconfig/requests.go#L508 + opts.EnvPrefix = "NO_ENV_VARIABLES_" + return opts +}