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.

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

Fix potential issue with frozen strings with openstack_endpoint_type #494

Merged
merged 1 commit into from
Jul 14, 2019

Conversation

ramereth
Copy link
Contributor

@ramereth ramereth commented Jul 3, 2019

It seems when using fog-openstack-1.x with Chef [1] , @openstack_endpoint_type
becomes a frozen string and fails with FrozenError. Unfortunately, I haven't
been able to reproduce this issue in unit tests but I know for a fact that this
fixes the issue. Looking through the code base, this is the only place where a
gsub! is used in this manner.

Further debugging shows this stack trace which lead me to this:

DEBUG: FrozenError: openstack_role[service] (openstack-identity::registration line 70) had an error: FrozenError: can't modify frozen String
/opt/chef/embedded/lib/ruby/gems/2.5.0/gems/fog-openstack-1.0.8/lib/fog/openstack/core.rb:200:in `gsub!'
/opt/chef/embedded/lib/ruby/gems/2.5.0/gems/fog-openstack-1.0.8/lib/fog/openstack/core.rb:200:in `setup'
/opt/chef/embedded/lib/ruby/gems/2.5.0/gems/fog-openstack-1.0.8/lib/fog/openstack/core.rb:43:in `initialize'
/opt/chef/embedded/lib/ruby/gems/2.5.0/gems/fog-core-2.1.2/lib/fog/core/service.rb:115:in `new'
/opt/chef/embedded/lib/ruby/gems/2.5.0/gems/fog-core-2.1.2/lib/fog/core/service.rb:115:in `new'
/tmp/kitchen/cache/cookbooks/openstackclient/libraries/openstack_base.rb:22:in `connection'
/tmp/kitchen/cache/cookbooks/openstackclient/libraries/openstack_role.rb:28:in `block in <class:OpenstackRole>' (eval):2:in `block in action_create'

[1] https://review.opendev.org/665827

It seems when using fog-openstack-1.x with Chef [1] , @openstack_endpoint_type
becomes a frozen string and fails with FrozenError. Unfortunately, I haven't
been able to reproduce this issue in unit tests but I know for a fact that this
fixes the issue. Looking through the code base, this is the only place where a
``gsub!`` is used in this manner.

Further debugging shows this stack trace which lead me to this:
```
DEBUG: FrozenError: openstack_role[service] (openstack-identity::registration line 70) had an error: FrozenError: can't modify frozen String
/opt/chef/embedded/lib/ruby/gems/2.5.0/gems/fog-openstack-1.0.8/lib/fog/openstack/core.rb:200:in `gsub!'
/opt/chef/embedded/lib/ruby/gems/2.5.0/gems/fog-openstack-1.0.8/lib/fog/openstack/core.rb:200:in `setup'
/opt/chef/embedded/lib/ruby/gems/2.5.0/gems/fog-openstack-1.0.8/lib/fog/openstack/core.rb:43:in `initialize'
/opt/chef/embedded/lib/ruby/gems/2.5.0/gems/fog-core-2.1.2/lib/fog/core/service.rb:115:in `new'
/opt/chef/embedded/lib/ruby/gems/2.5.0/gems/fog-core-2.1.2/lib/fog/core/service.rb:115:in `new'
/tmp/kitchen/cache/cookbooks/openstackclient/libraries/openstack_base.rb:22:in `connection'
/tmp/kitchen/cache/cookbooks/openstackclient/libraries/openstack_role.rb:28:in `block in <class:OpenstackRole>' (eval):2:in `block in action_create'
```

[1] https://review.opendev.org/665827
@theopenlab-ci
Copy link

theopenlab-ci bot commented Jul 3, 2019

Build succeeded.

@@ -198,7 +198,7 @@ def setup(options)

@openstack_service_type = options[:openstack_service_type] || default_service_type
@openstack_endpoint_type = options[:openstack_endpoint_type] || default_endpoint_type
Copy link

Choose a reason for hiding this comment

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

This still looks redundant, we should also drop the duplicate assignment in L177.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering the same thing while looking into the code. Shall I go ahead and remove it?

openstack-gerrit pushed a commit to openstack-archive/cookbook-openstack-identity that referenced this pull request Jul 5, 2019
fog-openstack-1.x already appends "auth/tokens" so we no longer need to
do that.  In addition, comment out endpoint type until this PR [1] gets
merged and released.

[1] fog/fog-openstack#494

Depends-On: https://review.opendev.org/666176
Change-Id: I2a73e87648bff58180c6ee2355a733a8e030fa4b
Signed-off-by: Lance Albertson <lance@osuosl.org>
openstack-gerrit pushed a commit to openstack/openstack that referenced this pull request Jul 5, 2019
* Update cookbook-openstack-identity from branch 'master'
  - Merge "Fixes to support fog-openstack-1.x"
  - Fixes to support fog-openstack-1.x
    
    fog-openstack-1.x already appends "auth/tokens" so we no longer need to
    do that.  In addition, comment out endpoint type until this PR [1] gets
    merged and released.
    
    [1] fog/fog-openstack#494
    
    Depends-On: https://review.opendev.org/666176
    Change-Id: I2a73e87648bff58180c6ee2355a733a8e030fa4b
    Signed-off-by: Lance Albertson <lance@osuosl.org>
openstack-gerrit pushed a commit to openstack-archive/cookbook-openstack-compute that referenced this pull request Jul 5, 2019
fog-openstack-1.x already appends "auth/tokens" so we no longer need to
do that.  In addition, comment out endpoint type until this PR [1] gets
merged and released.

[1] fog/fog-openstack#494

Depends-On: https://review.opendev.org/666176
Change-Id: I99dceff452695302865d267ca4cd5e8aa094ead5
Signed-off-by: Lance Albertson <lance@osuosl.org>
openstack-gerrit pushed a commit to openstack/openstack that referenced this pull request Jul 5, 2019
* Update cookbook-openstack-compute from branch 'master'
  - Fixes to support fog-openstack-1.x
    
    fog-openstack-1.x already appends "auth/tokens" so we no longer need to
    do that.  In addition, comment out endpoint type until this PR [1] gets
    merged and released.
    
    [1] fog/fog-openstack#494
    
    Depends-On: https://review.opendev.org/666176
    Change-Id: I99dceff452695302865d267ca4cd5e8aa094ead5
    Signed-off-by: Lance Albertson <lance@osuosl.org>
openstack-gerrit pushed a commit to openstack-archive/cookbook-openstack-image that referenced this pull request Jul 5, 2019
fog-openstack-1.x already appends "auth/tokens" so we no longer need to
do that.  In addition, comment out endpoint type until this PR [1] gets
merged and released.

[1] fog/fog-openstack#494

Depends-On: https://review.opendev.org/666176
Change-Id: Ic80d08b6ed0180ca9513e4b4e230e787b2f5c52e
Signed-off-by: Lance Albertson <lance@osuosl.org>
openstack-gerrit pushed a commit to openstack/openstack that referenced this pull request Jul 5, 2019
* Update cookbook-openstack-image from branch 'master'
  - Fixes to support fog-openstack-1.x
    
    fog-openstack-1.x already appends "auth/tokens" so we no longer need to
    do that.  In addition, comment out endpoint type until this PR [1] gets
    merged and released.
    
    [1] fog/fog-openstack#494
    
    Depends-On: https://review.opendev.org/666176
    Change-Id: Ic80d08b6ed0180ca9513e4b4e230e787b2f5c52e
    Signed-off-by: Lance Albertson <lance@osuosl.org>
openstack-gerrit pushed a commit to openstack-archive/cookbook-openstack-integration-test that referenced this pull request Jul 5, 2019
fog-openstack-1.x already appends "auth/tokens" so we no longer need to
do that.  In addition, comment out endpoint type until this PR [1] gets
merged and released.

[1] fog/fog-openstack#494

Depends-On: https://review.opendev.org/666176
Change-Id: I929722b032e37970fe74018e609b76c1682e0db9
Signed-off-by: Lance Albertson <lance@osuosl.org>
openstack-gerrit pushed a commit to openstack/openstack that referenced this pull request Jul 5, 2019
* Update cookbook-openstack-integration-test from branch 'master'
  - Fixes to support fog-openstack-1.x
    
    fog-openstack-1.x already appends "auth/tokens" so we no longer need to
    do that.  In addition, comment out endpoint type until this PR [1] gets
    merged and released.
    
    [1] fog/fog-openstack#494
    
    Depends-On: https://review.opendev.org/666176
    Change-Id: I929722b032e37970fe74018e609b76c1682e0db9
    Signed-off-by: Lance Albertson <lance@osuosl.org>
openstack-gerrit pushed a commit to openstack/openstack that referenced this pull request Jul 5, 2019
* Update cookbook-openstack-network from branch 'master'
  - Fixes to support fog-openstack-1.x
    
    fog-openstack-1.x already appends "auth/tokens" so we no longer need to
    do that.  In addition, comment out endpoint type until this PR [1] gets
    merged and released.
    
    [1] fog/fog-openstack#494
    
    Depends-On: https://review.opendev.org/666176
    Change-Id: Icc92d6c8eb868efbde904e6aeb9d36891ea8457a
    Signed-off-by: Lance Albertson <lance@osuosl.org>
openstack-gerrit pushed a commit to openstack-archive/cookbook-openstack-network that referenced this pull request Jul 5, 2019
fog-openstack-1.x already appends "auth/tokens" so we no longer need to
do that.  In addition, comment out endpoint type until this PR [1] gets
merged and released.

[1] fog/fog-openstack#494

Depends-On: https://review.opendev.org/666176
Change-Id: Icc92d6c8eb868efbde904e6aeb9d36891ea8457a
Signed-off-by: Lance Albertson <lance@osuosl.org>
openstack-gerrit pushed a commit to openstack-archive/cookbook-openstack-bare-metal that referenced this pull request Jul 5, 2019
fog-openstack-1.x already appends "auth/tokens" so we no longer need to
do that.  In addition, comment out endpoint type until this PR [1] gets
merged and released.

[1] fog/fog-openstack#494

Depends-On: https://review.opendev.org/666176
Change-Id: I068e7b85a052d6bd3f03e96878e67e8b1c3a974f
Signed-off-by: Lance Albertson <lance@osuosl.org>
openstack-gerrit pushed a commit to openstack/openstack that referenced this pull request Jul 5, 2019
* Update cookbook-openstack-bare-metal from branch 'master'
  - Fixes to support fog-openstack-1.x
    
    fog-openstack-1.x already appends "auth/tokens" so we no longer need to
    do that.  In addition, comment out endpoint type until this PR [1] gets
    merged and released.
    
    [1] fog/fog-openstack#494
    
    Depends-On: https://review.opendev.org/666176
    Change-Id: I068e7b85a052d6bd3f03e96878e67e8b1c3a974f
    Signed-off-by: Lance Albertson <lance@osuosl.org>
@ramereth
Copy link
Contributor Author

Please review @dhague, @Ladas, @seanhandley, @mdarby and @jjasghar

@seanhandley
Copy link
Member

Hi @ramereth

Thanks for the PR! I'd like to understand a little more about this issue.

First of all, how is the string becoming frozen? Is something in chef freezing it?

Second, I don't quite see how your code change would fix the offending behaviour. Can you help me understand why this change helps?

@ramereth
Copy link
Contributor Author

@seanhandley I'm not entirely sure what's causing the string to become frozen but I did some debugging and did confirm that when I run it within chef as a library, it returns as a frozen string. I could consult with some other Chef folks if you would like.

Regarding how this code changes the offending behavior, Using gsub! means the string variable gets changed in place which is a problem if the variable is frozen. To work around that, I use gsub instead which returns the changed string and I simply just copy the result back into the same variable.

Copy link
Member

@seanhandley seanhandley left a comment

Choose a reason for hiding this comment

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

Thanks for that @ramereth.

Regarding raising it with the Chef folks - I'm not sure this qualifies as a Chef bug, per se, since a String object passed to Fog could be frozen and that's a legitimate thing to do.

It's taken for granted in most libraries that a user-supplied string will be mutable... clearly that's not a good idea and something I hadn't given any thought to.

Thanks again for the PR!

@seanhandley seanhandley merged commit 69b817f into fog:master Jul 14, 2019
openstack-gerrit pushed a commit to openstack-archive/cookbook-openstackclient that referenced this pull request Oct 14, 2019
The new version contains the fix for the issue we were having with
specifying the endpoint_type for connections [0].

[0} fog/fog-openstack#494

Change-Id: I5d2c4d5c61256dcd7d00dc3007df966b350b66b3
openstack-gerrit pushed a commit to openstack/openstack that referenced this pull request Oct 14, 2019
* Update cookbook-openstackclient from branch 'master'
  - Bump version of fog-openstack gem
    
    The new version contains the fix for the issue we were having with
    specifying the endpoint_type for connections [0].
    
    [0} fog/fog-openstack#494
    
    Change-Id: I5d2c4d5c61256dcd7d00dc3007df966b350b66b3
ramereth added a commit to osuosl-cookbooks/cookbook-openstack-telemetry that referenced this pull request Oct 14, 2019
fog-openstack-1.x already appends "auth/tokens" so we no longer need to
do that.

In addition, re-add openstack_endpoint_type which is now fixed now that this [1]
has been released upstream.

[1] fog/fog-openstack#494

Change-Id: I06ee3681ff8bc90f15b2b0ecbdcaebe475709b31
Signed-off-by: Lance Albertson <lance@osuosl.org>
openstack-gerrit pushed a commit to openstack/openstack that referenced this pull request Nov 5, 2019
* Update cookbook-openstack-telemetry from branch 'master'
  - Fixes to support fog-openstack-1.x
    
    fog-openstack-1.x already appends "auth/tokens" so we no longer need to
    do that.
    
    In addition, re-add openstack_endpoint_type which is now fixed now that this [1]
    has been released upstream.
    
    [1] fog/fog-openstack#494
    
    Change-Id: I06ee3681ff8bc90f15b2b0ecbdcaebe475709b31
    Signed-off-by: Lance Albertson <lance@osuosl.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants