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

Fixes 19755: Publish app config with status #19754

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

Conversation

sushi30
Copy link
Contributor

@sushi30 sushi30 commented Feb 11, 2025

Describe your changes:

Publish appConfig as part of the status for ingestion pipelines.

Delivers #19755

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

@sushi30 sushi30 changed the title Publish app config with status MINOR: Publish app config with status Feb 11, 2025
@sushi30 sushi30 changed the title MINOR: Publish app config with status Fixes 19755: Publish app config with status Feb 11, 2025
@@ -110,6 +111,11 @@ def model_dump_json( # pylint: disable=too-many-arguments
ensure_ascii=True,
)

def model_dump_masked(self, *args, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

will this mask all the secrets when serialized? what about when we try to create a service from a YAML?

Copy link
Contributor Author

@sushi30 sushi30 Feb 11, 2025

Choose a reason for hiding this comment

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

It will only marked fields marked as secret like passwords, tokens etc.. Not the whole service. We can add more complex for things like service connections if we want to handle them differently.

For example this password field.

Copy link
Collaborator

Choose a reason for hiding this comment

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

for now i would at least keep the option to see if storeCreds: true/false in the workflow config and control the secrets. If we were to create a service from a YAML with this config, the server would just receive ****

Copy link
Contributor Author

@sushi30 sushi30 Feb 12, 2025

Choose a reason for hiding this comment

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

In this case we are dumping the configuration and sending it to the DB but we don't necessarily want to handle secrets. There should not be any secrets in appConfig but having this kind of functionality makes it future-proof so that if something sneaks in there it will be masked.

I don't think this functionality should be used for managing the workflow configuration.

Are you suggesting to dump everything in plaintext for this use-case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

what I'm thinking is that our model_dump_json() should accept an extra parameter to mask or not secrets on-demand. For the appConfig you mention, we can add the logic to mask=True while keeping a default mask=False for any other request so far, or at least for our POST in the ometa_api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ingestion safe to test Add this label to run secure Github workflows on PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants