search

optiopay / klar

Integration of Clair and Docker Registry
MIT License
507 stars 140 forks source link

Default port documentation is not consistent when using https #70

Open MichaelPereira opened 6 years ago

MichaelPereira commented 6 years ago

Hi,

First of all, thanks for the development of this CLI tool, it makes clair much easier to integrated inside a CI/CD workflow :)

The current documentation for CLAIR_ADDR in the usage section of the README indicates that the port 6060 is used by default if it is not set.

This is not the current behavior of the code (despite similar comments), as port 6060 will be set by default only for http scheme, as this condition is false for https due to the fact that the : character will be on 5th position due to the extra character.

Do you prefer to

  1. Fix the code to match the documentation (by transforming the < into <=)? This would break the current behavior and force people to specify port 443 in their configuration if they update their version of klar. I can make the PR if that's the chosen solution.

or

  1. Update the documentation to match the current version of the code? No breaking changes, but documentation will be harder to read since the behavior will look a little inconsistent between http and https (why would one set a default port and not the other?)

Regards, Michael

MichaelPereira commented 6 years ago

With the support of clair V3, the behavior has been moved into a V1-specific function newAPIV1 but the behavior stays the same when using V1.

hashmap commented 6 years ago

@MichaelPereira thanks for the report, do you remember which port is used by default for https in Clair? AFAIR it's also 6060, but not sure, need to check Calir docs. If so option 1 sounds like a right thing to do. If it's different (like 443) we should use it as a sane default for https.

MichaelPereira commented 6 years ago

It looks like clair does only expose 6060 by default from what I can see in the Dockerfile, config sample or default config, so using it with 443 for HTTPS is something that people have to explicitly set at other levels like a proxy or load balancer in front of the clair service.

I assume that not that many people would have went through those pains (and integrated it with klar) so far to rule out changing the current behavior for V1 then.