Rollback UI

[andresmgot]

Closes: #997

Add support for rolling back releases with the dashboard:

Note that the complexity of this PR comes from the fact that we need to resolve the chart of the release before doing a rollback. This is because we use the chart to determine if the user have permissions to modify the resources detailed on it. This is the same approach we use for upgrades.

For context, in order to resolve a chart we need to know from which repository it comes from. If we are not able to automatically get it, we ask the user to manually introduce the repo with SelectRepoForm. This will happen when the registry that contains the chart is not an AppRepository or if the call to get the update information fails.

TODO:

• Write tests

[andresmgot]

@Angelmmiguel @absoludity this should be ready to be reviewed!

[Angelmmiguel]

In general, the code LGMT. I added some minor comments/suggestions. The only blocker for me to accept this PR is the usage of the deprecated componentWillReceiveProps method. Please, check my comment to get more information about it.

[Angelmmiguel]

I believe we're not using React.Suspense and lazy in this project yet, but this is a good component to be loaded lazily in this view.

[andresmgot]

That's an interesting feature, didn't know about it. As far as I understand, this is to avoid rendering heavy components that will be used just in certain case. If I use lazy with this RollbackButtonContainer it will be always rendered isn't it? What would be the difference between using it or not?

[Angelmmiguel]

Actually, it's much powerful than that :). React.lazy allows you to download a component lazily when the user access to the view. This approach introduces two improvements in the applications:

• The component is not added to the main bundle of the application. As you mentioned, this is super useful for big components that are used only in a single view
• Since you can lazy load secondary components, the application performance increases due to the reduction of components in the view and the size of the bundle

React.Suspense is a component that controls lazy components in the UI. Any time you use React.lazy to load a component, it should be added as a child of a React.Suspense component. Suspense receives a fallback component as a prop that will be displayed while the lazy component is loaded. This fallback could be a text, a loader component or null if you don't need to display anything.

Another good feature about Suspense is that you can have several components in the component hierarchy. The closest one to the lazy component is the one that will render the fallback. It means you don't need to show a loader in the entire view if you're loading a lazy component.

If I use lazy with this RollbackButtonContainer it will be always rendered isn't it? What would be the difference between using it or not?

About the example you mentioned, RollbackButtonContainer won't be rendered at the beginning. If you scope this component inside a React.Suspense component, the fallback will be returned. In this case, you can even return null and don't display anything.

Once the component is fully loaded, it will be mounted in the UI.

[andresmgot]

ACK, let me try to use suspense and lazy here so we start adding that :)

[andresmgot]

I am actually undoing this change because I am not able to test this if I change it for a suspense/lazy block.

[Angelmmiguel]

Calling setProps forces the button to update right after shallowing them. If you are not testing this update behaviour, it's better to avoid updating the component right after shallowing it. You can use the spread operator to modify the defaultProps. For example:

const appProps = {
  ...defaultProps,
  app: {
    // ...
  }
}

const wrapper = shallow(<RollbackButton {...appProps} />);

[Angelmmiguel]

It's better to test how the component reacts to user interactions and lifecycle methods than forcing states manually. By forcing the state you're skipping the natural component behaviour and it may skip bugs from user interactions or other conditions.

[andresmgot]

In this case the setState is not needed but yes, I try to reproduce users behavior when possible. In some cases (not this one) I cannot do so because simulations like "onConfirm" don't work as expected.

[Angelmmiguel]

This method is deprecated and it will be removed in future React versions. Since we're in 16.8.6, we should stop adding more deprecated methods to the codebase.

For this use case, you can use the getDerivedStateFromProps lifecycle method.

[andresmgot]

I forgot about that, thanks

[Angelmmiguel]

Are these styles common across all the modals in the application? If it's the case, I believe it's better to create a common Modal. This can be done in a separate PR

[andresmgot]

👍 #1123

[Angelmmiguel]

LGTM! Good job!

[absoludity]

Looks good Andres, though due to me still learning the codebase, I have a bunch of questions in there that will help me learn :)

On a more general note, does the kubeapps community tend to do larger PRs like this? I think reviews can be a huge learning and collaboration opportunity, but I find large diffs like this either take a disproportionate amount of time to review (because of all the separate contexts and goals required to keep in mind), or tend to get worse reviews (because people don't look closely after the first 3-400 lines). This PR could have been a couple of small focused ones, IMO... breaking up a diff is a very small overhead for the author, but results in better (and faster) reviews. Again, just my 2c, happy to slog through the longer ones if that's the way things are done :)

[absoludity]

Is this a bug (ie. should it have an issue attached), or for a valid reason (in which case, maybe state the reason here, or link to where it is stated?). To me it looks like a bug, in that getAppWithUpdateInfo() explicitly dispatches a requestApps() and then in the next statement dispatches a getApp() (which also dispatches requestApps() straight away).

IMO, there should be no dispatch of requestApps() at the beginning of getAppWithUpdateInfo() given that it's emitted by the first statement getApp().

Related to that, I initially expected to see something like REQUEST_APP_ROLLBACK and RECEIVE_APP_ROLLBACK or similar in these expectedActions, but see this state isn't included for upgrade either. Looks like actions/apps.tsx currently only tracks redux actions for getting apps? Is that intentional? (not sure if the only benefit is when debugging, seeing which actions are triggered etc.)

[andresmgot]

This is probably a bug. TBH the redux store is something that would benefit from a refactor (and it's mostly my fault :) ). For the moment I just copied the approach form the upgrade and avoided changing more things to avoid this PR to grow even bigger. Let me open a follow-up issue.

#1126

[absoludity]

Woops, sorry. Not sure if I had the wrong linter or what at the time.

[andresmgot]

I am not sure why but I got this change automatically after committing something.

[absoludity]

Couple of questions as it's not clear to me (which I will try to answer myself):

• In what situations will we not have info about the chart? You mention in the description that this can happen when the repository is not an AppRepository, but it's not clear whether this means that Kubeapps does not have this info (I mean, helm must have it, right?), or the dashboard just doesn't have it (or can't currently get it), or it's one of stable etc.?
• In those situations, why wouldn't we call getAppWithUpdateInfo (I see IChartUpdateInfo includes the repository info? In fact, this could be called after the page loads, if the update info isn't yet present) rather than prompting the user to select which repo? Or is this situation only present if there is no info available?
• Isn't the repo required just to get the auth info for the request? If so, what's the implication of allowing me, the user, to select which auth to use? (security thought: I could setup my own repo which captures any auth credentials, then rollback/upgrade a release, but use the dashboard to specify another repo with secrets I want to know... won't the request then be done by tiller with the secrets from the other repo, so I can capture them?)

Without context, it's just not clear to my why we're getting the user to select something which we should be able to know (and could seem a bit clumsy both in the code and to users), but I assume it is because we can't know the info - given that this approach is also used for upgrade as you say.

Also, this is related to #1110 I think? (in that, we're only needing to read repositories because we need the auth... if instead we sent the repository name and tiller used that to get the auth, we'd only ever need the repo name and would not need to fetch the repo/repos). We just need to always have a repo name - interested to know why we don't know the repo name.

[andresmgot]

In what situations will we not have info about the chart? You mention in the description that this can happen when the repository is not an AppRepository, but it's not clear whether this means that Kubeapps does not have this info (I mean, helm must have it, right?), or the dashboard just doesn't have it (or can't currently get it), or it's one of stable etc.?

That could happen in several occasions:

• The user installs a chart using the helm CLI with a repository that doesn't exists in Kubeapps. Then we are not able to see where this application comes from.
• The chart doesn't have an appVersion. This field is optional but we use it to verify if a chart belongs to a repository. What we do is checking if chart: wordpress, version: 1.0.0, appVersion: 1.0.1 exists in the repo stable. If those three parameters match, we assume that chart comes from that repository. If we don't have that information we are not confident enough to make that assumption.

In those situations, why wouldn't we call getAppWithUpdateInfo (I see IChartUpdateInfo includes the repository info? In fact, this could be called after the page loads, if the update info isn't yet present) rather than prompting the user to select which repo? Or is this situation only present if there is no info available?

We cannot call getAppWithUpdateInfo if we don't have the chart name, version or app version.

Isn't the repo required just to get the auth info for the request? If so, what's the implication of allowing me, the user, to select which auth to use? (security thought: I could setup my own repo which captures any auth credentials, then rollback/upgrade a release, but use the dashboard to specify another repo with secrets I want to know... won't the request then be done by tiller with the secrets from the other repo, so I can capture them?)

The repo is required to retrieve the information about the chart. This is ultimately used to verify if the user foo can modify the deployment bar that is specified in the chart. This is not perfect though since we don't have a way to verify that the chart really belongs to that repo, we just verify that there is a chart with the same version/appversion is there.

The user doesn't interact with any other auth info so I am not sure if that's the case you are describing.

Also, this is related to #1110 I think? (in that, we're only needing to read repositories because we need the auth... if instead we sent the repository name and tiller used that to get the auth, we'd only ever need the repo name and would not need to fetch the repo/repos). We just need to always have a repo name - interested to know why we don't know the repo name.

We would still need to list the repos if we are not able to auto-retrieve the registry that contains the chart.

[absoludity]

In what situations will we not have info about the chart? You mention in the description that this can happen when the repository is not an AppRepository, but it's not clear whether this means that Kubeapps does not have this info (I mean, helm must have it, right?), or the dashboard just doesn't have it (or can't currently get it), or it's one of stable etc.?

That could happen in several occasions:

* The user installs a chart using the `helm` CLI with a repository that doesn't exists in Kubeapps. Then we are not able to see where this application comes from.

In this situation, helm/tiller then knows the repo though doesn't it? EDIT: I think not - this is the source of my confusion. Helm just finds a chart with whatever repos it has access to, and doesn't (apparently) store the repo of the chart it finds for later reference?

* The chart doesn't have an `appVersion`. This field is optional but we use it to verify if a chart belongs to a repository. What we do is checking if `chart: wordpress, version: 1.0.0, appVersion: 1.0.1` exists in the repo `stable`. If those three parameters match, we assume that chart comes from that repository. If we don't have that information we are not confident enough to make that assumption.

And this is all info which isn't necessary to roll back, but just to verify the current user is allowed to roll back? Seems odd (but I may not have my head around it). Why can't we assume that if the user has "write" access to the namespace in which the chart is deployed, then they can roll back and just let them do it? I think on upgrade it's a bit different because it requires access to the repo, but does a rollback require this at all? (not sure if the chart versions are stored by tiller, haven't checked).

[snip]

Isn't the repo required just to get the auth info for the request? If so, what's the implication of allowing me, the user, to select which auth to use? (security thought: I could setup my own repo which captures any auth credentials, then rollback/upgrade a release, but use the dashboard to specify another repo with secrets I want to know... won't the request then be done by tiller with the secrets from the other repo, so I can capture them?)

The repo is required to retrieve the information about the chart. This is ultimately used to verify if the user foo can modify the deployment bar that is specified in the chart. This is not perfect though since we don't have a way to verify that the chart really belongs to that repo, we just verify that there is a chart with the same version/appversion is there.

As above, why isn't it enough that the user has sufficient access to the namespace, to enable roll back?

The user doesn't interact with any other auth info so I am not sure if that's the case you are describing.

Also, this is related to #1110 I think? (in that, we're only needing to read repositories because we need the auth... if instead we sent the repository name and tiller used that to get the auth, we'd only ever need the repo name and would not need to fetch the repo/repos). We just need to always have a repo name - interested to know why we don't know the repo name.

We would still need to list the repos if we are not able to auto-retrieve the registry that contains the chart.

Only if we required the repo for authz though right? (Unlike an upgrade, a rollback doesn't require access to the repo per-se, since it's rolling back to a previous version of the chart which it has stored?)

[andresmgot]

Helm just finds a chart with whatever repos it has access to, and doesn't (apparently) store the repo of the chart it finds for later reference?

Right, that's the problem. The helm CLI downloads the chart locally and just send the manifests. Tiller (or the configmaps stored) has no clue of where those charts came from.

Why can't we assume that if the user has "write" access to the namespace in which the chart is deployed, then they can roll back and just let them do it?

There is not a simple check to verify if a user has "write" access. Note that a chart can contain any resource, they can even escape from a namespace. If a chart has a ClusterRole or a CRD we need to verify that the user has permissions to modify those cluster-wide resources. Even if it's in a single namespace, a user can have permissions to modify configmaps but not secrets. We need to verify all the resources to avoid an escalation of privileges.

I think on upgrade it's a bit different because it requires access to the repo, but does a rollback require this at all? (not sure if the chart versions are stored by tiller, haven't checked).

The rollback operation doesn't need to access the registry but we need to do so to be able to get the resources the chart modifies. Ideally we could get this information from Tiller since those resources are specified in the configmap but for simplicity we are not doing that right now. That could be an improvement of this approach but it's more difficult to implement.

[absoludity]

Oh - we're submitting the rollback when the state changes? Hmm, not according to the screenshot with the submit button. So how is onConfirm being called here without the simulated submit? (also, why is the state being set, rather than selecting the revision 4). Guessing there are hard-to-test things here, but still surprised to see the test verifying onConfirm has been called when I don't expect it to be called?

Also seems a bit odd that the current revision is 2 and this test rolls back to 4 - should that even be possible?

[andresmgot]

yes, this is a bit tricky, the property onConfirm returns a function, so the actual action (nested function) is not performed. I can rewrite this to be more descriptive.

Then the other problem is that I cannot simulate the select behavior so I had to directly change the state manually.

[absoludity]

Helm guarantees that the consecutive releases will always be present, I assume? (seems so when doing upgrades and rollbacks).

[andresmgot]

I don't understand what you mean? When you upgrade a release the revision gets increased (starting at 1). All of them are stored as configmaps so you can rollback to any previous revision.

[absoludity]

I meant to leave a +1 vote, but just keen know that the modal isn't submitted when the select changes, as the test reads (but I could be missing something).

[andresmgot]

On a more general note, does the kubeapps community tend to do larger PRs like this?

Not really, this is mostly my bad. I tend to think the changes are going to be a small PR but sometimes it ends up not being the case (like this PR). I wait until the feature is working before sending the PR because I usually don't know how to split them before hand. Even if I do, I am not sure if the code I am submitting is going to work end-to-end if the feature is not complete (so the same code may appear in several reviews).

I am aware of the pain of reviewing a big PR like this one so I will try to split them as much as possible in the future to avoid the kind of issues your mention.

[absoludity]

Thanks Andres. It's still not clear to me why we need to list the repos and have the user select one (when we can establish whether the user has access to the namespace I think) - I've left why in the comments. +1'ing so as not to block.