Replace ddmark with go-validator

[ptnapoleon]

What does this PR do?

• Adds new functionality
• Alters existing functionality
• Fixes a bug
• Improves documentation or testing

Please briefly describe your changes as well as the motivation behind them:

• Draft PR, wip

Code Quality Checklist

• The documentation is up to date.
• My code is sufficiently commented and passes continuous integration checks.
• I have signed my commit (see Contributing Docs).

Testing

• I leveraged continuous integration testing
  • by depending on existing unit tests or end-to-end tests.
  • by adding new unit tests or end-to-end tests.
• I manually tested the following steps:
  • x
  • locally.
  • as a canary deployment to a cluster.

[datadog-datadog-prod-us1]

🔵 Code Quality Violation

avoid using a bare return (...read more)

The "Avoid bare returns" rule in Go static analysis is designed to increase clarity and readability in your code. A bare return is when a function that has named return values returns those values implicitly, without explicitly stating what is being returned.

While Go's allowance for bare returns can make code more concise, it can also make it more difficult to understand and debug, especially in larger functions. Implicitly relying on the state of named return values can lead to unexpected behavior if those values are modified elsewhere in the function.

To adhere to this rule and promote better coding practices, always explicitly return values in your functions. This makes it clear what values are being returned and in what state, reducing the chance of bugs and making your code easier to understand. For example, instead of writing return in a function that returns an int and a bool, write return 0, false.

Learn More

• Named return values
• Proposal to remove bare returns

    

[datadog-datadog-prod-us1]

🔵 Code Quality Violation

code block could be outside the else (...read more)

Sometimes, the else block of an if is unnecessary and can be avoided, especially when there is a return statement inside the if block will be executed and the function will terminate. Hence, there is no need to explicitly specify the else clause.

By removing the else clause, the code becomes more concise and easier to read. It eliminates redundancy and makes the intention of the code clearer.

To avoid such situations, it is a good coding practice to write code with the fewest possible branches. This makes the code easier to understand and minimizes the chance of introducing bugs.

    

[datadog-datadog-prod-us1]

🔵 Code Quality Violation

Condition could be directly returned (...read more)

In Go, it is considered unnecessary and less readable to write a conditional statement that returns true or false explicitly based on a condition. Instead, it is recommended to directly return the condition itself. Here's why:

1. Code simplicity and readability: Writing return condition directly conveys the intent of the code more clearly and reduces unnecessary verbosity. It is easier for other developers to understand your code at a glance without having to analyze additional conditional statements.
2. Avoidance of redundancy: Explicitly returning true or false based on a condition introduces redundancy in the code. Since the condition itself already evaluates to a boolean value (true or false), there is no need to include additional return true or return false statements.
3. Maintainability and refactoring: The direct return condition approach is more maintainable and flexible for future code changes. If the condition or the desired return value changes, it is easier to modify a single line rather than multiple lines of code. This minimizes the chances of introducing errors or inconsistencies during refactoring.

Therefore, it is recommended to simply use return condition instead of if condition { return true } return false. By doing so, you improve code readability, reduce redundancy, and ensure better maintainability of your Go code.

    

[datadog-datadog-prod-us1]

🔵 Code Quality Violation

code block could be outside the else (...read more)

Sometimes, the else block of an if is unnecessary and can be avoided, especially when there is a return statement inside the if block will be executed and the function will terminate. Hence, there is no need to explicitly specify the else clause.

By removing the else clause, the code becomes more concise and easier to read. It eliminates redundancy and makes the intention of the code clearer.

To avoid such situations, it is a good coding practice to write code with the fewest possible branches. This makes the code easier to understand and minimizes the chance of introducing bugs.

    

[datadog-datadog-prod-us1]

🔵 Code Vulnerability

Use of unsafe reflection, make sure your code is safe (...read more)

In Go, unsafe reflection refers to the use of the reflect package in combination with the unsafe package to modify or access private or unexported fields of a struct. It allows bypassing the normal visibility rules and type safety of Go's language design. Unsafe reflection can be useful in certain situations, but it also comes with significant risks and should be used with caution.

To prevent unsafe reflection and maintain the safety and integrity of your code, you can follow the following practices:

1. Use Exported Fields: Ensure that the fields you need to access or modify are exported (start with an uppercase letter) and provide appropriate getter and setter methods. By using exported fields and methods, you enforce encapsulation and prevent direct access to private fields.
2. Restrict Access: Keep private or unexported fields inaccessible from outside packages. This encapsulation helps maintain the integrity of your codebase and prevents unintended modifications.
3. Avoid unsafe Package: Minimize the use of the unsafe package, especially when working with reflection. The unsafe package removes the safety features of Go and can lead to unpredictable behavior. Instead, try to solve the problem using idiomatic Go constructs whenever possible.
4. Use Tags for Reflection: When using reflection for serialization or deserialization, consider using struct tags (reflect tags) to provide metadata and annotations. This approach allows you to access struct fields without relying on direct access to their memory.
5. Avoid Untrusted Input: Be cautious when using reflection with user-supplied or untrusted input. Reflection can be a powerful tool, but it can also introduce security vulnerabilities if not used carefully. Validate and sanitize any input before using reflection to avoid potential risks, such as injection attacks.
6. Code Review and Testing: Review any usage of reflection and unsafe operations carefully. Conduct thorough code reviews and rigorous unit and integration testing to ensure that your code behaves as expected. This can help identify potential issues and validate the correctness and safety of your code.

It is important to note that reflection has its use cases and can provide powerful functionality when used appropriately. However, due to its potential for code complexity and security vulnerabilities, it is advisable to explore alternative solutions and use reflection sparingly.

By following these best practices and understanding the risks associated with unsafe reflection, you can prevent unsafe operations and maintain the safety and integrity of your Go code.

    

[datadog-datadog-prod-us1]

🔵 Code Quality Violation

avoid using a bare return (...read more)

The "Avoid bare returns" rule in Go static analysis is designed to increase clarity and readability in your code. A bare return is when a function that has named return values returns those values implicitly, without explicitly stating what is being returned.

While Go's allowance for bare returns can make code more concise, it can also make it more difficult to understand and debug, especially in larger functions. Implicitly relying on the state of named return values can lead to unexpected behavior if those values are modified elsewhere in the function.

To adhere to this rule and promote better coding practices, always explicitly return values in your functions. This makes it clear what values are being returned and in what state, reducing the chance of bugs and making your code easier to understand. For example, instead of writing return in a function that returns an int and a bool, write return 0, false.

Learn More

• Named return values
• Proposal to remove bare returns

    

[datadog-datadog-prod-us1]

🔵 Code Quality Violation

Condition could be directly returned (...read more)

In Go, it is considered unnecessary and less readable to write a conditional statement that returns true or false explicitly based on a condition. Instead, it is recommended to directly return the condition itself. Here's why:

1. Code simplicity and readability: Writing return condition directly conveys the intent of the code more clearly and reduces unnecessary verbosity. It is easier for other developers to understand your code at a glance without having to analyze additional conditional statements.
2. Avoidance of redundancy: Explicitly returning true or false based on a condition introduces redundancy in the code. Since the condition itself already evaluates to a boolean value (true or false), there is no need to include additional return true or return false statements.
3. Maintainability and refactoring: The direct return condition approach is more maintainable and flexible for future code changes. If the condition or the desired return value changes, it is easier to modify a single line rather than multiple lines of code. This minimizes the chances of introducing errors or inconsistencies during refactoring.

Therefore, it is recommended to simply use return condition instead of if condition { return true } return false. By doing so, you improve code readability, reduce redundancy, and ensure better maintainability of your Go code.

    

[datadog-datadog-prod-us1]

🟠 Code Quality Violation

Suggested change

	current.param = strings.Replace(strings.Replace(vals[1], utf8HexComma, ",", -1), utf8Pipe, "|", -1)
	current.param = strings.ReplaceAll(strings.Replace(vals[1], utf8HexComma, ",", -1), utf8Pipe, "|")

Do not call Replace with a negative limit, use ReplaceAll instead (...read more)

In Go, the strings.Replace() function is used to replace a certain substring within a string with another substring. The function takes in four parameters: the original string, the old substring to be replaced, the new substring that will replace the old one, and an integer limit dictating how many replacements to be made.

Calling strings.Replace() with a negative limit doesn't really make sense. According to the Go documentation, if limit is negative, there is no limit on the number of replacements. Which means it will replace all instances of old substring in the original string with a new substring.

For example:

fmt.Println(strings.Replace("oink oink oink", "k", "ky", -2))

In this example, Replace returns a copy of the string "oink oink oink" where "k" is replaced by "ky" everywhere it appears, because limit is -2.

So it's not necessarily "incorrect" to use a negative limit, but it can create misunderstandings in your code. It's best to use a limit of -1 when you want to replace all instances, as this convention is more commonly understood to mean "no limit".

But if you specifically want to avoid using negative limit for Replace or looking for replace method with better efficiency, using strings.NewReplacer() could be a better option when there are multiple string pairs need to be replaced, where you can specify a list of old-new string pairs.

Or you can use strings.ReplaceAll(). It is equivalent to Replace with a limit of -1. It's arguably clearer and more self-explanatory than using a negative limit with strings.Replace().

For example:

fmt.Println(strings.ReplaceAll("oink oink oink", "o", "ky"))

It replaces all instances of "o" in the string "oink oink oink" by "ky".

    

[datadog-datadog-prod-us1]

🟠 Code Quality Violation

Suggested change

	vals[i] = strings.Replace(vals[i], "'", "", -1)
	vals[i] = strings.ReplaceAll(vals[i], "'", "")

Do not call Replace with a negative limit, use ReplaceAll instead (...read more)

In Go, the strings.Replace() function is used to replace a certain substring within a string with another substring. The function takes in four parameters: the original string, the old substring to be replaced, the new substring that will replace the old one, and an integer limit dictating how many replacements to be made.

Calling strings.Replace() with a negative limit doesn't really make sense. According to the Go documentation, if limit is negative, there is no limit on the number of replacements. Which means it will replace all instances of old substring in the original string with a new substring.

For example:

fmt.Println(strings.Replace("oink oink oink", "k", "ky", -2))

In this example, Replace returns a copy of the string "oink oink oink" where "k" is replaced by "ky" everywhere it appears, because limit is -2.

So it's not necessarily "incorrect" to use a negative limit, but it can create misunderstandings in your code. It's best to use a limit of -1 when you want to replace all instances, as this convention is more commonly understood to mean "no limit".

But if you specifically want to avoid using negative limit for Replace or looking for replace method with better efficiency, using strings.NewReplacer() could be a better option when there are multiple string pairs need to be replaced, where you can specify a list of old-new string pairs.

Or you can use strings.ReplaceAll(). It is equivalent to Replace with a limit of -1. It's arguably clearer and more self-explanatory than using a negative limit with strings.Replace().

For example:

fmt.Println(strings.ReplaceAll("oink oink oink", "o", "ky"))

It replaces all instances of "o" in the string "oink oink oink" by "ky".

    

[datadog-datadog-prod-us1]

⚫ Library Vulnerability

golang.org/x/crypto → 0.26.0

Suggested change

	golang.org/x/crypto v0.26.0 // indirect
	golang.org/x/crypto vv0.31.0// indirect

Misuse of ServerConfig.PublicKeyCallback may cause authorization bypass in golang.org/x/crypto (...read more)

Applications and libraries which misuse the ServerConfig.PublicKeyCallback callback may be susceptible to an authorization bypass.

The documentation for ServerConfig.PublicKeyCallback says that "A call to this function does not guarantee that the key offered is in fact used to authenticate." Specifically, the SSH protocol allows clients to inquire about whether a public key is acceptable before proving control of the corresponding private key. PublicKeyCallback may be called with multiple keys, and the order in which the keys were provided cannot be used to infer which key the client successfully authenticated with, if any. Some applications, which store the key(s) passed to PublicKeyCallback (or derived information) and make security relevant determinations based on it once the connection is established, may make incorrect assumptions.

For example, an attacker may send public keys A and B, and then authenticate with A. PublicKeyCallback would be called only twice, first with A and then with B. A vulnerable application may then make authorization decisions based on key B for which the attacker does not actually control the private key.

Since this API is widely misused, as a partial mitigation golang.org/x/[email protected] enforces the property that, when successfully authenticating via public key, the last key passed to ServerConfig.PublicKeyCallback will be the key used to authenticate the connection. PublicKeyCallback will now be called multiple times with the same key, if necessary. Note that the client may still not control the last key passed to PublicKeyCallback if the connection is then authenticated with a different method, such as PasswordCallback, KeyboardInteractiveCallback, or NoClientAuth.

Users should be using the Extensions field of the Permissions return value from the various authentication callbacks to record data associated with the authentication attempt instead of referencing external state. Once the connection is established the state corresponding to the successful authentication attempt can be retrieved via the ServerConn.Permissions field. Note that some third-party libraries misuse the Permissions type by sharing it across authentication attempts; users of third-party libraries should refer to the relevant projects for guidance.