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

KEP 89: Lifecycle Toolkit - Context Information in Keptn Tasks #91

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

thschue
Copy link
Contributor

@thschue thschue commented Feb 8, 2023

Signed-off-by: Thomas Schuetz [email protected]

Signed-off-by: Thomas Schuetz <[email protected]>
@thschue thschue changed the title initial commit KEP 89: Lifecycle Toolkit - Context Information in Keptn Tasks Feb 8, 2023
Signed-off-by: Thomas Schuetz <[email protected]>
Copy link
Member

@thisthat thisthat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed feelings for this KEP. On one hand, I see the benefits of giving the tasks as much information as they need. Whenever we add extra info to our CRDs (e.g., KEP88), they get it for free without code change. On the other hand, tasks need to parse the JSON which require them to be aware of the version of App and Workload. Furthermore, as highlighted in the drawback section, there might be severe security issue in the future.

text/0089-klt-context-information.md Show resolved Hide resolved

## Assumptions / Definitions
* It might be easier to pass over the whole KeptnAppVersion or KeptnWorkloadInstance as a JSON string instead of passing over the individual fields.
* There is no sensitive information in the context information
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would follow RFC2119 and have extra emphasis on this requirement

Suggested change
* There is no sensitive information in the context information
* There MUST NOT be sensitive information in the context information

## Drawbacks
* The context information might be too big for some tasks
* The context information might contain sensitive information in the future which has to be filtered out

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Tasks need to be aware of the `KeptnAppVersion` and `KeptnWorkloadInstance` version

@malon875875
Copy link

  • I can’t think now of which information we would pass that would be sensitive
  • Only a specific kind of Keptn task requires to know additional information for the promotion, other tasks would receive context information that is too big
    • I wonder if there should be a CRD KeptnTaskPromotionDefinition that would receive the additional promotion information

@christian-kreuzberger-dtx christian-kreuzberger-dtx removed their request for review February 13, 2023 15:44
@thschue
Copy link
Contributor Author

thschue commented Feb 14, 2023

  • I can’t think now of which information we would pass that would be sensitive

  • Only a specific kind of Keptn task requires to know additional information for the promotion, other tasks would receive context information that is too big

    • I wonder if there should be a CRD KeptnTaskPromotionDefinition that would receive the additional promotion information

Which information would you like to pass over, for example? Personally, I'm not a fan of having too many CRDs, especially as the synchronization in a GitOps setup might get a bit tricky ...

@malon875875
Copy link

+1
I would love to see this PR implemented within the Keptn Roadmap

Co-authored-by: Giovanni Liva <[email protected]>
Signed-off-by: Moritz Wiesinger <[email protected]>
Copy link
Member

@mowies mowies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This KEP is basically fully implemented by keptn/lifecycle-toolkit#1394

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants