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

Decouple Templates for Kyma- and Community Modules? #1816

Closed
c-pius opened this issue Aug 29, 2024 · 4 comments
Closed

Decouple Templates for Kyma- and Community Modules? #1816

c-pius opened this issue Aug 29, 2024 · 4 comments
Assignees
Labels
decision Architecture decision record

Comments

@c-pius
Copy link
Contributor

c-pius commented Aug 29, 2024

Context

Side quest from: #1681

It is proposed that community modules are also released as ModuleTemplates. The customer may download the ModuleTemplate from the community repo and apply it to the cluster. Once applied, Kyma dashboard / CLI detect this ModuleTemplate and can be used for easy install (plain apply of the raw manifest to the cluster).

Remark: the same may be done for Kyma modules (ModuleTemplates synced to the SKR by KLM), but instead of using KLM for installation and management, may be applied manually.

Questions to be clarified:

  1. do we need to "decouple" the ModuleTemplates for Kyma and Community modules?
    • both share a large common core of information, but
      • Kyma ModuleTemplates require the Descriptor
        • also have a "mandatory" flag which is useless for community modules
      • Community ModuleTemplates don't require the Descriptor
    • we externalize our currently internal API making it harder to evolve the contract in the future
  2. how does this relate to mandatory modules? If decided to decouple the above, should this also be done for mandatory modules?

Goals: align on above questions so we can feed this back to #1681

Decision

It is decided that the eventual decision is deferred to when the first community module is created so that we can make an informed decision. As of now, the ModuleTemplate is adapted to carry the additional metadata for Kyma modules. These ModuleTemplates can also be used to apply the module directly in unmanaged clusters.

In the future, when the first community module arrives, we can re-asses the pros/cons and implications listed below. It is not expected that this will cause a significant delay for the community module as the implementation effort for both options should be manageable.

Consequences

  • none until re-assessed
@c-pius c-pius added the decision Architecture decision record label Aug 29, 2024
@c-pius
Copy link
Contributor Author

c-pius commented Sep 4, 2024

Feedback in arch meeting today was to keep using the same kind. This will make life easier for CLI and dashboard. Regarding the concerns of the API contract, we already have the compatibility requirements in place and no stricter ones when using them for community modules.

@c-pius c-pius self-assigned this Sep 5, 2024
@c-pius
Copy link
Contributor Author

c-pius commented Sep 5, 2024

The following data is either exclusive for Kyma modules or community modules:

  • attributes exclusive to Kyma
    • spec.mandatory: OPTIONAL, default false
    • spec.descriptor: REQUIRED
    • metadata.labels[operator.kyma-project.io/controller-name]
    • metadata.labels[operator.kyma-project.io/managed-by]
  • attributes exclusive to community
    • metadata.annotations[operator.kyma-project.io/community-module]

Comments on the impact:

  • making spec.descriptor OPTIONAL seems to be doable without significant impact. We currently access it directly only via internal/descriptor/provider which also has a return nil case already. Still, it pushes a restriction from the API contract to the implementation of KLM
  • mandatory flag is OPTIONAL already. CLI and Busola can just ignore it. (from KLM perspective, mandatory module templates are anyway not synced to the SKR).
  • inproperly set labels / annotations may lead to UI bugs (e.g., showing as regular instead of community modules)

@Tomasz-Smelcerz-SAP
Copy link
Member

Tomasz-Smelcerz-SAP commented Sep 9, 2024

I would like to express an opposite view on the issue. In my opinion we should have different kinds for these two types. The most obvious reason is the spec.descriptor field. It is required in one case and non-existing in the other.
Considering the significance of that attribute, it alone should be the rationale for decoupling the Kyma modules from the Community Modules.
Types exist in the K8s ecosystem for a purpose. They express a contract for a software system. The validation rules on them are important and allow us to simplify our implementations - certain constraints may be taken for granted.
Making the crucial attribute (spec.descriptor) optional only to have less kinds to deal with does not improve anything - it just makes our API contract blurry and goes against good engineering principles.
Why I don't like the idea of having just a single type:

  • spec.descriptor is required to process the module. Not having that as mandatory in the API means we need to handle that constraint the LM code (error handling) and cover that with unit tests. And maintain it. And still have a non-zero risk of introducing a bug there (remember: it's a crucial attribute)
  • API evolution: If more differences between the two types appear in the future we'll have to add more "optional" attributes that are actually mandatory in one of the two types. Increasing the "blur" of the contract.
  • API evolution: Future changes to Community Modules may be "blocked" by the fact that they are coupled to the ModuleTemplate API. ModuleTemplates are subject to certain "implementation pressure". It is NOT a public API. It is an API that is shaped in the way to simplify the implementation of Module Catalog in the Lifecycle Manager code. Certain tradeoffs are made there - for the purpose. For example, we don't expect this API to be very human-friendly - we rely on CLI tooling for certain tasks.
    CommunityModules seem to me much more like public API, that may - and should - evolve fast to meet user's demands.
    Having the two types coupled in a single API Kind restricts the one (Community Modules) and puts unnecessary pressure on the other (ModuleTemplate).
  • Implementation effort: It is NOT true that having two types is more complex. It is only a bit less performant because the code must execute TWO queries for modules instead of just one. Caching (already implemented in the go-client) reduces that factor to almost zero. As for the implementation code: either the types are very similar or not. If they are very similar then despite having two API Kinds, the same underlying implementation object model can be used in the business code so there's no increase in complexity. If the types are NOT very similar then one needs two different models anyway - having a distinct Kind changes nothing. Actually, having "two different models anyway" is beneficial for simplicity even if the types are similar. Because then we can get rid of almost all statements like: if spec.descriptor != nil from the codebase. Such statements are one of the worst complexity-increasing code smells. If "explicit is better than implicit", then having two separate models is better than having just one and handle the differences implicitly, with conditionals intertwined with business logic.

Besides the above one could also argue that these two types have different scopes, different lifetimes, different publishing policies etc. All of these factors IMO support the "two distinct Kinds" case.

@janmedrek
Copy link
Contributor

The decision is to postpone the change if needed - included in the ticket description.

@c-pius c-pius closed this as not planned Won't fix, can't repro, duplicate, stale Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
decision Architecture decision record
Projects
None yet
Development

No branches or pull requests

3 participants