uselagoon / build-deploy-tool

Tool to generate build resources
2 stars 5 forks source link

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

Open AlexSkrypnyk opened 2 months ago

AlexSkrypnyk commented 2 months ago

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 commented 2 months ago

There is an alternative way to solve this (since https://github.com/uselagoon/build-deploy-tool/pull/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
AlexSkrypnyk commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

This PR https://github.com/uselagoon/build-deploy-tool/pull/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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.