search

giantswarm / roadmap

Giant Swarm Product Roadmap
https://github.com/orgs/giantswarm/projects/273
Apache License 2.0
3 stars 0 forks source link

Enable organization-wide (rather than 'global') app catalogs (Prev. Migrate AppCatalog CRD to be namespace-scoped) #219

Closed cokiengchiara closed 3 years ago

cokiengchiara commented 3 years ago

Towards https://github.com/giantswarm/roadmap/issues/103 and https://github.com/giantswarm/giantswarm/issues/16250

Business value:

It's currently a priority to enable customers to create their own App Catalogs: https://github.com/giantswarm/roadmap/issues/291

We anticipate big customers such as ours (w many departments and sub orgs) might want to limit the scope of a catalog to within an organization (and not across the installation). It's less work and pain for customer and use for us to enable this feature today than later.

User value:

User Story

An an installation admin using multiple organizations with distinct cloud provider accounts (BYOC), I want AppCatalog CRDs to be namespace-scoped, so that CRs can be created in the organization namespace and members of one organization only have access to that organization's App Catalogs.

Clarification: CRDs can only be cluster or namespace scoped. So we need to make the AppCatalog CRD namespace scoped and then we will create the CRs in the organization namespace

cokiengchiara commented 3 years ago

Not doing it anytime soon. Will be useful once we have new organizations or when app platform is a hosted service.

rossf7 commented 3 years ago

@cokiengchiara I'm going to reference this issue in the spec for app-platform-operator that I'm writing.

Issue LGTM but I just wanted to clarify something. CRDs can only be cluster or namespace scoped. So we need to make the AppCatalog CRD namespace scoped and then we will create the CRs in the organization namespace.

snizhana-dynnyk commented 3 years ago

Hi @cokiengchiara and @giantswarm/team-batman :)

Since we are getting close to use SSO in Happa on all installations this issue becomes more relevant. All customers will have access to Management API and all its fancy features (including fain grained access control, organizations management, etc) within the next 3 months.

This is the main epic - https://github.com/giantswarm/giantswarm/issues/16250 [EPIC] Get all customers to use SSO in Happa

Could you please revisit the priority of this, I am adding it a dependency for transitioning to SSO.

rossf7 commented 3 years ago

Thanks for the heads up @snizhana-dynnyk. 👍 I had a look at this yesterday.

Since a CRD can't be cluster and namespace scoped I think we need to create a new appcatalogs.application.x-giantswarm.io CRD with the new version v1beta1 or maybe v1alpha2.

We can migrate all the catalogs except giantswarm, playground and default catalogs pretty easily. These 3 and their test catalogs are more difficult because they are used in releases. But the configmaps and secrets they reference are in the giantswarm namespace.

I think https://github.com/giantswarm/roadmap/issues/291 also ups the priority here. As if customers are creating their own catalogs it would be best to use the new CRD.

Let me know if this doesn't make sense or you have a better idea. 🙏

cokiengchiara commented 3 years ago

Makes sense! Do you want to do this during cooldown? Otherwise, we can prioritize in cycle planning. Main candidates so far are

  1. app sets spec
  2. docs so customer can create own app catalog
  3. this story
cokiengchiara commented 3 years ago

Business value: Removes technical debt: Get rid of legacy API. Now is a good time to do it alongside other Management API activities.

User value:

cokiengchiara commented 3 years ago

Business value: Removes technical debt: Get rid of legacy API. Now is a good time to do it alongside other Management API activities.

@rossf7 and maybe also @corest can chime in. Is the namespaced/org appcatalogs really blocking deprecation of legacy api or only enabling shared CP use cases (which we seem to not have right now)?

rossf7 commented 3 years ago

Is the namespaced/org appcatalogs really blocking deprecation of legacy api or only enabling shared CP use cases (which we seem to not have right now)

@cokiengchiara You can't create AppCatalog CRs via the legacy API. So I don't think this is blocking. @corest Unless you know of another reason?

For me, this is a high priority due to https://github.com/giantswarm/roadmap/issues/291

We want to document how customers can create AppCatalog CRs and I'd like us to do that for the new namespaced CRD. Otherwise, we have to cover the migration in the docs.

cokiengchiara commented 3 years ago

We want to document how customers can create AppCatalog CRs and I'd like us to do that for the new namespaced CRD. Otherwise, we have to cover the migration in the docs.

Got it! Then this is a perfectly good reason to do it, now. Along with #291 😁

teemow commented 3 years ago

Would we want to have global and organization wide catalogs? Because I am not sure that we have to migrate anything then.

cokiengchiara commented 3 years ago

Would we want to have global and organization wide catalogs? Because I am not sure that we have to migrate anything then.

When I joined, I think the answer was yes. For two use cases: (1) Customers (that is no longer that active w us) who manage their customers using one management cluster and (2) sth about Managed Apps-only customers, but I don't remember the specifics... Honestly, I think these are purely hypothetical and won't be relevant for at least 1 or 2 years or something...

Maybe Ross can share why this is thought to be important 😅

rossf7 commented 3 years ago

Would we want to have global and organization wide catalogs? Because I am not sure that we have to migrate anything then.

@teemow The problem is because CRDs can only be cluster scoped or namespace scoped. We can't introduce a new API version like v1alpha2.

So we need to create a new CRD and migrate the existing CRs but keep the public appcatalog CRs that are used in existing versions of app-operator.

The namespaces will use the design specced by @corest in https://github.com/giantswarm/giantswarm/pull/9814

giantswarm and playground catalogs will be stored in the default namespace, our internal catalogs in giantswarm and customer catalogs either in default or their organization namespace.

@cokiengchiara I think this issue misses linking to the spec. https://github.com/giantswarm/roadmap/issues/104

rossf7 commented 3 years ago

Would we want to have global and organization wide catalogs?

@teemow I'm not sure I answered your question. Do you mean can we keep all catalogs global?

I think that's a product question and depends how important multi-tenancy is.

From a dev POV I'd like to get this migration out of the way. But if catalogs stay global then we just document the current state in #291

My concern is the migration is tricky and is easier while we only have a low number of CRs to migrate.

puja108 commented 3 years ago

Currently, we are not running anyone on shared Control Planes AFAIK (and even if, they are not the ones open to using own catalogs yet) and have also not heard from customers that they'd have such a wide multi-tenancy model. But if at any point in time we make a decision that needs that, be that shared (or app only) CPs, or a customers that has more complex internal use cases and is open to cover them with our app platform, then there would be a migration, as a CRD can only be either namespaced or clusterscoped and even if the existing catalogs would remain global they'd need to be migrated into a specific "global" namespace. This is why we said better do it now when customer don't have them, so we won't have such an API change for them in the future.

teemow commented 3 years ago

Thanks @rossf7 for clarifying. This helped a lot.

@cokiengchiara what I understood now. At the moment we only anticipate that creating catalogs for one organization is a useful feature in the future. There are no concrete use cases yet. A shared installation is most likely the most obvious one. So since we have the "vision" that customers will use our catalog system more and increasing the usage is a goal - the likelihood that users might want to scope catalogs to organizations (think of big customers like vodafone with many departments and sub organizations, or shared installations) is relatively high. Given that enabling this feature is more painful in the future and less work today this could or should be prioritized earlier.

@rossf7 would you be able to give @cokiengchiara a rough understanding of how much more work this would be in the future?

rossf7 commented 3 years ago

would you be able to give Chiara a rough understanding of how much more work this would be in the future?

@cokiengchiara I'd say 3 days for the dev and 1 day for the migration since we'll need to use opsctl ensure appcatalogs.

@teemow @puja108 @corest One last thing I want to highlight is the new CRD will be named appcatalogs.application.x-giantswarm.io/v1beta1.

I'd like to keep the current naming but this would be our first CRD with x-giantswarm.io. So that's maybe controversial. If you have a better idea let me know. 🙏

tomahawk28 commented 3 years ago

Today, I discussed with @rossf7 how to use new AppCatalog CRs from app CR.

  1. Since appCatalog CRs would be not cluster-scoped anymore, we need to put the namespace of appCatalog in app CRs.
  2. Since .spec.catalog has already been used as a string type, we need to find other solutions.
  3. We have decided to create a new field in app CRs, .spec.catalogNamespace. With this value, app CR would use the appCatalog CR located in that namespace. If it's blank, app CRs would refer to the catalog in the default namespace.
tomahawk28 commented 3 years ago

Thanks to @marians we now know that we have a problem that a new appCatalog CRs would not be shown to kubectl results since both new and old appCatalog CRs share the same name. We have two options to solve this problem.

  1. Delete old appCatalog CRs temporarily while we install the new appCatalog.

    • For this, we need a maintenance time window and from experiments I have happened to notice there is a hiccup from CRD deletion.

  2. Give new appCatalog CRs different name (e.g. catalog) for kubectl usage

    • It would essentially mean all users should adapt to new CLI usage, (e.g. kubectl get catalog instead of kubectl get appcatalog )

All I want to avoid is having a special maintenance timeframe which we couldn't be sure whether it ends within a day.

@piontec @giantswarm/sig-product Can we make a decision on this topic?

rossf7 commented 3 years ago

I originally wanted to keep the appcatalog naming but I think the idea from @tomahawk28 of a new catalog CRD is a better option.

@puja108 @piontec Interested to hear your thoughts on this.

piontec commented 3 years ago

Hey guys! I like the idea with just catalog name for reasons mentioned by Ross. Also, actually, I think catalog is a better name than appCatalog :)

But I have a few doubts about changing the App CR:

  1. About the .spec.catalogNamespace: AFAIK, no namespace field value means by convention "the same namespace". So, in this case, I would expect the catalog CR to be in the same ns as app. 'no value' meaning default ns might be counterintuitive.
  2. Question: do we want to introduce ownership of App by Catalog using owner references? What happens/should happen with App CRs when someone deletes the catalog they come from?
tomahawk28 commented 3 years ago

So, in this case, I would expect the catalog CR to be in the same ns as app. 'no value' meaning default ns might be counterintuitive.

That means app CRs in the cluster namespace should change this value to have giantswarm or the other values. I don't want to introduce any breaking change in the current app CR's structure. Can we assume it as default or giantswarm for empty value?

To answer your question, app-operator could not proceed with its reconciliation because it could not find the relevant appCatalog CRs. We could percent its deletion from app-admission-controller, but this is out of topic for this issue.

rossf7 commented 3 years ago

That means app CRs in the cluster namespace should change this value to have giantswarm or the other values. I don't want to introduce any breaking change in the current app CR's structure. Can we assume it as default or giantswarm for empty value?

We cann make catalogNamespace an optional field. So if the app CR references an appcatalog CR we hide it. If it's a catalog CR we set it.

Longer-term I'd like to change the app CR structure so it matches what we did for appcatalogentry CRs. 

spec:
  catalog:
    name: giantswarm
    namespace: default

We have https://github.com/giantswarm/giantswarm/issues/11994 in backlog to make structural changes to the App CRD.

Maybe we should update that and move it to roadmap with the changes we'd like to make. WDYT? cc @cokiengchiara

Question: do we want to introduce ownership of App by Catalog using owner references?

Like @tomahawk28 says this has the potential to break things. Also in general it's not something we do with our CRs. appcatalogentry is the only place it's implemented AFAIK

@piontec What about doing an fmt PR or RFC so we can get input from other teams?

piontec commented 3 years ago
  1. @rossf7 @tomahawk28 , I like the idea of syncing the structure of catalog reference between App and AppCatalogEntry. Consistency FTW! But will it make the task much harder/bigger?
  2. @rossf7 The reference question had some hidden questions inside, I asked them to jihyuk:
    • If someone deletes (App)Catalog, should we delete all the App CRs that reference it? We think we shouldn't, right?
    • If someone wants to delete (app)Catalog CR that is used by some App CRs, should we allow it? I believe we should block such request and return an error, but not perform the delete. Any other solution will basically introduce inconsistency. How hard will it be to introduce such check in app-op?
rossf7 commented 3 years ago

I like the idea of syncing the structure of catalog reference between App and AppCatalogEntry. Consistency FTW! But will it make the task much harder/bigger?

@piontec We should do this separately. I posted in https://github.com/giantswarm/giantswarm/issues/11994#issuecomment-840380540

If someone deletes (App)Catalog, should we delete all the App CRs that reference it? We think we shouldn't, right?

Yes, we shouldn't delete the App CRs. Too easy for someone to delete a Catalog CR by accident.

If someone wants to delete (app)Catalog CR that is used by some App CRs, should we allow it? I believe we should block such request and return an error, but not perform the delete. Any other solution will basically introduce inconsistency. How hard will it be to introduce such check in app-op?

Yes, blocking deletion of the catalog CR would be good. I think a validating webhook is the best option. But we don't have a catalog label on app CRs which would be nice for that.

tomahawk28 commented 3 years ago

I had created the current PR https://github.com/giantswarm/apiextensions/pull/720 based on an assumption that we would use appCatalog CR with the different apiVersion. Since we decided to use catalog as a new name, we don't need to use v1alpha2 for the new CRD anymore. I will close this and create another PR.

cokiengchiara commented 3 years ago

Hoping to finish this week. No problems.

cokiengchiara commented 3 years ago

This kubectl gs release https://github.com/giantswarm/kubectl-gs/blob/master/CHANGELOG.md#1320---2021-07-16 is towards this issue right? 😁

tomahawk28 commented 3 years ago

@cokiengchiara I think this is more related to app-operator v5.0.0. From v5.0.0, app-operator uses Catalog CRs so I believe we can close this issue.

rossf7 commented 3 years ago

Yes like @tomahawk28 this is about the new Catalog CRD used in app-operator v5.0.0

Going to close this. The next step is to write public docs in https://github.com/giantswarm/roadmap/issues/291