spring-cloud
Tracking Git branch instead of tag
The JGitEnvironmentRepository has the method called containsBranch that is checking whether label is branch or not (line 743). Imaging we have
label = foo branch = feature/foo tag = foo
Given that it returns true because ref name ends with /foo.
Expected: Returns false.
See the section on labels here https://docs.spring.io/spring-cloud-config/docs/current/reference/html/#_git_backend
I'm very well aware of following statement from documentation
If the git branch or tag name contains a slash (/), then the label in the HTTP URL should instead be specified with the special string (_) (to avoid ambiguity with other URL paths).
But this is different. It has nothing to do with the HTTP URL.
How to reproduce
-
Given branch=feature/foo tag=foo
-
Send following request to Config Server
/application/default/foo -
JGitEnvironmentRepository.containsBranchreturns true becauserefs/remotes/origin/feature/fooends with/foo -
checkout.call(), line 468 throwsRefNotFoundException. Ref origin/foo cannot be resolved. Which is true as this branch does not exist.
I think the problem here is the ambiguity of label. If you have a branch and a tag with the same name how do we know which one we should be using? This problem is a subset of that problem where the branch name ends with the tag. I am not sure what the best way to solve the problem is. The only thing I can come up with is adding something to the label that identifies whether you are specifically talking about a branch or a tag. Or we might be able to not use endsWith and force the label to be the complete branch name when specifying a label.
This code has been this way for 8 years and this is the first time we have seen this issue, so while its an issue it appears to be rare.
If you have a branch and a tag with the same name how do we know which one we should be using?
In this case yes. We could say that branch has higher priority and request would not fail. However this is not the case. Branch name is different to tag one. The problem is using endsWith method and / as a separator. One possible solution is to normalize ref name before performing full match, i.e.
refs/heads/main -> main
refs/remotes/origin/feature/foo -> feature/foo
Or maybe the containsBranch method can be replaced by calling @Bean annotated component allowing people to overwrite default implementation.
This code has been this way for 8 years and this is the first time we have seen this issue, so while its an issue it appears to be rare.
Maybe people got used to it and found workarounds. This does not mean that Config Server should not support using / character to create a hierarchical name scheme.
One possible solution is to normalize ref name before performing full match
I don't think thats possible because it assumes the prefixes of the branch name are known, and thats not the case, that prefix can be anything technically. Yes we know there may be refs/head/ or refs/remotes/origin but there could be something like refs/remotes/my/remote/stuff/feature/foo.
Or maybe the containsBranch method can be replaced by calling @Bean annotated component allowing people to overwrite default implementation.
We could probably do that without much trouble I think. Would you be interested in making a PR?
I spend some time on this issue and don't think this is a bug.
The method containsBranch in class JGitEnvironmentRepository do have small problem dealing with branch name contains slashes (/). The return of this method may be wrong. But the wrong returns will not cause any problem.
First, there is nothing to do with tags. containsBranch only list branches, not tags. Please see the code below.
private boolean containsBranch(Git git, String label, ListMode listMode) throws GitAPIException {
ListBranchCommand command = git.branchList();
if (listMode != null) {
command.setListMode(listMode);
}
List<Ref> branches = command.call(); // <--- here, the result list contains only branches, no tags
for (Ref ref : branches) {
if (ref.getName().endsWith("/" + label)) {
return true;
}
}
return false;
}
Second, in this case containsBranch is only used in shouldTrack method. This method determines whether the remote branch should be tracked. When the branch does not exist, whether it needs to be tracked or not, the checkout sure to throw RefNotFoundException. Please see the code below.
private Ref checkout(Git git, String label) throws GitAPIException {
CheckoutCommand checkout = git.checkout();
if (shouldTrack(git, label)) { // <--- containsBranch is used here
trackBranch(git, checkout, label); // works for remote branches
}
else {
checkout.setName(label); // works for tags and local branches
}
return checkout.call(); // always throw when branch not exists
}
So actually there's nothing to worry about, everything works as designed.
Just so we understand each other. I have valid branch=feature/foo and tag=foo. Has Spring Cloud Config been designed to fail when I use valid tag name? Has containsBranch method been designed to return true even if repository does not contain foo branch?
But the wrong returns will not cause any problem
Sure it does. There would be no RefNotFoundException exception, should it return false.
You are right. This bug do causes problem when using tags. I agree with you.
As you mentioned, the key point to this bug is the code ref.getName().endsWith("/" + label). and I also think normalize ref name is a good idea. As @ryanjbaxter said
there could be something like
refs/remotes/my/remote/stuff/feature/foo
yes, git do allow / in remote names, so we can't separate branch names from remote names.
But suddenly I realized there is no way in config server to change the remote names. The remote name must be origin. And I found hard coded origin in JGitEnvironmentRepository
private static final String LOCAL_BRANCH_REF_PREFIX = "refs/remotes/origin/";
and
String originUrl = git.getRepository().getConfig().getString("remote", "origin", "url");
and
fetch.setRemote("origin");
and more ...
So it is possible to normalize the ref name to solve the problem.
Maybe we can change to code into
ref.getName().matches("refs/(heads|remotes/origin)/" + label)
or
ref.getName().equals("refs/heads/" + label) || ref.getName().equals("refs/remotes/origin/" + label)
Do you think this will solve the problem?
Like I said, i am not sure that solves the problem because the branch names can be anything really.
In my case exact match solution would work.
@ryanjbaxter Branch is not really an issue. And like @woshikid’s said origin is hardcoded all over the place so at the moment it cannot be anything else but origin.
Saying that I prefer non-regex solution for performance and security reasons as well.