[DNM without first PR and rebase] Add groups into the rolebindings #60
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Without this patch, we are filtering LDAP groups and take a decision on what to expose. This is a problem, as it removes the flexibility of rolebindings later. We intend to expose custom role bindings for extra services (for example Kong), which requires teams to be entering a different model. In that case, the platform team creates a new cluster role, but the grantee of the role might come from a deployment tool, hence outside the operator. I fixed it by first exposing the groups directly in the token provider. This means that further commits are required to filter properly + generate the right token groups directly.
Without this, the code was duplicated and the generation of claims was not very readable. For example, it contained steps that are part of the issuer initialisation. This is a problem, as it leads to difficult reviews and difficulty to iterate on the topic. This fixes it by creating a new constructor for the token issuer, which clarifies all what's necessary for it, and fatals if the requirements are not met. This makes sure the code does not error forever when the requirements are not met. On top of that, the signature of the token was separated, allowing easier testing. The testing has shown a lack of handling the errors in the JWT signature checks, which should be fixed in a later commit.
Without this, creating a token provider with no private key will still try to sign the token. This should never happen. However, acting on it, even on tests, means a generic panic, instead of a just an error. This handles the pointer dereferencing to ensure the code does not panic, as it should already fatal on main through the constructor.
Without this, the basic auth is included deep in the call stack. This is a problem, as it means multiple calls have the information about passwords, and need to carry useless data around. This fixes it by ensuring a new middleware for basicAuth was added, directly connecting to ldap. The user information is then filtered to keep only username, userDN, and email. It is then passed in a context for use in the next httpHandlers. At the same time, it allowed me to see that the GenerateConfig would not fail if the generation of the token results in an error. I fixed it by adding the same logic in GenerateConfig and in GenerateJWT http handlers.
Without this we will not be able to refactor GenerateJWT and GerateConfig. This further allows testability for the config generation.
This allows simplification of the code.
This makes it easier to reason around the getUser details.
Without this, the naming is a bit hard to follow, and the indentation is not really matching the usual go patterns. This fixes it to make the code more readable.
Without this, we validate the token format twice: one when doing parsewithclaims, one when we do our own validation. This serves no purposes, and introduces fragile code. This fixes it by removing the useless commits.
We use different but similiar constructor methods for the Has* calls. This is a problem, as it makes the ldap methods unnecessary harder to read. This fixes it by ensuring the code is more readable, and allowed us optimisations, like regrouping code to connect, query, and return results with our defaults.
Without this, one might expect to have groups[] to be always populated, and passed to JWTClaims generation. However, this is not the case, as the groups are empty if the user does not have direct access rights to the cluster. This is a problem, as it will prevent further evolution of the group management. To fix this, I made sure that _ALL THE GROUPS_, including the special groups (appops, customerops, cloudops, containerops) have their groups fetched and generated in the JWT. I also moved the code from ldap to project.go, as none of the code was actually relevant from ldap perspective: All the code was manipulating project objects. Tests were added to ensure the behaviour was intact. The code also took the opportunity to remove incorrectly exported functions back to internal functions (and fixing their tests).
Now that authprovider is merely doing ldap functions (it was already doing that), be explicit and call the package ldap.
Presenting the CA is a very small endpoint, lost in the middle of the "services" internal package. This is a problem, as it makes it annoying to find. On top of that, it needed to be passed global variables instead of having direct access to config data. This fixes it by making sure this endpoint (only used once) is directly readable from the main, as it's a two-liner.
Services does not really means what this code does. This is a problem, as it makes the debugging tedious for a new contributor. This fixes it by moving the middlewares to their own package.
Without this, one might wonder where the constants are used. One might even think that Service account is used to provision some parts of kubi, where in fact it is only used for auth. This is a problem, as it could lead to misunderstandings in the code. In other words, I believe that moving the constants will make it more explicit about their scope and maintenance. Therefore it makes clear that the constants moved here are ONLY usable for auth. To avoid issues, this removes the constant from generic "utils" use.
Without this, the helpers become a big mess of functions that are only partially used or are inferior in implementation to what you can find in other parts of the code. This fixes it by: - regroup the config parsing into config.go - inlining functions with little use, which can be replaced by a few idiomatic golang calls. - temporarily moving some implementations until the inlining is safe to implement (needs test coverage) - removing duplicate calls: ldap host parsing in the config, fatal + os.exit(1)...
This commit tidies the go.mod after cleaning the code removing the usage of ozzo validation.
the createAccessToken method is bubbling up errors, but we never show them. This is a problem, as it prevents observability of the errors. This fixes it by logging the issues at error level.
Without this, much code is relying on global variables, or to state in the different functions. This is a problem, as it removes the testability, and code is hard to refactor. This fixes it by slowly getting away from spaghetti code, starting by refactoring the data structures. This prevents leakage of data. It will also allow us later to get rid of the global variables. This makes the code more idiomatic, and split the internal packages based on what they should provide (and how can they interface). Therefore, the code should be more readable in the long run.
The code of the operator was in a massive single file. To improve readability, this was split into multiple files. It also took the liberty to merge the anonymous function with the handler doing approximatively the same work, as none of those were tested.
A few calls are long duplicates. Without this patch, the code for netpols, rolebindings, and projects is duplicated in a few methods. This is a problem, as it makes the code harder to read, and harder to maintain. This fixes it by refactoring the calls, making them more testable (although without adding tests), and making them simpler to read.
While those are not technically necessary, they have helped me in my refactors. Instead of throwing them away, I will keep them here. If necessary, delete them, as they were refactor unit tests.
Without this, we will rely on our legacy group names. This is a problem, as it means we do not gain the flexibility brought by the previous "add group" feature automatically in each project. This fixes it by ensuring the rolebinding adds a new line for the users of the group.
No reason to use it in utils if it is not used in a global way.
The kubeconfig generation does not need to use global variables, if they are passed from issuer/global CA. This simplifies the code further to improve testability.
If the public key is nil, the key validation function from ParseWithClaims should NOT return the issuer public Key. The code snippet checks if the EcdsaPublic field of the issuer (an instance of TokenIssuer) is nil. The EcdsaPublic field holds the public key used to verify the token's signature. If this field is nil, it means that the public key is not available, and the method cannot proceed with the token verification. If the EcdsaPublic field is indeed nil, the method now returns nil and an error. This ensures that the caller of the VerifyToken method is informed about the missing public key, which is crucial for debugging and handling the error appropriately. This check is essential to prevent the method from attempting to verify the token without a valid public key, which would result in a runtime error or incorrect verification results.
evrardjp-cagip
force-pushed
the
add_groups_followups
branch
from
ffb340e to
87989dd
Compare
evrardjp-cagip
changed the title
evrardjp-cagip
changed the title
|
Superseded by #61 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This needs the other PR about the addition of groups to merge first.