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

Inconsistent handling of the userame for container-registries in .lagoon.yml #309

Open
AlexSkrypnyk opened this issue Apr 25, 2024 · 13 comments
Labels
needs-docs documentation updates needed wontfix This will not be worked on

Comments

@AlexSkrypnyk
Copy link

AlexSkrypnyk commented Apr 25, 2024

Docs say

container-registries:
  my-custom-registry:
    username: myownregistryuser
    password: <registry_password_variable_name>
    url: my.own.registry.com

This works.

username: DOCKER_USER does not take the value from the $DOCKER_USER variable. It uses DOCKER_USER as a literal string.

But the expectation is that a username can also be provided from a variable.

container-registries:
  dockerhub:
    username: DOCKER_USER
    password: DOCKER_PASS

It is not clear why this configuration item is treated in a custom way.

This adds an additional step to the project setup where I need to explicitly modify the .lagoon.yml file to put the name instead of just populate the config map with another variable.

@shreddedbacon
Copy link
Member

shreddedbacon commented Apr 25, 2024

There is an alternative way to solve this (since #263) that the documentation needs to be updated to explain.

The way we address this, and the way we will support now, is to add variable(s) to the project or environment in the API that are named like this.

REGISTRY_dockerhub_USERNAME="mydockerhubusername"
REGISTRY_dockerhub_PASSWORD="mydockerhubpassword"

This way you don't need to update the the .lagoon.yml file with specific variable names.

If you use an alternative registry, and your .lagoon.yml looks like this (specifying the URL):

container-registries:
  quayio:
    username: quayuser
    password: QUAY_PASSWORD
    url: quay.io

Then your variables would be this, where the registry name quayio is used in the variable name

REGISTRY_quayio_USERNAME="myquayiousername"
REGISTRY_quayio_PASSWORD="myquayiopassword"

The downside to this is that you still need to define both the username and password in the .lagoon.yml, but they can just be set to anything, because the variables will be consumed if they exist, eg:

container-registries:
  dockerhub:
    username: placeholder
    password: placeholder

@shreddedbacon shreddedbacon added wontfix This will not be worked on needs-docs documentation updates needed labels Apr 25, 2024
@AlexSkrypnyk
Copy link
Author

AlexSkrypnyk commented Apr 25, 2024

@shreddedbacon
username: DOCKER_USER does not take the value from the $DOCKER_USER variable. It uses DOCKER_USER as a string.

Is this a deliberate approach or a defect?

@shreddedbacon
Copy link
Member

shreddedbacon commented Apr 25, 2024

@shreddedbacon username: DOCKER_USER does not take the value from the $DOCKER_USER variable. It uses DOCKER_USER as a string.

Yes, I know. We don't plan to have it ever consume a variable that is defined in the .lagoon.yml. You need to use the REGISTRY_dockerhub_USERNAME variable in the API then the username provided in the .lagoon.yml is ignored entirely.

@AlexSkrypnyk
Copy link
Author

This issue is not about any new ways of providing those variables. It is about inconsistency in handling configuration: one config value supports variables and another does not. this is weird.
Is there a reason to not support this?


REGISTRY_dockerhub_USERNAME don't you think that mixing lowercase and uppercase letters in the variable names is not a widely-spread pattern?

@shreddedbacon
Copy link
Member

This issue is not about any new ways of providing those variables. It is about inconsistency in handling configuration: one config value supports variables and another does not. this is weird. Is there a reason to not support this?

Yes, I understand your frustration. The documentation needs to be updated to indicate the recommended path. The way the password is consumed using a variable override is NOT going to be our recommended approach, and the documentation will be updated to reflect this. The recommended approach will be to use the REGISTRY_x_USERNAME|PASSWORD variables

REGISTRY_dockerhub_USERNAME don't you think that mixing lowercase and uppercase letters in the variable names is not a widely-spread pattern?

I understand the concern here, but this is just because the name is matched against what is defined in the .lagoon.yml. We could look to support uppercased versions too, but for now it is lowercase only.

@AlexSkrypnyk
Copy link
Author

This PR #263 allows you to override what is already defined in .lagoon.yml file. Cool

But

the name is matched against what is defined in the .lagoon.yml

Does it mean that i must have container-registries.dockerhub.username and container-registries.dockerhub.password set to something in .lagoon.yml to use the new way REGISTRY_dockerhub_USERNAME? Or can I just remove that section altogether and simple add REGISTRY_dockerhub_USERNAME and REGISTRY_dockerhub_PASSWORD to my variables in the project?

As a developer, I want to have 1 way and I want it to be consistent.

@shreddedbacon
Copy link
Member

Yes, I mentioned that above. I know this isn't great, but we need to work out how we can address it in a different way.

The downside to this is that you still need to define both the username and password in the .lagoon.yml, but they can just be set to anything, because the variables will be consumed if they exist, eg:

container-registries:
  dockerhub:
    username: placeholder
    password: placeholder

@AlexSkrypnyk
Copy link
Author

AlexSkrypnyk commented Apr 26, 2024

@shreddedbacon
Thank you for confirming.

Does this whole thing need to be so complicated? Can we just have 2 ways without magic naming like REGISTRY_x_USERNAME:

container-registries:
  dockerhub:
    username: actual_value
    password: actual_value

and

container-registries:
  dockerhub:
    username: $MY_USERNAME
    password: $MY_PASS

At the end, it all adds up to the developer's experience.

@shreddedbacon
Copy link
Member

shreddedbacon commented Apr 26, 2024

Does this whole thing need to be so complicated? Can we just have 2 ways without magic naming like REGISTRY_x_USERNAME:

It isn't really that complicated though, the reason we decided to go with the REGISTRY_x prefixed variables is so that what is in the .lagoon.yml does not really matter at all, and it is then known that to change the password or set the username for a specific registry, the variable name is not left up to guess work, or for a developer to think about what to call their variables. The convention is set in Lagoon, it is REGISTRY_${registryname}_USERNAME or REGISTRY_${registryname}_PASSWORD.

As it is possible to have multiple container-registries, for example if I had the following registries:

container-registries:
  dockerhub:
    username: ignoreduser
    password: ignoredpass
  quayio:
    username: ignoreduser
    password: ignoredpass
    url: quay.io
  ghcrio:
    username: ignoreduser
    password: anexpiredpassword
    url: ghcr.io

I know that I can quickly change the password for ghcrio by setting or changing the REGISTRY_ghcrio_PASSWORD in the API, without having to remember what the variable name could be in the .lagoon.yml.

It also means a user can on the fly change a hardcoded password in the .lagoon.yml with one from the API in the situation where they may not be able to update the .lagoon.yml with the new password quickly.

@AlexSkrypnyk
Copy link
Author

@shreddedbacon
thank you fro explaining in more detail. it all makes sense.

But my original question still stands: why is username field is not handled in the same was as password? is this on purpose? or is this a bug? (whether you are going to support this way or not going forward does not matter in this context - it is already exists in projects).

@shreddedbacon
Copy link
Member

But my original question still stands: why is username field is not handled in the same was as password? is this on purpose? or is this a bug? (whether you are going to support this way or not going forward does not matter in this context - it is already exists in projects).

It was a decision that was made at the time container registry support was introduced. At the time it was initially introduced we needed support for being able to change passwords. Usernames were probably not considered as something that would be changing often and hardcoding them was seen as acceptable.

After seeing how the majority of our customers in AIO cloud utilised this feature, and some feedback we received from one of our larger customers, we revised it and ended up on the REGISTRY_x method because it is more flexible overall due to not requiring any changes to the .lagoon.yml file.

It is also possible that in the future you may be able to define container-registries entirely in the API without needing to set anything in your .lagoon.yml at all.

@AlexSkrypnyk
Copy link
Author

Thank you for explaining. Looks like this was an intentional decision.

It would help people like me, who set up projects, to have a consistent way to define both username and password. To me, this issue is still relevant, but please feel free to close if there’s no intention to support this way going forward.

@shreddedbacon
Copy link
Member

We will get the documentation updated to mention using the REGISTRY_x variable method as our main supported way of doing usernames/passwords via envvars.

The current method described in the docs will be moved to a "still supported, but not recommended" section to remain backwards compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-docs documentation updates needed wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants