OctopusDeploy / Issues

| Public | Bug reports and known issues for Octopus Deploy and all related tools
https://octopus.com
162 stars 20 forks source link

Variable Unification #4390

Open mayuanyang opened 6 years ago

mayuanyang commented 6 years ago

There are bunch of issues reported around Tenant Variables. The main reason is that we treat them differently than regular variables and they are always behind in terms of features they provide. Their UI is also way behind of what Variable Editor offers. Recently Mike N came up with an idea to unify variables, tenant variables and variable templates.

Issues: https://github.com/OctopusDeploy/Issues/issues?q=is%3Aopen+is%3Aissue+label%3Afeature%2Fvariables

Mike's doco: https://docs.google.com/document/d/1uenDKcRDhRm6pGAwN-W_fCC4ZZOh-tufT6k3zrShoz8/edit#heading=h.9ewmmkmoqyqi

We should also come up with new API as the current one is confusing. Variable resource doesn’t represent variables it represent variable values.

Do we assign type (sensitive, text, cert, etc) at the value or variable level? At the moment it is specified at the value level.

Refactor UI code so XXXValue represent variable values and XXXVariable represent variables. Part of is already done but it's not finished.

There was a request to display a prompt variable as a drop down.

Make variable resolution case insensitive in F# and Bash, other places? https://octopusdeploy.slack.com/archives/C033W4273/p1518396948000118

Have a look at the code that dynamic generates UI for forms like auth settings. Is there any overlap there?

Allow the use of typed variables in drop down select, not just text, e.g. certificates.

Reconsider the data structure for sensitive values

Get rid of IsSensitive and use Type only. Check comments below for an idea proposed by Tom.

Tenant variables page usually can only see 5 at a time, user usually has a lot more than that https://help.octopus.com/t/issues-with-password-reset-and-updating-tenant-variables/19163

Perhaps there is a way we can change how sensitive values work to make "semantic correctness" the same as "syntactic correctness"? That way we can leverage the type system in TypeScript and C# as the first line of defence.

Sensitive variables require a well-behaved client

This may also be true for all complex variable types.

For sensitive variables/properties/parameters to work properly, the client needs to be well-behaved. We should make the server "safe" by enforcing the data sent from the client being in the expected format, otherwise we should tell them what to do to fix it.

Consider this example: https://github.com/OctopusDeploy/OctopusDeploy/issues/2048

The Server should load the template and validate that all parameter values sent from the client match the expectations of the template's parameters.

Try to fix also the problem with variable permissions if this makes sense: https://github.com/OctopusDeploy/Issues/issues/4514

Don't forget to unify the API https://secure.helpscout.net/conversation/582606642?folderId=557077

Create a single restriction that applies to all variables: Project, Tenant, Library, etc

Add ability to have a null\empty as valid values. This might be tricky because our API auto-magically treats null/empty as no value in many cases.

Add a new variable type for Feeds.

pawelpabich commented 6 years ago

Let's address this problem here: https://github.com/OctopusDeploy/Issues/issues/3590

MarkSiedle commented 6 years ago

fyi another source from a customer frustrated with variable editing with tenants: https://secure.helpscout.net/conversation/583371898/27191/?folderId=801354

Benny1007 commented 6 years ago

Library variable sets should be included in this scope of work. Currently if I scope a project variable to PROD, and a variable in a Library variable set to PROD, then give a user variable view permission for everything except PROD, the user is prevented from viewing the project variable, but can view the library variable set variable. I think this broke somewhere from 3.17

patnolan commented 6 years ago

Hi @michaelnoonan I've read your document associated with this issue. Attached is a document containing a fairly detailed description of our issues and proposed solution. Perhaps it could be included in your document?

I’m dealing with many customers of Octopus Deploy across the insurance and banking sectors on a daily basis and this document describes what seems to be some large and commonly encountered issues.

Please let me know if I can offer any more assistance in shaping the outcome of this work.

Unified Octopus Variable System.docx

michaelnoonan commented 6 years ago

Hi @patnolan I've included your thoughts into that document. :) Thanks!

TomPeters commented 6 years ago

Some related issues that we might aim to fix as part of this: https://trello.com/c/CZZ3IkqM/218-feature-request-cant-set-a-tenant-variable-to-an-empty-string https://trello.com/c/2QhAhCOT/399-missing-validation-of-setting-tenant-variable-type-if-variable-template-is-sensitive-but-api-sets-non-sensitive-variable https://trello.com/c/TSKjljZC/1013-would-be-nice-to-see-if-tenant-variables-are-using-the-default https://trello.com/c/sDRZI8jp/831-tenant-variables-dont-appear-to-be-versioned-concurrent-variable-updates-can-stomp-on-each-other

TomPeters commented 5 years ago

Another problem that we could fix: https://help.octopus.com/t/dirty-tenants/21953

whereisaaron commented 5 years ago

Unifying the variable handling sounds great, but the permissions should ideally be more fine grained than currently. We have a bunch of hassles creating roles with the 2019.x Variable permissions.

Use case: We would like Tenants to be able to update their own/Tenant variables but not view or update Variable Library Set Variables nor Project Variables. We can't do this right now because there are not separate permissions for Tenant variables

Also ViewLibraryVariableSets isn't scoped by the Projects you have access to, it immediately opens up the Library and gives access to all available Library Sets, and to view the values of the variables in the Library.

You can't not provide ViewLibraryVariableSets t users, because ProjectView requires ProcessView and ViewLibraryVariableSets even just for the 'Overview' tab. ViewLibraryVariableSets opens up the Library and the display all variable values, even if not used for projects you have access to. And ProcessView opens up the 'Process' tab, and the 'Process' tab requires 'ViewActionStepTemplate' or else all the (custom) icons in the 'Process' tab are broken. 'ViewActionStepTemplate' then also opens up the library and makes all action steps available, including viewing Action Step source code.

So roles basically have to be all or nothing right now. We just transitioned to 2019.x and the problems are noticeably worse. E.g. the Project problems above didn't occur in 2018.9.x, they are new problems in 2019.x.

I think while working on these it changes it would help a lot to review the available Variable permissions and how they cascade permissions in the UI.

michaelnoonan commented 5 years ago

Hi @whereisaaron - thanks for your feedback! We agree in general about the problems with permission grants related to variables, and we are on a path to cater to some of the most commonly encountered use cases. I'll hand over to @TomPeters and @NickJosevski who might be able to offer more insight.

NickJosevski commented 5 years ago

Hi @whereisaaron

Fine grained permissions has made the Octopus permission system very complex and difficult for customers to understand so we tread very carefully introducing more granularity. We are working towards securing Library Variable Sets in line with Project Variable Sets as part of that work we will be looking closely at Tenant Variables.

Your understanding of the Library Variable Set permissions is correct, Library Variable Sets currently are global. I've covered your queries about Project/Process view elsewhere, so I'll keep discussion here focused on Variables. The requirement for LibraryVariableSet was incorrectly linked to Deployments that will be fixed in an upcoming patch release of Octopus 2019.5.11.

Your Tenant variable suggestion sounds reasonable, we're still investigating the best solution to make variable related permissions better. Having an idea of what you're trying to achieve by this, we'll factor it into our decision making for this enhancement.

whereisaaron commented 5 years ago

Thanks @NickJosevski. You have a great User Roles system to hide any permissions complexity, where you guys, the experts, have hand-crafted a selection of pre-made User Roles. If users are frequently delving into the permissions level, possibly that indicates the current set of pre-made User Roles aren't covering some common use cases? Perhaps you could reach out with survey, 'what are your dream user roles, and what would that role be able to see and do?'

I know I wouldn't mess with permissions if the pre-made roles were suitable for us (makes upgrades easier!). The problem I found with the current pre-made User Roles was that the are were all additive. For example the pre-made 'Project deployer' role, can't just deploy releases, they also have all the 'Project contributor' abilities thrown in too, and can edit the whole deployment process. I would have liked disjoint 'Project editor' and 'Project deployer' roles, so I could grant one or both, as appropriate, to a user group.

When it comes to messing with permissions; The permission set is clearly named and not hard to understand. The challenge is that the web UI views are bit opaque as to which permissions they require. So you say, ok I want a User Role who can see the Project 'Process' tab, now... which combination of permissions does that require? So the complexity is not the permissions so much as reverse engineering which permissions/APIs a web UI view may invoke.

The exception error messages that name permissions are are very helpful in this regard, but it is still a trial and error process. And I don't mind going through that process to craft a role. The problem I hit is that permissions I discover are needed are not fine-grained enough, so the role I am targeting to create keeps getting 'bigger' than I want, because the small number of broad permissions adds too much to the role. I have to 'overshoot' my target role by often quite a lot.

I can see you are trying to have just one API and just one set of traditional CRUD permissions per resource class. But I think that misses that there are legit reasons for more than one view of some resources. With the current approach, just to displaying a list of resources of some class, you need to grant access to every single property of that resource class. I have advocated elsewhere for a more restricted FooList permission/API for some resource classes.

dave-sampson13 commented 5 years ago

Any update on how this is progressing / ETA?

NickJosevski commented 5 years ago

Hi @dave-sampson13,

This feature would be amazing to build, many people would benefit from it. But it's a very difficult and large task. Currently it isn't near the top of planned future work, there's too many competing great things we're building in Octopus!

The day we are ready to dive into this feature, we will absolutely discuss it publicly and drop a note here. Thanks for checking in :)

dgard1981 commented 3 years ago

It's been more than three years since this ticket was created, and 18 months since the last update. Are these incredibly desirable changes ever going to happen? Sadly, tenant variables are all but unmanageable right now.