Skip to content

Conversation

@ciyer
Copy link
Contributor

@ciyer ciyer commented Jul 14, 2025

No description provided.

Copy link
Member

@leafty leafty left a comment

Choose a reason for hiding this comment

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

Why not write the logic in internal/revproxy/main.go, in RegisterHandlers() and initializeAuth()? It would be much simpler to follow and understand which middlewares are affected by the change of configuration.

When enableV1Services is false, then you can:

  1. Skip initialization of:
    • coreSvcIdTokenAuth
    • dataGitlabAccessTokenAuth
    • gitlabTokenAuth
    • gitlabCliTokenAuth
    • notebooksGitlabAccessTokenAuth
  2. Update how the revproxy is initialized in RegisterHandlers()

Middleware() echo.MiddlewareFunc
}

func NewAuthMiddlewareProvider(ignoreGitlabTokens bool, options ...AuthOption) (AuthMiddlewareProvider, error) {
Copy link
Member

Choose a reason for hiding this comment

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

If I set ignoreGitlabTokens to true on a middleware that has nothing to do with GitLab, it will skip the auth entirely too, so the way the arguments are named here are an anti-pattern.

revproxy:
renkuBaseUrl: "https://renkulab.io"
externalGitlabUrl:
ignoreGitlabTokens: false
Copy link
Member

Choose a reason for hiding this comment

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

Misnomer: use enableV1Services: false, or use skipGitlabTokens: true

}
return srv, url
}
// TODO Unused code. Can it be deleted?
Copy link
Member

Choose a reason for hiding this comment

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

Probably yes. There may have been a test using this in the past.

@ciyer ciyer force-pushed the ciyer/no-inject-tokens branch from b8bb8ca to 20d3471 Compare July 15, 2025 09:10
@ciyer
Copy link
Contributor Author

ciyer commented Jul 15, 2025

Why not write the logic in internal/revproxy/main.go, in RegisterHandlers() and initializeAuth()? It would be much simpler to follow and understand which middlewares are affected by the change of configuration.

Thanks for the review. I made the changes around wording to avoid confusion in the names and semantics of some of the variables and parameters.

I agree that putting this logic in RegisterHandlers could make it easier to understand what exactly happens when V1 services are not enabled, but I am not familiar enough with the code to carry out the implementation this way. I chose to keep the interface the same and rely on polymorphism to realize the semantics implied by the configuration, since that seemed less risky given my knowledge of the code base.

It looks like I will not be able to complete this task, and, since it overlaps with some tasks you have coming up, feel free to use the changes in this PR, or ignore them and implement it in a different way.

@leafty
Copy link
Member

leafty commented Jul 17, 2025

Note: superseded by #770.

@ciyer ciyer closed this Oct 28, 2025
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.

3 participants