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

Secret management for discovery handler draft #61

Merged

Conversation

johnsonshih
Copy link
Contributor

No description provided.

Signed-off-by: Johnson Shih <jshih@microsoft.com>
Copy link
Collaborator

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

I like this K8s native experience!

- onvif://www.onvif.org/name/GreatONVIFCamera
- onvif://www.onvif.org/name/AwesomeONVIFCamera
discoveryTimeoutSeconds: 2
discoveryProperties:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why a generalized name of discoveryProperties instead of something more specific and narrowly scoped like deviceCredentials?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on purpose to use a generic name, users can use this field to pass information required for DH, not just credentials.

- onvif://www.onvif.org/name/AwesomeONVIFCamera
discoveryTimeoutSeconds: 2
discoveryProperties:
- name: dev-common-data
Copy link
Collaborator

Choose a reason for hiding this comment

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

In what scenario do we see this non-auth related dev-common-data needing to be passed to the DH and why here and not in discoveryDetails? IMO, if there is another category of data that needs to be passed that isnt filtering/discovery related (discoveryDetails) or auth related (`deviceCredentials) then we should make one rather than adding it to a box of everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it can be something related to credentials for example a url for DH to query information related to credentials by device id (it's similar to configMap but there is only one entry)

What's the concern of adding a generic field to pass in information to DH? If we just provide discoveryDetails and deviceCredentials, and later we have to support scenarios that is not filtering/discovery related, we will still need to add another field that is almost the same as what we provided for plain text here but just a different (redundant, IMO) field name.

Signed-off-by: Johnson Shih <jshih@microsoft.com>
@johnsonshih
Copy link
Contributor Author

implementation of this proposal: project-akri/akri#567

Signed-off-by: Johnson Shih <jshih@microsoft.com>
@johnsonshih johnsonshih merged commit ebd0e10 into project-akri:main Jul 26, 2023
@johnsonshih johnsonshih deleted the discovery-handler-auth-info branch July 26, 2023 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants