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

Proxy config affecting change events #1244

Merged
merged 3 commits into from Sep 28, 2019

Conversation

guicassolato
Copy link
Contributor

@guicassolato guicassolato commented Sep 25, 2019

What this PR does / why we need it:

It defines a new event to be issued whenever an object that is part of a proxy config changes (or gets created, or destroyed). This event will asynchronously touch a tracking record in a separate table, also introduced, that holds a 1-1 relationship with proxy.

Which issue(s) this PR fixes

This will make possible for the UI to highlight for the user a potential outdated proxy config, thus being relevant to #1235.

Closes THREESCALE-3555

TODOs

  • Create table to store the change history on proxy config affecting objects
  • Generate the schema for Oracle and PostgreSQL
  • Define a new type of event to be issued whenever a proxy config affecting change occurs (plus subscriber and worker)
  • Issue the event on every model of proxy config affecting object (in a after_commit, for each proxy potentially affected)
    • ProxyRule (Metric deletion will kick ProxyRule deletion, so already covered)
    • Policy
    • Proxy (Maybe only a few attributes – policies_config)
    • GatewayConfiguration

Special notes for your reviewer
Not sure if we should issue one event per proxy (passing proxy and affecting object) or just one event per change, passing only the affecting object and let the creation of the event to handle the logics of triggering one background job per proxy. In the latter case, the event would have to be aware of the different types of affecting objects (so it can get to the proxies).

To sum up (very roughly exemplifying):

Option A

# where the event is triggered (many places in the code) 
Event.new(object)

# what the event does
for each proxy using object
  Notifier.new(proxy, object)

Option B

# where the event is triggered (many places in the code) 
for each proxy using object
  Event.new(proxy, object)

# what the event does
Notifier.new(proxy, object)

Update: going with Option B.


I won't tag this as APIAP-only because I think this can be used for non-APIAP accounts as well.

@guicassolato guicassolato self-assigned this Sep 25, 2019
app/models/proxy.rb Outdated Show resolved Hide resolved
@guicassolato guicassolato force-pushed the apiap/track-proxy-config-changes branch 2 times, most recently from 1ecfa29 to 9fc9a4d Compare September 25, 2019 18:19
@Martouta Martouta self-requested a review September 25, 2019 18:34
@guicassolato guicassolato force-pushed the apiap/track-proxy-config-changes branch 3 times, most recently from 041236e to da87374 Compare September 26, 2019 13:02
@codecov
Copy link

codecov bot commented Sep 26, 2019

Codecov Report

Merging #1244 into master will decrease coverage by 0.99%.
The diff coverage is 99.17%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1244     +/-   ##
========================================
- Coverage    93.1%   92.1%     -1%     
========================================
  Files        2487    2413     -74     
  Lines       82372   79114   -3258     
========================================
- Hits        76693   72870   -3823     
- Misses       5679    6244    +565
Impacted Files Coverage Δ
test/unit/proxy_test.rb 100% <100%> (ø) ⬆️
.../subscribers/proxy_config_event_subscriber_test.rb 100% <100%> (ø)
...ts/proxy_configs/affecting_object_changed_event.rb 100% <100%> (ø)
...rkers/proxy_config_affecting_change_worker_test.rb 100% <100%> (ø)
...pp/workers/proxy_config_affecting_change_worker.rb 100% <100%> (ø)
app/lib/event_store/repository.rb 100% <100%> (ø) ⬆️
...oxy_configs/affecting_object_changed_event_test.rb 100% <100%> (ø)
app/lib/proxy_config_affecting_changes.rb 100% <100%> (ø)
app/models/proxy.rb 97.74% <100%> (+0.07%) ⬆️
app/models/proxy_rule.rb 98.63% <100%> (+0.01%) ⬆️
... and 217 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d21f8d5...bfceb47. Read the comment docs.

@guicassolato guicassolato marked this pull request as ready for review September 26, 2019 17:08

def issue_proxy_affecting_change_events
changes_attributes = previous_changes.keys
return if changes_attributes.include?('created_at')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return if changes_attributes.include?('created_at')
return if created_at_changed? || (changes_attributes & PROXY_CONFIG_AFFECTING_ATTRIBUTES).empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not changed_attributes, it's changes_attributes, a variable defined in the line before.

app/models/proxy.rb Outdated Show resolved Hide resolved
alias affecting_change_history find_or_create_proxy_config_affecting_change
private :find_or_create_proxy_config_affecting_change

def pending_affecting_changes?
Copy link
Contributor

Choose a reason for hiding this comment

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

Using this method (probably in the view) creates records right?
If yes, can we avoid that somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, if we have a proxy config, then we can also have an affecting change history I think. It also saves us from having to migrate data.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough

thomasmaas
thomasmaas previously approved these changes Sep 27, 2019
Copy link
Member

@thomasmaas thomasmaas left a comment

Choose a reason for hiding this comment

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

Still needs the schema for Oracle and PostgreSQL though


def issue_proxy_affecting_change_events
changes_attributes = previous_changes.keys
return if changes_attributes.include?('created_at') || (changes_attributes & PROXY_CONFIG_AFFECTING_ATTRIBUTES).empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

after_commit :issue_proxy_affecting_change_events, on: :update

def issue_proxy_affecting_change_events
changes_attributes = previous_changes.keys
Copy link
Contributor

Choose a reason for hiding this comment

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

So you can use previously_changed? https://github.com/3scale/porta/blob/d21f8d5e7a6dc740db6652e0604dcf0b639edb52/lib/previously_changed.rb

Which will be introduced in rails 5.0

@hallelujah hallelujah merged commit 8356104 into master Sep 28, 2019
@hallelujah hallelujah deleted the apiap/track-proxy-config-changes branch September 28, 2019 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants