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

Feature/random hash #107

Merged
merged 38 commits into from
Nov 10, 2021
Merged

Feature/random hash #107

merged 38 commits into from
Nov 10, 2021

Conversation

jaskeerat789
Copy link
Contributor

@jaskeerat789 jaskeerat789 commented Nov 5, 2021

This PR focuses on adding random hashes to the nodes and firewalls names in task #85. The random hash generator takes in the length of the hash that needs to be appended to the name, for now, it's hardcoded to 7. The default character set for the hash is capital and small alphabet and digits.

The generator function is defined in /util/randomHash.go

  • CreateHashWithCharSet:- takes length(int) and charset(string), returning hash of type string
  • CreateHash:- takes length(int) and uses default charset, returning hash of type string

The generator is called in services/scheduler/scheduler.go:221. We check if the cluster already has a hash generated for it , if found, we go on to use it else generate a new one.

Changes also include updating of proto files to support the Hash property under Config.Cluster and change of import statement to support the new protoc.

Two functions are defined:-
- CreateHash: Takes of the hash that needs to be genreated
- CreateHashWithCharSet: takes length(length of hash) and charset(characters to be used)
Two functions are defined:-
- CreateHash: Takes of the hash that needs to be genreated
- CreateHashWithCharSet: takes length(length of hash) and charset(characters to be used)
* update Make file
* update imports in protofiles
* update server files to implement new GRPC gen code
* add Hash to config.proto
@jaskeerat789 jaskeerat789 linked an issue Nov 5, 2021 that may be closed by this pull request
4 tasks
@jaskeerat789 jaskeerat789 requested a review from a user November 5, 2021 10:42
@jaskeerat789 jaskeerat789 self-assigned this Nov 5, 2021
@bernardhalas
Copy link
Member

In general math/rand provides only a little entropy. While it's OK for node hashes, I just wanted to point this out so that we bear it in our minds. Ref. here.

Anyways, thanks for the implementation Jas!

@jaskeerat789
Copy link
Contributor Author

In general math/rand provides only a little entropy. While it's OK for node hashes, I just wanted to point this out so that we bear it in our minds. Ref. here.

Anyways, thanks for the implementation Jas!

Thanks for pointing it out @bernardhalas, but will low entropy be a problem because we just want a unique hash for a config?.... as far as I know, low entropy results in predictable hashes that why low entropy is not recommended for secrets and keys, but in our case, we can afford our hash to be predictable and unique.

If necessary, we can use the crypto/rand instead of math/rand

@bernardhalas
Copy link
Member

Thanks for pointing it out @bernardhalas, but will low entropy be a problem because we just want a unique hash for a config?.... as far as I know, low entropy results in predictable hashes that why low entropy is not recommended for secrets and keys, but in our case, we can afford our hash to be predictable and unique.

If necessary, we can use the crypto/rand instead of math/rand

For the resource hashes, I think we can keep math/rand without being worried at all.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

The changes should be ok from what I can see. Thank You @jaskeerat789 for your work and also @samuelstolicny for the asisstance.

@jaskeerat789 jaskeerat789 merged commit ddd55c2 into master Nov 10, 2021
@jaskeerat789 jaskeerat789 deleted the feature/random-hash branch November 10, 2021 08:48
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.

Feature: Create a random hash to a node name
2 participants