Skip to content
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

Jump to bottom

Bug 1813949: ignore local env variables when we create a service client #4426

Merged
merged 1 commit into from Jan 15, 2021
Merged

Bug 1813949: ignore local env variables when we create a service client #4426

merged 1 commit into from Jan 15, 2021

Conversation

Fedosin
Copy link
Contributor

@Fedosin Fedosin commented Nov 27, 2020 •
edited

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.

@Fedosin
Copy link
Contributor Author

Fedosin commented Nov 27, 2020

/label platform/openstack

@mdbooth
Copy link
Contributor

mdbooth commented Nov 27, 2020

We need some library code for this. We're already doing the os.Unsetenv("OS_CLOUD") dance in 4 places:

To avoid inconsistent behaviour we're going to want the new hack in all those places, too. We should really pull this out somewhere. This would also allow us to unit test it.

@pierreprinetti
Copy link
Member

Is this a fix for https://bugzilla.redhat.com/show_bug.cgi?id=1813949 ?

@pierreprinetti
Copy link
Member

Can we apply this to all the steps (install-config generation, manifest creation, ignition... etc), and remove our various calls to unset OS_CLOUD?

@Fedosin Fedosin force-pushed the validate_clouds_yaml branch 2 times, most recently from 6e26d44 to 0822d42 Compare December 2, 2020 11:58
@Fedosin Fedosin changed the title OpenStack: ignore local env variables when we create a service client Bug 1813949: ignore local env variables when we create a service client Dec 2, 2020
@openshift-ci-robot openshift-ci-robot added bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Dec 2, 2020
@openshift-ci-robot
Copy link
Contributor

@Fedosin: This pull request references Bugzilla bug 1813949, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.7.0) matches configured target release for branch (4.7.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1813949: ignore local env variables when we create a service client

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mandre
mandre reviewed Dec 2, 2020
View reviewed changes
Copy link
Member

@mandre mandre left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2020
@mandre
Copy link
Member

mandre commented Dec 2, 2020

/retest

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2020
mdbooth
mdbooth reviewed Dec 2, 2020
View reviewed changes
pkg/asset/installconfig/openstack/validvaluesfetcher.go Outdated
conn, err := clientconfig.NewServiceClient("network", &clientconfig.ClientOpts{
Cloud: cloud,
})
conn, err := clientconfig.NewServiceClient("network", defaultValidValuesFetcherClientOpts(cloud))
Copy link
Contributor

Choose a reason for hiding this comment

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

Related, but not necessarily in this patch, I would refactor all of these functions to take a ProviderClient instead of a cloud name. The reason is you pull the code managing authentication options into a single place, and also you only hit keystone once rather than every time you create a new service client.

pkg/asset/installconfig/openstack/session.go
@@ -23,11 +22,6 @@ type Session struct {
func GetSession(cloudName string) (*Session, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to see every case where we instantiate an openstack client call this method.

pkg/asset/installconfig/openstack/validvaluesfetcher.go Outdated
// 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_"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is duplicating what we're already doing with defaultClientOpts(). I'd prefer we just called that.

@openshift-merge-robot
Copy link
Contributor

openshift-merge-robot commented Dec 2, 2020 •
edited

@Fedosin: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-fips fad78a3 link /test e2e-aws-fips
ci/prow/e2e-libvirt fad78a3 link /test e2e-libvirt
ci/prow/e2e-crc fad78a3 link /test e2e-crc
ci/prow/e2e-aws-workers-rhel7 fad78a3 link /test e2e-aws-workers-rhel7
ci/prow/e2e-aws fad78a3 link /test e2e-aws

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@EmilienM EmilienM mentioned this pull request Dec 2, 2020
Merged
EmilienM
EmilienM suggested changes Dec 8, 2020
View reviewed changes
Copy link
Member

@EmilienM EmilienM left a comment •
edited

Choose a reason for hiding this comment

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

Probably need some re factorization if possible.

@Fedosin Fedosin force-pushed the validate_clouds_yaml branch 2 times, most recently from 68b692f to 1938a23 Compare January 11, 2021 11:45
@pierreprinetti
Copy link
Member

/approve
/lgtm
/hold for CI

Note for self:
For Terraform, we already set OS_CLOUD explicitly here:

installer/data/data/openstack/main.tf

Line 4 in 4bd58eb

cloud = "${var.openstack_credentials_cloud}"

and an explicit value is supposed to override the env var default:

https://github.com/hashicorp/terraform-provider-openstack/blob/fc6a9169ef533cbaf9f914d44db92a36c4e0b8a3/openstack/provider.go#L223-L228

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 11, 2021
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2021
@Fedosin
Bug 1813949: ignore local env variables when we create a service client
05453ef 
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
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

25 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 0c58270 into openshift:master Jan 15, 2021
@openshift-ci-robot
Copy link
Contributor

@Fedosin: All pull requests linked via external trackers have merged:

Bugzilla bug 1813949 has been moved to the MODIFIED state.

In response to this:

Bug 1813949: ignore local env variables when we create a service client

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

2uasimojo added a commit to 2uasimojo/installer that referenced this pull request Mar 26, 2024
@2uasimojo
OSASINFRA-3421: OpenStack: Customizable clientOpts for CheckNetworkEx…
e181b1c 
…tensionAvailability

External consumers of `MachineSets()`, such as
[hive](https://github.com/openshift/hive/), need to be able to customize
the client that queries the OpenStack cloud for trunk support.

A prior commit (openshift#8187 / b99470c), eliminating what looked like tech
debt, removed that enablement, which was most recently added via openshift#4638 /
3c4235c, which was *itself* a revert of a previous commit (openshift#4426 /
05453ef) that had removed it similarly.

Here we reinstate the customizability, and include a docstring
explanation to hopefully prevent it being removed again.

OSASINFRA-3421
@2uasimojo 2uasimojo mentioned this pull request Mar 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mandre mandre mandre approved these changes

@mdbooth mdbooth mdbooth left review comments

@staebler staebler staebler left review comments

@EmilienM EmilienM EmilienM requested changes

@pierreprinetti pierreprinetti Awaiting requested review from pierreprinetti

Assignees

@mandre mandre

@pierreprinetti pierreprinetti

@staebler staebler

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. platform/openstack
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@Fedosin @mdbooth @pierreprinetti @openshift-ci-robot @mandre @openshift-merge-robot @Gal-Zaidman @openshift-bot @EmilienM @staebler