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

defaultRegistryUrls does not work when the manager provides a custom defaultUrl #13925

Closed
Chumper opened this issue Jan 31, 2022 · 25 comments · Fixed by #13950
Closed

defaultRegistryUrls does not work when the manager provides a custom defaultUrl #13925

Chumper opened this issue Jan 31, 2022 · 25 comments · Fixed by #13950
Labels
auto:reproduction A minimal reproduction is necessary to proceed manager:maven Maven (Java) package manager priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others stale status:requirements Full requirements are not yet known, so implementation should not be started type:bug Bug fix of existing functionality

Comments

@Chumper
Copy link
Contributor

Chumper commented Jan 31, 2022

How are you running Renovate?

Self-hosted

If you're self-hosting Renovate, tell us what version of Renovate you run.

0.0.0-semantic-release (so current main branch)

Please select which platform you are using if self-hosting.

GitHub Enterprise Server

If you're self-hosting Renovate, tell us what version of the platform you run.

doesn't matter

Was this something which used to work for you, and then stopped?

I never saw this working

Describe the bug

Assuming the following config:

{
      "managers": [
        "maven"
      ],
      "defaultRegistryUrls": [
        "https://artifactory.corp.company.com"
      ]
    },

In that case the following is expected:

  • Lookup in Maven Central
  • Lookup in the default url defined above

What happens is that Renovate will only lookup in maven central.

Reason:
The maven manager add the maven central repo as default to the dependency as seen here:
https://github.com/renovatebot/renovate/blob/main/lib/manager/maven/extract.ts#L60-L66

Result:
The result is that defaultRegistries will not work with maven as manager.

Remediation:

  • We need to make the maven manager return defaultRegistryUrls (this may need additional code).

Relevant debug logs

No response

Have you created a minimal reproduction repository?

No reproduction repository

@Chumper Chumper added priority-5-triage status:requirements Full requirements are not yet known, so implementation should not be started type:bug Bug fix of existing functionality labels Jan 31, 2022
@HonkingGoose
Copy link
Collaborator

From our defaultRegistryUrls docs: 1

defaultRegistryUrls

Override a datasource's default registries with this config option.
The datasources's customRegistrySupport value must be true for the config option to work.

Default registries are only used when both:

  • The manager did not extract any registryUrls values, and
  • No registryUrls values have been applied via config, such as packageRules

Think of defaultRegistryUrls as a way to specify the "fallback" registries for a datasource, for use when no registryUrls are extracted or configured.
Compare that to registryUrls, which are a way to override registries.

It looks like we hard-code a path to the maven registry in the Maven manager code:

const registryUrls = [MAVEN_REPO];

I think the code wouldn't reach the defaultRegistryUrls fallback code as there is a proper registryURls hardcoded into the Maven manager, so the second condition on this list is not met:

Default registries are only used when both:

  • The manager did not extract any registryUrls values, and
  • No registryUrls values have been applied via config, such as packageRules

Are you sure you're using the right config option? Maybe you actually need the registryUrls config option?

Footnotes

  1. https://docs.renovatebot.com/configuration-options/#defaultregistryurls

@Chumper
Copy link
Contributor Author

Chumper commented Jan 31, 2022

My use case is a combination of different options.

I contributed a PR to be able to extract maven registries from a settings.xml in #13592
Before that I defined registryUrls for the maven manager to have a set of registries for all repos I manage.
The problem is that this does not scale well because each registry is contacted for each maven package.

My feature mitigates that, so that I now do not need to define registryUrls anymore.
The problem is that both options cannot coexist together because the registryUrls is overwriting any maven registry even extracted one from the manager.

If I remove the registryUrls then repos that have no settings.xml and relied on the provided registries are broken.

So we introduced defaultRegistryUrls exactly for that purpose.
I can have default registries defined, that get used when no registries are extracted from the manager.

The problem is that the maven manager adds a default registry to all packages as you noticed.
This breaks the new defaultRegistryUrls feature.

@rarkins
Copy link
Collaborator

rarkins commented Jan 31, 2022

Assuming the following config:

{
      "managers": [
        "maven"
      ],
      "defaultRegistryUrls": [
        "https://artifactory.corp.company.com"
      ]
    },

In that case the following is expected:

  • Lookup in Maven Central
  • Lookup in the default url defined above

I don't think that's a reasonable expectation. If you want to look up in both, you'd need both configured in defaultRegistryUrls. Those who don't want to look up Maven Central can configure only one like you've done.

What happens is that Renovate will only lookup in maven central.

This part is not intended.

Remediation:

  • We need to make the maven manager return defaultRegistryUrls (this may need additional code).

I'm not sure we need this because the datasource already has it:

export const defaultRegistryUrls = [MAVEN_REPO];

Instead, I'd prefer that we fix the maven manager by not inserting a default registry into registryUrls and only populating it if there's something found (e.g. in settings.xml in the repo).

@rarkins rarkins added manager:maven Maven (Java) package manager priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others and removed priority-5-triage labels Jan 31, 2022
@Chumper
Copy link
Contributor Author

Chumper commented Jan 31, 2022

Instead, I'd prefer that we fix the maven manager by not inserting a default registry into registryUrls and only populating it if there's something found (e.g. in settings.xml in the repo).

If we do that, we break all repos, that have neither registryUrls or defaultRegistryUrls defined nor any settings.xml in the repo.
I am sure that are quite a lot repos.

We can only do that if we provide a default defaultRegistryUrls that has the maven central url set.

@rarkins
Copy link
Collaborator

rarkins commented Jan 31, 2022

The maven datasource already has defaultRegistryUrls. Wouldn't it still work fine?

@Chumper
Copy link
Contributor Author

Chumper commented Jan 31, 2022

Not sure, need to test that first.

@Chumper Chumper closed this as completed Jan 31, 2022
@Chumper Chumper reopened this Jan 31, 2022
@Chumper
Copy link
Contributor Author

Chumper commented Jan 31, 2022

It is at least not being used in the maven datasource as far as I can see. 🤔

@rarkins
Copy link
Collaborator

rarkins commented Jan 31, 2022

It's used by the datasource index code prior to passing to the datasource implementation itself. Take a look at the code I modified in the recent PR because it's part of that

@Chumper
Copy link
Contributor Author

Chumper commented Jan 31, 2022

You are correct, that should work. So we just need to remove the default maven central url from the extraction phase.

@HonkingGoose HonkingGoose added status:ready and removed status:requirements Full requirements are not yet known, so implementation should not be started labels Feb 1, 2022
@renovate-release
Copy link
Collaborator

🎉 This issue has been resolved in version 31.66.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@rarkins rarkins reopened this Feb 7, 2022
@rarkins
Copy link
Collaborator

rarkins commented Feb 7, 2022

Reopened due to #14057

@astellingwerf
Copy link
Collaborator

astellingwerf commented Feb 7, 2022

From my reading of the Maven 3.8.4 code, one can prevent the use of Maven Central by declaring a repository in the POM and using the ID "central".

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://maven.apache.org/POM/4.0.0"
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>
    <groupId>io.astellin</groupId>
    <artifactId>prove-13950-is-bad</artifactId>
    <version>0.0.1-SNAPSHOT</version>

    <repositories>
        <repository>
            <id>central</id>
            <name>OT Artifactory Releases Repo</name>
            <url>https://astellin.io/artifactor/does-not-exist</url>
        </repository>
    </repositories>

    <dependencies>
        <dependency>
            <groupId>junit</groupId>
            <artifactId>junit</artifactId>
            <version>4.13.1</version>
            <scope>test</scope>
        </dependency>
    </dependencies>
</project>

results in

bash-4.4# mvn org.apache.maven.plugins:maven-dependency-plugin:3.2.0:tree
[INFO] Scanning for projects...
Downloading from central: https://repo.maven.apache.org/maven2/org/apache/maven/plugins/maven-dependency-plugin/3.2.0/maven-dependency-plugin-3.2.0.pom
Downloaded from central: https://repo.maven.apache.org/maven2/org/apache/maven/plugins/maven-dependency-plugin/3.2.0/maven-dependency-plugin-3.2.0.pom (18 kB at 60 kB/s)
Downloading from central: https://repo.maven.apache.org/maven2/org/apache/maven/plugins/maven-plugins/34/maven-plugins-34.pom
Downloaded from central: https://repo.maven.apache.org/maven2/org/apache/maven/plugins/maven-plugins/34/maven-plugins-34.pom (11 kB at 232 kB/s)
Downloading from central: https://repo.maven.apache.org/maven2/org/apache/maven/maven-parent/34/maven-parent-34.pom
Downloaded from central: https://repo.maven.apache.org/maven2/org/apache/maven/maven-parent/34/maven-parent-34.pom (43 kB at 840 kB/s)
Downloading from central: https://repo.maven.apache.org/maven2/org/apache/apache/23/apache-23.pom
Downloaded from central: https://repo.maven.apache.org/maven2/org/apache/apache/23/apache-23.pom (18 kB at 526 kB/s)
Downloading from central: https://repo.maven.apache.org/maven2/org/apache/maven/plugins/maven-dependency-plugin/3.2.0/maven-dependency-plugin-3.2.0.jar
Downloaded from central: https://repo.maven.apache.org/maven2/org/apache/maven/plugins/maven-dependency-plugin/3.2.0/maven-dependency-plugin-3.2.0.jar (205 kB at 3.2 MB/s)
[INFO]
[INFO] -------------------< io.astellin:prove-13950-is-bad >-------------------
[INFO] Building prove-13950-is-bad 0.0.1-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
Downloading from central: https://astellin.io/artifactor/does-not-exist/junit/junit/4.13.1/junit-4.13.1.pom
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  21.901 s
[INFO] Finished at: 2022-02-07T11:28:25Z
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal on project prove-13950-is-bad: Could not resolve dependencies for project io.astellin:prove-13950-is-bad:jar:0.0.1-SNAPSHOT: Failed to collect dependencies at junit:junit:jar:4.13.1: Failed to read artifact descriptor for junit:junit:jar:4.13.1: Could not transfer artifact junit:junit:pom:4.13.1 from/to central (https://astellin.io/artifactor/does-not-exist): transfer failed for https://astellin.io/artifactor/does-not-exist/junit/junit/4.13.1/junit-4.13.1.pom: Connect to astellin.io:443 [astellin.io/35.156.220.145] failed: Connection refused -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/DependencyResolutionException

@Chumper
Copy link
Contributor Author

Chumper commented Feb 7, 2022

yes, that is also my understanding.

Ok, here is a quick overview of my understanding on how Maven works.

  1. There is a super pom which every pom.xml explicitly extend from: https://maven.apache.org/guides/introduction/introduction-to-the-pom.html#super-pom
    Here the default maven repo is defined.
    So the maven repo is always set unless explicitly overwritten.

  2. Each repository has an id that can be used to create mirrors for it, which are defined in the settings.xml

What we need to do is the following:

  • When extracting dependencies, we should NOT set the registry, because we do not know which ones it belongs to.
  • When extracting registries, we need to extract the id as well.
  • When extracting from the settings.xml, then we replace registries with the same id.

I am not sure yet how we can add the registryUrls and fallbackRegistryUrls to this.

@Chumper
Copy link
Contributor Author

Chumper commented Feb 7, 2022

I still think that registryUrls should overwrite all registries for maven dependencies.
It is not that useful in a corporate environment, like mine, but it still is useful.

For fallbackRegistryUrls we should do the following:

  • when looking up updates for dependencies and no registry is able to answer the request,
  • only then we should add the fallbackRegistryUrls

Alternatively, because this is such a special use case (which only I would be using) we should disable fallbackRegistryUrls alltogether.

In that case I would still improve the extraction from the settings.xml to be more robust.
And I would tell people that have no settings.xml defined, that they are our of luck and need to migrate their config.

@Chumper
Copy link
Contributor Author

Chumper commented Feb 7, 2022

TLDR:

  • Remove fallbackRegistryUrls
  • Improve registry extraction from settings.xml and pom.xml files
  • Merge registries based on IDs with settings.xml taking precedence

@rarkins @astellingwerf Any opinions on that?

@astellingwerf
Copy link
Collaborator

I'm not too aware of the implementation of manager/maven, so my rather uninformed idea: Would it make sense to have Maven itself be responsible to render the effective pom:
mvn org.apache.maven.plugins:maven-help-plugin:3.2.0:effective-pom -Doutput=...

@Chumper
Copy link
Contributor Author

Chumper commented Feb 7, 2022

Afaik the reason we are not using maven for the pom parsing/generation is that Renovate is much faster with the typescript implementation, although not that feature rich.

@rarkins
Copy link
Collaborator

rarkins commented Feb 7, 2022

Yeah, we now have zero calls to child_process for parsing so we really want to avoid that at all costs

@astellingwerf
Copy link
Collaborator

astellingwerf commented Feb 7, 2022

OK, fair enough, but that means that the cost is to reimplement Maven's POM parsing logic in Renovate (or reuse a npm package that can do it?).

@rarkins
Copy link
Collaborator

rarkins commented Feb 7, 2022

I don't think we necessarily need to pass the global pom if the only field we care about is the default repository and we already know what it contains

@EvertonSA
Copy link

#18803

@rarkins rarkins added the auto:reproduction A minimal reproduction is necessary to proceed label Apr 21, 2023
@github-actions
Copy link
Contributor

Hi there,

Get your issue fixed faster by creating a minimal reproduction. This means a repository dedicated to reproducing this issue with the minimal dependencies and config possible.

Before we start working on your issue we need to know exactly what's causing the current behavior. A minimal reproduction helps us with this.

To get started, please read our guide on creating a minimal reproduction.

We may close the issue if you, or someone else, haven't created a minimal reproduction within two weeks. If you need more time, or are stuck, please ask for help or more time in a comment.

Good luck,

The Renovate team

@rarkins rarkins added status:requirements Full requirements are not yet known, so implementation should not be started and removed status:ready labels Apr 21, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2023

When a bug has been marked as needing a reproduction, it means nobody can work on it until one is provided. In cases where no reproduction is possible, or the issue creator does not have the time to reproduce, we unfortunately need to close such issues as they are non-actionable and serve no benefit by remaining open. This issue will be closed after 7 days of inactivity.

@github-actions github-actions bot added the stale label May 8, 2023
@github-actions
Copy link
Contributor

This bug report has been closed as we need a reproduction to work on this. If the original poster or anybody else with the same problem discovers that they can reproduce it, please create a new issue, and reference this issue.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 15, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto:reproduction A minimal reproduction is necessary to proceed manager:maven Maven (Java) package manager priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others stale status:requirements Full requirements are not yet known, so implementation should not be started type:bug Bug fix of existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants