text:
rework rhel plugin install from ln to cp; move action from dockerfile…

… to run script
first step for https://bugzilla.redhat.com/show_bug.cgi?id=1519034
@openshift/sig-developer-experience fyi / ptal

[test]

Hmmm ... test passed, but I'm not seeing messages I was expecting .... investigating

OK it looks like we don't always dump the container logs in the test; think I'm going to add that, even if we decide it should be temporary, just to confirm some of the changes in RHEL

I am wondering if we should be considering flavors like https://stackoverflow.com/questions/8377312/how-to-convert-symlink-to-regular-file in the new use of cp

ok with https://github.com/openshift/jenkins/pull/425/commits/3b17bd5f339eda20fce326d1d0b211f5ea003182 in place I was able to confirm that the new copy path in the run script worked as expected
I'm going to circle back and try/test the "link from the prior release exists, and now we are trying to copy" scenario.

I think I've overthought / got turned around wrt https://github.com/openshift/jenkins/pull/425#issuecomment-348302460
That said, I'm still trying to reconcile some details wrt the pod logs provided from the customer and the run script, now that I've revisited it in detail.  Going to do some more local testing, as well as probably ping @bparees and @jim-minter to cross reference a couple of details.

OK, yeah, this change as is not sufficient.

I do need --remove-destination, otherwise I get cp: not writing through dangling symlink ... ; you get that with that sequence the follows from installing pre 3.7, then moving up to 3.7, on a PV

ln -s filea fileb
cp fileb filec  (filec is on the PV)
remove filea (i.e. the /usr/lib64 file ... it goes away with the new image)
cp newfilea (i.e. the /usr/lib file) filec (filec on the PV is still linked to filea)

install_plugins will not get installed on the PV based upgrade, cause ${JENKINS_HOME}/configured is present ... that is why I did not see any of the expected echo statements around copying files that would have indicated as such
the error occurs because the cp -r /opt/openshift/plugins/* ${JENKINS_HOME}/plugins/ call from install_plugins from the first startup preserves the link to /usr/lib64 (vs. copying the file)

So we either
a) do the cp --remove-destination outside the ${JENKINS_HOME}/configured check ... make sure we don't break various scenarios where the customer can override the plugin list
b) follow @jupierce 's suggestion of simply linking /usr/lib64/jenkins -> /usr/lib/jenkins in the Dockerfile
I'm beginning to lean toward b), but want to walk through the permutations to be sure ...

ln -s /usr/lib/jenkins /usr/lib64/jenkins would seem to work ...a local approximation:
gmontero /tmp/ln-test $ mkdir usr usr64 opt var
gmontero /tmp/ln-test $ cp ~/Downloads/openshift-client-102.hpi usr64
gmontero /tmp/ln-test $ ln -s /tmp/ln-test/usr64/openshift-client-102.hpi /tmp/ln-test/var/plugin.jpi
gmontero /tmp/ln-test $ ls -la var
total 0
drwxrwxr-x. 2 gmontero gmontero  60 Nov 30 16:18 .
drwxrwxr-x. 6 gmontero gmontero 120 Nov 30 16:18 ..
lrwxrwxrwx. 1 gmontero gmontero  43 Nov 30 16:18 plugin.jpi -> /tmp/ln-test/usr64/openshift-client-102.hpi
gmontero /tmp/ln-test $ cp ~/Downloads/openshift-client-102.hpi usr
gmontero /tmp/ln-test $ rm -rf usr64
gmontero /tmp/ln-test $ ln -s /tmp/ln-test/usr /tmp/ln-test/usr64
gmontero /tmp/ln-test $ ls -la
total 0
drwxrwxr-x.  5 gmontero gmontero 120 Nov 30 16:20 .
drwxrwxrwt. 24 root     root     500 Nov 30 15:37 ..
drwxrwxr-x.  2 gmontero gmontero  40 Nov 30 16:18 opt
drwxrwxr-x.  2 gmontero gmontero  60 Nov 30 16:19 usr
lrwxrwxrwx.  1 gmontero gmontero  16 Nov 30 16:20 usr64 -> /tmp/ln-test/usr
drwxrwxr-x.  2 gmontero gmontero  60 Nov 30 16:18 var
gmontero /tmp/ln-test $ ls -la var
total 0
drwxrwxr-x. 2 gmontero gmontero  60 Nov 30 16:18 .
drwxrwxr-x. 5 gmontero gmontero 120 Nov 30 16:20 ..
lrwxrwxrwx. 1 gmontero gmontero  43 Nov 30 16:18 plugin.jpi -> /tmp/ln-test/usr64/openshift-client-102.hpi
gmontero /tmp/ln-test $ jar tvf var/plugin.jpi
     0 Tue Nov 07 12:31:36 EST 2017 META-INF/
   881 Tue Nov 07 12:31:34 EST 2017 META-INF/MANIFEST.MF
     0 Tue Nov 07 12:31:34 EST 2017 WEB-INF/
     0 Tue Nov 07 12:31:34 EST 2017 WEB-INF/lib/
     0 Tue Nov 07 12:31:34 EST 2017 BaseStep/
   660 Tue Nov 07 12:31:34 EST 2017 WEB-INF/licenses.xml
110935 Tue Nov 07 12:31:34 EST 2017 WEB-INF/lib/openshift-client.jar
   132 Tue Nov 07 12:31:34 EST 2017 BaseStep/help-credentials.html
    61 Tue Nov 07 12:31:34 EST 2017 BaseStep/help-logLevel.html
    97 Tue Nov 07 12:31:34 EST 2017 BaseStep/help-advancedArguments.html
   121 Tue Nov 07 12:31:34 EST 2017 BaseStep/help-project.html
    64 Tue Nov 07 12:31:34 EST 2017 BaseStep/help-cluster.html
     0 Tue Nov 07 12:31:36 EST 2017 META-INF/maven/
     0 Tue Nov 07 12:31:36 EST 2017 META-INF/maven/com.openshift.jenkins.plugins/
     0 Tue Nov 07 12:31:36 EST 2017 META-INF/maven/com.openshift.jenkins.plugins/openshift-client/
  4985 Tue Nov 07 12:31:16 EST 2017 META-INF/maven/com.openshift.jenkins.plugins/openshift-client/pom.xml
   130 Tue Nov 07 12:31:34 EST 2017 META-INF/maven/com.openshift.jenkins.plugins/openshift-client/pom.properties
gmontero /tmp/ln-test $

i'm fine w/ symlinking /usr/lib64/jenkins to /usr/lib/jenkins.

had typo in docker file ... pushing new commit momentarily

per previous sign off from @bparees merging
[merge]

I'm reminded that another reason to prefer copying things is if we ship a 3.8 image that doesn't even install a particular plugin that we previously shipped in 3.7, the PV will still have a broken symlink to that plugin.

so i think we should do both.  create the symlink, but also going forward, copy the plugins to the target dir instead of symlinking them, for fresh installs.

(aborted the merge)

doing both, fixed order, pushed

ok - dockerfile update, no longer symlink specific files, pushed

new cp pushed

[test]

move on ln pushed

this lgtm. @jim-minter can you please give it a once over as well?

update for @jim-minter comments pushed

all lgtm, thanks @gabemontero

lgtm, merge at will (not sure if you're doing any additional offline testing... at a minimum you should make sure you can "update" a 3.6 jenkins to this image, when using a PV since that was the point of the exercise.

thanks @bparees @jim-minter
unfortunately, the test failure may be legitimate ... s2i test failed; my presumption to not copy files other than .jpi from /opt/openshift/plugins has proven incorrent (now, I thought I ran the full test suite locally, but seems something got lost in the multi-task shuffle).
New update coming, where we can revisit yet again my love for hash :-(
And +1 on manual upgrade test @bparees

my feelings on the multitude of ways to extended the jenkins image are not the best right now either ;-)

ahh ... i did not see the s2i failure because I did not have an s2i binary, but I only caught "SUCCESS" at the end, as the make test ... still passes
which: no s2i in (/usr/lib64/qt-3.3/bin:/usr/local/bin:/usr/local/sbin:/usr/bin:/usr/sbin:/home/gmontero/go/bin:/usr/local/go/bin:/usr/local/ec2/ec2-api-tools-1.7.4.0/bin:/home/gmontero/go/src/github.com/openshift/origin/_output/local/bin/linux/amd64:/home/gmontero/go/src/github.com/openshift/source-to-image/_output/local/bin/linux/amd64:/home/gmontero/etcd-v2.1.1-linux-amd64:/home/gmontero/apache-maven-3.3.3/bin:/home/gmontero/protoc/bin:/home/gmontero/.local/bin:/home/gmontero/bin:/home/gmontero/go/bin:/usr/local/go/bin:/usr/local/ec2/ec2-api-tools-1.7.4.0/bin:/home/gmontero/go/src/github.com/openshift/origin/_output/local/bin/linux/amd64:/home/gmontero/go/src/github.com/openshift/source-to-image/_output/local/bin/linux/amd64:/home/gmontero/etcd-v2.1.1-linux-amd64:/home/gmontero/apache-maven-3.3.3/bin:/home/gmontero/protoc/bin)
WARNING: Please install the s2i binary to run the s2i tests.
SUCCESS!
Stopping and removing container 0811d79e99ed8946d6ea29e6b6b69342ded79624f35c155b15c66b8f6138274c...
0811d79e99ed8946d6ea29e6b6b69342ded79624f35c155b15c66b8f6138274c
0811d79e99ed8946d6ea29e6b6b69342ded79624f35c155b15c66b8f6138274c
Done.

my feelings on the multitude of ways to extended the jenkins image are not the best right now either ;-)

yeah.  Now perhaps you (and more so @csrwng) understand why I'm really risk averse whenever we want to add one more codepath hack for use case foo.

adjustment for s2i scenario pushed (in separate commit to facilitate review)
will squash if everyone is happy
moving on to constructing PV upgrade scenario

On Fri, Dec 1, 2017 at 2:56 PM Ben Parees notifications@github.com wrote:

@bparees commented on this pull request.
In 2/contrib/s2i/run
https://github.com/openshift/jenkins/pull/425#discussion_r154435035:

@@ -131,16 +146,17 @@ if [ "${CONTAINER_MEMORY_IN_BYTES}" -lt "${DEFAULT_MEMORY_CEILING}" ]; then

 JAVA_MAX_HEAP_PARAM="-Xmx${CONTAINER_HEAP_MAX}m"
 if [ -z $CONTAINER_INITIAL_PERCENT ]; then

 CONTAINER_INITIAL_PERCENT=0.07

 # jboss default was 100% or ms==mx

 JAVA_INITIAL_HEAP_PARAM="-Xms${CONTAINER_HEAP_MAX}m"

else

 CONTAINER_INITIAL_HEAP=$(echo "${CONTAINER_HEAP_MAX} ${CONTAINER_INITIAL_PERCENT}" | awk '{ printf "%d", $1 * $2 }')

 JAVA_INITIAL_HEAP_PARAM="-Xms${CONTAINER_INITIAL_HEAP}m"

what's all this?

made the change on the wrong version - redoing

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/jenkins/pull/425#pullrequestreview-80604545,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADbadAPtkQe0GqwPk3wp5bF1ymrAzFisks5s8FoIgaJpZM4Qw93C
.

ok wrong branch snafu fixed ... s2i commit has the change to copy all files (including those with the pinned suffix)
(as the comment notes, other elements there around count, general check, looks at .jpi still)

lgtm pending your successful migration test.

I squashed the commits and pushed; will update the 3.7 branch version momentarily.
I'm hoping to get to the ec2 / rhel based migration scenario testing either later this evening or Sunday.

ok docker.io/gmontero/jenkins-2-rhel7-test:latest is ready ... going to pull it down locally and give it a go

the migration test failed ... though we talked about install_plugins not getting called once the configured file is present, and I'm pretty sure we had a prior incarnation with the copies outside in install_plugins, this current incarnation does not.
so what happens is the existing plugins in /var/lib/jenkins/plugins point to the latest plugins, but any new dependencies for existing plugins where their versions changed between 3.6 and 3.7 are not copied.  existing plugins at the later versions then complain about dependency plugins missing
ALWAYS copying the /usr/lib/jenkins plugins (i.e. outside of install_plugins and the is configured file present block) MAY be ok if it is done prior to that section, so that any overrides following are then copied ... would like to ponder more at a more decent hour.  But what about extension images that change existing plugins, but then call our run script as the entry point.
Though a bit more complicated, I'm leaning toward perhaps ALWAYS copying plugins from /usr/lib/jenkins that DO NOT exist in /var/lib/jenkins/plugins.

The "ALWAYS copying plugins from /usr/lib/jenkins that DO NOT exist in /var/lib/jenkins/plugins." passed the migration test.
Will push up latest commit (non-squashed) in a couple

change to make actual upgrade scenario work pushed

link based overhaul pushed (single commit)
embarking on migration testing while the repo test starts up here - will report back here on migration test status

while the test passesd for me locally for centos, I'm already seeing an issue for the rhel side on my ec2 instance:
Linking /usr/lib/jenkins/ace-editor.hpi RPM installed Jenkins plugins to /var/lib/jenkins ...
ln: failed to create symbolic link ‘/var/lib/jenkins/plugins/ace-editor.jpi’: No such file or directory

not sure it makes sense to me off the cuff ... will debug there while this PR test continues

I see ... when starting up the image via docker the first time the volume exists, but it is empty... need to create the plugins dir.

OK, rhel image start up looks good with the latest commit.
The migration scenario "worked" in that the image started up.  The /usr/lib64/jenkins -> to /usr/lib/jenkins link is there.  All the plugins, including the new ones introduced by 3.7 are in /var/lib/jenkins/plugins, but there are not sym links.  They are actual files.  i.e. the sym link was lost in all the machinations.
Given the new "keep the links" mantra, investigating.

Must have did something wrong before.
With the same image, I brought up on a PV from scratch, are the links from /var/lib/jenkins/plugins/.jpi to /usr/lib/jenkins/.hpi are there.
I then added some debug, but now in both the "from scratch" and "migration" scenarios, the links are there.
Going to remove the debug just to make sure it did not have some unintended side effect.

Yep ran a few times both ways (migration and from scratch) with a rhel image from this commit only ... links look good.
@bparees - if we are good with the implementation specifics, this is merge-able.

So let me attempt to summarize the behavior:

plugins baked into the image(in /usr/lib*/jenkins) are, at startup, created as symlinks in /var/lib/jenkins/plugins unless the file already exists in /var/lib/jenkins/plugins

content installed by the user(via the jenkins console or the various image plugin install mechanisms) is copied into /var/lib/jenkins/plugins (overwriting any symlink that might be there already, and blocking the future overwrite of that plugin by something from the image)

/usr/lib/jenkins and /usr/lib64/jenkins are aliases so any existing symlinks in /var/lib/jenkins that point to /usr/lib*/jenkins will not be broken when using this new image.

In a future image if we update a plugin, nothing needs to happen (the symlink will point to the new version.
If we add a new plugin to the image, it will be symlinked into /var/lib/jenkins during startup (because we run this logic on every startup, not just the first one).  (unless the user has already installed that plugin at some level in which case we leave it alone).
Does that sound right?

The one problem I have is broken symlinks.  If we remove a plugin in the future, the symlink will still exist in /var/lib/jenkins, and it sounds like that will prevent jenkins from starting up?  Can we scan /var/lib/jenkins for broken symlinks and remove them?

On Mon, Dec 4, 2017 at 4:14 PM Ben Parees notifications@github.com wrote:

So let me attempt to summarize the behavior:

plugins baked into the image(in /usr/lib*/jenkins) are, at startup,
created as symlinks in /var/lib/jenkins/plugins unless the file already
exists in /var/lib/jenkins/plugins
content installed by the user(via the jenkins console or the various
image plugin install mechanisms) is copied into /var/lib/jenkins/plugins
(overwriting any symlink that might be there already, and blocking the
future overwrite of that plugin by something from the image)
/usr/lib/jenkins and /usr/lib64/jenkins are aliases so any existing
symlinks in /var/lib/jenkins that point to /usr/lib*/jenkins will not be
broken when using this new image.
In a future image if we update a plugin, nothing needs to happen (the
symlink will point to the new version.
If we add a new plugin to the image, it will be symlinked into
/var/lib/jenkins during startup (because we run this logic on every
startup, not just the first one). (unless the user has already installed
that plugin at some level in which case we leave it alone).
Does that sound right?

Yes it does.
—

You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/jenkins/pull/425#issuecomment-349107687,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADbadGXLv-1Zdsy7Geb5lXvLqwKznwkFks5s9GC5gaJpZM4Qw93C
.

On Mon, Dec 4, 2017 at 4:15 PM Ben Parees notifications@github.com wrote:

The one problem I have is broken symlinks. If we remove a plugin in the
future, the symlink will still exist in /var/lib/jenkins, and it sounds
like that will prevent jenkins from starting up?

We could add an unlink in the run script when we remove from plugin text
file.

Can we scan /var/lib/jenkins for broken symlinks and remove them?

will ask google when i get back to my desk

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/jenkins/pull/425#issuecomment-349107975,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADbadFVokFskXCIWJ0H8KUBqUsk4i9cpks5s9GD4gaJpZM4Qw93C
.

We could add an unlink in the run script when we remove from plugin text file.

yeah i'd rather just do it generically for broken symlinks.
here's an easy way to test it, though cat'ing large binary files to /dev/null may prove non-performant, so you'll want to check that before doing it for N .jpi files.
$ ls -l a b broken
-rw-rw-r--. 1 bparees bparees 0 Dec  4 16:23 a
lrwxrwxrwx. 1 bparees bparees 1 Dec  4 16:24 b -> a
lrwxrwxrwx. 1 bparees bparees 1 Dec  4 16:23 broken -> c

$ cat b >& /dev/null ; echo $?
0

$ cat broken >& /dev/null ; echo $?
1

On Mon, Dec 4, 2017 at 4:25 PM, Ben Parees notifications@github.com wrote:

We could add an unlink in the run script when we remove from plugin text
file.
yeah i'd rather just do it generically for broken symlinks.
here's an easy way to test it, though cat'ing large binary files to
/dev/null may prove non-performant, so you'll want to check that before
doing it for N .jpi files.
$ ls -l a b broken
-rw-rw-r--. 1 bparees bparees 0 Dec  4 16:23 a
lrwxrwxrwx. 1 bparees bparees 1 Dec  4 16:24 b -> a
lrwxrwxrwx. 1 bparees bparees 1 Dec  4 16:23 broken -> c
$ cat b >& /dev/null ; echo $?
0
$ cat broken >& /dev/null ; echo $?
1

stat -L b will do it wrt existence ... test -h will let us know if it
is a symlink ... I'll cook up combination of those two

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/jenkins/pull/425#issuecomment-349110756,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADbadB8zVsQI1RF8hp0QF72tR0TasHNdks5s9GM_gaJpZM4Qw93C
.

unlink if link points to non-existent pushed

manual migration test passed

lgtm, would like one more second review from @jim-minter though since we've established this is not as trivial as we thought :)

lgtm too - happy for this to merge as is.

Thanks @jim-minter @bparees

@bparees and I discussed updating the README to explain the particulars of all this ... going to do that in a separate PR.

[merge]

authors: bparees gabemontero jim-minter

repo: openshift/jenkins


text:
Remove kubernetes plugin support

This PR removes the included Kubernetes plugin (can be installed by custom assemble script) and the auto-slave-discovery code (can be done by the custom run script).
@bparees do you want to remove the examples/ as well? I think we should move them into a example repo with example S2I scripts.

@gabemontero FYI

[test]

yeah we should remove those as well or we're just going to get questions about them.

@bparees done

lgtm i think.

merge then I think :-)

source: gharchive/pull-request

authors: bparees mfojtik

license_type: permissive

license_source: github-api


text:
OCPBUGS-23913: Fix nil pointer exception when deleting machines without a root volume

Before this patch, machines could get stuck in Deleting phase when they didn't have a root volume defined.

/jira refresh

/jira refresh

expected the bug to target the "4.15.0" version, but no target version was set

This target version field is itself a damn moving target in Jira

This is a test blocker and I have assessed the bug as "critical" in Jira.
/label acknowledge-critical-fixes-only

/lgtm
/approve

Now with the correct test names!

/lgtm
/approve

/retest-required

/retest

authors: pierreprinetti stephenfin


text:
Replace default regex lib with pcre regex lib

I have a context where i need to match all string of 6 or 8 char (with number) in a yaml file.
This is a real problem today because he match only on the value in yaml and not key - value.
There is a solution where i can use a lookahead or behind to restrict the match of a regex but the default go library can't handle this kind of syntax since RE2 not supported it.
So we made for this usage a fork with pcre syntax support. We can now use lookahead or behind to handle this kind of context.

@lmzuccarelli done & pushed

@bachoudev - thanks for contributing

/lgtm

/approved

@bachoudev and @lmzuccarelli this library does not compile for MAC (amd64 + ARM) and ARM on all platforms, what was the rationale to use this? That's a breaking change and I can't create a 0.0.2 release with this.

@tjungblu the main objectif was to benefit from pcre to use look arround and other regex features wich are not supported by RE2 syntaxe in go.
This change was needed for our usage but maybe we need to fork this on our hand and revert this change like your PR proposal #91 for now.
And for the future i can check if it's possible to edit the source lib to make it work with other arch.

authors: bachoudev lmzuccarelli tjungblu

repo: openshift/must-gather-clean


text:
docs: make retrieve pprof data script shorter and stricter

Test: https://github.com/openshift/openshift-docs/pull/47075#discussion_r917772751

/lgtm

/label docs-approved
/label qe-approved
/label px-approved

repo: openshift/node-observability-operator


text:
Adding prometheus metrics to OADP must-gather

Adding prometheus metrics to OADP must-gather

Looks good, I'm going to hold off on merging as the actual exposed metrics are changing potentially... will get this in the next few days.

authors: dymurray savitharaghunathan

repo: openshift/oadp-operator


text:
[WIP] Bug 2075051: fix: adds handling for channel with different prefixes in cincinnati.go

Currently, the prefix for the target channels is being inferred from the source channel
when calculating Cincinnati graphs. This can cause an issue when The prefix of the channel
is different for the starting and target version. Now, the starting prefix is only
used when calculating upgrades through the channel until the target major and minor version
is reached.
Signed-off-by: Jennifer Power barnabei.jennifer@gmail.com
Description
Type of change
Please delete options that are not relevant.

[X] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] This change requires a documentation update

How Has This Been Tested?

[ ] Case added to unit test for channels with different prefixes

Checklist:

[ ] My code follows the style guidelines of this project
[ ] I have performed a self-review of my own code
[ ] I have commented my code, particularly in hard-to-understand areas
[ ] I have made corresponding changes to the documentation
[ ] My changes generate no new warnings
[ ] I have added tests that prove my fix is effective or that my feature works
[ ] New and existing unit tests pass locally with my changes
[ ] Any dependent changes have been merged and published in downstream modules

/bugzilla refresh

/retest

/retest

repo: openshift/oc-mirror


text:
Add openshift ci build root container /etc/passwd permissions

The latest attempt to run make red-hat-storage-ocs-ci Openshift CI in prow revealed an issue with the randomized uid Openshift CI uses when executing the e2e test. ocs-ci expects an entry in /etc/passwd to exist for the user, but since the user is random there is no entry.
To address this, this PR dynamically updates the /etc/passwd file when we detect Openshift CI is executing ocs-ci tests.  I also made the ocs-ci related ENV VARS dynamic so I can alter those variables in the prow config in order to test out different ocs-ci source versions.
Changes

adds /etc/passwd permissions to Dockerfile.tools
makes ocs-ci environment variables dynamic

/cc @jarrpa

@jarrpa i had to rebase to fix a merge conflict, needs LGTM again.

/lgtm

authors: davidvossel jarrpa

repo: openshift/ocs-operator


text:
Ask for images work on s390x arch

/kind bug
What versions of software are you using?
Operating System:
Red Hat Enterprise Linux 8.1 (Ootpa
Output of odo version:
odo v1.2.4 (b3cf59550)
Server: https://api.ocp.openstack:6443
Kubernetes: v1.18.3+6025c28
How did you run odo exactly?
Enable odo CI on s390x platform
Actual behavior
./scripts/openshiftci-presubmit-all-tests.sh
Expected behavior
All the test passed
Any logs, error output, etc?
There are some hard code to import S2I images. These images only support amd64 arch which causes failed on s390x arch.
For odo supported images e2e tests:
These images are:
oc --request-timeout 5m import-image nodejs:latest --namespace=vsnyjhnygd --from=docker.io/centos/nodejs-12-centos7:latest --confirm
oc --request-timeout 5m import-image nodejs:latest --namespace=hcvvpyounm --from=docker.io/centos/nodejs-10-centos7:latest --confirm
oc --request-timeout 5m import-image nodejs:latest --namespace=qjycpyjbmo --from=registry.access.redhat.com/rhoar-nodejs/nodejs-10:latest --confirm
For odo source e2e tests:
oc --request-timeout 5m import-image dotnet:2.0 --namespace=uezadyekmn --from=registry.centos.org/dotnet/dotnet-20-centos7 --confirm

/area testing

@zhengxiaomei123 there is no centos on z at all.. so apparently need code change
can you paste the link of the code so that it will be easier to know the code comes from?

@jichenjc
nodejs-10-centos7:latest
nodejs-12-centos7:latest
rhoar-nodejs/nodejs-10

I think these tests are aim to verify odo supported images  , the registry is not important. So Can I request the PR to replaces these above images from other registry? If it is aim to verify the specific image from the specific registry, I think it is meaningless to test those images only for x86 arch. Is my understanding right?

@zhengxiaomei123 the supported statement might prove here:
https://github.com/openshift/odo  (Table 1. Supported container images )
for our platform, we can either use our own image to replace for s390x arch or skip the test..

nodejs-10 and nodejs-12 for OCP on s390 is available in the RH Registry.
https://catalog.redhat.com/software/containers/ubi8/nodejs-12/5d3fff015a13461f5fb8635a?architecture=s390x&container-tabs=overview
https://catalog.redhat.com/software/containers/ubi8/nodejs-10/5c839aa3d70cc51dd4c425d9?architecture=s390x&container-tabs=gti
https://catalog.redhat.com/software/containers/rhel8/nodejs-10/5ba1f2365a134643ef2f0188?architecture=s390x
https://catalog.redhat.com/software/containers/rhel8/nodejs-12/5d3fff06d70cc5521d0009c2?architecture=s390x
These images are also available for ocp on x86.

@zhengxiaomei123 but that's UBI, correct?  if so we need update doc to indicate what's really available and point to correct link

@amitkrout Since the odo-init-image has supported multi-arch platform, I think there should not be images which only for one x86 platform, Is it OK to replace the nodejs-12-centos7:latest with ubi8/nodejs-12 and replace the nodejs-10-centos7:latest with ubi/nodejs-10  and replace the rhoar-nodejs/nodejs-10  with rhel8/nodejs10?

@zhengxiaomei123 but that's UBI, correct? if so we need update doc to indicate what's really available and point to correct link

I think we can add those images to doc.

@amitkrout Since the odo-init-image has supported multi-arch platform, I think there should not be images which only for one x86 platform, Is it OK to replace the nodejs-12-centos7:latest image with ubi8/nodejs-12 and replace the nodejs-10-centos7:latest image with ubi/nodejs-10 and replace the rhoar-nodejs/nodejs-10 image with rhel8/nodejs10?

TBH i am not sure about the image part. @girishramnani @mohammedzee1000 PTAL

nodejs-10 and nodejs-12 for OCP on s390 is available in the RH Registry.
https://catalog.redhat.com/software/containers/ubi8/nodejs-12/5d3fff015a13461f5fb8635a?architecture=s390x&container-tabs=overview
https://catalog.redhat.com/software/containers/ubi8/nodejs-10/5c839aa3d70cc51dd4c425d9?architecture=s390x&container-tabs=gti
https://catalog.redhat.com/software/containers/rhel8/nodejs-10/5ba1f2365a134643ef2f0188?architecture=s390x
https://catalog.redhat.com/software/containers/rhel8/nodejs-12/5d3fff06d70cc5521d0009c2?architecture=s390x
These images are also available for ocp on x86.

I think this should be ok so long as they are redhat supported. Still it is better if we can get confirmation from inside about these images
@hhorak is part of software collections. whyt?

also @kadel @girishramnani wdyt

It would be interesting what images people using s390x typically use.
I don't think that it makes sense to have the same set of images for both x86 and s390x.
We can drastically reduce the number of tests that we do on s390x to just bare minimum

It would be interesting what images people using s390x typically use.
I don't think that it makes sense to have the same set of images for both x86 and s390x.
We can drastically reduce the number of tests that we do on s390x to just bare minimum

The images are all about nodejs and test-e2e-images.  Nodejs is the offical supported in Odo. The tests are running to test these offical nodejs images. It's not s390x specific.

@kadel @mohammedzee1000 To seperate images for s390x and x86, I would like to create one different context and run all the spec that needs to be skipped on s390x and apply condition
if runtime.GOARCH == "s390x" {
Skip("Skipping test because there is no supported builder image for s390x.")
}
in ginkgo built-in tear down steps JustBeforeEach. I think it a good solution and will request the PR. Then please help review it. Thanks.

@amitkrout When testing the ubi8/nodejs-10 and ubi8/nodejs-12 and rhel8/nodejs-10 and rhel8/nodejs-12 for test-e2e-images case, I also go the error which you mentioned on #3307 and #3420 . It's here always. But for ubi7/nodejs-10 and ubi7/nodejs-12, the case passed and the error is not here.
The full log I pasted here: https://tpaste.us/WRv0

I think this should be ok so long as they are redhat supported. Still it is better if we can get confirmation from inside about these images
@hhorak is part of software collections. whyt?

I can confirm that these images are supported by Red Hat on s390x as well.

@amitkrout @kadel  can we then consider adding these images to our testing without removing existing ones.
They can be put under a separate test group if needed

@mohammedzee1000 Yes, I created a new separate context to involes those images for x86 not removing them in PR # 3744.

source: gharchive/issue

authors: amitkrout hhorak jichenjc kadel mohammedzee1000 zhengxiaomei123

repo: openshift/odo


text:
odo push with alternate --run-command should push complete file set upon new pod creation

/kind bug
What versions of software are you using?
Operating System: Win10
Output of odo version:   This is close to 1.2.6, with https://github.com/openshift/odo/pull/3478 built in
How did you run odo exactly?
Roughly the workflow is:

$ odo create java-openliberty --starter
$ odo push
<edit a .java file>
$ odo push --run-command run-test-off

I say "roughly" because with some devfile syntax tweaks the starter devfile keeps getting broken at a fixed level of 'odo' by devfile changes.   So you might have to adjust this a bit to recreate.
Actual behavior
With the new run command, the deployment spec changes, so k8s starts a new pod, but the problem is that only the one .java file I changed gets copied into the new container, not the whole project.   My new container only has:   /projects/src/...my.java not the pom.xml even to build it.   I can force the whole proj. to get copied with odo push -f but that's not obvious.
Expected behavior
Since this is a new pod, 'odo' should sync the entire project into the new pod, not just the delta/changed file set.
Any logs, error output, etc?
Executing devfile commands for component java-openliberty
 <  Executing devbuild command "if [ -e /projects/.disable-bld-cmd ]; then echo \"found the disable file\" && echo \"devBuild command will not run\" && exit 0; else echo \"will run the devBuild command\" && echo \"...moving liberty\" && mkdir -p /projects/target/liberty

 ...

  X  Executing devbuild command "if [ -e /projects/.disable-bld-cmd ]; then echo \"found the disable file\" && echo \"devBuild command will not run\" && exit 0; else echo \"will run the devBuild command\" && echo \"...moving liberty\" && mkdir -p /projects/target/liberty && mvn -Dmaven.repo.local=/mvn/repository package && mv /opt/ol/wlp /projects/target/liberty && touch ./.disable-bld-cmd; fi", if not running [4s]
 X  Failed to start component with name java-openliberty. Error: Failed to create the component: command execution failed: unable to execute the run command: unable to exec command [/bin/sh -c cd /projects && if [ -e /projects/.disable-bld-cmd ]; then echo "found the disable file" && echo "devBuild command will not run" && exit 0; else echo "will run the devBuild command" && echo "...moving liberty" && mkdir -p /projects/target/liberty && mvn -Dmaven.repo.local=/mvn/repository package && mv /opt/ol/wlp /projects/target/liberty && touch ./.disable-bld-cmd; fi]:
will run the devBuild command
...moving liberty
[INFO] Scanning for projects...
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  0.111 s
[INFO] Finished at: 2020-09-09T11:26:16Z
[INFO] ------------------------------------------------------------------------
[ERROR] The goal you specified requires a project to execute but there is no POM in this directory (/projects). Please verify you invoked Maven from the correct directory. -> [Help 1]

I could zip up the -v 9 logs if it helps.

I can't reproduce @scottkurz 😕
Can you check to see if the branch of odo you're using has the fix from #3838 in it? This sounds similar to the bug that it fixed. Basically, we weren't detecting that the component pod had restarted and that the source code needed to be re-synced. It looks like a similar thing is happening here.
Because we're changing the run command, the run command env var in the container is changing and the container gets restarted, but we're not properly detecting that the container has restarted, and not force-syncing as required. But I would expect the fix from #3838 to catch this.

Can you check to see if the branch of odo you're using has the fix from #3838 in it (commit id a7408ed)?

Guessing not based on the dates.  I'll wait and try this on the beta release.  Thanks.

Seems fixed on the odo v2.0.0-beta-1.

authors: johnmcollier scottkurz


text:
Bugfixes to ref.arch./ocp-dns

What does this PR do?
As a Red Hat Consultant i've tried this out together with one of our customers.
This fixes some bugs found during testing/deployment.

sat6_organization != sat6_hostname.

In our use-case we only specify Satellite activationkey, no username/password necessary, so don't check for that in your condition

How should this be manually tested?
cat >vars.yaml
sat6_hostname: "satellite.corp"
sat6_organization: "myorg"
sat6_activationkey: "openshift"
ansible-playbook deploy-dns.yaml -e @vars.yaml
Is there a relevant Issue open for this?
This PR!
Who would you like to review this?
cc: @tomassedovic @bogdando PTAL

@markllama ping

@markllama can you please take a look?

It does look like it requires a rebase now.  rhsm parameters have been pushed down a level.

authors: dav1x markllama sterburg tomassedovic

repo: openshift/openshift-ansible-contrib


text:
Store container network in etcd

Container network should be stored in etcd by openshift-sdn master and read by openshift-sdn minions from there.

Closed via #38

repo: openshift/openshift-sdn

license_source: bigquery


text:
Move angular-ui-router to dev dependency and bump to latest 1.0.X

Fixes issue #654

@TheRealJon were you able to verify the sample app still worked?

/assign

Yes, sample app still works with angular-ui-router v1.0.3

I'd expect a package-lock.json change.

I did just notice that there is a deprecation warning for that package. I might as well see if I can fix that too.

authors: TheRealJon spadgett

repo: openshift/origin-web-catalog


text:
[OSD-7154] add support for user impersonation

As we rolled out backplane, some actions require access escalation by impersonating backplane-cluster-admin, and osdctl needs to be updated to follow the same behavior when needed.
This PR adds support for the --as flag used in Kubernetes for user impersonation.
Note: this is a draft PR, as soon as it tested comprehensively with backplane, I will update its status.

/retest

authors: dastergon rogbas

repo: openshift/osdctl


text:
Add Pratik to Owners file

What type of PR is this?
cleanup
What this PR does / why we need it?
Update OWNERS to add Pratik

Codecov Report

Merging #141 (fba5a5d) into master (6957795) will decrease coverage by 10.64%.
The diff coverage is n/a.

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #141       +/-   ##
===========================================
- Coverage   71.77%   61.14%   -10.64%
===========================================
  Files          10        8        -2
  Lines         489      471       -18
===========================================
- Hits          351      288       -63
- Misses        131      176       +45
  Partials        7        7

see 9 files with indirect coverage changes

/lgtm
/approve

authors: codecov-commenter mrbarge shitaljante

repo: openshift/rbac-permissions-operator


text:
Revert "OCM-6119 | fix: block users from passing region flag when creating an oidc provider"

Reverts openshift/rosa#1786

/lgtm

authors: gdbranco willkutler

repo: openshift/rosa


text:
OCM-11201 | fix : fix make target ensuring release builds and publishes to github

WHAT
At the moment release target creates a snapshot, this change removes the --snapshot param ensuring the the release is built and published to github
JIRA - https://issues.redhat.com/browse/OCM-11201

/lgtm
/approve

authors: ciaranRoche gdbranco


text:
controller: The service should be enabled by the OS

kata-osbuilder-generate.service should be part of the RHCOS preset,
making it "enabled" by default whenever it's present / installed.
With that in mind, we can remove this logic from the operator and rely
on the OS to do the right thing for us.
Please, for more information about the work done on RHCOS side, refer
to: https://bugzilla.redhat.com/show_bug.cgi?id=1941342
Signed-off-by: Fabiano Fidêncio fidencio@redhat.com

- Description of the problem which is fixed/What is the use case
- What I did
- How to verify it
- Description for the changelog

/hold for testing ... and also let's wait till RHCOS BZ gets solved before proceeding with this one.

/unhold

The needed bits were merged as part of openshift OS (https://github.com/openshift/os/pull/524) and are part of the kata-containers-2.1.0-1.alpha2.el8 RPM.

Ah, by the way, this should be tested in an altogether build of the operator + the latest extensions containing the RPM mentioned in the previous comment.  Only after the test, this one should be merged,

repo: openshift/sandboxed-containers-operator


text:
Use cobra for parsing multiple env variables

Fixes: https://github.com/openshift/source-to-image/issues/417
@bparees this one is tricky :-) Basically, you can specify a multiple environment variables, by providing multiple -e FOO=BAR flags. The syntax we supported before was that you can use something like: -e FOO=BAR,TEST=FOO.
So this is introducing a backward incompatible change. I wonder if we want to make it backward compatible (basically do additional string.Split() in the Set() function) or leave it as this (which is consistent with other flags).

@GrahamDumpleton FYI

[test]

bleah.  I don't like our options here :)
but yes, maybe continuing to support comma as well is the best thing to do.  though it means you can't have a comma in your env variable value still, right?

@bparees yes that was always the case... you can workaround that by using -e FOO=bar, -e BOO=far

@bparees updated :-)

@bparees bump

[merge]

repo: openshift/source-to-image


text:
docs/user_guide.md: backport minor fixes from OpenShift documentation

Backport of fixes to https://github.com/openshift/openshift-docs/pull/2697 and https://github.com/openshift/openshift-docs/pull/2724
PTAL @bparees

lgtm [merge]

authors: bparees php-coder


text:
Refactoring to use manifestival/manifestival

This will bump from jcrossley3/manifestival to
manifestival/manifestival as that repo is
archived now

/retest

/retest

/hold

/hold cancel

/lgtm

authors: piyush-garg sthaha

repo: openshift/tektoncd-pipeline-operator


text:
OCPBUGS-14023: Log vcenter version information

Fixes https://issues.redhat.com/browse/OCPBUGS-14023

/lgtm
/approve

authors: gnufied jsafrane

repo: openshift/vsphere-problem-detector


text:
previewDocsServer.sh doesn't work in Fedora with SELinux on

The docker invocations in $REPO_HOME/scripts/previewDocsServer do not work in Fedora if SELinux is on because it is blocking access to the user's directory. To solve this problem, Docker needs to be run in privileged mode by adding the --privileged flag.
Also, the script invocation needs to be done either using an account in the docker group (which is a security hole, and is discouraged), or as root. It should either be documented that the script should be run as root either using sudo or su -c, or the docker invocations should be prepended themselves.
Notes:

If the whole script is run using sudo, the $HOME directory resolves to /root, which effectively means that all the Maven packages will be downloaded into root's .m2 directory.
If the docker invocations are prepended with sudo, it is not obvious to the user why root access is required.
When using pkexec instead of sudo, each invocation of docker requires the user to enter the password separately, which results in inferior user experience.

If you really want to do this, I'd recommend using sudo just for the docker invocations and linking to one of the many explanations, e.g. https://www.projectatomic.io/blog/2015/08/why-we-dont-let-non-root-users-run-docker-in-centos-fedora-or-rhel/

Yes, that seems like the most reasonable solution for me too. Since I believe we can expect the script's users to review and understand the code if they're spooked by the sudo prompt, I'll implement it like that.

authors: Ladicek tradej

repo: openshiftio/appdev-documentation


text:
Docs: Data Collective terms of service

What is the improvement or update you wish to see?
We should review the terms of service of our data sources and make sure that we write a compatible terms of service for members of our data collective
Is there any context that might help us understand?
https://docs.github.com/en/site-policy/github-terms/github-terms-of-service#h-api-terms
Does the docs page already exist? Please link to it.
No response

Gonna close this for now.
With these 2 PRs
https://github.com/opensource-observer/oso/pull/1700
https://github.com/opensource-observer/oso/pull/1698
We now have a data overview section that lists each dataset and the licenses that govern their use.
I also added a CC-BY-4.0 for OSO data pipeline.
We can make it more permissive in the future if we want, but I think its the best fit for now

repo: opensource-observer/oso


text:
Update 2023-02-03-fosdem-2023-annoucement.md

added past

@Erioldoesdesign I think we also need to update https://github.com/opensourcedesign/opensourcedesign.github.io/blob/master/_events/2022-11-09-fosdem-cfp-2023.md to past?

authors: Erioldoesdesign SaptakS


text:
🛑 Weblate is down

In 1c53fb8, Weblate (https://translate.opensourcepos.org) was down:

HTTP code: 0
Response time: 0 ms

Resolved: Weblate is back up in e98fa26.

repo: opensourcepos/upptime

license: MIT


text:
Putting the Open Data Into Open Budgets

http://openspending.org/resources/osi/appendix-open-budgets-open-data.html
Here's where I'd mention all of the confusion around 'open' and 'standard' language. And perhaps if we need to mention it in text above, we can just link here?
"We have looked in detail in this report at criteria which make it difficult for organisations to use data that has been released by governments." -> "In this report we have looked at obstacles to civil society organisations using data on public finance to support their mission."
Again I'd harmonise titles/nesting, which is currently somewhat confusing. Or hold the hand of the reader a bit more and guide them through narrative of the text.

Closing as I assume the text of the report has been finalized (at least for now).

authors: danfowler jwyg

repo: openspending/dotorg


text:
Create manual-apk-release.yml

IMPORTANT: Where possible all PRs must be linked to a Github issue
Fixes [link to issue]
Engineer Checklist

[ ] I have written Unit tests for any new feature(s) and edge cases for bug fixes
[ ] I have added any strings visible on UI components to the strings.xml file
[ ] I have updated the  CHANGELOG.md file for any notable changes to the codebase
[ ] I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the project's style guide
[ ] I have built and run the FHIRCore app to verify my change fixes the issue and/or does not break the app
[ ] I have checked that this PR does NOT introduce breaking changes that require an update to Content and/or Configs? If it does add a sample here or a link to exactly what changes need to be made to the content.

Code Reviewer Checklist

[ ] I have verified Unit tests have been written for any new feature(s) and edge cases
[ ] I have verified any strings visible on UI components are in the strings.xml file
[ ] I have verifed the CHANGELOG.md file has any notable changes to the codebase
[ ] I have verified the solution has been implemented in a configurable and generic way for reuseable components
[ ] I have built and run the FHIRCore app to verify the change fixes the issue and/or does not break the app

Merging this in for development and troubleshooting

repo: opensrp/fhircore


text:
The export of P12 files doesn't work as expected

openssl version: OpenSSL 1.1.1  11 Sep 2018
command : openssl pkcs12 -export -in cert.pem -inkey cert.key -passout pass:1234 -out cert.p12
test : using the desktop environment (GNOME), double-click the cert.p12 file, then type "1234" as password.
result: GNOME indicates that the password is incorrect.

I cannot reproduce this. I just tried this command with the exact same version of OpenSSL and GNOME is able to read it just fine. Possibly a GNOME bug?

I retested today again. I cannot reproduce the bug.
Maybe I made a mistake in my first test.
You can close this issue

authors: h-rueda mattcaswell

repo: openssl/openssl


text:
The change of unexpected EOF reporting - revert and release 1.1.1f ?

Re the change in EOF reporting in 1.1.1e:
From a Fedora list post, it is stated:

It shows that this does not only break Python's own test suite, but various Python packages are failing as well (incl. python-pip).

They seem to be reverting the change in OpenSSL itself.
The change has also affected Ruby and Ruby Gems using OpenSSL.
I don't care about master (see Issue https://github.com/openssl/openssl/issues/11423), as any major release is likely to require changes.  But, 1.1.1e should not break code that worked in 1.1.1d.
As to the problem in Ruby (and I suspect the same with Python), in Ruby's OpenSSL code, some OpenSSL errors are translated into socket or IO errors that are already defined in Ruby.
With the change in the EOF error response, that translation doesn't happen, and the new SSL error is not caught, as the old EOF error was.
Hence, please consider releasing 1.1.1f with the EOF change reverted.

The breaking change has already been reverted on 1.1.1 (see 30d190caf311d534867df97e26b552e628cb7d85 and 0cd2ee64bffcdece599c3e4b5fac3830a55dc0fa) and the release of 1.1.1f is scheduled for next tuesday.

Thank you.  I apologize for not looking at the recent commits.  Closing.

No problem :-)

authors: MSP-Greg mspncp


text:
OpenSSL 3.0.0 with 1.1.1 compatibility and FIPS property

Will it be possible for a client to use OpenSSL 3.0.0 without changing the source code (that was created for the OpenSSL 1.1 API), and nevertheless make use of the global FIPS setting? So basically keep the old client source code, and make it FIPS aware just by adding the global FIPS setting?

The intention is for it to work as much as possible with only a configuration file change. We're not making an absolute guarantee, though, but the expectation is in the very high range.

There's quite a lot of discussion about what you need to do to make an application "FIPS" aware on the 3.0 wiki page:
https://wiki.openssl.org/index.php/OpenSSL_3.0

authors: gschaber levitte mattcaswell


text:
SSL_OP_BIT macro makes constant incompatible with preprocessor

Commit f04bb0bce490de847ed0482b8ec9eabedd173852 introduced the SSL_OP_BIT bit macro and uses that to rewrite various #define constants, e.g. SSL_OP_TLS_ROLLBACK_BUG.
For example, squid fails to build with an error like this:
In file included from ../../compat/openssl.h:42,
                 from ../../src/security/Context.h:15,
                 from ../../src/security/forward.h:14,
                 from ../../src/security/PeerOptions.h:14,
                 from PeerOptions.cc:16:
PeerOptions.cc:330:5: error: missing binary operator before token "1"
  330 | #if SSL_OP_TLS_ROLLBACK_BUG
      |     ^~~~~~~~~~~~~~~~~~~~~~~

If I recall correctly, haproxy is similarly afflicted.
The application fix is easy enough: Replace #if with #ifdef. It should probably be mentioned in the porting guide.
If OpenSSL can use ULL constants in public header files, you could define the macro like this:
#define SSL_OP_BIT(n)  (((uint64_t)+1ULL) << ((uint64_t)+(n)))

uint64_t expands to 0 in preprocessor expressions, so the resulting expression will be syntactically valid.

I do not think we can use ULL constants. In that case we might want to just drop the SSL_OP_BIT and use the constants directly.

But would the definition with a cast # define SSL_OP_NO_EXTENDED_MASTER_SECRET                (uint64_t)0x00000001 work?

Not really, so this issue was actually introduced in an earlier commit (56bd17830f2d5855b533d923d4e0649d3ed61d11).

The C standard has an UINT64_C macro which appears related, but it does not provide the right type (it's uint_least64_t instead) and does not require that the result is a valid preprocessor expression. So I don't think it helps here.
The + kludge could result in false negative results for #if in cases where the preprocessor arithmetic overflows and the C implementation does not provide a diagnostic for that (it's not required to do so).

So perhaps just documenting the breakage in migration guide is the only reasonable way.

Looks like it. Using #if instead #ifdef does not really provide any benefit, and there is no OpenSSL 1.1 compatibility impact to worry about, either.

Doc addition in #16084

I know you already came to a (correct IMO) decision, but the reason for introducing the macro is to make it easier to find "free" bits, re-used bits, etc.  It's much easier to compare 24 and 21 versus their hex equivalents.

authors: fweimer-rh richsalz t8m


text:
SSL handshake failing with latest Apple clang from Command Line Tools for Xcode 14.3 on MacOS

OpenSSL 1.1.1 stable has been working fine on multiple platforms until recently the MacOS version broke after installing the latest Xcode Command Line Tools, which includes Apple clang 14.0.3
The symptom we received (we use OpenSSL through CURL), was a Curl SSL connect error returning "error:1012606B:elliptic curve routines:EC_POINT_set_affine_coordinates:point is not on a curve" which is an OpenSSL error.
After a long investigation, it was determined that the latest Apple clang 14.0.3 is the issue. This has been reproduced on two separate machines, across a number of 1.1.1 tags (k..t). Rolling back to clang 14.0.0 solves the problem.

I've tried to reproduce this on the sslecho demo - https://github.com/openssl/openssl/tree/master/demos/sslecho but wasn't able to. If there's more info you need about our SSL server details let me know and I can follow it up.

Please try openssl s_client if that reproduces the issue.

I tried s_client on our domain and it seems to connect ok:
---
SSL handshake has read 4161 bytes and written 351 bytes
---
New, TLSv1/SSLv3, Cipher is AEAD-CHACHA20-POLY1305-SHA256
Server public key is 256 bit
Secure Renegotiation IS NOT supported
Compression: NONE
Expansion: NONE
No ALPN negotiated
SSL-Session:
    Protocol  : TLSv1.3
    Cipher    : AEAD-CHACHA20-POLY1305-SHA256
    Session-ID:
    Session-ID-ctx:
    Master-Key:
    Start Time: 1684127652
    Timeout   : 7200 (sec)
    Verify return code: 0 (ok)
---

Using it on what? Which domain?

So we ended up solving this by re-enabling asm.
Previously we did this
./Configure darwin64-arm64-cc no-asm -isysroot ${CMAKE_OSX_SYSROOT}
Removing the 'no-asm' fixed our issue.

Closing as this as resolved.

Actual there's more to the story, re-opening.

Apple clang 14.0.3 is the issue

Do you mean that compiling with 14.0.3 created binaries that didn't work, but that the binaries complied with 14.0.0 did work?
Could you see if there was much difference in the generated object files, and if so, in which functions?

Do you mean that compiling with 14.0.3 created binaries that didn't work, but that the binaries complied with 14.0.0 did work?
yes

I'll get back to you with the specific details...sorry don't have them on hand right now

The version 1.1.1 is not supported anymore. Closing. If this is still an issue with currently supported releases, please open a new issue.

authors: pierre-aimi t8m tom-cosgrove-arm


text:
Fix for the bug in ssl_version_supported()

Bug in ssl_version_supported() - the chosen SSL_METHOD returned from the function is always a client side method, ever for the server side.
This is a typical sequence for TLS server side code to reproduce the bug:

Create new SSL_CTX_new( TLS_server_method() );
Call SSL_accept() that calls SSL_set_accept_state() that sets s->handshake_func = s->method->ssl_accept; i.e. initialises handshake_func of SSL* to the correct ssl_accept function of TLS_server_method()
Afterwards in the flow ssl_choose_server_version() is called that calls ssl_version_supported() that changes s->method to the method of the best suitable version but always to the client method, even for the server side
Therefore say we are using min and max TLS version TLS 1.2 - for the server side SSL* we have get s->method == tlsv1_2_client_method() nevertheless s->server == 1
This is not harmful meanwhile but could cause a bug in the further code development.

Checklist

[x] tests are added

Travis errors are relevant.
Also, please could you submit a CLA?
https://www.openssl.org/policies/cla.html

This PR has the label 'hold: cla required' and is stale: it has not been updated in 30 days. Note that this PR may be automatically closed in the future if no CLA is provided.  For CLA help see https://www.openssl.org/policies/cla.html

This PR has the label 'hold: cla required' and is stale: it has not been updated in 61 days. Note that this PR may be automatically closed in the future if no CLA is provided.  For CLA help see https://www.openssl.org/policies/cla.html

This PR has the label 'hold: cla required' and is stale: it has not been updated in 92 days. Note that this PR may be automatically closed in the future if no CLA is provided.  For CLA help see https://www.openssl.org/policies/cla.html

This PR has the label 'hold: cla required' and is stale: it has not been updated in 123 days. Note that this PR may be automatically closed in the future if no CLA is provided.  For CLA help see https://www.openssl.org/policies/cla.html

This PR has the label 'hold: cla required' and is stale: it has not been updated in 154 days. Note that this PR may be automatically closed in the future if no CLA is provided.  For CLA help see https://www.openssl.org/policies/cla.html

This PR has the label 'hold: cla required' and is stale: it has not been updated in 185 days. Note that this PR may be automatically closed in the future if no CLA is provided.  For CLA help see https://www.openssl.org/policies/cla.html

This PR has the label 'hold: cla required' and is stale: it has not been updated in 185 days. Note that this PR may be automatically closed in the future if no CLA is provided.  For CLA help see https://www.openssl.org/policies/cla.html

This PR has the label 'hold: cla required' and is stale: it has not been updated in 216 days. Note that this PR may be automatically closed in the future if no CLA is provided.  For CLA help see https://www.openssl.org/policies/cla.html

This PR has the label 'hold: cla required' and is stale: it has not been updated in 247 days. Note that this PR may be automatically closed in the future if no CLA is provided.  For CLA help see https://www.openssl.org/policies/cla.html

This PR has the label 'hold: cla required' and is stale: it has not been updated in 278 days. Note that this PR may be automatically closed in the future if no CLA is provided.  For CLA help see https://www.openssl.org/policies/cla.html

authors: OlegPekar mattcaswell openssl-machine


text:
Coverity Fixes for issue #12531

Fixes #12531 on master branch.
Address comments from
https://github.com/openssl/openssl/pull/12531

Checklist

[ ] documentation is added or updated
[ ] tests are added or updated

No worries at all.
On Jul 29, 2020, at 9:18 PM, Shane <notifications@github.commailto:notifications@github.com> wrote:
@slontis approved this pull request.
Thanks for your patience..
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHubhttps://github.com/openssl/openssl/pull/12557#pullrequestreview-457999992, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AFSCNZMDFKSKJQJWA4ABOD3R6DC6ZANCNFSM4PLXUDZQ.

This pull request is ready to merge

Thanks for fixing. Merged to master.

authors: ashman-p openssl-machine slontis


text:
Add support for rsaesOaep keys

rsaesOaep keys are identical to standard rsa keys except they have a few extra algorithm parameters, so we should be able to import and parse them.  The standard_methods array (in which all key support is added) mujst be in sort order, so add a test to check for that as well.
Signed-off-by: James Bottomley jejb@linux.vnet.ibm.com

This will not happen in 1.1.0 FYI.

On Wed, 2016-08-10 at 06:51 -0700, Rich Salz wrote:

This will not happen in 1.1.0 FYI.

That's OK, this is just upstream FYI.  I actually need this for 1.0.2h
which is already in OBS.
James

Thanks!

Confused by  the mingw64 asn1 fuzz failure:
wine ../fuzz/asn1-test.exe ../fuzz/corpora/asn1/a8ae08b58b33fc791d3e7a79ccf53c725fa66453 => 135

     #   Failed test at ../test/recipes/05-test_fuzz.t line 37.

That passes just fine here (Fedora 24, ./Configure mingw64 --cross-compile-prefix=x86_64-w64-mingw32- no-pic), and doesn't seem to be related to James's changes.

RSA-OAEP keys can't be used with any other algorithms, which this patch fails to enforce. Also note the text about parameters in RFC 4055 and RFC 5756 (which I think just kills the SPKI parameters? You'll want to check me on that. Anyway, this should be enforced). It's different from id-rsaEncryption which mandates a NULL instead.
OpenSSL has historically been very lax about parsing things. Due to OpenSSL's position in ecosystem, this has led to people minting invalid keys unnoticed because it works with OpenSSL. Going forward, please make new parsers stricter to we avoid more of this situation. draft-thomson-postel-was-wrong-00 and all that.

On Wed, 2016-08-10 at 08:32 -0700, David Benjamin wrote:

RSA-OAEP keys can't be used with any other algorithms, which this
patch fails to enforce.

That would be because this is only used to print and validate the TPM
certificate.  To create actual signatures, I'd need a private key,
which I don't have because it's sequestered within the TPM.  The TPM
itself outputs in its own weird format (not asn1) so it's easier just
to fold internal verifiers than to try to coerce this to an x509 or
smime format.
My only real use case is loading the key to validate signatures in TPM
quote.  There is a possible use case for creating private CA
certificates for the AIKs, but I haven't got to that yet.

Also note the text about parameters in RFC 4055 and RFC 5756 (which
I think just kills the SPKI parameters? You'll want to check me on
that. Anyway, this should be enforced). It's different from id
-rsaEncryption which mandates a NULL instead.

Actually, all of this is already in openssl.  It's the
crypto/rsa/rsa_oape.c and rsa_ameth.c routines which are used by the
cms utility.
The way cms currently works is that you have to add this padding type
with -keyopt.  The smime generated then has the OAEP parameters as part
of the algorithm specifier, so it's not key type dependent.

OpenSSL has historically been very lax about parsing things. Due to
OpenSSL's position in ecosystem, this has led to people minting
invalid keys unnoticed because it works with OpenSSL. Going forward,
please make new parsers stricter to we avoid more of this situation.
draft-thomson-postel-was-wrong-00 and
all that.

I really think you don't want to do that.  Since openssl today cannot
parse OAEP keys, everyone using the OAEP padding scheme in cms is using
standard rsa keys. If you try to enforce OAEP keys only, every current
use case breaks.
James

Actually, all of this is already in openssl.  It's the
crypto/rsa/rsa_oape.c and rsa_ameth.c routines which are used by the
cms utility.

That's for CMS. I believe your patch will also accept RSA-OAEP public keys in X.509 certificates without enforcing the parameters.

I really think you don't want to do that.  Since openssl today cannot
parse OAEP keys, everyone using the OAEP padding scheme in cms is using
standard rsa keys. If you try to enforce OAEP keys only, every current
use case breaks.

No, I meant that OpenSSL should not parse RSA-OAEP SubjectPublicKeyInfos by ignoring the parameters. Not that it should enforce no other kind of keys are used with RSA-OAEP. For better or worse, id-rsaEncryption SPKIs are usable with all operations, so those should continue to be used that way.

On Wed, 2016-08-10 at 14:25 -0700, David Benjamin wrote:

Actually, all of this is already in openssl.  It's the
crypto/rsa/rsa_oape.c and rsa_ameth.c routines which are used by
the cms utility.

That's for CMS. I believe your patch will also accept RSA-OAEP public
keys in X.509 certificates without enforcing the parameters.

it's a patch for EVP_PKEY which allows that type to be loaded from the
rsaesOaep NID.  It would therefore enable this type of key in anything
that takes asn1 formatted public keys, including x509, yes.
I don't really understand what you mean by "enforcing the parameters":
The x509 certificate itself could, in theory, be OAEP signed, but I've
never seen one.  Even the internal certificate verification algorithm
of a TPM certificate containing an rsaesOaep key is
sha1WithRsaEncryption.

I really think you don't want to do that.  Since openssl today
cannot parse OAEP keys, everyone using the OAEP padding scheme in
cms is using standard rsa keys. If you try to enforce OAEP keys
only, every current use case breaks.

No, I meant that OpenSSL should not parse RSA-OAEP
SubjectPublicKeyInfos by ignoring the parameters. Not that it should
enforce no other kind of keys are used with RSA-OAEP. For better or
worse, id-rsaEncryption SPKIs are usable with all operations, so
those should continue to be used that way.

So you want cms to import them automatically when it constructs a
signature using a certificate which contains a key of type rsaesOaep so
there's no need to specify them on the command line?
James

This has bitrot too much for even considering given the providers in 3.0. And yes enforcement of not using rsaesOAEP keys for signatures and stuff like that would be a requirement.


text:
Implemented installation support for GNU/Linux

Implemented installation support for GNU/Linux 866

Looks like a spam

seems to be crypto-mining using the CI/CD infrastructure.  you should close this and kill any jobs. and report the user.

81 contributions in 2 hours, ouch!...
https://github.com/Mr5-maurice-ramos

authors: FdaSilvaYY Mr5-maurice-ramos beldmit richsalz


text:
Reject SCTs with timestamps in the future

As per RFC6962, "TLS clients MUST reject SCTs whose timestamp is in the future.".

So being able to set the time lets you do SCT validation at any point in time.  That's reasonable, I supose.  Do we need it?  Can we always just use the  current time via time() et al?  If we want this feature, then I think some re-wording on the docs and comments to replace "current time" with "eval time" or something.

We could just use time(), though there'd need to be some way of faking the time for the tests still. Being able to override the time also allows users to make verification tolerant of SCT timestamps that are slightly in the future (by setting the time to, for example, a few seconds from now).

Sure, that’s reasonable.  Just need to fix the "current time" wording.

Done.

By the way, I could have CT_POLICY_EVAL_CTX_new() call time() to set the default value to the current time. The reasons I haven't are:

For the common case of libssl calling CT_POLICY_EVAL_CTX_new(), it can provide the time returned from SSL_SESSION_get_time(), so this call to time() would be unnecessary.
time() can theoretically fail (return -1), which means it wouldn't be guaranteed to set CT_POLICY_EVAL_CTX.epoch_time_in_ms to the current time.

The downside of not doing this is that any user code calling SCT_validate() at the moment will work, but will stop working after this change until a call to CT_POLICY_EVAL_CTX_set_time() is added. I suppose I should at least add a line to CHANGES.

Breaking existing code, even in a brand-new feature in a brand-new release is very bad.  If you're  concerned about the extra syscall, then just set the time field to zero and call time when doing the eval.  If time() fails we've got much worse things to worry about.

Note that ((time_t) -1) is not by itself (sufficiently) indicative of an error. On sane systems, to determine whether an error took place, you'd have to clear errno before the the call to time() and test for a non-zero errno immediately after. However, sane behaviour is not required by either C99 or POSIX.
The only plausible error is giving time() a bad pointer argument for the result, but one almost always calls time(NULL), in which case errors don't happen. So testing for -1 does more harm than good IMHO.

Ok, happy for me to set the CT_POLICY_EVAL_CTX.epoch_time_in_ms to the result of time() * 1000 unconditionally in CT_POLICY_EVAL_CTX_new then?

I would build-in a document some reasonable clock drift (300s) into the default ceiling on the issue time.  If users want tigher bounds then they call the API to set an explicit tight ceiling.  So, no, not fond of time * 1000 without something like +300000.

I wasn't convinced that relaxed defaults was the right call. Given that the user is probably already doing something a bit more advanced CT-wise if they're calling SCT_validate() directly, I think they should have to make a decision on whether and how much clock drift they want to tolerate. If they don't, and it causes SCT validation to fail, they'll get the error code CT_R_SCT_FUTURE_TIMESTAMP, which should make it fairly obvious what the issue is.
I'd like to highlight that whatever this default value is set to, it won't affect the clock drift tolerance of SCT validation for TLS connections, as they currently override the default. This purely affects whether users who are directly validating SCTs get informed that an SCT has a timestamp in the future. Getting an error back that points out clock drift seems like a sensible default and then, if they don't care, they can override it with a single line of code.

I've updated this PR according to your suggestions @vdukhovni, but I think hard-coding a timestamp tolerance should still be up for debate.

@RJPercival I'd like to highlight that whatever this default value is set to, it won't affect the clock drift tolerance of SCT validation for TLS connections, as they currently override the default.

Well, what tolerance, if any, is used with TLS?  It should not be zero.

No tolerance is used with TLS. Why should it not be zero?

There should be some tolerance for modest clock drift.  What's wrong with accepting SCTs that are 300s in the future that is not wrong with accepting SCTs that are 300s in the past?

If an SCT is younger than the MMD (maximum merge delay) of the CT log that issued it, the certificate it was issued for hasn't necessarily been incorporated and published by that log. That means the older the SCT timestamp, the better, as it's more likely to have been observed by monitors. Allowing SCTs to have timestamps in the future buy additional time for attacks before the offending certificate has to be disclosed. Admittedly, a mere 5 minutes in this case, but this seems unnecessary. I'm not overly concerned about this though, which is why I've already implemented the requested clock skew tolerance.

Could someone take another look at this please?

For reference, I haven't read relevant specs, which is why I'm reluctant to leave decisive vote. But I can attest that general concerns that I brought up were addressed.

@benlaurie, as someone who has most certainly "read the relevant specs", mind taking a look please?

+1 from me, still.

we have off-line approval from @vdukhovni .  Can you just do a one last rebase on master, @RJPercival ?  Thanks!

Done.

merged to master and 1.1.0, thanks!

authors: RJPercival dot-asm richsalz vdukhovni


text:
CMP: Add missing OldCertID check to test server; add missing related API elements

Preparing #15778, I found that the CMP test server was missing a critical check.
Implementing it, I found that some CMP and CRMF API elements were missing, part of which are needed for the fix.
Hope it is not too late to fix this and add those elements now.

[x] documentation is added or updated
[x] tests are added or updated

Too late for 3.0 - this really needed to be in for the final alpha and not proposed the day before beta close.

If this missing check is making the CMP implementation useless we should at least discuss this within OTC for inclusion in 3.0 as an exception.

So definitely not to be included in beta1 but we can discuss it within OTC for 3.0.

Also, would there be some way how to make the change less invasive and not adding a new public APIs?

If this missing check is making the CMP implementation useless we should at least discuss this within OTC for inclusion in 3.0 as an exception.

The check is important for the server-side handling of KUR: (key update requests, i.e, cert updates) in situations where the client cannot use the private key related to cert-to-be-updated for signature-based protection of the request. Then the server needs the OldCertID to identify the cert-to-be-updated, which may then also be needed to authorize the request.
The client side is not affected, and this is why so far the missing checks and API elements did not come up: They are needed for implementing a realistic CMP server.

Also, would there be some way how to make the change less invasive and not adding new public APIs?

For the OldCertID check, two API functions need to be added.
The rest of the newly added API functions are for completeness of the server-side API,
so not strictly needed for fixing the issue in the test server, but would be needed for implementing more capable servers, which may need to access further controls and registration information provided by the client in any types of requests.

So definitely not to be included in beta1 but we can discuss it within OTC for 3.0.

Thanks - would be great if we can still squeeze at least part of it into 3.0.

Hmm, given this is affecting only the server side, I do not think it is likely to get an exception. But we should discuss it at the OTC meeting next Tuesday anyway. If you could attend it to present the problem, it would be helpful.

But we should discuss it at the OTC meeting next Tuesday anyway.
If you could attend it to present the problem, it would be helpful.

Tuesday should work out for me. At which time should I attend?

Tuesday should work out for me. At which time should I attend?

10:00 UTC+2 - the same link as previously.

OTC: reduce this to the minimal PR with only getters included and avoid changing reason codes and altering signatures (adding the extension parameters) for OTC vote consideration as an exception to the beta rules in next weeks OTC meeting.

OTC: reduce this to the minimal PR with only getters included and avoid changing reason codes and altering signatures (adding the extension parameters) for OTC vote consideration as an exception to the beta rules in next weeks OTC meeting.

Done.
Moreover, as requested by @levitte, split the commit into

API (and reason code) extensions
fix of mock server, making use oft two new getters.
This includes new test case as witness for improved oldCertID checks in test server.

Will need to wait for the OTC to make a yes/no call before merging or not.

OTC vote passed to allow this change post-beta

Thanks @openssl/otc for discussing this.
I'm glad that the vote passed to include in 3.0 the getter functions so far missing from the API.
As agreed, also changed the result ofOSSL_CRMF_{CERTTEMPLATE,CERTID}_get0_serialNumber() to const,
for consistency and because this fits better with the intended use.

Merged to master. Thank you.

authors: DDvO paulidale t-j-h t8m


text:
Make EVP_PKEY_check() an alias for EVP_PKEY_pairwise_check()

The implementation of EVP_PKEY_pairwise_check() is also changed
to handle the legacy keys.
Fixes #16046
Checklist

[x] documentation is added or updated

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set.  Please review the updates and set the label manually.

Merged, thanks.

authors: openssl-machine paulidale t8m


text:
test/cmp_vfy_test.c: free before return

ChecklistQ

[ ] documentation is added or updated
[x] tests are added or updated

This pull request is ready to merge

@PeiweiHu when editing files, please verify that you do not keep trailing spaces as with this patch. Unfortunately github diff view does not show them prominently. You can easily verify with git diff as the trailing spaces are shown in red.

Merged to master and 3.0 branches (after removing the trailing spaces). Thank you.

Thanks for your reminder. I will keep my eyes open next time.

authors: PeiweiHu openssl-machine t8m


text:
Prevent crash in SSL_stream_reset for streams which have received all expected data

When calling SSL_stream_reset on a QUIC stream object that has received
all data that is expected to be sent (i.e. when the sender has sent a
STREAM frame with the FIN bit set), we encounter the following segfault:
    Program received signal SIGSEGV, Segmentation fault.
    0x00007ffff7f0bd28 in ossl_quic_sstream_get_final_size (qss=0x0, final_size=0x0) at ssl/quic/quic_sstream.c:273
    273         if (!qss->have_final_size)
    (gdb) bt
    0)  0x00007ffff7f0bd28 in ossl_quic_sstream_get_final_size (qss=0x0, final_size=0x0) at ssl/quic/quic_sstream.c:273
    1)  0x00007ffff7ef65bf in quic_validate_for_write (xso=0x5555555efcb0, err=0x7fffffffd5e0) at ssl/quic/quic_impl.c:2513
    2)  0x00007ffff7ef8ae3 in ossl_quic_stream_reset (ssl=0x5555555efcb0, args=0x0, args_len=0) at ssl/quic/quic_impl.c:3657
    3)  0x00007ffff7ebdaa6 in SSL_stream_reset (s=0x5555555efcb0, args=0x0, args_len=0) at ssl/ssl_lib.c:7635
    4)  0x0000555555557527 in build_request_set (
        req_list=0x55555555ebd0 "neil1.txt neil2.txt neil3.txt neil4.txt neil5.txt neil6.txt neil7.txt neil8.txt neil9.txt neil10.txt neil11.txt neil12.txt neil13.txt neil14.txt neil15.txt neil16.txt neil17.txt neil18.txt neil19.txt "..., ssl=0x5555555b6f80)
        at demos/guide/quic-hq-interop.c:545
    5)  0x00005555555587b2 in main (argc=4, argv=0x7fffffffe568) at demos/guide/quic-hq-interop.c:941

This occurs because:
1) When the stream FIN bit is set, the quic stack frees the underlying
stream structures immediately within the QUIC stack
and
2) when SSL_stream_reset is called, the call stack indicates we call
quic_validate_for_write, which attempts to access the
xso->stream->sstream QUIC_SSTREAM object, which was already freed in
(1)
The fix I think is pretty straightforward.  On receipt of a STREAM frame
with a FIN bit set, the QUIC stack sets the QUIC_STREAM object state to
QUIC_SSTREAM_STATE_DATA_RECVD, which means we can use that state to
simply assert that the stream is valid for write, which allows it to be
reset properly.
Checklist

[x] tests are added or updated

waiving style as the mutlistream scripts all violate coding style

The question is what SSL_stream_reset() should return in this situation. The RESET_STREAM frame cannot be sent due to RFC 9000 s. 3.3. However returning 1 should be OK anyway because the application cannot really know whether the sending stream is in data received state or not yet.

I think behaviorally, the right thing to do is return 0 here, which will inform the caller of SSL_stream_reset that the stream can't be reset, as its in a dead end state, referencing:
https://datatracker.ietf.org/doc/html/rfc9000#section-3.1
When a stream is in the DATA_RECVD, theres no way to reset it to its initial state, so the call to SSL_stream_reset should fail (which returning zero does), it just shouldn't crash, which this change resolves

I've fixed up the test.  I needed to ensure that we spent enough time waiting for the quic reactor to process all the needed ack frames so that the stream moved into the DATA_RECVD state, which frees the underlying stream data.  I also neglected to note that, in this scenario, a call to SSL_STREAM_reset fails (which is correct behavior), and the test needs to be able to expect that, which it now does.  With the test in its current form, reverting the fix in this PR causes the added test to crash, which is what this PR is meant to fix.

@t8m please reconfirm

This pull request is ready to merge

merged to master, 3.4, 3.3, and 3.2.  Thank you for the reviews

authors: mattcaswell nhorman openssl-machine t8m


text:
Mark DTLS records as read when we have finished with them (1.1.0)

The TLS code marks records as read when its finished using a record. The DTLS code did
not do that. However SSL_has_pending() relies on it. So we should make DTLS consistent.
This is the 1.1.0 version of #6159.

Pushed. Thanks.


text:
Fix 'no-ecdh' build [1.0.2]

Fixes #3302
(... and more warnings/errors)

Merged, thanks!


text:
Handle the renamed command POD files in find-doc-nits

This handles foo.pod and openssl-foo.pod so it can be cherry-picked.
When the command POD files were renamed from foo.pod to openssl-foo.pod, it broke find-doc-nits (the -c option).

Merged.
bc5a80910dccbc1e417f96bb7f0a3814d3ad5a4d Handle the renamed command POD files in find-doc-nits

authors: levitte richsalz


text:
[deploy_bmh] Fix labels identation in bmh jinja

As a pull request owner and reviewers, I checked that:

[x] Appropriate testing is done and actually running
[x] Appropriate documentation exists and/or is up-to-date:

[x] README in the role
[x] Content of the docs/source is reflecting the changes

/approve

authors: pablintino rebtoor


text:
Fix broken URL in the network config section

The "Configuring the network for the RHOSO deployment" section had a broken link. It now points to the proper direction.

Done, thanks for the review! Should I leave it like this and assume it will work upstream once the changes are ready?

I think we can merge and add the attributes in case the link happens to be broken upstream. IIUC the link was broken everywhere before, so we can't be worse off :).

authors: elvgarrui jistr


text:
Refactor dataplane

Consists of 2 commits:

Drop service reconciliation from nodeset controller
Services should be pre-req for the deployment and not
nodeset. Also, they're currently overwritten after they
are patched using oc apply -f which is not desirable.

Refactor nodeset/deployment CRDs

Moves ansibleee related fields to deployment
Moves services to deployment

Depends-On: https://github.com/openstack-k8s-operators/install_yamls/pull/579
Depends-On: https://github.com/openstack-k8s-operators/install_yamls/pull/580

recheck

I don't think refactoring is necessarily the way to go. Personally, I think we need to focus on stabilizing what we enabled in Q3. We have not had time or feedback to really see if the changes we made in Q3 were effective.

I don't agree as this is not a big change that would make things unstable. PR is already in good shape and once https://github.com/openstack-k8s-operators/install_yamls/pull/579 merges the kuttl job would be green.

I'd prefer to keep the NodeSet the declarative representation of the NodeSet, and the Deployment models the execution.

So you want to store things like what networks would be attached to aee pod and what verbose level ansible would be run in the nodeset properties?

It makes sense to me to keep that information stored on the NodeSet services. That way if an OpenStackDataPlaneDeployment is triggered for that NodeSet, the right set of services is run for each unique NodeSet, where the services may be different.

What set of services would be run should be deployment specific IMO. There is already the default set of services for a deployment as before.

With this proposed change, the set of services, mounts, networkAttachments are the same across all NodeSets for a Deployment. That really limits the flexibility of the Deployment. All nodes need to be the same, other than ansible vars.

We would not use anything other than ctlplane as the network attached to aee pod. aee pod can't talk to openstack services using other networks with the current design using mcvlan. It has to use the openshift internal network. Also, I mentioned earlier services to be run are deployment specific and can change from time to time. We can still have the option to provide services per nodeset in deployment spec.

As for the service reconciliation (EnsureServices), it seems you are expecting the user to do that going forward? That seems to be the case given the install_yamls PR (openstack-k8s-operators/install_yamls#579). I'm also not in agreement with that approach. I don't see why dataplane-operator can't create the default services automatically. If we want to enable editing the default services after they're created, we could still allow that by dropping the part that patches the full service Spec if the service is already created. We could also have the NodeSet "own" the services (setControllerReference), so that each service could be customized per NodeSet if desired.

It can't create those as it does not own them and still reconciles them from the image which is an anti-pattern and patching them externally has no impact. If nodeset owns the services with the full spec and then reconciles it, that would probably would ok. But I still say that what services has to be run should be deployment specific and users can run a subset of them when required.

Dropping the concept of a servicesOverride means it's on the user to remember the list of services for each NodeSet

I don't understand the issue here. The default services in a nodeset and deployment (proposed) are the same, if they use a custom set services in the last run, then deployment CR would have that info in both cases.

Moving these fields here to Deployment means they can't be unique per NodeSet.

These are mounts on the ansible pod right and what ansible expects? Why we want to tie them with nodeset? If we want to have different ones per nodeset we can still do that in deployment spec.

What "it" do you mean by "It can't"? The NodeSet controller is reconciling them today.

NodeSet controller does not own them neither watches them. If you patch the services it would reflect the modified CR in etcd. But when any nodeset CR is reconciled it would flip it back to the old definition in the dataplane-operator image. This is terrible design IMO.

There are dataplane-operator defaults (The hardcoded list of defaults).

NetworkAttachments are attached to aee and who owns the aee? I think it's the deployment controller[1] which also owns deployment.

And the change makes the Deployment more similar to the old DataPlane that we just refactored away.

No I don't agree. Deployment does not have nodeset definition. It just have everything that a deployment run would need to deploy services.

If you changed it so that all these fields were individual per NodeSet, then it is very similar to what we already have with NodeSet, except it's a list of NodeSets

Does not matter if a deployment has nodeset references, creating the deployment CR does not reconcile nodesets. So it's not the same .

What "it" do you mean by "It can't"? The NodeSet controller is reconciling them today.

NodeSet controller does not own them neither watches them. If you patch the services it would reflect the modified CR in etcd. But when any nodeset CR is reconciled it would flip it back to the old definition in the dataplane-operator image. This is terrible design IMO.

I'm not convinced that a use case of editing our default OpenStackDataPlaneService definitions is needed outside of a dev/test context, for which there are various other easier methods. We did not support or encourage editing our rpm packaged tripleo-heat-templates services either. You had to copy and maintain that copy, and refer to it as a different service name or mapping. I don't recall that causing all that much consternation.
The current design works the same way. If an edit is needed, instead of overwriting what we ship as a default resource, you can customize it using the supported composable services interfaces, and use a different name.

I'm not convinced that a use case of editing our default OpenStackDataPlaneService definitions is needed outside of a dev/test context

How do we differentiate between standard services vs custom services in the nodeset spec? Customers will definitely write and use custom services and for me everything is a service (read playbook) with single interface. We can publish definition of some standard ones, which customers can use and change as per their requirement.  I also saw the note on the install_yamls PR that nova and other services won't be using defaults we have for them.
The current way of using a different image for a so called default service is to patch the AEE default image (which will impact all default services) or build dataplane-operator with changed services https://github.com/openstack-k8s-operators/dataplane-operator/blob/main/Dockerfile#L64 and then patch dataplane-operator CSV.
The fact that some services would be reconciled by nodeset controller and some not and then custom ones can be created/patched outside is a confusing/bad user experience and can lead to number of customer case. I've seen several questions on that behavior on slack till now. Anyway, I don't really understand the concerns  here exactly, but I can't argue more, if we want to block this. I rest my case.

I'm not convinced that a use case of editing our default OpenStackDataPlaneService definitions is needed outside of a dev/test context

How do we differentiate between standard services vs custom services in the nodeset spec?

Other than by name, there is no differentiation in the NodeSet spec. The difference between a standard vs custom is:

RH ships the standard services in the dataplane-operator image (I'm assuming this is what you mean by standard)
The standard services are reconciled by the NodeSet controller
Custom services are reconciled by the user (oc apply)

Customers will definitely write and use custom services and for me everything is a service (read playbook) with single interface. We can publish definition of some standard ones, which customers can use and change as per their requirement.

I don't think the original intent is that customers would change the standard services directly on the same resource. They can however copy the standard service YAML, change the name, change the service spec in any way (including setting a different ansibleEE image), and then create it as a new service with the different name.

I also saw the note on the install_yamls PR that nova and other services won't be using defaults we have for them.

Yes, some will be, some won't be depending on the customizations.

The current way of using a different image for a so called default service is to patch the AEE default runner image (which will impact all default services) or build dataplane-operator with changed services https://github.com/openstack-k8s-operators/dataplane-operator/blob/main/Dockerfile#L64 and then patch openstack-operator CSV.

I think this is exclusive to dev/test context. There are also other ways, such as running dataplane-operator from git and editing under config/services. For prod, then the user creates a new service with a different name.

The fact that some services would be reconciled by nodeset controller and some not and then custom ones can be created/patched outside is a confusing/bad user experience and can lead to number of customer case. I've seen several questions on that behavior on slack till now. Anyway, I don't really understand the concerns here exactly, but I can't argue more, if we want to block this. I rest my case.

My main objection is to the CRD changes. I don't have strong objections to changes to how the services are reconciled. But, there are some base lines I feel are needed, such as the set of standard services that can be created automatically (as we know all the values).
Service reconciliation and the user experience workflow around it could be separate from refactoring the CRDs. We can leave CRDs as-is, and either drop the service reconciliation entirely so it's all manual, or fine tune it so that it does not undo user edits, or something else. I can't say there will be broad agreement on that (I have not heard from others), but they could be separate proposals if you feel strongly about it.

I can't say there will be broad agreement on that (I have not heard from others), but they could be separate proposals if you feel strongly about it.

I'm not sure there was a broad agreement on reconciling services in nodeset/role controller? I did point out my concerns in https://github.com/openstack-k8s-operators/dataplane-operator/pull/236 and the reasoning I  got was we've to fix the kuttl tests and similar stuff has been done elsewhere. The fact that the so called 'standard` services are not specific to nodeset (not owned by nodeset/nodeset controller) and live ever after till they're manually deleted, makes it a bad design.

My main objection is to the CRD changes.

For me, NodeSet is the definition of set of provisioned nodes that are ready for deployment or fully deployed. Deployment is the definition of what needs to be deployed and how. So all AEE related details should be in Deployment.If I understand correctly, you're saying  Deployment is just a trigger and nothing more than that and NodeSet contains all properties including what should be deployed in it and how, then the ServicesOverride in deployment spec does not really make sense. Any change in services to be deployed in a nodeset should be changed in the nodeset spec.

My main objection is to the CRD changes.

For me, NodeSet is the definition of set of provisioned nodes that are ready for deployment or fully deployed. Deployment is the definition of what needs to be deployed and how. So all AEE related details should be in Deployment.If I understand correctly, you're saying Deployment is just a trigger and nothing more than that and NodeSet contains all properties including what should be deployed in it and how, then the ServicesOverride in deployment spec does not really make sense. Any change in services to be deployed in a nodeset should be changed in the nodeset spec.

Deployment is the trigger with optional overrides which are offered for convenience and specific use cases (upgrades, etc).

Deployment is the trigger with optional overrides which are offered for convenience and specific use cases (upgrades, etc).

That's where I think the design is broken. We're saying it's just a trigger, but then the deployment resource owns the aee resources and also can override what would be deployed (services). Looking at the nodeset spec no one can tell what has been deployed on it at any point as the services could be overridden with more (including new ones) could have been deployed with service override.

I'm going to close this one, in the interest of closing stale PRs.

authors: rabi slagle


text:
Add github labels and relevant pipelines

Add github labels and relevant pipelines
- Added check and gate pipelines to tests-list

recheck

recheck

we can consider adding validating yaml job in github-check/check jobs before merging

recheck

recheck

authors: danpawlik raukadah son-vyas


text:
Correcting broken link to intro blog post

I noticed this link was broken, so I'm submitting a correction.

thanks!

authors: EricPaulson jamesturk

repo: openstates/documentation


text:
pdf(git): more accurate error message when incorrect slug is entered

open 'create a new job' modal
click on PDF(git) option
enter incorrect slug, e.g. repo
Result: this message is shown: repo-name/slug-name
Could we change it to something like this: A valid collection repo/slug is required, e.g. repo-name/slug-name

This has been fixed in the latest versions of corgi for both pdf-git and preview-git options

@ne-newton closing this issue as fixed

repo: openstax/output-producer-service


text:
Not recognized as logged in when viewing notes

When viewing nodes in iD I am presented with the You must log in to change or comment on this note box and cannot open notes from in the editor.
I have reloaded, refreshed the page cache, logged in and out of osm.org with no change.
I can open notes from the osm.org main page just fine.
Examples

Specs
Ubuntu 18.04 LTS
Vivaldi 2.1.1337.51 (64 bit)
OS is up-to-date.

Thanks for reporting @rivermont  - can you check that your browser is able to write to localStorage?  Anything that prevents that (e.g. incognito mode or similar) will prevent iD from authenticating.

It seems to have solved itself, either a browser restart or rebooting worked. Someone in OSM-US slack said they also had this happen only once and never again, so maybe it's a one-time issue. Feel free to reopen if we need a better explanation.

authors: bhousel rivermont

repo: openstreetmap/iD

license: ISC


text:
add 4 tags which imply oneway=yes

Most of these are rarely used but I don't see any harm in adding them to the list

Thanks!

authors: k-yle tyrasd


text:
[BUG] Javascript Debug Terminal 不能通过 exit 命令退出

描述你的问题（Describe the bug）

启动一个 Javascript Debug Terminal 后，输入 exit 不能退出，执行其它命令正常
复现路径（To Reproduce）

启动 web 版本
创建一个 Javascript Debug Terminal
输入 exit 就会复现
exit is a shell builtin

预期表现（Expected behavior）

问题截图（Screenshots）

环境信息（Environment）

OS: macOS catalina
Browser: chrome
OpenSumi Version: main 分支

补充信息（Additional context）

@erha19 这个也顺便看看

repo: opensumi/core


text:
Prioritise and add new non-eQTL V2G data

New eQTL data will come from the eQTL database. We have a document of other functional datasets that we may like to add.
These need prioritising and adding to the portal.

Possible other sources that @elipapa listed previously: https://github.com/opentargets/genetics/issues/23

repo: opentargets/genetics


text:
Add projectId to Open Targets Genetics evidence data table

Details TBC - pending work in #1606 and #1607 and the final pipeline run

@carcruz, can we please adjust the Open Targets Genetics evidence data table and add a column "Study source" after the "Publication" column.
Within the "Study source" column, can we please display the mapped term depending on the projectId value returned by the GraphQL API?

API value
Mapped value to be displayed in UI

FINNGEN
FinnGen

GCST
GWAS Catalog

SAIGE
UK Biobank

NEALE
UK Biobank

any other value
N/A

For an example association with all 4 sources, see the TPO (ENSG00000115705) in hypothyroidism (EFO_0004705) evidence page and GraphQL API query.

closing this as work is done

authors: andrewhercules ktsirigos

repo: opentargets/platform