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

Individual Device Snapshot Selection in Developer Mode #2484

Closed
10 tasks
MarianRaphael opened this issue Jul 13, 2023 · 20 comments · Fixed by #2835
Closed
10 tasks

Individual Device Snapshot Selection in Developer Mode #2484

MarianRaphael opened this issue Jul 13, 2023 · 20 comments · Fixed by #2835
Assignees
Labels
customer request requested by customer size:M - 3 Sizing estimation point story A user-oriented description of a feature
Milestone

Comments

@MarianRaphael
Copy link
Contributor

MarianRaphael commented Jul 13, 2023

Epic

No response

Description

As a FlowForge user,
I want the ability to select a Snapshot for an individual device independently from the target snapshot when Developer Mode is activated.
So that, I can compare different Snapshot versions more easily for enhanced version understanding and efficient debugging.

Which customers would this be availble to

Licensed Edition (EE)

Acceptance Criteria

  • When Developer Mode is activated, the user should see an additional option labeled "Select different Snapshot for Device"
  • Upon clicking the "Select Snapshot for Device" option, the user should be presented with a list of available Snapshots for that specific device.
  • The user should be able to click on a Snapshot to select it for the individual device.
  • Selection should not affect the target Snapshot that other devices may be using.
  • Upon selecting a Snapshot, a confirmation dialog should appear, asking the user to confirm the Snapshot selection for that specific device and inform the user that the current flows will be overwritten.
  • After confirming the selection, a success notification should appear indicating that the Snapshot has been successfully applied to the individual device.
  • The interface should display a visual indicator (e.g., an icon or text label) next to the device name, signaling that a specific Snapshot has been applied independently of the target Snapshot.
  • If the Snapshot fails to apply, an error message should be displayed explaining the issue and suggesting potential solutions.
  • There should be an option to revert the individual device back to the target Snapshot that is applied globally to all devices.
  • All actions related to Snapshot selection for an individual device should be recorded in the audit logs.

Have you provided an initial effort estimate for this issue?

I have provided an initial effort estimate

@MarianRaphael MarianRaphael added story A user-oriented description of a feature size:M - 3 Sizing estimation point labels Jul 13, 2023
@MarianRaphael MarianRaphael moved this from Medium to Short in ☁️ Product Planning Aug 1, 2023
@MarianRaphael MarianRaphael added this to the 1.11 milestone Aug 1, 2023
@MarianRaphael MarianRaphael added customer request requested by customer consideration A potential feature or improvement that is under review for possible development and implementation labels Aug 1, 2023
@MarianRaphael MarianRaphael removed this from the 1.11 milestone Aug 1, 2023
@griemenschneider
Copy link

As a FF customer this is a high priority item for us; it allows our team to save incremental snapshot checkpoints on individual devices without affecting the devices around it in the same instance. In addition, it allows us to look back at previous snapshots without having to set as target for all other devices

@MarianRaphael MarianRaphael added this to the 1.12 milestone Aug 10, 2023
@MarianRaphael MarianRaphael removed the consideration A potential feature or improvement that is under review for possible development and implementation label Aug 10, 2023
@haroldschuler
Copy link

Thank you for adding this item to the list of improvements. This feature will be very useful to us for quickly changing the snapshot of a specific device and comparing to other snapshots without changing the target on the other devices.

@Steve-Mcl Steve-Mcl moved this from Todo to In Design in 🛠 Development Sep 4, 2023
@Steve-Mcl Steve-Mcl moved this from In Design to Todo in 🛠 Development Sep 4, 2023
@Steve-Mcl
Copy link
Contributor

@MarianRaphael, Can the issue please be updated with some Acceptance Criteria?

Do we have any design drawings in figma for this? /cc @joepavitt

While I understand the benefits targeted snapshot selection will bring, I am struggling to see a clean layout/design for this feature especially considering the new type of snapshot "Device at Application Level" will bring.

@joepavitt
Copy link
Contributor

I hadn't seen this, so no UX designs for this atm, but can put something together this week.

@Steve-Mcl
Copy link
Contributor

I hadn't seen this, so no UX designs for this atm, but can put something together this week.

That would be great thanks Joe. Appreciated.

@joepavitt
Copy link
Contributor

So, my high-level thinking here:

  • Single Snapshots tab for Devices, which shows all available snapshots for that Device (i.e. anything defined at the Application or Instance level depending on where it is bound)
  • Behaviour is heavily driven by whether or not the Device is in "Developer Mode" or not, with the new visual toggle (already available as a component in source) the key indicator as to whether or not it's in dev mode or not.

Design: Default State when Developer Mode: Disabled
Screenshot 2023-09-05 at 12 36 07

Design: Hover Behaviour when Developer Mode: Disabled
Screenshot 2023-09-05 at 12 36 36

Design: Default State when Developer Mode: Enabled
Screenshot 2023-09-05 at 12 36 57

@joepavitt
Copy link
Contributor

The "Developer Mode" tab is just an extraction of the content, currently hidden in "Settings", to try and encourage more users to explore it, and make it easily accessible. It's not essential for this piece of work, however, I would say that the "Dev Mode" toggle (top-right) would be very much desired.

@joepavitt
Copy link
Contributor

A key missing ingredient is then also the highlighting of a position where a Device is bound to an Instance, and that Instance itself as a Target Snapshot (I'm working on it)

@joepavitt
Copy link
Contributor

When attached to an Instance:

Screenshot 2023-09-05 at 14 46 31

@joepavitt
Copy link
Contributor

Small update with the "Inherited Target Snapshot". When in Developer Mode, visual changes to make it clear that this Device is not in-sync with the rest of the Devices attach to the Snapshot

Screenshot 2023-09-12 at 10 42 52

@Steve-Mcl
Copy link
Contributor

FAO @MarianRaphael @knolleary @joepavitt

Question:

When a snapshot from another device/instance is deployed to a different device should we duplicate the snapshot (and prompt the user for details)

Background:

When a device or instance is deleted, we lose the original credential secret meaning a snapshots credentials are essentially lost.

This issue (kinda) already exists but it becomes much more visible when we permit other-device->device and instance->device snapshot deploys.

Side Benefits:

A new device with a snapshot deployed from another device/instance will appear to have zero snapshots in its list. If we perform a duplicate (and request a snapshot name/description), the device will now have a self owned, aptly named snapshot and it will appear in its own list.

Another side benefit is, should we decide to cascade delete snapshots when the owner (device/instance) is deleted, we wont break* devices that have had a snapshot deployed.

_* break: by break I mean have a snapshot assigned that might have credentials that can never be reloaded because the creds decryption fails/throws)


Alternative solution

We store the credential secret with the snapshot object, meaning snapshot can live beyond device or instance deletion. This has benefits too. 1) We dont need to look at snapshot clean up. 2) The user might actually want to keep snapshots beyond the life of a device/instance, 3) less duplicates. 4) decouples snapshot from source.

@joepavitt
Copy link
Contributor

joepavitt commented Oct 4, 2023

When a snapshot from another device/instance is deployed to a different device should we duplicate the snapshot (and prompt the user for details)

Struggling to follow this - don't understand the problem, where/why are credentials relevant?

@Steve-Mcl
Copy link
Contributor

Struggling to follow this - don't understand the problem, where/why are credentials relevant?

Because if a flow has any encrypted credentials (ie username/password in a HTTP-Request, MQTT config etc), they are decrypted (using the source device/instance) and re-encrypted using the target device.

If the source is deleted, the secret is lost.

And even if we side stepped this issue and ignored the decryption error, thus allowing a deploy with bad creds, you will see this:

image


But additional to all that, a device with a snapshot deployed from another device/instance appears to have no snapshot. It feels somewhat "magical" (opaque).

@knolleary
Copy link
Member

When a snapshot from another device/instance is deployed to a different device should we duplicate the snapshot (and prompt the user for details)

This is straying into the work being done via #2756 to allow Devices be participants in a DevOps pipeline. I'm concerned we're implementing the same capability via two different methods.

In the Pipeline model, deploying a snapshot to anything involves copying the source snapshot - and reencrypting its credentials for the target.

If this item ends up doing the same, then why would someone use one method versus the other?

We store the credential secret with the snapshot object

That is an option, but not a quick change to make and will still need to handle existing snapshots without the secret. If we went in this direction, the stored secret would be for internal use only - allowing us to decrypt the credentials before re-encrypting with the target's secret and sending the snapshot over the network.

@Steve-Mcl
Copy link
Contributor

If this item ends up doing the same, then why would someone use one method versus the other?

For one-off/ad-hoc action scenarios, this method is far simpler, minimum effort, more accessible, quick and easy IMO. I personally imagine this feature being used far more than pipelines for them scenarios.

And with all the above said, as it stands, we still have a potential issue to resolve or at least handle.

@knolleary
Copy link
Member

So to summarise, the two options are:

  1. Create a device-owned copy of any snapshot deployed to the device.
  2. Include the credentialSecret in the snapshot object internally (never exposed on the API) - to allow any snapshot to be decrypted internally before being re-encrypted for the target)

Option 1 still assumes the 'owner' of the snapshot still exists so we can get ahold of the decryption key at that point in time.

Option 2 solves that for new snapshots, but all existing snapshots won't have the key property so the code will need to handle that (with the knowledge the key is lost of the instance is deleted.)

It feels like option 2 is cleaner. However it has potentially wider implications and would need discussion with @Pezmc as this has knock-on effects to how pipelines handle snapshots.

Given this is an existing edge case, I don't want it to delay getting all of the other work completed on this item. There's already a lot of work here that will need reviewing - this is only going to make it even larger and harder to review.

I suggest we get what you have reviewed and merged, and then we can tackle the snapshot encryption changes which will touch a lot of different areas.

@Steve-Mcl
Copy link
Contributor

Given this is an existing edge case

Yes, but to make sure it is clear, I would like to point out this work will make the issue "less edge" / "more prominent".

For example, it is perfectly reasonable to suppose a user will do the following:

  1. snapshot an old/failing device
  2. push snapshot to new device
  3. delete old device
    IMO, it is far less likely a old orphaned toe rag snapshot would be used over a fresh (just performed) snapshot in pretty much most scenarios.

So I will finish up this work and include concerns in the PR for later task generation.

@knolleary
Copy link
Member

Yes, but to make sure it is clear, I would like to point out this work will make the issue "less edge" / "more prominent".

I am not saying we don't address it. I'm saying do not make your current PR more complicated by trying to address it at the same time.

So I will finish up this work and include concerns in the PR for later task generation.

Get an issue raised for this item - point to this discuss for context and detail. Addressing this will be the next task to tackle.

@Steve-Mcl Steve-Mcl moved this from In Progress to Review in 🛠 Development Oct 5, 2023
@Steve-Mcl Steve-Mcl moved this from Review to Verify in 🛠 Development Oct 10, 2023
@Steve-Mcl
Copy link
Contributor

Verified on staging in accordance with the extensive tests laid out in the PR here: #2835 (comment)

@knolleary knolleary moved this from Verify to Done in 🛠 Development Oct 13, 2023
@MarianRaphael MarianRaphael moved this from Short to Closed / Done in ☁️ Product Planning Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer request requested by customer size:M - 3 Sizing estimation point story A user-oriented description of a feature
Projects
Archived in project
Archived in project
6 participants